feat(detector): flag BLS12-381 proof points reaching a pairing without a subgroup check - #947
Conversation
…t a subgroup check Adds `bls_subgroup_check` (SANCT_BLS_SUBGROUP_UNCHECKED), which flags BLS12-381 pairing usage where a proof point reaches the pairing call with nothing having established subgroup or curve membership. The pairing e: G1 x G2 -> GT is defined on the prime-order subgroups, not the full curve. BLS12-381 has a cofactor in both groups (~2^64 in G1, ~2^318 in G2), so the curve holds plenty of points outside the subgroup a proof system reasons about. Pairing one of those evaluates the map outside the domain the soundness argument covers, and two things follow: - Malleability. Given a valid proof point P, an attacker can often produce P + T for a small-order T such that the verification equation still holds. The proof serializes differently but verifies identically, so any replay defence keyed on the proof bytes — a used-proof set, a nullifier derived from the encoding — is bypassed with a fresh-looking submission. - Forgery. Off-subgroup, the relations the verifier checks no longer pin the witness down. This is the mechanism behind the small-subgroup attacks that made subgroup checks mandatory in the BLS signature standard. The check costs a scalar multiplication per point, which is exactly why implementations reach for the _unchecked variants and why this survives review. Skipping it is safe only for points the contract produced itself. Two shapes are reported, both of which put an unvalidated point in front of a pairing: 1. A point built with an _unchecked constructor or deserializer (deserialize_uncompressed_unchecked, from_compressed_unchecked, new_unchecked, ...). In arkworks these are precisely the entry points that skip the subgroup check. 2. A point arriving already typed as G1Affine / G2Affine / G1Projective / G2Projective. Deserialization happened elsewhere, so nothing in this function establishes membership. False positives are held down three ways. The rule gates on the file actually using BLS12-381 before parsing at all, so an _unchecked constructor in unrelated code means nothing here. It requires a pairing, miller_loop, final_exponentiation or verify call to be reached — deserializing a point for storage makes no verification claim. And any membership check in the function silences it, including the correct arkworks idiom of unchecked deserialization followed by an explicit is_in_correct_subgroup_assuming_on_curve. Mapping into the subgroup with clear_cofactor / mul_by_cofactor is accepted too, since that is a valid remedy where rejecting is not appropriate. `#[cfg(test)]` modules are skipped and inline `sanctifier:ignore[SANCT_BLS_SUBGROUP_UNCHECKED]` is honoured, matching the conventions of the surrounding rules. Documented in docs/detectors/bls_subgroup_check.md with the cofactor figures, the vulnerable shape, the fix (both conditions — is_in_correct_subgroup_- assuming_on_curve assumes on-curve, as the name says), and references: the CFRG BLS signature draft's KeyValidate requirement, Sean Bowe on BLS12-381's cofactor structure, the 0xPARC ZK Bug Tracker, Trail of Bits on proof malleability, and arkworks' Validate::Yes/No documentation for what the _unchecked variants actually skip. Rows added to the detector catalog and the finding-code table; the docs-coverage test enforces both. Tests: 7 unit tests covering both flagged shapes, both accepted check forms, the non-BLS and never-pairs cases, and suppression. Note for reviewers: two pre-existing failures in `memory::tests` reproduce on an unmodified checkout of main on macOS (memory sampling) and are unrelated to this change.
1618a2c to
30762c8
Compare
|
Force-pushed a fix for the failing CI.
"bls_subgroup_check": "SANCT_BLS_SUBGROUP_UNCHECKED",
"SANCT_BLS_SUBGROUP_UNCHECKED": "SANCT_BLS_SUBGROUP_UNCHECKED"All 13 test targets pass locally now, including The corpus entry is anchored next to The workflow runs on the new commits are sitting at |
Closes #629
Summary
Adds
bls_subgroup_check(SANCT_BLS_SUBGROUP_UNCHECKED): flags BLS12-381 pairing usage where a proof point reaches the pairing call with nothing having established subgroup or curve membership.Why it matters
The pairing
e: G1 × G2 → GTis defined on the prime-order subgroups, not the full curve. BLS12-381 has a cofactor in both groups —h₁ ≈ 2⁶⁴in G1,h₂ ≈ 2³¹⁸in G2 — so the curve holds plenty of points outside the subgroup a proof system reasons about. Pairing one of them evaluates the map outside the domain the soundness argument covers:Malleability. Given a valid proof point
P, an attacker can often produceP + Tfor a small-orderTsuch that the verification equation still holds. The proof serializes differently but verifies identically — so any replay defence keyed on the proof bytes (a used-proof set, a nullifier derived from the encoding) is bypassed with a fresh-looking submission.Forgery. Off-subgroup, the algebraic relations the verifier checks no longer pin the witness down. This is the mechanism behind the small-subgroup attacks that made subgroup checks mandatory in the CFRG BLS signature standard, and the same reasoning carries to any pairing-based verifier.
The check costs a scalar multiplication per point. That is precisely why implementations reach for the
_uncheckedvariants, and why the bug survives review — skipping it is safe only for points the contract produced itself, never for anything a caller supplied.What it reports
Two shapes, both putting an unvalidated point in front of a pairing:
_uncheckedconstructors and deserializers —deserialize_uncompressed_unchecked,from_compressed_unchecked,new_unchecked, … In arkworks these are exactly the entry points that skip the subgroup check.G1Affine,G2Affine,G1Projective,G2Projectiveas a parameter. Deserialization happened elsewhere, so nothing in this function establishes membership.False positives
Held down three ways:
_uncheckedconstructor in unrelated code is invisible to it.pairing,miller_loop,final_exponentiationorverify— deserializing a point for storage makes no verification claim and is not flagged.is_in_correct_subgroup_assuming_on_curve().clear_cofactor()/mul_by_cofactor()count too, since mapping into the subgroup is a valid remedy where rejecting is not appropriate.#[cfg(test)]modules are skipped and inline// sanctifier:ignore[SANCT_BLS_SUBGROUP_UNCHECKED]is honoured, matching the surrounding rules.Documentation
docs/detectors/bls_subgroup_check.mdcovers the cofactor figures, both attack consequences, the vulnerable shape, and the fix — including that both conditions are needed, sinceis_in_correct_subgroup_assuming_on_curveassumes on-curve, as the name says.References, per the acceptance criterion:
KeyValidate/ subgroup validationValidate::YesvsValidate::No— what the_uncheckedvariants actually skipRows added to the detector catalog and the finding-code table —
tests/detector_docs_coverage.rsenforces both.Acceptance criteria
Tests
7 unit tests: both flagged shapes, both accepted check forms, the non-BLS and never-pairs cases, and suppression.
cargo fmt --all -- --checkclean,cargo clippy -p sanctifier-core --all-targetsclean,cargo test -p sanctifier-corepassing.Pre-existing failures, unrelated to this PR:
memory::tests::memory_guard_rejects_above_zero_limitandmemory::tests::memory_tracker_samples_increase_peakfail on an unmodified checkout ofmainon macOS (memory sampling). Verified by stashing this change and re-running — same two failures.Local runs used
--no-default-featuresbecause the defaultsmtfeature needsz3.hpresent; that is a local toolchain gap, not a change here.Conflicts
This and #946 (for #628) share four files by necessity — the detector registry (
rules/mod.rs),finding_codes.rs, and the two docs tables. Rather than both appending to the same place, each PR's insertions are anchored far apart in every shared file: this one registers afteredge_amountand inserts nearBALANCE_EQUALITY; the other registers aftervk_provenanceand inserts nearPROOF_LENGTH_UNVALIDATED, tens of lines away in each case, so no two hunks share context.Both merge orders were tested locally against
main— clean either way, with fmt, clippy, the docs-coverage test and 294 passing tests on the combined result.