-
Notifications
You must be signed in to change notification settings - Fork 7
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
Potentially dangerous sign_msg function should be removed #90
Comments
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. |
Thanks for reporting @MarvinJanssen. If you can help me understand the varint prefix, 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 |
actually, the variant prefix is \x19 |
@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. Counting digits does not work if the users inputs a message like @kyranjamie you're right, stupid mistake on my part, To answer your question, I was unaware that the message signing function in the firmware rejects non-ASCII messages. Here is one such example (that I even made myself): 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 Having said all that, I still wonder what the usefulness of |
we can add this double check as well.
@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 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. |
You have the opportunity to fix it, so why not fix it? There is no reason for parity with Ethereum's
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? |
we have already fixed that, it is how the parser was implemented.
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).
not sure, fixing a number maybe just too much or too few for some users |
Good question. I am not sure what the feature is meant for so maybe @kyranjamie can shed light on a good limit. |
@MarvinJanssen @neithanmo leather-io/extension#1051 I believe this issue prompted the ask of this feature. |
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)?.
for now X = 60. the election of this number considered the nanoS screen, so a message with this length would require around ~3 clicks. |
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? |
Although some upper bound surely is necessary still, lest dapps may try to send very large strings to be signed. |
@kyranjamie @markmhx is there anything pending here? |
we agreed on showing the complete message to users.. |
The potentially dangerous
sign_msg
function introduced in #82 should be removed. It is based on Ethereum'spersonal_sign
, the use of which is discouraged since the introduction of the much superior EIP712 standard. EIP712 addresses the many issues and shortcomings ofpersonal_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: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. Thesign_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
\x19
is a (Bitcoin)varint
representing the byte length of the prefix, which is correct forEthereum Signed Message:\n
as it's25
, but not forStacks Signed Message:\n
. It seems like it was copy and pasted thoughtlessly. It should have been\x18
.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.The text was updated successfully, but these errors were encountered: