-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
WIP: eth_signPrefixedData - prefixed data signing #683
Conversation
I'm strongly against this strategy in general (prefixing signed messages), as I have expressed in Parity and Geth GitHub issues. This feels like a hack solution that results in end-user confusion and frustration to solve a much more serious and real problem which is people accessing untrusted dApps with unlocked accounts or signing arbitrary opaque messages. The reason this causes problems is because users sign the message and then expect to be able to verify the message signature against the message and public key with any ECDSA tool. However, with this EIP the thing the user requested to be signed is not the thing that is actually signed which means it will not verify unless the user happens to know the implementation details specified in this EIP. I think that Geth should follow Parity's lead and build a signing UI so unlocking accounts isn't a common activity. Both should also follow MetaMask's lead and give users a big warning when arbitrary messages are being signed. It is not uncommon to sign a hash of some data, so I think there would be value in adding an API that takes a large byte array and a content type (e.g., utf-8) and hashes it in the node then signs it. This will allow signing tools to display meaningful details about what is being signed to the user. |
Then it is good that we discuss this here and I submitted a WIP and did not finalized/polished the whole thing and then a discussion arises that changes everything ..-) |
I don't see any reason why not to sign using a standardized prefix. I will wait for @MicahZoltu to provide links to Geth/Parity issues, but I really doubt these will bring any fundamental arguments to the table. As for hashing the data before signing - this is fine, but still you can (and should!) prepend the hash-digest with the prefix.
I don't agree. Users expect things to just work and this standard is trying to make things secure and unified at the same time. |
Average users will never themselves use an ECDSA tool, since they will have no clue what signing even means. If they are developers building on top, then they can be expected to understand the security implications and also to implement a simple prefix. |
Warning users about signing arbitrary messages doesn't make a dent in being more secure. It's the same as with "terms of use" when installing some applications. Who even reads those? I want the app, I don't care about the terms. It's a horrible approach, yet it is the one people live by. If the user wants to use a DApp, which needs to sign something, the user will sign it irrelevant of the contents. Maybe the DApp is even benevolent for now. All it takes is one future update with a malicious hack to strip users of their funds. Telling users that it's their fault for trusting a malicious DApp while deliberately not implementing a known, clean, easy solution to prevent security issues is negligent at best and borderline criminal at worst. |
The prefix inconsistencies across clients is really bad, and if this solves it, that's great news to me, and I'm in favor of it. I'm not looking to re-hash the prefix debate, I was comfortably persuaded in previous threads, and I have not seen a use case that couldn't work fine with a prefix, ie. end-users don't use ECDSA tools directly. MetaMask's warning on eth-sign was always intended to be a deprecation warning, a way to scare users and devs away from that method until we remove it, but I just saw a new project using it for registration (0x protocol), so we might need to just aggressively remove it, to avoid being borderline criminals. |
OK I think I can go forward - unfortunately not (yet) unanimous - but I think with a good majority in favor going forward - thanks @prusnak , @karalabe , @FlySwatter for your input. Also thanks for raising the concern @MicahZoltu - hope we find a way to also get you on board. An Idea I just had writing this : we could formalize the signing output in this also - not only the input. My suggestion would be:
and to reference this method we could use: method_name=ERC683 (or what ever number we end up with in this PR) EDIT: thinking about this a bit more - that way we could also make a standard for common claims - but I would love them to have the format of a processable URL and I think a hash there would be better to not have a variable length and the message body. There could then be a set of common claim-hashes on chain - perhaps in a contract. |
I'll try to respond more in depth later (busy at the moment) but I wanted to get this out there: If a user is signing whatever the dApp asks them to sign, they are already compromised if the dApp is malicious. This prefix doesn't change anything. The attacker merely needs to ask them to sign a transaction to send all of their ETH, or all of their tokens, or participate in some smart contract, whatever. The same is true if the account is unlocked. My argument centers around the fact that this behavior is not solving any problems, but it is creating annoyances and inconveniences and complicating the spec unnecessarily. To get on board, I would like to be convinced that an attack vector exists where a dumb user that signs everything or runs Geth with an unlocked account and interacts with untrusted dApps is protected by this behavior. All of the arguments I have heard for this behavior center around it protecting users from themselves, but I haven't seen compelling evidence that that is the case. At best it changes the attack vector slightly, though realistically I think going after the user's funds directly is easier. The reason this proposal makes sense in Bitcoin is because Bitcoin transactions have historically only been value transfers. Bitcoin doesn't have smart contracts and users are generally not signing anything meaningful other than value transfers. |
but this is the very thing this EIP is trying to address. If we prefix: the attacker cannot just merely ask to sign a transaction - we could even protect him more by specifying a minimum prefix length to avoid collision attacks. There can also be common prefixes like "Ethereum Signed Message:" - with this one I think the chance of a collision is close enough to zero. Glad that this whole problem is based on a miss understanding as it seems! But I gladly delay the thing a bit more until you are not busy anymore - I am out with my bicycle and no laptop on the weekend anyway - I am also not really in a hurry with this - just want to have it specified before I implement yet another incompatible way of this use-case with WALLETH ..-) |
@ligi I believe you may have misunderstood the point I was attempting to make. My point is that if someone is running an untrusted dApp with an unlocked account they are already compromised, even if this endpoint didn't exist at all. Similarly, if someone is signing anything the dApp throws at them (e.g., transactions) they are similarly already compromised, even if this endpoint didn't exist at all. This proposal is trying to protect this one endpoint from an attack vector (users signing things for untrusted dApps) that is a systemic problem. "Protecting" this single endpoint doesn't close any attack vectors because those same users will still be able to sign transactions that spend all of their money, maliciously execute contracts, add owners to multisigs, etc. Entertaining AnalogyIts like if you have a giant gaping hole in a boat and you hold your fist in the middle of it and claim that you are fixing a problem. Unless you plug the entire hole, the ship will sink. You can't just patch a tiny portion of a giant gaping hole and claim that you are helping, especially when the useless act of standing there with your fist in the middle of the flood of incoming water is wasting manpower that could be doing other things. |
If people insist on message prefixing then I would strongly prefer that instead this method returns an error if the caller doesn't provide a properly prefixed string. This way, the caller at least will know very clearly what the requirements are so that they can later validate. The following set of calls should not fail in any solution:
This proposal will make the above validation fail. Instead the author must know that instead they need to do:
I propose that if we insist on following through with disallowing signing of arbitrary messages then it should instead be:
This not only makes it clear to future developers what they must do in order to get things signed, but it also makes it so the sign and validate are symmetric. |
can you elaborate on this? |
I like this proposal, but please do provide some examples in the spec. |
Great! I will provide examples once the discussion is finished |
I hope to create a test for this spec on cdetrio's compat table, (PR Here), I think this is an excellent application of that test suite, considering it's easy input/output that is byte-specific.
If our attack vector is "people who will sign anything a dapp throws at them", then blockchains themselves are probably hopeless, but I don't believe in this mythical "signs-anything" person, (although I am aware of geth's unlocked mode, and have long been an advocate of avoiding it). Isolating tx signatures from other signatures both creates a baseline of anti-tx-phishing, by allowing proposed TXs to be displayed accurately, and parsed for user consideration, and also avoids chosen-ciphertext attacks, which can leak private key material. This is covered in the other prefix discussions. On non-breaking APIsWe've seen at least 3 different signature formats now, and each time they break real applications. I'd love to see new signature formats also propose a versioning scheme. Previous methods have been:
Maybe a third optional parameter can be introduced, specifying the signature format: |
I'm not sure which part in particular you would like elaboration on, but I'll give it a shot. As a user, if I am running an unlocked Geth node and I navigate to a malicious dApp, the dApp can submit transactions on my behalf. This means that it can send all of my ETH away, take ownership of any smart wallets I may have, transfer tokens, etc. The fact that it can also sign a message is rather inconsequential because it can already wipe me out completely and burn the address entirely. The same goes for a user that isn't running with an unlocked account but who clicks accept to any signer pop-up that appears from Parity/MetaMask signers. If the user navigates to a malicious dApp and the dApp asks them to sign a request to transfer all of their tokens, their ETH, transfer ownership of wallets, etc. and the user blindly clicks OK... they are wiped out. The fact that the dApp can also get them to sign arbitrary messages is once again inconsequential because the account has already been burned. This is all to say that the prefixing thing doesn't actually solve any real problems. It solves a tiny subset of a giant gaping hole of a real problem and solving only one subset doesn't help solve the larger problem (this isn't a thing where you can iteratively solve the larger problem by addressing the individual problems. |
Chosen-ciphertext attacks are only relevant if the user is signing arbitrary data presented by a dApp, which as you asserted in the paragraph prior isn't something we are trying to defend against. Either we are trying to protect people from signing anything presented, in which case my argument that this doesn't help stands, or we aren't in which case this proposal is pointless because it isn't trying to solve anything. My argument is basically that there is not a real attack vector that this actually protects against.
I'm not sure I follow on how this is related to this PR? I definitely think that the more info that can be presented to a user when they are prompted to sign is better and I would advocate strongly for adding an API call for "Sign string" which presents the exact string being signed to the user and signs exactly that. This is the most common use case I have seen of Counter ProposalPerhaps something that would be more appealing to everyone here is if instead of this, we deprecate this method entirely and add a new method like: We could have other signing methods for other data types ( This would also solve the secondary problem of semi-trusted dApps needing to present the user with opaque data to sign when they really want a string signed. |
@FlySwatter Can you describe how you think that this EIP helps "People we can help"? Can you describe a hypothetical attack that this protects them against? |
No, actually, this EIP is a pretty simple byte-specification, which is very needed, and you just re-hashed the prefixing debate. The prefixing distinction does help us parse transactions and make them more reviewable to people, though, but
|
@FlySwatter I believe you are misinterpreting this EIP. This EIP is the official proposal to do message prefixing. Previously, Geth went off on its own and implemented it without doing an EIP or getting feedback from the community. Then Parity followed suit, because they strive for compatibility. This EIP is someone finally deciding to "do things right" which is the first time the community (i.e., me) has had an opportunity to voice a concern without being shutdown with "sorry, that is the way it already is". I feel like if we push this through because "Geth already implemented it without an EIP and Parity followed without an EIP" that encourages Geth and Parity to use that as a strategy of getting contentious changes forced through the EIP process (implement first, propose EIP second, get it pushed through despite negative feedback). Now that this is an EIP I believe it is the appropriate time to have the prefix debate which previously was hidden away from the eyes of most of the community burried in a Geth issue. |
I think Geth and Parity have already made clear they are happy implementing RPC methods without the EIP process, and treating this as an opportunity to "do it right" is going to slow down this useful binary spec without actually holding any sway over those clients. I think the case for prefixing has been made just fine both in the original geth issue and in this thread.
Sure, we'll need legible message signing as more contracts might use custom signature formats to authorize internal transactions, but that's no reason to not do what we can in the meanwhile. |
If Geth/Parity assert that the "we should prefix messages" part of this EIP is just a formality and they will not change their behavior even if the community believes the current behavior is bad, then I agree that debating on what is right is pointless and we should not let it detract from the discussion about the binary encoding (though, I do worry about what that would say about the ecosystem if a small group of people can push changes through via discussions in the closet). I would like to get someone from Geth and Parity to weigh in on that before I give up on arguing for what I believe is best for the ecosystem.
If we do continue the discussion, my argument is that neither of those things is true. :) The prefix doesn't improve user-legible TX approval in any way and in fact if you check out my counter proposal above I argue that we should change this method so that it is more user-legible. The chosen-ciphertext protection this EIP offers only protects the subset of people that you asserted are a lost cause, and since I agree that they are a lost cause I think that is not a valid argument for keeping this EIP as is. However, lets wait until Parity/Geth weigh in and assert whether or not they will change their behavior in the face of further discussion before we continue more down this path. |
I would be more than happy to introduce only RPC changes introduced after a proper EIP process. We try to keep official namespaces ( Regarding this issue, while I think the prefixing is needed (I do review standard transactions and knowing that signing arbitrary data won't trigger a transaction makes me feel safe about signing it) I think Micah's point is valid - it's difficult to verify the signature without actually reimplementing the prefixing process. Maybe this could be solved by responding with not only a signature but also a hash (or a message) that was actually signed? {
"hash": "0x..",
"signature": "0x.."
} |
@tomusdrw If the only way to get a node to sign a transaction is with the prefix, then any user who participates in any kind of state-channel or off-chain transaction mechanism (e.g., EtherDelta) will still be at risk of a dApp having them sign something that could result in losses to them. I believe that your assumption that "because its prefixed its safe" is a perfect example of how this technique results in a false sense of security and should be avoided. To give a scenario for the attack I'm imagining against you @tomusdrw: Imagine if you have an open state channel with someone with a lot of money on the line. You trust the mechanism behind the state channel though so this is a trustless interaction. You then open a dApp that asks you to sign some hash and it tells you that you are signing something innocuous and because you know about the prefixing you think to yourself, "what's the worst that could happen, they could pretend to be me on the internet?" so you sign it. However, you forgot that because the only way to sign anything with a node is with the prefix, this means that the state channel you have open with all that money in it also expects state channel updates to be prefixed with the magic string. You later find out that the attacker did a mutual close of the state channel such that you gave up all of your assets. What the above shows is that Ethereum transactions are only one mechanism for transacting on the Ethereum network. State channels are likely to become more and more common with time as they solve some hard scaling problems and we can already see baby channels in the form of EtherDelta and 0x. Because of this, anything we do to mangle the thing being signed will not have any positive security effect on off-chain transactions. However, it will give people (such as yourself) a false sense of security. I would not be surprised if over the next couple of years we see the majority of Ethereum transactions move into state channels. If this happens then I would argue that this PR actually introduces security risks due to the false sense of security it provides. |
@MicahZoltu Thanks for the comment, that's a very valid point. EDIT: |
@tomusdrw Can you describe the attack vector that you believe this EIP solves? I have listed counter arguments that explain why I don't believe this actually solves the attack vectors people have claimed, but so far anytime I ask for an example real attack scenario that this protects against I only get vague answers like "chosen-cipher" and "signed transactions", even though as I have argued above this doesn't really protect against either of those in any real scenario (only vague hypotheticals). |
@MicahZoltu Let me try with the following reasoning:
|
@rmeissner: I suggest you read the issue linked in the first post for reasoning why varint is so far the best. If you have a better idea, don't hesitate to share, but AFAIAC there has not been a better proposal. |
Are you talking about ethereum/go-ethereum#14794? I do understand why it should be varint instead of just the number, but why do we need the length of the message at all. Also you could just use an encoded uint256 since varint also doesn't support anything close to the max data length that is possible on ethereum. (But I know that varint length should be enough for anything done on ethereum right now :D ) |
@prusnak I am sorry for this discussion 😄 I just wanted to understand the all the details, therefore my question about the message length part. If we understand why it is necessary, it is easier to explain to others why changes need to be mad. Currently I couldn't answer when my peers asked me "Why do we need the message length there". |
Most probably you don't.
|
@prusnak TBH I think he might be onto something. We can calculate the the message-length from the total length minus the prefix length. We would need he message length if there is more content in this package - but there is not. |
Think I will drop the message-length then - If somebody (especially looking in your direction @prusnak) comes up whit a case where we cannot calculate it from total-prefix then please speak up - otherwise I will just remove it as this makes it easier/cheaper for smart-contracts. |
I would suggest asking on Bitcoin-Dev mailinglist why they did it that
way.
If you want to drop message length, maybe you could drop also prefix
length as it is fixed anyway.
|
thanks will do - glad to know it is nothing obvious I overlooked there ..-)
no - I really want variable prefix length to separate different use-cases. That said - thinking about simplifying things one could say we limit the length so we need no varint - I don't yet see use-cases for huge prefixes. |
via https://bitcoin.stackexchange.com/questions/68844/explicit-message-length-in-bitcoin-signed-message So I think we can safely drop this "quirk" - good that we talked about this @rmeissner @prusnak |
@ligi do we want to push this, so that it is possible to make use of signed messages with ledger and trezor in a simple way again 😄 |
@rmeissner I wanted to let thins cool down and see how #712 develops - but I will now push this again. I think we can combine #712 with this one |
as discussed in the github PR: this is not really needed was just there for historical reasons (Bitcoin did it) also minor formattin
I just wanted to (add a useless) comment: this discussion was/is awesome. You guys are great! |
An alternative to this is #191. 191 has the advantage that the existing 'Ethereum signed data' personal signatures can be grandfathered in as a valid format, as well as specifying a very simple standard for signatures that can be verified by smart contracts without them having to do string parsing or large comparisons. |
This is a courtesy notice to let you know that the format for EIPs has been modified slightly. If you want your draft merged, you will need to make some small changes to how your EIP is formatted:
If your PR is editing an existing EIP rather than creating a new one, this has already been done for you, and you need only rebase your PR. In addition, a continuous build has been setup, which will check your PR against the rules for EIP formatting automatically once you update your PR. This build ensures all required headers are present, as well as performing a number of other checks. Please rebase your PR against the latest master, and edit your PR to use the above format for frontmatter. For convenience, here's a sample header you can copy and adapt:
|
@ligi is this PR still relevant? I think the problem has been addressed by multiple changes over the last year and a half. |
dropping this in favor of 721 and 191 |
@ligi If u don't mind, can u give me an example of this?, cuz I still can't deeply understand why prefixing makes avoid collision attacks.. as far as I know, prefixing is to ensure by signing the hash of a string that begins with %%PREFIX%%, I don't have to worry about it being a transaction am I right? |
Correct. It ensures that what your signing isn't a transaction. However, it doesn't guarantee that what you are signing doesn't collide with something else that is signed with the same prefix. |
@MicahZoltu Thanks for reply :) just one more question If u don't mind :) how could just adding %%PREFIX%% to message makes transaction invalid? what I've found is that '\x19' <- this makes the transaction invalid, but as far as I know, its just RLP encode expression. |
Legacy transactions are RLP encoded lists, which means the first by is always in the range |
@MicahZoltu so much thanks for kind explanation.. I would never find out by myself without your help lol.. |
As discussed in ethereum/go-ethereum#14794
cc @prusnak, @karalabe