Skip to content
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

Merged
merged 3 commits into from
Jan 6, 2023

Conversation

jannotti
Copy link
Contributor

@jannotti jannotti commented Aug 7, 2022

Return false from ecdsa_verify if library panics.

@codecov
Copy link

codecov bot commented Aug 7, 2022

Codecov Report

Merging #4368 (d7c87d7) into master (00b37f2) will increase coverage by 0.01%.
The diff coverage is 86.36%.

@@            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     
Impacted Files Coverage Δ
data/transactions/logic/eval.go 89.45% <86.36%> (-0.10%) ⬇️
agreement/cryptoVerifier.go 67.60% <0.00%> (-2.12%) ⬇️
agreement/proposalManager.go 96.07% <0.00%> (-1.97%) ⬇️
catchup/peerSelector.go 98.95% <0.00%> (-1.05%) ⬇️
cmd/tealdbg/debugger.go 72.69% <0.00%> (-0.81%) ⬇️
ledger/acctupdates.go 69.59% <0.00%> (-0.60%) ⬇️
catchup/service.go 69.38% <0.00%> (ø)
network/wsNetwork.go 64.82% <0.00%> (+0.19%) ⬆️
network/wsPeer.go 68.21% <0.00%> (+2.73%) ⬆️
ledger/tracker.go 77.87% <0.00%> (+2.97%) ⬆️
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jannotti jannotti marked this pull request as ready for review August 7, 2022 15:51
@jannotti jannotti requested a review from algorandskiy August 7, 2022 15:51
@michaeldiamant
Copy link
Contributor

For more complete context, here's the commit changing Go behavior: golang/go@a218b35.

Co-authored-by: Michael Diamant <[email protected]>
@cce
Copy link
Contributor

cce commented Sep 6, 2022

Invalid points would have led to false before too, right?

@jannotti
Copy link
Contributor Author

jannotti commented Sep 6, 2022

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 0 instead of a guaranteed program failure.

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 {
Copy link
Contributor

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.

@algorandskiy algorandskiy merged commit 19fa6c2 into algorand:master Jan 6, 2023
jannotti added a commit to jannotti/go-algorand that referenced this pull request Jan 9, 2023
jannotti added a commit that referenced this pull request Jan 9, 2023
winder pushed a commit to winder/go-algorand that referenced this pull request Jan 18, 2023
winder pushed a commit to winder/go-algorand that referenced this pull request Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants