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. |
|
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.
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_verifyif library panics.