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

Fix Int64 sign checks and mod #1660

Merged
merged 10 commits into from
May 28, 2024
Merged

Fix Int64 sign checks and mod #1660

merged 10 commits into from
May 28, 2024

Conversation

mitschabaude
Copy link
Collaborator

@mitschabaude mitschabaude commented May 20, 2024

Security fixes on Int64 which are enabled when switching to "v2" versions of some methods.

The problem that this attempts to solve:

Int64s represent signed integers as a sign: Sign and magnitude: UInt64. This allows for many efficient implementations. However, it also means that 0 has two representations: +0 and -0.
This leads to ambiguity in the isPositive() function, which ignores the magnitude.
[...]
A malicious prover may choose to use either positive or negative zero.
[...]
For example, Int64.mod() uses the expression Provable.if(this.isPositive(), rest, y_.value.sub(rest)) to select between rest and y_ - rest when computing the remainder modulo y_. By choosing this to -0, an attacker may output y_ instead of 0.

  • isPositive() is deprecated, because it is both misleadingly named and incorrect
    • returns true for +0 and false for -0
    • depending on whether you interpret it as "> 0" or ">= 0", that result is wrong for either +0 or for -0.
    • the new isPositiveV2() avoids the ambiguity by checking "> 0", returns false for all versions of 0
  • mod() is deprecated, because it is wrong on -0 inputs
    • modV2() is correct
  • There's also a new "v2" version of check(), which fixes the root of the problem: That 0 can be represented in two different ways. It can only be enabled when upgrading to v2, because it breaks many circuits

* Negates the value.
*
* `Int64.from(5).neg()` will turn into `Int64.from(-5)`
* @deprecated Use {@link negV2()} instead.
Copy link
Member

Choose a reason for hiding this comment

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

should we add this part to the changelog as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok we can do that

@mitschabaude mitschabaude merged commit 0049cf1 into main May 28, 2024
14 checks passed
@mitschabaude mitschabaude deleted the fix/is-positive branch May 28, 2024 14:31
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.

2 participants