search

ethereum / go-ethereum

Go implementation of the Ethereum protocol
https://geth.ethereum.org
GNU Lesser General Public License v3.0
47.56k stars 20.13k forks source link

Sign message is using incorrect prefix #14794

Closed prusnak closed 6 years ago

prusnak commented 7 years ago

https://github.com/ethereum/go-ethereum/blob/bf468a81ec261745b25206b2a596eb0ee0a24a74/internal/ethapi/api.go#L361 is using ASCII value for length of the message.

This is just plain wrong and shows misunderstanding of the concept of prefix introduced by Bitcoin.

In Bitcoin, the message structure is the following:

<varint_prefix_length><prefix><varint_message_length><message>

So for the message Foo you'll get:

\x18Bitcoin Signed Message:\n\x03Foo

The author of the Ethereum code got the <varint_prefix_length> correctly, because this was adapted to \x19 (length == 25), but the code for length is wrong as it uses ASCII value. This is wrong, because you cannot guess where the prefix ends and message starts, e.g. when you have message "00", you'll get

\x19Ethereum Signed Message:\n200 which is really bad.

Best fix would be to implement varint from Bitcoin and use that instead.

prusnak commented 7 years ago

It seems that MyEtherWallet and Etherscan.io do not use prefixes at all, which is also bad, because user can unintentionally sign arbitrary data (e.g. valid transaction).

I am planning to roll out a TREZOR firmware update which will use the correct varint prefix for Ethereum signed messages and it would be great to have all parties at the same page (as currently they are not and we have great chance to get everything right)

// CC @ligi @kvhnuke

ligi commented 7 years ago

Sounds good - perhaps this should even be standardized via EIP? Edit: what about using RLP encoding? So no guessing is needed to see how long varint_message_length is and we can use Ethereum primitives

prusnak commented 7 years ago

I am not strictly against using RLP (although I personally do think it is horrible), but I think it makes sense to use RLP for encoding the Ethereum Signed Message part as well.

karalabe commented 7 years ago

I would also recommend an EIP for this. Regarding RLP, I'm not sure that's a good idea. Such signatures need to be verifiable by the EVM too, so if you need an entire RLP parsing contract to check the signature, it might be overkill.

The ascii encoding is indeed unfortunate. Not sure why that was used. @holiman @bas-vk ?

prusnak commented 7 years ago

@karalabe Ah, I thought that RLP parsing is cheap operation in EVM. If that is not the case, then I suggest to go for varint option. Couple of reasons to support the idea:

a) it is already used for encoding the first part b) varint parsing and creating is extremely simple (even for EVM) c) there are lots of implementations in various languages already (e.g. Go, Javascript, C)

prusnak commented 7 years ago

No reaction for 4 weeks :-/

We released a new TREZOR firmware a week ago which fixes the issue by using the format described in the initial comment.

karalabe commented 7 years ago

I would have expected the trezor to just sign whatever given, and leave it to the node to append any black magic.

prusnak commented 7 years ago

Like said above I don't want to allow this because "user can unintentionally sign arbitrary data (e.g. valid transaction)" (and this happens in the wild, because most of the implementations I've seen do not prepend/append any black magic).

karalabe commented 7 years ago

Sure, but now we have yet another incompatible way to sign stuff :)

prusnak commented 7 years ago

That is unfortunate, but my mission is to make using cryptocurrencies easy and errorless and this is the area where one should not make compromises.

ligi commented 7 years ago

I second that - and to try to stop getting more incompatible ways I would volunteer to write the EIP that was called and agreed on the need for it here earlier. Will start with it once I am finished with the paid gig for today.

prusnak commented 7 years ago

Thanks, much appreciated

3esmit commented 6 years ago

Related https://github.com/ethereum/EIPs/issues/191

EvilJordan commented 6 years ago

I'm lost here. I cannot make geth verify a signed trezor message, even with manually adding the magic message beforehand. Is it just not possible?

Edit: Trezor implemented their own way of signing messages (using varInt instead of ASCII like everyone else for message length) breaking compatibility with everyone else who doesn't branch their code to look for both cases. That EIP is desperately needed!

erickearns commented 6 years ago

@prusnak

"I am planning to roll out a TREZOR firmware update which will use the correct varint prefix for Ethereum signed messages and it would be great to have all parties at the same page (as currently they are not and we have great chance to get everything right)"

Great. So in your unilateral decision to "fix" message signing, Trezor now differs from

Etc., etc.

Without disputing that varint would have been the "correct" choice originally, you've created an ecosystem in which the tools are literally incompatible, without improving security (unless you can describe a situation in which having a user sign a message string that begins with integers is exploitable to any gain).

prusnak commented 6 years ago

@erickearns Trezor currently does not support any ethereum message signing as there are at least three efforts to come up with a new one.

erickearns commented 6 years ago

@prusnak

I seem to have no issue signing messages (other than the varint prefix) with

https://github.com/trezor/python-trezor/blob/master/trezorlib/client.py#L613 against the most recent firmware (https://github.com/trezor/trezor-mcu/blob/master/firmware/ethereum.c#L638)

prusnak commented 6 years ago

@erickearns Sorry, I was talking about trezor-core. I will implement the old method in TREZOR Model One (trezor-mcu repo) and in TREZOR Model T (trezor-core repo), as there seems to be at least consensus on using this one for now.

prusnak commented 6 years ago

Implemented in https://github.com/trezor/trezor-mcu/commit/1f470cf1f18de0235675cf71091bba7bf4fb762f and https://github.com/trezor/trezor-core/commit/4de376acd68decf9dfe3d247bec15f7a6c65e6c4

rralev commented 6 years ago

@prusnak Could you please clarify what is the prefix you use in the latest version of the firmware?

prusnak commented 6 years ago

@rralev we use the old one as can be seen from the commits above

prusnak commented 6 years ago

I am closing this issue as no change was done and no change is planned (except for creating new standards, but this is out of scope of this issue).

rralev commented 6 years ago

@prusnak So, are you still using varInt or you are using eth_sign prefix?

alexdupre commented 5 years ago

@prusnak It looks like the Trezor implementation of the old method contains a serious bug, the length is stored incorrectly if it's exactly a power of 10 (the initial 1 is omitted). All these > should be >=: https://github.com/trezor/trezor-mcu/commit/1f470cf1f18de0235675cf71091bba7bf4fb762f#diff-12950855dc4a2a268fb19ece6da37668R632

prusnak commented 5 years ago

@alexdupre can you please send a pull request to https://github.com/trezor/trezor-firmware (the new monorepo)?