-
Notifications
You must be signed in to change notification settings - Fork 583
chore: secp fixes #19931
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
Open
suyash67
wants to merge
8
commits into
merge-train/barretenberg
Choose a base branch
from
sb/guido-secp-bugs
base: merge-train/barretenberg
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
chore: secp fixes #19931
+321
−10
Conversation
This file contains hidden or 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
…n result The ECDSA verification was missing a constraint to enforce that the result of the batch multiplication (u1*G + u2*P) is not the point at infinity. This allowed crafted signatures to bypass verification in the circuit. The fix adds an assertion that fails the circuit when the result is infinity, matching the documented constraint in the docstring (constraint 6). Also updates the test expectations for Secp256k1NativeStdlibDiscrepancy to expect circuit failure since the attack scenario now correctly fails.
6cd4864 to
2c6b937
Compare
suyash67
commented
Jan 27, 2026
| } | ||
| } | ||
|
|
||
| TEST(EcdsaTests, Secp256k1PointAtInfinityRegression) |
Contributor
Author
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm keeping regression tests separate from the main test suite
federicobarbacovi
approved these changes
Jan 27, 2026
Contributor
federicobarbacovi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for doing this!
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.
Fixes two issues found by @guidovranken in our ecdsa circuit.
Issue 1: Point-at-Infinity in ecdsa verification
ecdsa verification can lead to a point at infinity for carefully crafted signature$(r, s)$ with message $z$ :
where$u_1 = z/s \bmod n$ , $u_2 = r/s \bmod n$ , and $\mathcal{O}$ is the point at infinity ($n$ is the scalar field modulus of secp256k1). We do not allow the output of ecdsa mul to be point at infinity in the ecdsa circuit, so by design such signatures will not be able to generate a satisfying proof. However, the boolean output of the ecdsa verification must also be false in those cases. The bug is that the output of the ecdsa verification is still
trueeven if the ecdsa mul is a point at infinity. The fix is to update the boolean output of the ecdsa verification to ensure the mul result is not a point at infinity.Issue 2: NAF Reconstruction$2^{136}$ Overflow
In$2L$ bits (where $L = 68$ limb size) are all 0 then there is an overflow in reconstructing the lower half of the scalar. For a 256-bit scalar $a$ ($N = 256$ ) with skew $\mathfrak{s}_a \in {0, 1}$ we reconstruct it as
compute_naf()function in biggroup, if the input scalar is even (skew = 1) and its least-significantwhere$a'_1, a'_2, \dots, a'_{N-1}$ are inverted bits of $a$ (that is, $a'_{i+1} = 1 - a'_{i+1}$ ). See biggroup README for more details.
We reconstruct the positive and the negative parts separately, and each of those parts is a bigfield element (non-native). So while reconstructing the negative part's lower$2L$ bits, we have:
This overflows when$a'_{i+1} = 1$ for all $i \in [0, 2L)$ . So we need to detect such overflow and then carry it over the higher limb of the negative part.
More details on the issues: https://hackmd.io/JZ_uFRZPTvq4rOVcwNuX9A?view