chore: crypto primitives external audit response 0#22263
Merged
johnathan79717 merged 6 commits intoApr 2, 2026
Merged
Conversation
…onversion Replace BB_ASSERT(val.on_curve()) with an explicit throw_or_abort call in both FrCodec and U256Codec deserialization paths. BB_ASSERT always aborts via assert_failure(); throw_or_abort follows the standard error path that is catchable by bbapi's try-catch in native builds.
Add chmod(0600) after bind() on both macOS and Linux socket paths, matching the permission mode already used for the SHM transport. Add null-guard to init_bn254_mem_crs_factory() to prevent replacing an already-initialized SRS, matching the existing guards on the net and file CRS factory initializers.
Encodes the invariant that MAX_SLICE_BITS < 64 so that get_scalar_slice's 1ULL << lo_slice_bits shift remains well-defined. Currently MAX_SLICE_BITS is 20, far from the threshold, but this documents the constraint explicitly.
Move the SRS bounds check before the subspan() call in batch_commit(). std::span::subspan() has UB when offset > size(); validating first eliminates this and brings batch_commit in line with commit().
Document that BitVector::set() performs a non-atomic read-modify-write and is not safe for concurrent use on indices in the same 64-bit word. Current usage is safe due to per-thread BucketAccumulators ownership.
Change batch_mul's public interface from std::span<const Fr> to std::span<Fr>, making the mutation contract explicit. The MSM internally converts scalars from/to Montgomery form, so callers must provide mutable scalars. This eliminates the const_cast UB risk for callers that pass genuinely const-backed memory. Updated call sites: HyperNova prover/verifier wrappers (drop const), IPA reduce_batch_opening_claim (mutable copy of scalars).
iakovenkos
approved these changes
Apr 2, 2026
iakovenkos
left a comment
iakovenkos
left a comment
Contributor
There was a problem hiding this comment.
LGTM, thanks for addressing these issues!
johnathan79717
merged commit 5c2e6d9
into
merge-train/barretenberg
21 of 25 checks passed
AztecBot
added a commit
that referenced
this pull request
Apr 2, 2026
…lidation PR #22263 replaced BB_ASSERT(val.on_curve()) with throw_or_abort() in field_conversion.hpp. This bypasses BB_DISABLE_ASSERTS() and causes batch_check to crash when deserializing corrupted IPA proof points. Wrap batch_check internals in try-catch so corrupted proofs trigger bisection instead of terminating the process.
johnathan79717
pushed a commit
that referenced
this pull request
Apr 2, 2026
## Summary PR #22263 replaced `BB_ASSERT(val.on_curve())` with `throw_or_abort()` in `field_conversion.hpp` for the external audit response. This bypasses `BB_DISABLE_ASSERTS()` and causes `ChonkBatchVerifierTests.RandomMixedBatches` to crash when deserializing corrupted IPA proof points during `batch_check()`. Added try-catch in `batch_check()` so exceptions from corrupted proof data return `false` (triggering bisection) instead of terminating the process. This is consistent with the existing try-catch in `parallel_reduce()`. All 29 chonk tests pass. Full analysis: https://gist.github.com/AztecBot/71089def650112d95757a7d3af6c0d67 ClaudeBox log: https://claudebox.work/s/fd385651d6fa6262?run=1
AztecBot
added a commit
that referenced
this pull request
Apr 2, 2026
…d proofs PR #22263 replaced BB_ASSERT(val.on_curve()) with throw_or_abort in field_conversion.hpp. Unlike BB_ASSERT, throw_or_abort is not gated by BB_DISABLE_ASSERTS(), so corrupted IPA proof data now throws std::runtime_error during deserialization in batch_check(). This crashed ChonkBatchVerifierTests.RandomMixedBatches which relies on graceful failure handling for tampered proofs. Wrap batch_reduce_verify in try-catch so exceptions trigger bisection (isolating the bad proof) instead of crashing.
3 tasks
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Apr 2, 2026
BEGIN_COMMIT_OVERRIDE fix: verify accumulated pairing points in native ChonkVerifier (#22224) chore: enable _GLIBCXX_DEBUG in debug build presets (#22218) feat: add --memory_profile_out flag for Chonk memory profiling (#22145) fix: disable max capacity test in debug + tiny gate separator improvements (#22215) fix: WASM build for memory_profile.cpp (#22231) fix: translator audit fixes (#22242) fix: remove constexpr from functions using std::vector for _GLIBCXX_DEBUG compat (#22239) fix: pippenger edge case (#22256) fix: avoid dereferencing past-the-end vector iterators in serialize.hpp (#22261) chore: crypto primitives external audit response 0 (#22263) feat: switch memory profiling from peak RSS to live heap usage (#22266) fix: replace UB end-iterator dereference in serialize.hpp (#22262) fix: catch exceptions in ChonkBatchVerifier::batch_check (#22270) END_COMMIT_OVERRIDE
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
2 participants
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.
Audit Context
Addresses findings from the "Aztec - Cryptographic Primitives" external audit. This is response 0, covering the findings that have straightforward fixes.
Changes Made
Finding 1: Off-Curve Proof Commitment Crashes WASM Verifier
Replace
BB_ASSERT(val.on_curve())with explicitthrow_or_abortin both FrCodec and U256Codec deserialization paths (field_conversion.hpp). This routes the error through the standard error path that is catchable by bbapi's try-catch in native builds, rather than going throughassert_failure.Finding 2: WASM Process DOS via Oversized Polynomial in Prover Commit Path
No changes in this PR. Requires a WASM-compatible recovery boundary (setjmp/longjmp or extending try_catch_shim.hpp). Will be addressed in a follow-up.
Finding 3: SRS Downloaded Using HTTP
No changes in this PR. Already mitigated by SHA-256 chunk hash verification (PR #21113). Switching to HTTPS requires resolving the OpenSSL cross-compilation dependency. Deferred.
Finding 4: bbapi Unix Socket Accepts Unauthenticated SRS Replacement
chmod(socket_path, 0600)afterbind()on both macOS and Linux socket paths, matching the 0600 mode already used for the SHM transport.init_bn254_mem_crs_factory()to prevent replacing an already-initialized SRS, matching the existing guards oninit_bn254_net_crs_factoryandinit_bn254_file_crs_factory.Finding 5: Latent Shift-UB in get_scalar_slice
Add
static_assert(MAX_SLICE_BITS < 64, ...)to encode the invariant that the shift inget_scalar_sliceremains well-defined.Finding 6: batch_commit() Subspan Constructed Before Bounds Check
Move the SRS bounds check before the
subspan()call inbatch_commit().std::span::subspan()has UB when offset > size(). This bringsbatch_commitin line withcommit()which already validates first.Finding 7: Witness Polynomial Coefficients Vulnerable to Leakage
No changes. Threat model does not support this being a real vector: PXE in an extension runs in a separate origin, and for embedded wallets there is no trust boundary. Not prioritized.
Finding 8: BitVector::set() Non-Atomic RMW Has No Thread-Safety Guard
Add NOT THREAD-SAFE documentation to
BitVectorclass noting that concurrentset()calls on indices in the same 64-bit word will race. Current usage is safe due to per-threadBucketAccumulatorsownership.Finding 9: batch_mul Mutates Scalars Through const Interface
Change
batch_mul's public interface fromstd::span<const Fr>tostd::span<Fr>, making the mutation contract explicit. The MSM internally converts scalars from/to Montgomery form, so callers must provide mutable scalars. Updated HyperNova prover/verifier wrappers (drop const) and IPAreduce_batch_opening_claim(mutable copy).Checklist
ninjaclean build)