Security Notice improvement in ECDSA lib

SteMak commented 3 months ago

Motivation

The following security notice is incorrect

     * IMPORTANT: `hash` _must_ be the result of a hash operation for the
     * verification to be secure: it is possible to craft signatures that
     * recover to arbitrary addresses for non-hashed data. ...

Details

Actually the security issue arises from the fact that it is possible to generate a valid bytes32 + signature pair which will recover to a given address if you have another valid bytes32 + signature pair for that address The concern doesn't depend on if the bytes32 pair part is hash or data

However, due to the nature of new bytes32 + signature pair generation, the new bytes32 pair part is quite random

In case your smart contract process the bytes32 pair part as data, it may be interpreted somehow even for quite random set of bytes
In case your smart contract process the bytes32 pair part as hash, it would be quite hard to find some data corresponding to such a random hash

Conclusion

The security notice notice should be adjusted to not mislead developers

For example:

     * IMPORTANT: `hash` _must_ be the result of a hash operation for the
     * verification to be secure: it is possible to craft `data + signature`
     * pairs such that recover to a given address with quite random `data`  
     * part. While it is quite hard to find message such that
     * `hash(message) = data`, the data itself may be interpreted as valid 
     * message if the hashing stage was omitted. ...

I'm not sure that it is clear enough, though, opening the discussion

ernestognw commented 3 months ago

Hi @SteMak, thanks for submitting.

The note dates from 5 years ago. It seems that it lost context at some point when we split the ECDSA library from the MessageHashUtils so I agree it's confusing.

If I'm not mistaken, the note's intention is to warn a user from verifying raw hashed messages. The general recommendation is to use along with MessageHashUtils.toEthSignedMessage for Ethereum messages and MessageHashUtils.toTypedDataHash so that the prefix disambiguate the kind of message from any RLP encoded transaction.

A good example is @spalladino's submission to the Solidity Underhanded Contest in 2022. It shows how a non-standard message structure could lead to tricking a user into signing a message that can be reinterpreted maliciously. Here's his explainer thread.

What do you think about the following formulation?

* IMPORTANT: Consider using {MessageHashUtils-toEthSignedMessageHash} (or similar)
* to generate the verification `hash`. Most wallet providers append https://eips.ethereum.org/EIPS/eip-191[ERC-191] 
* prefixes to different types of messages to ensure there are no encoding collisions 
* between signed messages and signed RLP-encoded transactions.

SteMak commented 3 months ago

Hi @ernestognw, nice to see your response here.

You're right about the recommendation to use MessageHashUtils.toTypedDataHash or MessageHashUtils.toEthSignedMessage. You're right about lack of signed message prefix may cause collision with RLP encoded transactions and usage of techniques above saves users from replay attacks.

However, this doesn't cover the point I've mentioned: usage of totally unhashed messages that fit 32-bytes. As it described in this article, ECDSA signatures are malleable (not only reversing v and s values to get valid signature for same message): you can generate new bytes32 + signature pair from the old one and it will point to the same signer but the bytes32 would be quite random.

In case an SC processed this bytes32 as raw message (for example, uint256 airdropValue) without hashing, it is possible for anyone to generate unlimited amount of such signed messages and ask the SC to process them.
If an SC processed this bytes32 as hash of some data, attacker is required to find data under given hash what is quite hard for keccak256.

That CTF explanation involves the that unlimited valid signatures generation.

Though, yes, users need to hash their messages as signature malleability protection and should use prefixes for replay protection. I think the security notice in the ECDSA lib should be strict as much as possible about need of message hashing as it is not a lot of public non-math-friendy signature malleability explanations.

P.S. I mean, let's keep the word _must_ in the notice)

OpenZeppelin / openzeppelin-contracts