Potentially dangerous sign_msg function should be removed

[MarvinJanssen]

The potentially dangerous sign_msg function introduced in #82 should be removed. It is based on Ethereum's personal_sign, the use of which is discouraged since the introduction of the much superior EIP712 standard. EIP712 addresses the many issues and shortcomings of personal_sign. If the function is adopted in Stacks wallets, it will put the entire ecosystem on a perilous path. We should learn from the Ethereum space and skip this function completely.

personal_sign had two main use cases:

1. Signing messages to authenticate with some app/dapp.
2. Signing messages that authorise some on-chain action.

When it comes to the first, we already have an authentication scheme. Right now it does not allow us to prove address ownership, but that can be achieved in different ways. (I will come back to this at the end.)

The second one is more important, so I will focus on that. Signed messages have been used for many different things in the Ethereum space. I will take on one such example for brevity: off-chain order signing. 0xProtocol allows users to sign an order, which can later be submitted to a smart contract to make a token trade happen. Early versions of the protocol would hash an order structure and request users to sign the order hash via personal_sign to authorise the trade.

It first looked like this:

And then this:

Does the user have any idea what he or she is actually signing? Obviously not. The user will just have to trust that the dapp fed the right message to the wallet. The messages are also not domain specific, which means that a malicious app could trick a user into signing something meant for another platform. And since the messages that are passed to the wallet are hashes, there is no way to reconstruct the original message in the wallet. It is just all-round terrible UX and highly dangerous. In hindsight it is obvious that the function should never have existed, but back then it is all we had and such signed messages were cutting edge. They allowed for many new an exciting things. Although personal_sign was largely left behind, it is still causing longer-lasting damage because it normalised the concept of signing byte strings. Many people are now so used to signing on-chain actions in this way, that they just hit Confirm when that wallet screen pops up. The sign_msg function opens us up to all of it.

Signing byte strings should not be normalised in the Stacks ecosystem.

I cannot stress that enough. We have so many tools at our disposal to make the UX a lot better and safer. Ethereum did not have the benefit of a human-readable interpreted language like Clarity. There is no good reason to introduce a personal_sign equivalent for Stacks.

Besides, it seems to be carelessly implemented and actually commits the same mistake as the original personal_sign implementation.

https://github.com/Zondax/ledger-blockstack/blob/abaf1d01a0bef27b2a603ec3d15bb6a20ddbcc96/js/src/index.ts#L245

• The \x19 is a (Bitcoin) varint representing the byte length of the prefix, which is correct for Ethereum Signed Message:\n as it's 25, but not for Stacks Signed Message:\n. It seems like it was copy and pasted thoughtlessly. It should have been \x18.
• The message length is appended as an ASCII string, so you have no idea where the actual message starts. They made the same mistake in geth. Sign message is using incorrect prefix ethereum/go-ethereum#14794

The language in #76 almost makes it sound like sign_msg is a stopgap solution until we have something better. I really do not think that is the right approach. I rather have nothing, over this function, until a good message signing standard arrives. Hiro, as a shepherd of the Stacks ecosystem, should consider very carefully whether adding such a function is worth it.

There is a draft SIP that describes a structured data signing method much like EIP712. It describes a general structured message signing standard that leverages Stacks wire format, while still allowing safe human-readable text message signing. (If people insist.) I would welcome such a standard or one like it. And to go back to proving address ownership, the linked SIP would make that effortless as well.

I hope that all this is convincing enough to remove sign_msg. For reference, we are building a hardware wallet at Ryder and will not implement an unstructured message signing function such as this one for the reasons laid out above.

[neithanmo]

thanks for this clear explanation. in our case, because of the limitation the device has in regards to the characters that can be shown on the display, the ledger app only processes ASCII string messages, So users would be able to see the message they are signing. .. trying to sign a message like the one you described would cause an error.

[kyranjamie]

Thanks for reporting @MarvinJanssen.

If you can help me understand the varint prefix, Stacks Signed Message:\n is 24 chars, so wouldn't that make the prefix \x18 in little endian encoding?

You describe off-chain order signing as an example of how message signing is used, but as we follow the message prefix scheme, doesn't that mute the ability to build features like this and thus prevent any similarly related attacks? What would be an abuse vector of the sign_msg function as is, or are you more concerned about the practice of signing raw input messages in general?

[neithanmo]

If you can help me understand the varint prefix, Stacks Signed Message:\n is 24 chars, so wouldn't that make the prefix \x18 in little endian encoding?

actually, the variant prefix is \x19

[MarvinJanssen]

@neithanmo I see. That makes things better. In that case I suggest the JS lib to enforce this and not have it sent to the Ledger at all if there are non-printable characters present in the message. I did not dive into the Ledger code and made the report solely on the linked PR so my apologies for not going deeper.

I think the length as ASCII should still be fixed. If you make it a (var)int then you can easily determine where the actual message starts.

https://github.com/Zondax/ledger-blockstack/blob/a7a0caa8434cfda23bc54b82555db7104e0c0a3a/app/rust/src/parser/message.rs#L94

Counting digits does not work if the users inputs a message like "00", you end up with \x19Stacks Signed Message:\n200.

@kyranjamie you're right, stupid mistake on my part, 24 == \x18.

To answer your question, I was unaware that the message signing function in the firmware rejects non-ASCII messages. personal_sign was at some point in the past (ab)used to sign hashes of data structures. The ASCII prefix had nothing to do with it. They did it by hashing an ABI encoded struct and requesting a signature from the user. So the final message would look like: \x19Ethereum Signed Message:\n32<32 bytes here>. Then on the contract they would reconstruct that message to verify the signature.

Here is one such example (that I even made myself):

https://github.com/wyvernprotocol/wyvern-v3/blob/b20ae7873d3f0e9b8f5ea6d0ff3947646830dbb7/contracts/exchange/ExchangeCore.sol#L196-L201

https://github.com/wyvernprotocol/wyvern-v3/blob/b20ae7873d3f0e9b8f5ea6d0ff3947646830dbb7/contracts/exchange/ExchangeCore.sol#L25

My concern with it is that users cannot verify what they are signing. But now that I know that the Ledger app won't accept non-ASCII messages, this is less of an issue as developers will be hard pressed to (ab)use msg_sign in the same way as Eth devs did with personal_sign.

Having said all that, I still wonder what the usefulness of msg_sign is. What are the use-cases? Even more so now I heard that Hiro is working on a SIP018(-like) implementation for Hiro Wallet. You could use SIP018 to sign a string-ascii CV if you want a text message.

[neithanmo]

@neithanmo I see. That makes things better. In that case I suggest the JS lib to enforce this and not have it sent to the Ledger at all if there are non-printable characters present in the message. I did not dive into the Ledger code and made the report solely on the linked PR so my apologies for not going deeper.

we can add this double check as well.

I think the length as ASCII should still be fixed. If you make it a (var)int then you can easily determine where the actual message starts.

https://github.com/Zondax/ledger-blockstack/blob/a7a0caa8434cfda23bc54b82555db7104e0c0a3a/app/rust/src/parser/message.rs#L94

Counting digits does not work if the user's inputs a message like "00", you end up with \x19Stacks Signed Message:\n200.

@MarvinJanssen we considered that case as it is clearly described in the EIP-712 documentation as a possible flaw, that is why the parser verifies if \x19Stacks Signed Message:\n200 correspond to a str.len = 200 or a str = "00" with length=2. for this message the device will show the user the string "00" and will ask the user if they want to sign it.
the case for strings starting with numbers is also considered, and last but not least, trailing zeros are also invalid(there are tests for each).

there is one important thing. because of the small screen in ledger devices, and considering that string_messages can be huge. we limited the number of characters the device is going to show on the screen, which means that depending on the size of the screen and the message length, the message users see might not be a full message.

[MarvinJanssen]

@MarvinJanssen we considered that case as it is clearly described in the EIP-712 documentation as a possible flaw, that is why the parser verifies if \x19Stacks Signed Message:\n200 correspond to a str.len = 200 or a str = "00" with length=2. for this message the device will show the user the string "00" and will ask the user if they want to sign it.

You have the opportunity to fix it, so why not fix it? There is no reason for parity with Ethereum's personal_sign.

there is one important thing. because of the small screen in ledger devices, and considering that string_messages can be huge. we limited the number of characters the device is going to show on the screen, which means that depending on the size of the screen and the message length, the message users see might not be a full message.

That sounds a little scary. Perhaps it is better if the user has to explicitly enable it, like with "blind signing" for the Ethereum Ledger app. What is the limit of how many pages of text you can show?

As an aside, what kind of huge string messages are you expecting?

[neithanmo]

You have the opportunity to fix it, so why not fix it? There is no reason for parity with Ethereum's personal_sign.

we have already fixed that, it is how the parser was implemented.

That sounds a little scary. Perhaps it is better if the user has to explicitly enable it, like with "blind signing" for the Ethereum Ledger app. What is the limit of how many pages of text you can show?

we can show the complete message but it would be annoying the number of times users have to click..for any message we are going to show only the first 60 characters (header not included).
What we could do for cases where strings.len > 60 is to process them only if expert mode was configured on the device. but what is a proper limit here ?

As an aside, what kind of huge string messages are you expecting?

not sure, fixing a number maybe just too much or too few for some users

[MarvinJanssen]

What we could do for cases where strings.len > 60 is to process them only if expert mode was configured on the device. but what is a proper limit here ?

Good question. I am not sure what the feature is meant for so maybe @kyranjamie can shed light on a good limit.

[kyranjamie]

@MarvinJanssen @neithanmo leather-wallet/extension#1051

I believe this issue prompted the ask of this feature.

[neithanmo]

i opened another issue to restrict signing messages that are longer than X bytes. If the app is in expert mode, such messages can be signed as long as they are valid. users know what they are doing so, the restriction of showing only the first X characters could remain active in both modes(I do not see an expert doing a lot of clicks)?.
To summarize:

• in normal mode, any message longer than X would fail
• if in expert mode longer messages are allowed.
• in both modes, only the first X bytes are shown.

for now X = 60. the election of this number considered the nanoS screen, so a message with this length would require around ~3 clicks.

[markmhendrickson]

If our expectation is that most messages will be less than a certain value X (e.g. 60 characters), and that their length won't require users to "click too much" (e.g. <4 clicks), then any cap we put into place for that will be irrelevant for most users.

For the few users that do encounter longer messages and more clicks, my sense is that it's better to show the full message and require more clicks, without requiring the extra step of enabling "expert mode" (and without limiting the display to only the first X characters). That way there is no possibility of "blind signing" even part of the message.

Of course, some subset of these few users may encounter many clicks (10+?) if they're signing something particular complex. But presumably that increased complexity will correlate with one's increased desire to verify the complete message and increased tolerance for clicks?

[MarvinJanssen]

Although some upper bound surely is necessary still, lest dapps may try to send very large strings to be signed.

[jleni]

@kyranjamie @markmhx is there anything pending here?
should this result in some actions/tasks?

[neithanmo]

we agreed on showing the complete message to users..
please reopen if it makes sense or to start a discussion in regards to an alternative approach.