Skip to content

Commit 5c2e6d9

Browse files
chore: crypto primitives external audit response 0 (#22263)
### 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 explicit `throw_or_abort` in 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 through `assert_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** - Add `chmod(socket_path, 0600)` after `bind()` on both macOS and Linux socket paths, matching the 0600 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 `init_bn254_net_crs_factory` and `init_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 in `get_scalar_slice` remains well-defined. **Finding 6: batch_commit() Subspan Constructed Before Bounds Check** Move the SRS bounds check before the `subspan()` call in `batch_commit()`. `std::span::subspan()` has UB when offset > size(). This brings `batch_commit` in line with `commit()` 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 `BitVector` class noting that concurrent `set()` calls on indices in the same 64-bit word will race. Current usage is safe due to per-thread `BucketAccumulators` ownership. **Finding 9: batch_mul Mutates Scalars Through const Interface** 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. Updated HyperNova prover/verifier wrappers (drop const) and IPA `reduce_batch_opening_claim` (mutable copy). ### Checklist - [x] Confirmed and documented security issues found - [x] Verified that tests cover all critical paths - [x] Verified build passes (`ninja` clean build) - [x] Ran ecc_tests (830 passed), srs_tests (29 passed), commitment_schemes_tests (88 passed), hypernova_tests (9 passed)
1 parent fccec82 commit 5c2e6d9

14 files changed

Lines changed: 32 additions & 13 deletions

File tree

barretenberg/cpp/src/barretenberg/commitment_schemes/commitment_key.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,14 +111,14 @@ template <class Curve> class CommitmentKey {
111111
std::vector<std::span<Fr>> scalar_spans;
112112

113113
for (auto& polynomial : polynomials.subspan(i, batch_end - i)) {
114-
std::span<const Commitment> point_table = get_monomial_points().subspan(polynomial.start_index());
115114
size_t consumed_srs = polynomial.start_index() + polynomial.size();
116115
if (consumed_srs > get_monomial_size()) {
117116
throw_or_abort(format("Attempting to commit to a polynomial that needs ",
118117
consumed_srs,
119118
" points with an SRS of size ",
120119
get_monomial_size()));
121120
}
121+
std::span<const Commitment> point_table = get_monomial_points().subspan(polynomial.start_index());
122122
scalar_spans.emplace_back(polynomial.coeffs());
123123
points_spans.emplace_back(point_table);
124124
}

barretenberg/cpp/src/barretenberg/commitment_schemes/ipa/ipa.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -832,7 +832,7 @@ template <typename Curve_, size_t log_poly_length = CONST_ECCVM_LOG_N> class IPA
832832
{
833833
// Extract batch_mul arguments from the accumulator
834834
const auto& commitments = batch_opening_claim.commitments;
835-
const auto& scalars = batch_opening_claim.scalars;
835+
auto scalars = batch_opening_claim.scalars; // mutable copy: batch_mul temporarily modifies scalars
836836
const Fr& shplonk_eval_challenge = batch_opening_claim.evaluation_point;
837837
// Compute \f$ C = \sum \text{commitments}_i \cdot \text{scalars}_i \f$
838838
GroupElement shplonk_output_commitment = GroupElement::batch_mul(commitments, scalars);

barretenberg/cpp/src/barretenberg/ecc/fields/field_conversion.hpp

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,9 @@ class FrCodec {
133133
T val;
134134
val.x = deserialize_from_fields<BaseField>(fr_vec.subspan(0, BASE));
135135
val.y = deserialize_from_fields<BaseField>(fr_vec.subspan(BASE, BASE));
136-
BB_ASSERT(val.on_curve());
136+
if (!val.on_curve()) {
137+
throw_or_abort("Deserialized point is not on the curve");
138+
}
137139
return val;
138140
} else {
139141
// Array or Univariate
@@ -268,7 +270,9 @@ class U256Codec {
268270
if (val.x == BaseField::zero() && val.y == BaseField::zero()) {
269271
val.self_set_infinity();
270272
}
271-
BB_ASSERT(val.on_curve());
273+
if (!val.on_curve()) {
274+
throw_or_abort("Deserialized point is not on the curve");
275+
}
272276
return val;
273277
} else {
274278
// Array or Univariate

barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,7 @@ template <typename Fq_, typename Fr_, typename Params_> class alignas(64) affine
199199
* @param masking_scalar Ignored for native (needed for safe offset generators in stdlib)
200200
*/
201201
static affine_element batch_mul(std::span<const affine_element> points,
202-
std::span<const Fr> scalars,
202+
std::span<Fr> scalars,
203203
size_t max_num_bits = 0,
204204
bool with_edgecases = true,
205205
const Fr& masking_scalar = Fr(1)) noexcept;

barretenberg/cpp/src/barretenberg/ecc/groups/element.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ template <class Fq, class Fr, class Params> class alignas(32) element {
107107
* @details Delegates to affine_element::batch_mul. Provided for interface compatibility with stdlib.
108108
*/
109109
static affine_element<Fq, Fr, Params> batch_mul(std::span<const affine_element<Fq, Fr, Params>> points,
110-
std::span<const Fr> scalars,
110+
std::span<Fr> scalars,
111111
size_t max_num_bits = 0,
112112
bool with_edgecases = true,
113113
const Fr& masking_scalar = Fr(1)) noexcept

barretenberg/cpp/src/barretenberg/ecc/groups/element_batch_mul.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ template <> struct curve_for_element<fr, fq, grumpkin::G1Params> {
2525
template <class Fq, class Fr, class Params>
2626
affine_element<Fq, Fr, Params> affine_element<Fq, Fr, Params>::batch_mul(
2727
std::span<const affine_element<Fq, Fr, Params>> points,
28-
std::span<const Fr> scalars,
28+
std::span<Fr> scalars,
2929
[[maybe_unused]] size_t max_num_bits,
3030
bool with_edgecases,
3131
[[maybe_unused]] const Fr& masking_scalar) noexcept
@@ -42,14 +42,14 @@ affine_element<Fq, Fr, Params> affine_element<Fq, Fr, Params>::batch_mul(
4242
// Explicit instantiations for supported curves
4343
template affine_element<fq, fr, Bn254G1Params> affine_element<fq, fr, Bn254G1Params>::batch_mul(
4444
std::span<const affine_element<fq, fr, Bn254G1Params>> points,
45-
std::span<const fr> scalars,
45+
std::span<fr> scalars,
4646
size_t max_num_bits,
4747
bool with_edgecases,
4848
const fr& masking_scalar) noexcept;
4949

5050
template affine_element<fr, fq, grumpkin::G1Params> affine_element<fr, fq, grumpkin::G1Params>::batch_mul(
5151
std::span<const affine_element<fr, fq, grumpkin::G1Params>> points,
52-
std::span<const fq> scalars,
52+
std::span<fq> scalars,
5353
size_t max_num_bits,
5454
bool with_edgecases,
5555
const fq& masking_scalar) noexcept;

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/bitvector.hpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717
* @brief Custom class to handle packed vectors of bits
1818
* @details The cpp std::vector<bool> does not guarantee memory adjacency of values, and has no fast primitive for
1919
* clearing all bits in the vector. This is to avoid needing to clear all Pippenger buckets every round
20+
* @note NOT THREAD-SAFE. Concurrent set() calls on indices sharing the same 64-bit word will race.
21+
* Current usage is safe because each BucketAccumulators instance is per-thread.
2022
*/
2123
class BitVector {
2224
public:

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.hpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,9 @@ template <typename Curve> class MSM {
3939

4040
// Maximum bits per scalar slice (2^20 = 1M buckets, far beyond practical use)
4141
static constexpr size_t MAX_SLICE_BITS = 20;
42+
static_assert(MAX_SLICE_BITS < 64,
43+
"get_scalar_slice uses 1ULL << lo_slice_bits where lo_slice_bits <= MAX_SLICE_BITS - 1; "
44+
"shifting uint64_t by >= 64 is UB.");
4245

4346
// Number of points to look ahead for memory prefetching
4447
static constexpr size_t PREFETCH_LOOKAHEAD = 32;

barretenberg/cpp/src/barretenberg/hypernova/hypernova_prover.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ namespace bb {
1414

1515
template <size_t N>
1616
HypernovaFoldingProver::Commitment HypernovaFoldingProver::batch_mul(const RefArray<Commitment, N>& _points,
17-
const std::vector<FF>& scalars)
17+
std::vector<FF>& scalars)
1818
{
1919
std::vector<Commitment> points(N);
2020
for (size_t idx = 0; idx < N; ++idx) {

barretenberg/cpp/src/barretenberg/hypernova/hypernova_prover.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ class HypernovaFoldingProver {
113113
/**
114114
* @brief Utility to perform batch mul of commitments.
115115
*/
116-
template <size_t N> Commitment batch_mul(const RefArray<Commitment, N>& _points, const std::vector<FF>& scalars);
116+
template <size_t N> Commitment batch_mul(const RefArray<Commitment, N>& _points, std::vector<FF>& scalars);
117117
};
118118

119119
} // namespace bb

0 commit comments

Comments
 (0)