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

Add multi-range check to assertAlmostReduced #1703

Merged
merged 3 commits into from
Jun 27, 2024

Conversation

MartinMinkov
Copy link
Contributor

@MartinMinkov MartinMinkov commented Jun 25, 2024

Summary

Addressing an issue with EcsdaSignature and ForeignCurve overriding check() to use assertAlmostReduced. This skips the option to multi-range check all the input variables, which can lead to passing assertions if a malicious prover wishes to do so.

Changes

  • Added a range check on inputs within the check functions of EcdsaSignature and ForeignCurve
  • Update the regression verification key artifacts

@MartinMinkov MartinMinkov force-pushed the fix/check-ecdsa-foreignfield branch 2 times, most recently from 7ba0cee to 5e07eb9 Compare June 25, 2024 23:11
@MartinMinkov MartinMinkov marked this pull request as ready for review June 25, 2024 23:11
@MartinMinkov MartinMinkov force-pushed the fix/check-ecdsa-foreignfield branch 2 times, most recently from 9a672dc to e79912e Compare June 26, 2024 00:57
Copy link
Member

@mitschabaude mitschabaude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MartinMinkov the change looks good to me now, but it's breaking and we should ship it in a backwards-compatible way. check() is called internally just by using the overall class as method input. So it's the class that we have to create an alternative version for.

How about this:

  • Leave ForeignCurve class untouched
  • New ForeignCurveV2 which extends ForeignCurve and just overrides the check() method
  • new createForeignCurveV2() which is exactly like createForeignCurve() but uses ForeignCurveV2
  • deprecate createForeignCurve(), move doccomments over to createForeignCurveV2()

And same for EcdsaSignature!

@MartinMinkov MartinMinkov force-pushed the fix/check-ecdsa-foreignfield branch 2 times, most recently from a7d29b2 to 60f70cb Compare June 26, 2024 18:56
Copy link
Member

@mitschabaude mitschabaude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect!

@MartinMinkov MartinMinkov merged commit 72b8e2c into main Jun 27, 2024
14 checks passed
@MartinMinkov MartinMinkov deleted the fix/check-ecdsa-foreignfield branch June 27, 2024 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants