Skip to content
This repository has been archived by the owner on Apr 16, 2019. It is now read-only.

SignMessage / VerifyMessage does not work correctly for Segwit addresses #169

Closed
prusnak opened this issue Jun 6, 2017 · 14 comments
Closed

Comments

@prusnak
Copy link
Member

prusnak commented Jun 6, 2017

... because:

  1. SignMessage specifies address by address_n - add segwit flag?

  2. VerifyMessage specifies address by string - segwit prefix is not recognized

@karelbilek
Copy link
Contributor

karelbilek commented Jun 6, 2017

As of interest

bitcoin core does not support signing with segwit (what BIP141/BIP49 calls P2WPKH-nested-in-P2SH) addresses. The RPC just throw an error "Address does not refer to key".

I will ask bitcoin core if that is a feature or a bug.

edit: ...here

bitcoin/bitcoin#10542

@jhoenicke
Copy link
Collaborator

SignMessage/VerifyMessage work on public keys. There is nothing in the signature that indicates, whether the public key should be encoded as p2pkh address, as p2sh, or p2wpkh (optionally over p2sh). I think nobody has made a standard for this.

We could easily change VerifyMessage in the firmware to detect p2wpkh over p2sh address or the besh32 p2wpkh address and accept them if they match the public key. For signing, the firmware doesn't need any change (unless someone defines a new format for segwit message signatures). The web wallet need to detect its segwit addresses, though.

@karelbilek
Copy link
Contributor

I am not sure if you can get the pubkey hash from just the address, which is just scripthash.

From the discussion on the linked thread, it seems that you cannot, so we would somehow need to attach the pubkey hash.

Which complicates everything.

As luke notes in the other thread, someone should make a standard if this is to be supported on segwit addresses. Because we want to release the segwit wallet+firmware sooner than later, I suggest we just disable the sign/verify for segwit accounts for now until the standard is made, if it ever is.

@jhoenicke
Copy link
Collaborator

You can always go from pubkey to pubkey hash to script hash. The VerifyMessage call already has to recover the public key from the signature to check that its hash matches the address. Then going from there to the p2wpkh or p2sh address is possible.

@karelbilek
Copy link
Contributor

Oh you are right. I was confused about that. So it's not that hard actually

@karelbilek
Copy link
Contributor

Hm, I am trying to find what exactly signMessage does :D and how is the pubkey encoded there, right now, and how much could that change with segwit

@karelbilek
Copy link
Contributor

karelbilek commented Jun 6, 2017

Hm, when I am reading more, without change of the format for "segwit" p2sh, the signatures would not be very helpful.

An "evil" signer could use the same private key for both p2pkh and p2sh-p2wpkh addresses, since the address itself is not in the signature/message.

So the signature would be meaningless, since you are not sure for which address it actually is - you are not signing with the address, but with the private key. You could not do such a trick with trezor, but you could with desktop software. [edit] - actually yes you can do it also with trezor, with the same path you get the same privkey. :)

We would somehow need to add into the format that it is p2sh-p2wpkh or there could be confusion. I am not sure if it could be used for actual attacks though; if you own a p2pkh address with a given key, you also own the other one and vice versa.

@jhoenicke
Copy link
Collaborator

The signature consists of 1 byte value "v", 32 byte value "r", 32 byte value "s". r and s is the ECDSA signature. Q can be reconstructed using the ECSDA equation except that there are up to four possible values. Value v encodes which of the four values and whether it is compressed or uncompressed. It would be possible to extend v to also encode whether segwit p2wpkh or p2sh address should be used.
Currently only eight different values for v are defined (ethereum uses different values for v for replay protection, though).

@prusnak
Copy link
Member Author

prusnak commented Jul 17, 2017

I think we'll have to come with the Segwit signing scheme. There are more and more people asking about this and it would be good to have this ready for Bitcoin once Segwit is activated there.

Probably the best way would be to encode type of address into v. Let's start just with
P2PKH and P2WPKH-in-P2SH, or do we want to include other options as well from the beginning?

@karelbilek
Copy link
Contributor

karelbilek commented Jul 17, 2017

We can send those people to bitcoin-core, there is already an issue for that :)

see bitcoin/bitcoin#10542 (linked above already)

@jhoenicke
Copy link
Collaborator

jhoenicke commented Jul 17, 2017

If we want it soon, we should do something simple like encoding the address type in v. And then hope that others wallets will recognize these signatures in the future. Maybe we can ask electrum developers if they want to do something similar.

Current encoding:

  • low two bit encode parity and overflow
  • 27-30 uncompressed pubkey,
  • 31-34 compressed pubkey.

My suggestion (segwit pubkeys are always compressed, so there is no need to distinguish compressed and uncompressed):

  • 35-38 p2sh segwit pubkey (base58).
  • 39-42 segwit pubkey (bech32).

Note that all these signatures are malleable, since v does not go into the hash. If you have a signature for somebody's segwit address, you can change it into a signature for the uncompressed or compressed 1... address, or even into the signature of the negative address. It's not an attack vector as I can even trivially change it into a signature of my own address ☺.

@prusnak
Copy link
Member Author

prusnak commented Jul 18, 2017

35-38 p2sh segwit pubkey (base58)
39-42 segwit pubkey (bech32)

My thoughts exactly!

@prusnak
Copy link
Member Author

prusnak commented Jul 18, 2017

We'll proceed by adding optional InputScriptType script_type; to SignMessage like we do in GetAddress. We won't need this field for VerifyMessage, because we can guess the address type like we do when providing address for output.

Correct?

@prusnak
Copy link
Member Author

prusnak commented Jul 25, 2017

Merged to Master in b5f9a57 (segwit-in-p2sh only)

Unit tests added in python-trezor here: trezor/python-trezor@11b686a

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants