search

ethereum / eth-keys

A common API for Ethereum key operations.
MIT License
161 stars 64 forks source link

Validate s value in signature #75

Closed carver closed 3 years ago

carver commented 3 years ago

What was wrong?

It appears that s is validated against secpk1_n but should be validated against s < secpk1_n / 2 + 1.

How can it be fixed?

  1. Verify our understanding of the yellow paper
  2. Add a test with an s == secpk1_n / 2 + 1 that should fail validation, for the test to pass.
  3. Add another test with s == secpk1_n // 2 + 1 that should succeed validation.
  4. Validate r and s separately, since they have different constraints. Instead of validating them the same way:

https://github.com/ethereum/eth-keys/blob/dd4f00a5d2f2b394665ccecc9817f753e58cc7bc/eth_keys/validation.py#L108-L111

kclowes commented 3 years ago

After looking into this some, it looks like disallowing s > secpk1_n / 2 + 1 from transaction signatures was changed in the Homestead hard fork. The ecrecover precompile should still accept s-values greater than secp256k1n / 2 + 1.

EIP-2 (the Homestead hard fork EIP) states:

All transaction signatures whose s-value is greater than secp256k1n/2 are now considered invalid. The ECDSA recover precompiled contract remains unchanged and will keep accepting high s-values; this is useful e.g. if a contract recovers old Bitcoin signatures.

Further elaboration in EIP-2:

Allowing transactions with any s value with 0 < s < secp256k1n, as is currently the case, opens a transaction malleability concern, as one can take any transaction, flip the s value from s to secp256k1n - s, flip the v value (27 -> 28, 28 -> 27), and the resulting signature would still be valid. This is not a serious security flaw, especially since Ethereum uses addresses and not transaction hashes as the input to an ether value transfer or other transaction, but it nevertheless creates a UI inconvenience as an attacker can cause the transaction that gets confirmed in a block to have a different hash from the transaction that any user sends, interfering with user interfaces that use transaction hashes as tracking IDs. Preventing high s values removes this problem.

carver commented 3 years ago

Thanks for hunting this down.

kclowes commented 3 years ago

I think this should stay open because this library is used outside of the context of the ecrecover precompile, right?

carver commented 3 years ago

Hm. So maybe you're arguing that there should be an API specifically for signatures that follow the ethereum transaction signature rules?

My presumption was that the normal, basic rule is correct for this base library. Then tighter rules, like the transaction signature, should be applied at other layers (like eth-account and/or py-evm).

kclowes commented 3 years ago

Ah, okay. Applying at higher levels makes sense to me!