Remediate security audit findings with regression tests - #8
Merged
Conversation
seunlanlege
force-pushed
the
security/audit-remediation
branch
2 times, most recently
from
June 30, 2026 08:31
f6cbe3b to
5cdfe3c
Compare
Addresses the valid findings from the gnark-APK-Proofs security audit (Dr. Cyprian Sakwa, 2026-04-22) across all layers, with tests. Circuit / Go FFI: - Enforce on-curve + prime-order subgroup checks on public keys at the FFI trust boundary (new apk.ParseG1; findings 1, 3, 14). All-zero is treated as the identity padding point. - Validate participation indices (range + uniqueness) before deriving the bitlist, and require exact witness length (findings 5, 10, 12, 13). This is fail-loud input hygiene; the key set is committed and the bitlist/ExpectedApk are public, so it is not a soundness control. - Document the 5-limb bitlist encoding and the validation trust boundary (findings 2; corrected misleading apk.go comments for 1, 3). Rust prover: - Validate every public key on-curve + in subgroup, reject duplicate indices (findings 15, 16). - Check FFI output buffers for null/zero length before dereferencing raw pointers (finding 18). - parse_public_inputs returns a structured error instead of panicking (findings 19, 20). Rust PLONK verifier: - Subgroup checks on all deserialized G1/G2 points, validate decompressed points (findings 25, 26). - Validate proof structure lengths, domain-size power-of-two, and Lagrange index bounds (findings 27, 30, 34). - Simplify batch-random field reduction to remove a potential panic (finding 31); remove the misleadingly-named endianness trait (finding 28); add RFC 9380 references (findings 32, 33). Integration test: - Replace cross-version mem::transmute with canonical-serialization conversion between arkworks 0.4 (w3f-bls) and 0.5 (finding 35). Build system: - Reproducible Go build (-trimpath, -buildvcs=false), cfg-gated system linking, and go.mod/go.sum rebuild tracking (findings 43, 46, 47). Solidity: - Document the EIP-2537 trust boundary and stateless/replay model (findings 50, 53). Regression tests: 9 Go (apk), 10 Rust prover, 8 Rust verifier.
seunlanlege
force-pushed
the
security/audit-remediation
branch
from
June 30, 2026 08:36
5cdfe3c to
e5b6324
Compare
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
1 participant
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.
Addresses the valid findings from the gnark-APK-Proofs security audit (Dr. Cyprian Sakwa, 2026-04-22). Each finding was verified against the code before fixing; fixes come with regression tests.
cargo clippy,gofmt,go vetare clean and all new tests pass.Fixes by layer
Circuit / Go FFI — new cgo-free
circuits/apk/validation.go(testable), wired intocircuits/ffi/main.go:ParseG1enforces on-curve + prime-order subgroup at the trust boundary (all-zero = identity padding). In-circuit subgroup checks on 1024 emulated points are impractical, so enforcement lives at the FFI/prover boundaries; corrected the misleading "no checks needed" comments inapk.go.Rust prover (
rust/prover/src/lib.rs): F15 point validation, F16 dedup, F18 FFI null/len checks beforefrom_raw_parts, F19/F20parse_public_inputsreturnsResult, F17 size asserts.Rust PLONK verifier: F25/F26 subgroup checks on all deserialized G1/G2 points, F27 proof-structure length validation, F30 Lagrange-index bounds, F34 power-of-2 domain check, F31 simplified field reduction (removes a potential panic), F28 removed the misleadingly-named endianness trait, F32/F33 RFC 9380 docs.
Integration test (
bls_verify.rs): F35 (CRITICAL) — replaced all 5 cross-versionmem::transmutes with canonical-serialization conversion between arkworks 0.4 (w3f-bls) and 0.5.Build (
rust/ffi/build.rs): F43-trimpath/-buildvcs=false, F46 cfg-gatedresolv, F47 go.mod/go.sum rebuild tracking.Solidity (
ApkProof.sol): documented EIP-2537 trust boundary + stateless/replay model (F50/F53); RFC 9380 + DST were already cited (F49/F55).Marked invalid / already-handled
serialize_compressedwould break it.ensure_bytes.Lower-value trust-model/ergonomics findings (F21/F22, F37–F40, F42 beyond buffer validation, F44/F45/F48) were left for follow-up.
Tests
ParseG1; index range/dedup; bitlist edge indices.parse_public_inputserror paths.Not run here
The
#[ignore]d end-to-endtest_full_verify(needs SRS download +forge build) compiles cleanly with the F35 changes but was not executed. Worth running once in an environment with SRS + Foundry to confirm the added verifier validations don't reject legitimate proofs.