Harden deserialization paths#497
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens elliptic-curve deserialization by adding validation checks that reject off-curve points and (where relevant) points not in the prime-order subgroup, reducing the risk of accepting malformed or adversarial inputs across provider serialization/IO paths.
Changes:
- Validate affine point deserialization via checked constructors (
from_xy) to reject off-curve coordinates. - Enforce prime-order subgroup membership for bn256 G2 points by requiring torsion-free points during deserialization / ptau loading.
- Add targeted unit tests that exercise acceptance/rejection of valid, identity, off-curve, and non-subgroup points.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/provider/traits.rs |
Adds on-curve validation for EVM deserialization in the provider trait macro and expands DlogGroup affine bounds. |
src/provider/ptau.rs |
Adds a G2 subgroup (torsion-free) check when reading PTAU files and introduces tests for rejecting non-subgroup G2 points. |
src/provider/bn256_grumpkin.rs |
Hardens bn256 G2Affine EVM deserialization with on-curve + subgroup checks and adds EVM serde round-trip/rejection tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Validate the decoded (x, y) coordinates with the checked from_xy constructor, which enforces the curve equation and still accepts the canonical identity. Adds on-curve, identity, and off-curve tests.
Validate the decoded coordinates with from_xy and require the point to be torsion-free, since bn256 G2 has a non-trivial cofactor. Adds round-trip, off-curve, and non-subgroup tests.
srinathsetty
force-pushed
the
fix/evm-deserialize-oncurve
branch
from
4402a38 to
f56bcdf
Compare
…group read_points (used by both the ptau SRS loader and the Pedersen commitment-key loader) now checks every loaded G1/G2 point is on the curve; read_ptau additionally requires G2 points to be torsion-free, since bn256 G2 has a non-trivial cofactor (G1 is cofactor 1). Adds PtauFileError::PointNotOnCurve and PointNotInSubgroup, the CofactorCurveAffine bound, and on-curve / non-subgroup rejection tests.
…zation CommitmentKey.tau_H now deserializes via a format-preserving helper that adds the prime-order-subgroup check on top of the default on-curve decode (bn256 G2 has a non-trivial cofactor). The G2 CustomSerdeTrait deserialize performs the same torsion check in non-evm builds, so VerifierKey G2 points are subgroup-checked regardless of the evm feature. Adds a CommitmentKey non-subgroup rejection test.
Property tests for the halo2curves v0.9.0 primitives this crate relies on (finite-field arithmetic incl. the asm backend, the elliptic-curve group law, the BN256 pairing, and msm_best), so a future dependency bump that changes the arithmetic is caught. Covers bn256/grumpkin/secp256k1: field inverse, commutativity, unbiased from_uniform_bytes, group law, pairing bilinearity and non-degeneracy, and msm_best vs naive across the affine-batch boundary. Build with and without --features asm to cover both field backends.
srinathsetty
force-pushed
the
fix/evm-deserialize-oncurve
branch
from
f56bcdf to
5c18c41
Compare
sai-deng
left a comment
sai-deng
left a comment
There was a problem hiding this comment.
Non-blocking scope question: this PR hardens the deserialization / load paths, but invalid affine values can still be created in-memory via public coordinate fields and then passed through APIs like CommitmentKey::new(...) or write_ptau(...); they’ll be rejected when reloaded/deserialized, but not necessarily at construction time.
Is that intentionally out of scope for this fix? I think that’s fine if the threat model is untrusted serialized/setup input, but it may be worth documenting that distinction or adding constructor-side validation if these APIs are expected to enforce valid-point invariants too.
|
LGTM |
Addresses review feedback on #497: invalid affine points can be built in-memory (halo2curves coordinate fields are public) and passed through CommitmentKey::new, only to be rejected later on reload. Validate at the entry boundaries where data enters the in-memory domain -- construction (CommitmentKey::new) and load/deserialize (read_ptau and the serde paths, unchanged) -- while write_ptau stays an exit point that trusts already-validated data. CommitmentKey::new now returns Result and rejects off-curve G1 bases / h and an off-curve or non-prime-order-subgroup tau_H, via a new NovaError::InvalidCommitmentKey. Document the trust boundaries on CommitmentKey::new, read_ptau, and write_ptau. Tests cover accept, off-curve, and non-subgroup cases; the deserialize test now builds the tampered key with a struct literal since the constructor rejects it.
Good point! Added validation on CommitmentKey::new() path. write_ptau does not need the validation since we already validated incoming paths (added doc). |
No description provided.