-
Notifications
You must be signed in to change notification settings - Fork 493
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
AVM: Catch any panic in edcsa verifying #4368
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4368 +/- ##
==========================================
+ Coverage 55.28% 55.30% +0.01%
==========================================
Files 398 398
Lines 50336 50340 +4
==========================================
+ Hits 27829 27839 +10
+ Misses 20178 20173 -5
+ Partials 2329 2328 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
For more complete context, here's the commit changing Go behavior: golang/go@a218b35. |
Co-authored-by: Michael Diamant <[email protected]>
Invalid points would have led to false before too, right? |
I sure hope so! I am a little worried about this fix, because it assume nothing used to panic. If there was something that caused a panic before, that thing would now be caught and cause I have done an alternate that explicitly tests for being on the curve instead. Maybe that's better? |
func (cx *EvalContext) ecdsaVerify(curve EcdsaCurve, pkX, pkY []byte, msg []byte, sigR, sigS []byte) (result bool) { | ||
// Go 1.19 panics on bad inputs. Catch it so that re can return false cleanly. | ||
defer func() { | ||
if recover() != nil { |
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.
Leaving a note for when we return to the PR: I'm open to discussion, but initial my preference is to catch the specific failure condition rather than the catch-all presented by the PR.
Additionally - I opened an issue to track upgrading to Go v1.19 with a checklist item referencing this PR: #4759. Intent is to confirm we finalize the PR prior to completing a v1.19 upgrade.
This reverts commit 19fa6c2.
…orand#4985) This reverts commit 19fa6c2.
Return false from
ecdsa_verify
if library panics.