-
Notifications
You must be signed in to change notification settings - Fork 65
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
Fix coincurve backend signature validation #62
Conversation
|
||
|
||
def coerce_low_s(value: int) -> int: | ||
return min(value, -value % SECPK1_N) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this probably deserves a docstring.
@@ -75,7 +78,8 @@ def ecdsa_verify(self, | |||
msg_hash: bytes, | |||
signature: BaseSignature, | |||
public_key: PublicKey) -> bool: | |||
der_encoded_signature = der.two_int_sequence_encoder(signature.r, signature.s) | |||
low_s = coerce_low_s(signature.s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably deserves a comment to indicate why it is necessary.
Going to wait until Monday to release this since I'm out of town over the weekend and we try to avoid cutting releases on Fridays in case anything un-expected happens. |
I ran both the py-evm fixtures and the failing Trinity test with this PR and everything seems to pass. So I think we can merge and release (would do it myself, but I don't have permissions in this repo). |
@jannikluhn v0.2.4 released with this merge! |
Use updated CircleCI images
What was wrong?
ECDSA signatures can be represented with a low or a high s: https://bitcoin.stackexchange.com/questions/83408/in-ecdsa-why-is-r-%E2%88%92s-mod-n-complementary-to-r-s
High-s signatures are forbidden in Ethereum, but only since Homestead, so
eth-keys
needs to support both forms. However, Coincurve rejects high-s signatures during verification.This error was introduced with the non recoverable signatures (prior to this we just recovered the signer and compared, so we didn't use the Coincurve signature verification function).
How was it fixed?
Convert an incoming high-s signature to its equivalent low-s representation before passing it to coincurve. Also, add some tests for this.
Cute Animal Picture