Fix: Prevent underconstraint in xor(), and(), and not() functions #1745
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MartinMinkov
force-pushed
the
audit/bound-check-pad-length
branch
from
2430572 to
ab43852
Compare
src/lib/provable/gadgets/bitwise.ts
Outdated
| length < Field.sizeInBits, | ||
| `Length ${length} exceeds maximum of ${Field.sizeInBits} bits.` | ||
| ); | ||
| validateBitLength(length, Field.sizeInBits, 'not'); |
There was a problem hiding this comment.
not() also pads the length so let's do 240 here as well
src/lib/provable/gadgets/bitwise.ts
Outdated
| length <= Field.sizeInBits, | ||
| `Length ${length} exceeds maximum of ${Field.sizeInBits} bits.` | ||
| ); | ||
| validateBitLength(length, Field.sizeInBits, 'and'); |
There was a problem hiding this comment.
and() also pads the length so let's do 240 here as well
not necessary IMO |
MartinMinkov
force-pushed
the
audit/bound-check-pad-length
branch
from
5d34ad4 to
8fe7149
Compare
MartinMinkov
force-pushed
the
audit/bound-check-pad-length
branch
from
8fe7149 to
aeedff1
Compare
Trivo25
approved these changes
Jul 18, 2024
ymekuria
approved these changes
Jul 18, 2024
| @@ -343,3 +326,17 @@ function leftShift32(field: Field, bits: number) { | |||
| let { remainder: shifted } = divMod32(field.mul(1n << BigInt(bits))); | |||
| return shifted; | |||
| } | |||
|
|
|||
| function validateBitLength( | |||
…e function to reduce code duplication refactor(bitwise): reduce maximum bit length for xor operation from 254 to 240 bits to improve performance and simplify implementation docs(bitwise): improve error messages for bit length validation to provide more context and clarity on the cause of the error
…40 in validateBitLength calls The hardcoded value of 240 is now used instead of Field.sizeInBits when calling the validateBitLength function in the not and and functions. This change was made to avoid relying on the Field.sizeInBits property and instead use a fixed value for the maximum bit length validation. Using a hardcoded value provides more explicit control over the allowed bit length and reduces the dependency on the Field class.
…r, notUnchecked, notChecked methods and error message The bit size was reduced from 254 to 240 in multiple places within the Bitwise ZkProgram and the corresponding test case. This change was likely made to optimize performance or to align with some specific requirements of the application. By using a smaller bit size, the computations and proofs involving bitwise operations will be more efficient in terms of time and space complexity.
…nd operations from 254 to 240 bits to improve performance and simplify implementation
MartinMinkov
force-pushed
the
audit/bound-check-pad-length
branch
from
aeedff1 to
d840bd9
Compare
…bit length at 240 bits in not(), xor(), and and() functions to prevent potential underconstraint issues in the circuit docs(bitwise): add JSDoc comments for validateBitLength() function to describe its purpose, parameters, and thrown error
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This PR addresses a potential overflow vulnerability in the bitwise operations (xor, and, not) as identified in a recent audit. The changes reduce the maximum allowed bit length from 254 to 240 bits and refactor the bit length validation logic to prevent underconstraining of circuits.
Changes
Motivation
The audit revealed that for input lengths between 240 and 254 bits, the padLength calculation could result in a 256-bit value, potentially causing an overflow when constraining the weighted sum of 16-bit limbs. This could lead to underconstrained circuits, posing a security risk.
Technical Details
Testing
Updated existing test cases in bitwise.unit-test.ts to use 240 bits instead of 254.
Added new test cases to verify behavior at the new maximum length.