AVM: Go19 ecdsa curve check#4917
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4917 +/- ##
==========================================
+ Coverage 54.06% 54.07% +0.01%
==========================================
Files 427 427
Lines 53520 53522 +2
==========================================
+ Hits 28935 28943 +8
+ Misses 22316 22314 -2
+ Partials 2269 2265 -4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Are we planning to release a new consensus go-algorand only for that? Do we suspect that there will be a valid signature the will fail on |
ahangsu
left a comment
ahangsu
left a comment
There was a problem hiding this comment.
Generally looks right to me, I agree with Idan's idea of saving one consensus param by && secp256r1.IsOnCurve(x, y), such that we can ensure the pk to be on curve without panicing in verify.
Only one question for Idan @algoidan
Do we suspect that there will be a valid signature the will fail on IsOnCurve?
Do you mean we want to check the signature on curve? (I think signature elements should be?)
|
looks good to me, I see you swapped the calls to SetBytes and PublicKey but they look independent also now its in the order of the argument list for Verify |
Now that there's a potential early failure for being off curve, I figured I'd defer the work to create the big ints unless the check passes. |
| Curve: elliptic.P256(), | ||
| X: x, | ||
| Y: y, | ||
| if !cx.Proto.EnablePrecheckECDSACurve || secp256r1.IsOnCurve(x, y) { |
There was a problem hiding this comment.
if !IsOnCurve() shouldn't that be an error? Or is it just result = false not verified?
There was a problem hiding this comment.
I think there's an argument for making it an error, but we did not before, and I wanted the change to be as small as possible. Do you think there are use cases that need to distinguish an error like this from failing to verify?
There was a problem hiding this comment.
I will think it is somewhat sus to make a different case of erroring and failing to verify... feeling like giving a larger attack surface against verifier?
https://www.reddit.com/r/cryptography/comments/qyg723/what_happens_when_a_public_key_point_does_not_lie/ Makes me recall something like verifier rejection attack (replay attack) and invalid curve attack...
There was a problem hiding this comment.
I think this patch improves the code.
bbroder-algo
left a comment
bbroder-algo
left a comment
There was a problem hiding this comment.
Returns false on verifications before performing ECDSA verification for Secp256r1 curves if x, y components of public keys are not on the curve to begin with.
cce
left a comment
cce
left a comment
There was a problem hiding this comment.
Bringing back some more of the context as I recall it from when this PR was opened
Go 1.19 release notes: https://go.dev/doc/go1.19
crypto/elliptic
Operating on invalid curve points (those for which the IsOnCurve method returns false, and which are never returned by Unmarshal or by a Curve method operating on a valid point) has always been undefined behavior and can lead to key recovery attacks. If an invalid point is supplied to Marshal, MarshalCompressed, Add, Double, or ScalarMult, they will now panic.
Go issue + CL where the panic was introduced golang/go#50975
7 participants
Go 1.19 has changed the semantics of ecdsa.Verify to panic if the public key is not on the appropriate curve. This change prepares for supporting Go 1.19 by first establishing a new rule for the AVM opcode ecdsa_verify. By doing this in a consensus upgrade while still using Go 1.17, we force the change to occur across all nodes at once, and we ensure no changes occur in evaluating old blocks.