Signature Malleability:

[tbocek]

If you allow both values 0/1 and 27/28, you allow two different signatures both resulting in a same valid recovery. (r,s,0/1) and (r,s,27/28) would both be valid, recover the same public key and sign the same data. Furthermore, given (r,s,0/1), (r,s,27/28) can be constructed by anyone. In my contracts, I removed line 37 to line 39.

I further checked with geth, and in the transaction and ethapi, only 27/28 is considered. Thus, I added a function fixSignature() to convert 0/1 to 27/28 resulting from web3.eth.sign().

The current implementation opens attacks on contracts that rely where a signature is unique. This is for example ERC 865 as implemented in this pull request.

[frangio]

Hi @tbocek. Thanks for the report. I can see how this is a problem.

If I recall correctly accepting both 0/1 and 27/28 is very common, though. Have you reported this to web3 or anywhere else?

[tbocek]

@frangio I have not reported it elsewhere. I looked around and it seems to be a known issue in web3. I think the proper way could be to use web3.eth.accounts.sign (https://web3js.readthedocs.io/en/1.0/web3-eth-accounts.html#sign), but I have not tested it.

As far as I can tell, geth only supports 27/28.

[tinchoabbate]

Further supporting @tbocek , in the Ethereum's yellow paper, see Appendix F "Signing transactions", it is stated that among other conditions for an ECDSA signature to be valid, v must be 27 or 28.

[tbocek]

This bugs needs further fixing. After discussion in ERC865, there also needs to be a check that s needs to be lower than the half order as it is still vulnerable to signature malleability. In EIP-2 they state:

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.

That means ecrecover sees both signatures as valid, with lower s and upper s. So, my idea is the check for that as well and only allow lower s.

Alternatively, an other fix could be in the function recover() to not check and make sure that the signature is unique, but to return with the address in addition a unique signature (if s upper -> s=N-s, v+27 if v<27), that the caller of recover() can use, and knows its unique. However, I leaning towards checking only as this is seems less error prone as you don't have two kind of signatures in the application code.

[cwhinfrey]

@tbocek I was just going to mention that, as you said, the signatures are still malleable because both lower and upper s values are accepted. I have no strong opinions regarding accepting versions 0/1 but am in favor of allowing malleable signatures but possibly adding a warning for developers to never use them as unique identifiers as @yondonfu states in this post. https://yondon.blog/2019/01/01/how-not-to-use-ecdsa/

[tbocek]

@cwhinfrey I'm for either all or nothing. Fixing only 0/1 does not solve the problem of signature malleability. I now added a check to allow only s-values for lower than the half order in this pull request. This way a signature needs to be unique. That also means if you are using an outside signing library that creates non-unique signatures, the signatures need to be converted first.

I would rather add a warning, that recover() only accepts unique signatures, and if you have a library that creates values with 0/1 and s-values higher that the half order, either replace the order check with:

// Version of signature should be 27 or 28, but 0 and 1 are also possible versions
if (v < 27) {
    v += 27;
}

or convert the signatures from that library to a unique signature.

[nventuro]

Further supporting @tbocek , in the Ethereum's yellow paper, see Appendix F "Signing transactions", it is stated that among other conditions for an ECDSA signature to be valid, v must be 27 or 28.

Appendix F also mentions that the valid range for s is 0 < s < secp256k1n ÷ 2 + 1. This is IMO more than enough to restrict both the s and v values as done by @tbocek here.

[nventuro]

Thanks a lot @tbocek! Sorry for taking so long to review this, you caught me in the middle of my vacations.

Left a couple comments regarding style, etc., but overall the changes are quite simple - what's important is the research you carried out :) I can take care of getting this up to speed so you don't have to waste time reformatting, etc. - just let me know!

[cwhinfrey]

Appendix F also mentions that the valid range for s is 0 < s < secp256k1n ÷ 2 + 1. This is IMO more than enough to restrict both the s and v values as done by @tbocek here.

Happy with this going either way but I wanted to point out that I believe Appendix F is specifically talking about singing transactions. EIP-2 which introduced that restriction also says this:

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.

[nventuro]

I'd move for making all signing schemes as equal as possible though: it'd be very annoying if we accepted data that was signed in a way that wouldn't work for a regular transaction. Additionally, the proposed use case (recovering old BTC signatures) sounds so obscure that I'd expect whoever is working on that to implement it themselves, and wouldn't compromise on this point here to support it.

That said, I'm definitely not an expert in this matter and it is entirely possible that I'm misunderstanding the situation: if that's the case, please let me know.

[cwhinfrey]

Additionally, the proposed use case (recovering old BTC signatures) sounds so obscure

I was thinking the same thing. 😆 I think that all makes a lot of sense.

[tbocek]

I'm guessing it made sense to leave ecrecover() unchanged in EIP-2 (allowing both s) in Ethereum as changing it could potentially break smart contracts. There may be some obscure contracts relying on this feature. Since a unique signature can be enforced by smart contracts, I'm speculating that this was the reason to leave ecrecover() unchanged and rely on developers solving that in smart contracts.

Thus, I would like to see such an unique signature enforcement by a trusted library.

[chriseth]

Another reason was to be able to verify bitcoin signatures.

[nventuro]

I added a hardcoded test for a high-s value signature, but we should eventually work on those. Created #1667.

[stevenlcf]

@tbocek @nventuro it seems that go-ethereum's implementation is not internally consistent. Yes, ethapi.EcRecover() requires 27 or 28, but I found some examples in go-ethereum's latest version(v1.8.27) which are using 0 or 1 for V value:

• types.SignatureValues() (this was post by @tbocek as an example using 27 or 28 in the original issue description, yet it uses 0 or 1 now. The original link goes to master branch so only latest code is shown. I think go-ethereum might have changed the code later.)
• secp256k1.Sign(), which is used by crypto.Sign() and further used by types.SignTx()

Since the sign functions in go-ethereum still uses 0 or 1 for V, I think it would be better if we could keep both 0/1 and 27/28 for V for now. Otherwise, tx signed by the latest go-ethereum would get reverted by solidity code using the latest open-zeppelin lib.

[nventuro]

This is very interesting @stevenlcf, thank you for reporting this! From my understanding of the reply you received from the geth team here, we can still count on 27/28 being valid v values, since that's what the high-level interfaces return.

[tbocek]

@stevenlcf @nventuro. This interesting! I agree with @nventuro that when using high level signers, 27l28 should be used. Its unfortunate that they switched back to 0/1 internally, but I guess they had their reasons. Switching back means being vulnerable to signature malleability attacks for inexperienced Solidity users.

[ukstv]

Apparently, web3 1.0 and ethereumjs-util both export 0/1 instead of 27/28. Any ideas on how to handle that, in addition to go-ethereum?

[tbocek]

@ukstv web3 returns for getSignatureParameters 27/28. Also ethereumjs-utils adds 27.

What could be done, is to provide a function to canonicalize the signature (if s in upper half order-> s=N-s, v+27 if v<27). Then you can pass your signature to this function and always get a unique signature back. This would also work if you got a signature with 0/1.

[frangio]

@ukstv Can you share sample code that generates 0/1 signatures?

[ukstv]

@frangio https://github.com/ukstv/faulty-sig Hopefully, I did something wrong with the code. See https://github.com/ukstv/faulty-sig/blob/master/test/metacoin.js in conjunction with https://github.com/ukstv/faulty-sig/blob/master/contracts/MetaCoin.sol please.

[ukstv]

Ok, sorry. It is ganache’s fixed wrongdoing.

[mrwillis]

@ukstv Have you been able to figure out how to get this to work with ganache?. I currently can't get signatures to verify since 2.2

[frangio]

I believe this will be fixed in the upcoming Ganache 2.6.1.

[mrwillis]

@frangio Thanks. Yes just read this on ganache-core

[mrwillis]

@ukstv Did you get this to work? I still cannot verify signatures with ganache-cli@6.7.0-beta.0 which is ganache-core: 2.8.0-beta.0

[ytrezq]

How this is a problem if signature is not used as an identifier ?

[ukstv]

@mrwillis Yes, I did, through fromRpcSig of ethereumjs-util that happens to adjust v number. It ended up a helper properSignature: https://gist.github.com/ukstv/5bc79c4f6498cd9fa8fcae0d224ca68d

[tbocek]

How this is a problem if signature is not used as an identifier ?

Its not, its only a problem if the signature is used as an identifier.