Skip to content

Fix panics on out-of-range CurveID and oversized hash-to-curve domains - #52

Merged
adecaro merged 2 commits into
mainfrom
fix/curveid-hashdomain-panics
Jul 27, 2026
Merged

Fix panics on out-of-range CurveID and oversized hash-to-curve domains#52
adecaro merged 2 commits into
mainfrom
fix/curveid-hashdomain-panics

Conversation

@adecaro

@adecaro adecaro commented Jul 20, 2026

Copy link
Copy Markdown
Member

Summary

Fixes the panics described in #51:

  • Critical: Zr/G1/G2/Gt UnmarshalJSON indexed the Curves slice with an unbounded, attacker-controlled CurveID from the JSON payload, causing an unrecovered index out of range panic on malformed input (e.g. {"curve": 9999, ...}). Added a validCurveID bounds check that returns an error instead.
  • Critical: Curve.HashToG1WithDomain/HashToG2WithDomain panicked on the gnark-backed curves when given a domain longer than 255 bytes. Wrapped both in recover(), consistent with the existing panic-to-nil pattern used elsewhere in math.go, returning nil instead.
  • Low: Clarified FP256BN's HashToG2/HashToG2WithDomain panic message (AMCL has no G2 hash map) in both driver/amcl/fp256bn.go and fp256bn_miracl.go.
  • Doc-only: Corrected Mul2/JointScalarMultiplication doc comments that implied a wall-clock speedup — verified against gnark-crypto source that neither implementation uses the GLV endomorphism, so the joint scalar-mult is an allocation optimization only, not a wall-clock one.
  • Doc-only: Documented the rawBigInt dual-representation invariant on BLS12-381's Zr type (driver/gurvy/bls12381/bls12-381.go) so a future change doesn't silently drop GroupOrder's raw value via Plus/Minus/Mul.

Two AMCL-side performance findings (naive MultiScalarMul, Neg() via full scalar-mult instead of native negation) are intentionally out of scope for this PR — tracked as follow-up perf work, not correctness fixes.

Test plan

  • Added TestJSONUnmarshalerInvalidCurveID — regression for the CurveID panic (rejects out-of-range/negative IDs with an error, still accepts all valid IDs).
  • Added TestHashToGWithDomainOverlongDomain — regression for the oversized-domain panic across all curves, including AMCL's differing behavior (HMAC-based HashToG1WithDomain never panics; HashToG2WithDomain panics unconditionally, now caught by recover() and returned as nil).
  • Added FuzzUnmarshalJSONCurveID and FuzzHashToGWithDomain native Go fuzz targets — ran locally for 20s each (~2.2M and ~209k executions) with zero crashes.
  • go build ./..., go vet ./..., go test ./... all pass.

Closes #51.

adecaro and others added 2 commits July 20, 2026 12:53
UnmarshalJSON for Zr/G1/G2/Gt indexed the Curves slice with an
attacker-controlled CurveID from the JSON payload with no bounds
check, causing an unrecovered index-out-of-range panic on malformed
input. Curve.HashToG1WithDomain/HashToG2WithDomain likewise panicked
on the gnark-backed curves when given a domain longer than 255 bytes.

Also clarifies FP256BN's HashToG2 panic message, and corrects doc
comments on Mul2/JointScalarMultiplication and BLS12-381's Zr.rawBigInt
that overstated performance characteristics or omitted a documented
invariant.

Fixes #51.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>
Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>
@adecaro
adecaro requested a review from ale-linux July 20, 2026 11:13
@adecaro adecaro self-assigned this Jul 20, 2026
@adecaro
adecaro merged commit b61a9af into main Jul 27, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unbounded CurveID / oversized hash-to-curve domain cause panics in UnmarshalJSON and HashToGWithDomain

2 participants