Sign message is using incorrect prefix

[prusnak]

go-ethereum/internal/ethapi/api.go

Line 361 in bf468a8

msg := fmt.Sprintf("\x19Ethereum Signed Message:\n%d%s", len(data), data)

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]

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]

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]

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]

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]

@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]

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]

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

[prusnak]

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]

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

[prusnak]

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]

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]

Thanks, much appreciated

[3esmit]

Related ethereum/EIPs#191

[EvilJordan]

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]

@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

• Ledger (https://github.com/LedgerHQ/blue-app-eth/blob/master/src_genericwallet/main.c#L2461)
• Web3.py (https://github.com/ethereum/eth-account/blob/master/eth_account/internal/signing.py#L47)
• Geth (https://github.com/ethereum/eth-account/blob/master/eth_account/internal/signing.py#L47)
• Parity (https://github.com/paritytech/parity/blob/master/rpc/src/v1/helpers/dispatch.rs#L243)
• EthereumJS (https://github.com/ethereumjs/ethereumjs-util/blob/master/index.js#L383)

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]

@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]

@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]

@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]

Implemented in trezor/trezor-mcu@1f470cf and trezor/trezor-core@4de376a

[rralev]

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

[prusnak]

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

[prusnak]

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]

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

[alexdupre]

@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 >=: trezor/trezor-mcu@1f470cf#diff-12950855dc4a2a268fb19ece6da37668R632

[prusnak]

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