Skip to content

Commit 30762c8

Browse files
committed
feat(detector): flag BLS12-381 proof points reaching a pairing without 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.
1 parent 8710a74 commit 30762c8

7 files changed

Lines changed: 564 additions & 0 deletions

File tree

docs/detectors/README.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ detector page and to the relevant [Glossary](../glossary.md) term.
2828
| [`unhandled_result`](unhandled_result.md) | [`S009`](../error-codes.md) | logic | Medium | A `Result` that is silently dropped |
2929
| [`hardcoded_addr`](hardcoded_addr.md) | [`S012`](../error-codes.md) | code_hygiene | High | Hardcoded admin address / secret literal in an auth context |
3030
| [`edge_amount`](edge_amount.md) | [`S013`](../error-codes.md) | code_hygiene | Medium | `transfer`/`mint`/`burn` missing `amount > 0` / `from != to` guards |
31+
| [`bls_subgroup_check`](bls_subgroup_check.md) | [`SANCT_BLS_SUBGROUP_UNCHECKED`](../error-codes.md) | cryptography | Error | BLS12-381 proof point reaches a pairing call with no subgroup or curve membership check |
3132
| [`balance_equality`](balance_equality.md) | [`SANCT_BALANCE_EQ`](../error-codes.md) | logic | Info | Balance gated with `==`/`!=` where `>=`/`<=` was intended |
3233
| [`unused_variable`](unused_variable.md) | [`S015`](../error-codes.md) | code_hygiene | Info | Unused local bindings (dead code) |
3334
| [`error_code_collision`](error_code_collision.md) | [`S016`](../error-codes.md) | code_hygiene | Medium | Duplicate/inconsistent `#[contracterror]` discriminants |
Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
# `bls_subgroup_check` — BLS12-381 point reaches a pairing without a subgroup check
2+
3+
| | |
4+
| --- | --- |
5+
| **Finding code** | [`SANCT_BLS_SUBGROUP_UNCHECKED`](../error-codes.md) |
6+
| **Category** | cryptography |
7+
| **Severity** | Error |
8+
| **Source rule** | [`rules/bls_subgroup_check.rs`](../../tooling/sanctifier-core/src/rules/bls_subgroup_check.rs) |
9+
| **Glossary** | [Proof](../glossary.md#proof) · [Pairing](../glossary.md#pairing) |
10+
11+
## What it catches
12+
13+
In a file that uses BLS12-381, a function that reaches a pairing call
14+
(`pairing`, `miller_loop`, `final_exponentiation`, or any `verify`) with a
15+
proof point that nothing has checked for subgroup membership. Two shapes:
16+
17+
1. **An `_unchecked` constructor or deserializer**
18+
`deserialize_uncompressed_unchecked`, `from_compressed_unchecked`,
19+
`new_unchecked`, and friends. In arkworks these are precisely the entry
20+
points that skip the subgroup check.
21+
2. **A point arriving as a typed parameter**`G1Affine`, `G2Affine`,
22+
`G1Projective`, `G2Projective`. Deserialization happened elsewhere, so
23+
nothing in this function establishes membership.
24+
25+
## Why it matters
26+
27+
The pairing `e: G1 × G2 → GT` is defined on the **prime-order subgroups**, not
28+
on the full curve. BLS12-381 has a cofactor in both groups — `h₁ ≈ 2⁶⁴` in G1
29+
and `h₂ ≈ 2^318` in G2 — so the curve contains plenty of points outside the
30+
subgroup a proof system reasons about. Feeding one to the pairing evaluates it
31+
outside the domain the soundness argument covers, and two things follow:
32+
33+
**Malleability.** Given a valid proof point `P`, an attacker can often produce
34+
`P + T` for a small-order `T` such that the verification equation still holds.
35+
The proof serializes differently but verifies identically, so any replay
36+
defence keyed on the proof bytes — a "this proof was already used" set, a
37+
nullifier derived from the encoding — is bypassed with a fresh-looking
38+
submission.
39+
40+
**Forgery.** Off-subgroup, the algebraic relations the verifier checks no
41+
longer pin the witness down. This is the mechanism behind the small-subgroup
42+
attacks that motivated mandatory subgroup checks in the BLS signature standard,
43+
and the same reasoning applies to any pairing-based proof verifier.
44+
45+
The check is not free — it is a scalar multiplication per point — which is
46+
exactly why implementations reach for the `_unchecked` variants and why this
47+
bug survives review. Skipping the check is safe only for points the contract
48+
itself produced, never for anything a caller supplied.
49+
50+
## Vulnerable example
51+
52+
```rust
53+
use ark_bls12_381::{Bls12_381, G1Affine, G2Affine};
54+
55+
fn verify_proof(proof_bytes: &[u8], vk: &VerifyingKey) -> bool {
56+
// `_unchecked` skips the subgroup check by design, and nothing here
57+
// performs it afterwards.
58+
let a = G1Affine::deserialize_uncompressed_unchecked(proof_bytes).unwrap();
59+
let b = G2Affine::deserialize_uncompressed_unchecked(proof_bytes).unwrap();
60+
Bls12_381::pairing(a, b) == vk.alpha_beta
61+
}
62+
```
63+
64+
## The fix
65+
66+
Either use the checked deserializer, or keep the fast path and do the check
67+
explicitly:
68+
69+
```rust
70+
let a = G1Affine::deserialize_uncompressed_unchecked(proof_bytes)?;
71+
if !a.is_on_curve() || !a.is_in_correct_subgroup_assuming_on_curve() {
72+
return Err(Error::InvalidProofPoint);
73+
}
74+
```
75+
76+
Both conditions are needed: `is_in_correct_subgroup_assuming_on_curve`, as its
77+
name says, assumes the point is on the curve to begin with. Mapping into the
78+
subgroup with `clear_cofactor()` / `mul_by_cofactor()` is also accepted by the
79+
detector where rejecting is not appropriate.
80+
81+
## What it does *not* flag
82+
83+
- Files that do not use BLS12-381 at all.
84+
- BLS code that never reaches a pairing — deserializing a point for storage
85+
makes no verification claim.
86+
- Any function where a membership check is present.
87+
- `#[cfg(test)]` modules, and lines carrying
88+
`// sanctifier:ignore[SANCT_BLS_SUBGROUP_UNCHECKED]`.
89+
90+
## References
91+
92+
- [RFC 9380 §2.1 / IRTF CFRG BLS signatures draft, "Subgroup checks"](https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-bls-signature-05#section-5.2) — mandatory `KeyValidate` / subgroup validation and why
93+
- [Sean Bowe — *BLS12-381: New zk-SNARK Elliptic Curve Construction*](https://electriccoin.co/blog/new-snark-curve/) — cofactor structure of G1 and G2
94+
- [0xPARC — *ZK Bug Tracker*: "Missing curve/subgroup checks"](https://github.com/0xPARC/zk-bug-tracker)
95+
- [Trail of Bits — *Breaking the Shield*](https://blog.trailofbits.com/2022/04/13/part-1-coordinated-disclosure-of-vulnerabilities-affecting-girault-bulletproofs-and-plonk/) — proof malleability from unchecked group elements
96+
- [arkworks `CanonicalDeserialize``Validate::Yes` vs `Validate::No`](https://docs.rs/ark-serialize/latest/ark_serialize/enum.Validate.html) — what the `_unchecked` variants actually skip
97+
- [`verifier-checklist.md`](../verifier-checklist.md) — the wider verifier integration checklist

docs/error-codes.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ the fix, and references.
2222
| `S016` | code_hygiene | Duplicate/inconsistent `#[contracterror]` discriminants | [`error_code_collision`](detectors/error_code_collision.md) |
2323
| `S017` | arithmetic | Fee/interest integer division that rounds to zero for micro-amounts | [`fee_rounding`](detectors/fee_rounding.md) |
2424
| `SANCT_ARG_DOS` | denial_of_service | `Vec`/`Map` argument iterated without a length cap | [`arg_dos`](detectors/arg_dos.md) |
25+
| `SANCT_BLS_SUBGROUP_UNCHECKED` | cryptography | BLS12-381 proof point reaches a pairing call with no subgroup or curve membership check | [`bls_subgroup_check`](detectors/bls_subgroup_check.md) |
2526
| `SANCT_UNWRAP` | panic_handling | `unwrap` / `expect` / risky `unwrap_or_default` inside `#[contractimpl]` entrypoints; replace with typed errors or explicit domain defaults | [`sanct_unwrap`](detectors/sanct_unwrap.md) |
2627
| `SANCT_VISIBILITY` | authentication | Helper-shaped state mutator exposed through `#[contractimpl]` without authorization | [`sanct_visibility`](detectors/sanct_visibility.md) |
2728
| `SANCT_UNBOUNDED_STORAGE` | denial_of_service | Persistent/instance collection grows via append/insert with no removal or length cap | [`unbounded_storage`](detectors/unbounded_storage.md) |

tooling/sanctifier-core/src/finding_codes.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ pub const LEDGER_SECONDS: &str = "S021";
2323
pub const EXCESSIVE_CLONE: &str = "S020";
2424
pub const ARG_DOS: &str = "SANCT_ARG_DOS";
2525
pub const BALANCE_EQUALITY: &str = "SANCT_BALANCE_EQ";
26+
pub const BLS_SUBGROUP_UNCHECKED: &str = "SANCT_BLS_SUBGROUP_UNCHECKED";
2627
pub const SANCT_UNWRAP: &str = "SANCT_UNWRAP";
2728
pub const INIT_HARDCODED_ADMIN: &str = "SANCT_INIT_HARDCODED_ADMIN";
2829
pub const SANCT_VISIBILITY: &str = "SANCT_VISIBILITY";
@@ -127,6 +128,12 @@ pub fn all_finding_codes() -> Vec<FindingCode> {
127128
description:
128129
"Balance gated against an amount with `==`/`!=` where `>=`/`<=` was likely intended",
129130
},
131+
FindingCode {
132+
code: BLS_SUBGROUP_UNCHECKED,
133+
category: "cryptography",
134+
description:
135+
"BLS12-381 proof point reaches a pairing call with no subgroup or curve membership check",
136+
},
130137
FindingCode {
131138
code: DEPRECATED_SDK,
132139
category: "code_hygiene",

0 commit comments

Comments
 (0)