New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sign message is using incorrect prefix #14794

Closed
prusnak opened this Issue Jul 12, 2017 · 23 comments

Comments

Projects
None yet
7 participants
@prusnak
Copy link
Contributor

prusnak commented Jul 12, 2017

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

This comment has been minimized.

Copy link
Contributor

prusnak commented Jul 12, 2017

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

This comment has been minimized.

Copy link
Member

ligi commented Jul 12, 2017

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

This comment has been minimized.

Copy link
Contributor

prusnak commented Jul 12, 2017

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

This comment has been minimized.

Copy link
Member

karalabe commented Jul 13, 2017

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

This comment has been minimized.

Copy link
Contributor

prusnak commented Jul 13, 2017

@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

This comment has been minimized.

Copy link
Contributor

prusnak commented Aug 9, 2017

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

This comment has been minimized.

Copy link
Member

karalabe commented Aug 9, 2017

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

@prusnak

This comment has been minimized.

Copy link
Contributor

prusnak commented Aug 9, 2017

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

This comment has been minimized.

Copy link
Member

karalabe commented Aug 9, 2017

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

@prusnak

This comment has been minimized.

Copy link
Contributor

prusnak commented Aug 9, 2017

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

This comment has been minimized.

Copy link
Member

ligi commented Aug 9, 2017

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

This comment has been minimized.

Copy link
Contributor

prusnak commented Aug 9, 2017

Thanks, much appreciated

@3esmit

This comment has been minimized.

Copy link

3esmit commented Feb 1, 2018

@EvilJordan

This comment has been minimized.

Copy link

EvilJordan commented Feb 9, 2018

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

This comment has been minimized.

Copy link

erickearns commented May 24, 2018

@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

This comment has been minimized.

Copy link
Contributor

prusnak commented May 24, 2018

@erickearns

This comment has been minimized.

Copy link

erickearns commented May 24, 2018

@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

This comment has been minimized.

Copy link
Contributor

prusnak commented May 25, 2018

@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

This comment has been minimized.

Copy link
Contributor

prusnak commented May 25, 2018

@rralev

This comment has been minimized.

Copy link

rralev commented Sep 29, 2018

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

@prusnak

This comment has been minimized.

Copy link
Contributor

prusnak commented Sep 29, 2018

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

@prusnak

This comment has been minimized.

Copy link
Contributor

prusnak commented Sep 29, 2018

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).

@prusnak prusnak closed this Sep 29, 2018

@rralev

This comment has been minimized.

Copy link

rralev commented Sep 29, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment