chore: reject non-canonical x coordinate (native affine_element)#22908
Open
suyash67 wants to merge 3 commits into
Open
chore: reject non-canonical x coordinate (native affine_element)#22908suyash67 wants to merge 3 commits into
suyash67 wants to merge 3 commits into
Conversation
federicobarbacovi
approved these changes
May 5, 2026
federicobarbacovi
left a comment
federicobarbacovi
left a comment
Contributor
There was a problem hiding this comment.
LGMT, one minor comment
| // Fq(x_coordinate) silently reduces mod p, so two distinct compressed bytestrings differing by | ||
| // a multiple of p would decompress to the same point (encoding malleability). | ||
| if (x_coordinate >= Fq::modulus) { | ||
| return affine_element(Fq::zero(), Fq::zero()); |
Contributor
There was a problem hiding this comment.
Shouldn't we throw in this case? Or is there a downward call that checks the points are not infinity?
Contributor
Author
There was a problem hiding this comment.
One reason why we return (0, 0) is because grumpkin SRS generation uses (0, 0) as a "failure" case and it retries generating the point.
There's only one user-facing call to this in bbapi_ecc and there we validate that the point is on curve (so (0, 0) would be rightfully rejected). The only place we don't have an on-curve check is bbapi_srs but adding such a check there would be unnecessary overhead.
So I think we should keep returning (0, 0).
suyash67
force-pushed
the
sb/ecc-finding-2
branch
from
bd68101 to
674f25f
Compare
suyash67
added a commit
that referenced
this pull request
May 14, 2026
## Summary Addresses [findings](https://cantina.xyz/code/3dc4ffe5-40f6-4d08-926c-b17315a5bedb/findings) from the ecc/groups audit. ## Commits - **finding 1** — extend `batch_affine_double_impl` slope formula to include `a` for curves with `a ≠ 0` - **finding 2** — ~reject non-canonical x-coordinate encodings in `affine_element::from_compressed`~ (Opened its own [PR](#22908)) - **finding 3** — route ECDSA / Schnorr secret-nonce scalar mul through a new constant-time `element::mul_const_time` (Montgomery ladder + Coron blinding) - **finding 4** — handle `rhs = ∞` in mixed-coordinate `operator+=` - **finding 5** — zero `y` (and `z`) in `self_set_inf` so the infinity representation is canonical - **finding 6** — fix `pairing.FinalExponentiation` reference helper to iterate the full 256-bit exponent - **finding 7** — extend the G1/G2 SRS defenses from #22858 across the bb.js boundary: in `SrsInitSrs::execute`, SHA-256-verify every fully-present compressed-G1 chunk against `BN254_G1_CHUNK_HASHES` before decompression, pin `g1_points[0]` to the canonical generator and `g1_points[1]` to `τ·G` after parsing, and SHA-256-pin the 128-byte G2 input. The subgroup check, on-disk G2 hash pin, infinity rejection, and `is_in_prime_subgroup` implementation itself already landed in #22858. - **finding 8** — make `field::is_zero` constant-time ## Test plan - `ecc_tests` — pass (audit-related filter: 110 pass, 8 skipped, 0 failures) - `srs_tests` (`CrsFactory.*`) — pass - `bbapi_tests` — pass (30/30) - New regression tests in `affine_element.test.cpp`, `element.test.cpp`, `pairing.test.cpp`, `prime_field.test.cpp`
suyash67
force-pushed
the
sb/ecc-finding-2
branch
from
4284e12 to
674f25f
Compare
The from_compressed fix changes ~62% of derived grumpkin SRS points, so the regenerated SRS is published as grumpkin_g1_v2.dat (sha256 87fe782860dd58f0 9a81f797b56346398aabe6e0ed98e0a5ccf14604dd8baee2). Local cache filenames and the browser IndexedDB key are bumped as well so stale caches are not reused. Also fix format.sh staging tracked files as deletions when the pre-commit hook runs in a linked git worktree (GIT_DIR must be unset).
ludamad
force-pushed
the
sb/ecc-finding-2
branch
from
6c52bde to
c57d6e6
Compare
The fixed ECCVM VK commitments (lagrange_first/lagrange_last) and vk_hash are MSMs over the grumpkin SRS, so the v2 SRS changes them. Stale values made the verifier transcript diverge from the prover, failing fresh prove+verify at the IPA G_0 check. Values regenerated via ECCVMTests.FixedVK.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Fix for finding 2 from ecc-groups audit. Basically rejects non-canonical x-coordinate encodings in
affine_element::from_compressed.Since grumpkin SRS generation used this function, we need to regenerate the grumpkin SRS.