Skip to content

Commit 26f56c8

Browse files
authored
fix: ecc/groups audit findings (#22864)
## Summary Addresses [findings](https://cantina.xyz/code/3dc4ffe5-40f6-4d08-926c-b17315a5bedb/findings) from the ecc/groups audit. ## Commits - **finding 1** — extend `batch_affine_double_impl` slope formula to include `a` for curves with `a ≠ 0` - **finding 2** — ~reject non-canonical x-coordinate encodings in `affine_element::from_compressed`~ (Opened its own [PR](#22908)) - **finding 3** — route ECDSA / Schnorr secret-nonce scalar mul through a new constant-time `element::mul_const_time` (Montgomery ladder + Coron blinding) - **finding 4** — handle `rhs = ∞` in mixed-coordinate `operator+=` - **finding 5** — zero `y` (and `z`) in `self_set_inf` so the infinity representation is canonical - **finding 6** — fix `pairing.FinalExponentiation` reference helper to iterate the full 256-bit exponent - **finding 7** — extend the G1/G2 SRS defenses from #22858 across the bb.js boundary: in `SrsInitSrs::execute`, SHA-256-verify every fully-present compressed-G1 chunk against `BN254_G1_CHUNK_HASHES` before decompression, pin `g1_points[0]` to the canonical generator and `g1_points[1]` to `τ·G` after parsing, and SHA-256-pin the 128-byte G2 input. The subgroup check, on-disk G2 hash pin, infinity rejection, and `is_in_prime_subgroup` implementation itself already landed in #22858. - **finding 8** — make `field::is_zero` constant-time ## Test plan - `ecc_tests` — pass (audit-related filter: 110 pass, 8 skipped, 0 failures) - `srs_tests` (`CrsFactory.*`) — pass - `bbapi_tests` — pass (30/30) - New regression tests in `affine_element.test.cpp`, `element.test.cpp`, `pairing.test.cpp`, `prime_field.test.cpp`
1 parent 632f6ee commit 26f56c8

11 files changed

Lines changed: 278 additions & 18 deletions

File tree

barretenberg/cpp/src/barretenberg/bbapi/bbapi_srs.cpp

Lines changed: 38 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,15 @@
55
#include "barretenberg/bbapi/bbapi_srs.hpp"
66
#include "barretenberg/common/serialize.hpp"
77
#include "barretenberg/common/thread.hpp"
8+
#include "barretenberg/crypto/sha256/sha256.hpp"
89
#include "barretenberg/ecc/curves/bn254/g1.hpp"
910
#include "barretenberg/ecc/curves/bn254/g2.hpp"
1011
#include "barretenberg/ecc/curves/grumpkin/grumpkin.hpp"
1112
#include "barretenberg/numeric/uint256/uint256.hpp"
13+
#include "barretenberg/srs/factories/bn254_crs_data.hpp"
14+
#include "barretenberg/srs/factories/bn254_g1_chunk_hashes.hpp"
1215
#include "barretenberg/srs/global_crs.hpp"
16+
#include <span>
1317

1418
namespace bb::bbapi {
1519

@@ -30,6 +34,23 @@ SrsInitSrs::Response SrsInitSrs::execute(BB_UNUSED BBApiRequest& request) &&
3034
}
3135
});
3236
} else if (bytes_per_point == COMPRESSED_POINT_SIZE) {
37+
// Verify SHA-256 of every 4 MB chunk against the in-binary pin BN254_G1_CHUNK_HASHES.
38+
// Require chunk-aligned input so every byte is covered (no partial trailing chunk).
39+
if (points_buf.size() == 0 || points_buf.size() % bb::srs::SRS_CHUNK_SIZE_BYTES != 0) {
40+
throw_or_abort("SrsInitSrs: compressed points_buf size " + std::to_string(points_buf.size()) +
41+
" must be a positive multiple of " + std::to_string(bb::srs::SRS_CHUNK_SIZE_BYTES));
42+
}
43+
size_t num_full_chunks = points_buf.size() / bb::srs::SRS_CHUNK_SIZE_BYTES;
44+
size_t chunks_to_verify = std::min(num_full_chunks, static_cast<size_t>(bb::srs::SRS_NUM_FULL_CHUNKS));
45+
for (size_t i = 0; i < chunks_to_verify; ++i) {
46+
auto chunk = std::span<const uint8_t>(points_buf.data() + i * bb::srs::SRS_CHUNK_SIZE_BYTES,
47+
bb::srs::SRS_CHUNK_SIZE_BYTES);
48+
auto hash = bb::crypto::sha256(chunk);
49+
if (hash != bb::srs::BN254_G1_CHUNK_HASHES[i]) {
50+
throw_or_abort("SrsInitSrs: g1 compressed chunk " + std::to_string(i) + " SHA-256 mismatch");
51+
}
52+
}
53+
3354
// Compressed: decompress and return uncompressed bytes for caller to cache
3455
parallel_for([&](ThreadChunk chunk) {
3556
for (auto i : chunk.range(static_cast<size_t>(num_points))) {
@@ -50,11 +71,23 @@ SrsInitSrs::Response SrsInitSrs::execute(BB_UNUSED BBApiRequest& request) &&
5071
std::to_string(bytes_per_point));
5172
}
5273

53-
// Parse G2 point from buffer (128 bytes). `serialize_from_buffer` validates that the bytes
54-
// decode to a curve point but does NOT enforce subgroup membership. BN254 G2 has a non-trivial
55-
// cofactor (h2 ≈ 2^254), so a curve point may lie in a small cofactor subgroup of order
56-
// dividing h2 rather than the prime-order subgroup of order r. Reject anything outside
57-
// the prime-order subgroup before it reaches the SRS factory.
74+
// Pin the first two G1 points to their canonical trusted-setup values. Defense in depth on the
75+
// compressed path; the only gate on the uncompressed (cached) path.
76+
if (num_points >= 1 && g1_points[0] != bb::srs::BN254_G1_FIRST_ELEMENT) {
77+
throw_or_abort("SrsInitSrs: g1_points[0] is not the canonical BN254 generator");
78+
}
79+
if (num_points >= 2 && g1_points[1] != bb::srs::get_bn254_g1_second_element()) {
80+
throw_or_abort("SrsInitSrs: g1_points[1] does not match the canonical trusted-setup tau·G");
81+
}
82+
83+
// Defense in depth: hash-pin AND subgroup-check the G2 input. Hash equality alone is sufficient
84+
// for the canonical case (it implies prime-order membership); the subgroup check is kept so
85+
// that any future relaxation of the hash gate (e.g. a flag to allow a different trusted setup)
86+
// does not silently reopen audit finding #7's small-subgroup attack.
87+
auto g2_hash = bb::crypto::sha256(std::span<const uint8_t>(g2_point.data(), g2_point.size()));
88+
if (g2_hash != bb::srs::BN254_G2_ELEMENT_SHA256) {
89+
throw_or_abort("SrsInitSrs: g2_point bytes do not match the canonical Aztec [x]_2 SHA-256");
90+
}
5891
auto g2_point_elem = from_buffer<g2::affine_element>(g2_point.data());
5992
if (!g2_point_elem.is_in_prime_subgroup()) {
6093
throw_or_abort("SrsInitSrs: g2_point is not in the BN254 G2 prime-order subgroup");

barretenberg/cpp/src/barretenberg/crypto/ecdsa/ecdsa_impl.hpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,9 @@ ecdsa_signature ecdsa_construct_signature(const std::string& message, const ecds
2929
Fr k = crypto::deterministic_nonce_rfc6979<Hash, Fr>(message, pkey_buffer);
3030
secure_erase(pkey_buffer);
3131

32-
// Compute R = k * G
33-
typename G1::affine_element R(G1::one * k);
32+
// Compute R = k * G. k is the secret RFC6979 nonce, so use the constant-time multiplication
33+
// to defend against the Hamming-weight / bit-length timing leak in operator*.
34+
typename G1::affine_element R(typename G1::element(G1::one).mul_const_time(k));
3435

3536
// Compute the signature
3637
Fr r = Fr(R.x);

barretenberg/cpp/src/barretenberg/crypto/schnorr/schnorr.tcc

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,9 @@ schnorr_signature schnorr_construct_signature(const std::string& message, const
9090
//
9191
Fr k = Fr::random_element();
9292

93-
typename G1::affine_element R(G1::one * k);
93+
// k is a secret nonce; use the constant-time multiplication to defend against the
94+
// Hamming-weight / bit-length timing leak in operator*.
95+
typename G1::affine_element R(typename G1::element(G1::one).mul_const_time(k));
9496

9597
auto e_raw = schnorr_generate_challenge<Hash, G1>(message, public_key, R);
9698
// the conversion from e_raw results in a biased field element e

barretenberg/cpp/src/barretenberg/ecc/curves/bn254/pairing.test.cpp

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -268,10 +268,11 @@ TEST(pairing, PairingLinearityCheck)
268268
TEST(pairing, FinalExponentiation)
269269
{
270270
auto pow = [](const fq12& a, const fq& b) {
271+
const uint256_t exponent(b);
271272
fq12 result = fq12::one();
272273
fq12 base = a;
273274
for (size_t i = 0; i < 256; ++i) {
274-
if ((b.data[0] >> i) & 1) {
275+
if (exponent.get_bit(i)) {
275276
result *= base;
276277
}
277278
base = base.sqr();
@@ -291,10 +292,11 @@ TEST(pairing, FinalExponentiation)
291292
fq12 result = pairing::final_exponentiation_easy_part(element);
292293
result = pairing::final_exponentiation_tricky_part(result);
293294

294-
fq12 expected = element;
295-
expected = pairing::final_exponentiation_easy_part(expected);
296-
expected = pow(expected, mu0) + pow(expected, mu1).frobenius_map_one() + pow(expected, mu2).frobenius_map_two() +
295+
fq12 expected = pairing::final_exponentiation_easy_part(element);
296+
expected = pow(expected, mu0) * pow(expected, mu1).frobenius_map_one() * pow(expected, mu2).frobenius_map_two() *
297297
pow(expected, mu3).frobenius_map_three();
298+
299+
EXPECT_EQ(result, expected);
298300
}
299301

300302
TEST(pairing, Constants)

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -752,8 +752,13 @@ template <class T> constexpr uint64_t field<T>::is_msb_set_word() const noexcept
752752

753753
template <class T> constexpr bool field<T>::is_zero() const noexcept
754754
{
755-
return ((data[0] | data[1] | data[2] | data[3]) == 0) ||
756-
(data[0] == T::modulus_0 && data[1] == T::modulus_1 && data[2] == T::modulus_2 && data[3] == T::modulus_3);
755+
// Use bitwise OR (not || or && operator) so neither chain short-circuits: the running time must not depend on
756+
// whether the value is zero, on which limb of the modulus first matches/diverges, or on which form
757+
// (raw 0 vs the modulus) is being tested.
758+
const uint64_t raw_zero = data[0] | data[1] | data[2] | data[3];
759+
const uint64_t mod_zero =
760+
(data[0] ^ T::modulus_0) | (data[1] ^ T::modulus_1) | (data[2] ^ T::modulus_2) | (data[3] ^ T::modulus_3);
761+
return static_cast<bool>(static_cast<uint64_t>(raw_zero == 0) | static_cast<uint64_t>(mod_zero == 0));
757762
}
758763

759764
template <class T> constexpr field<T> field<T>::get_root_of_unity(size_t subgroup_size) noexcept

barretenberg/cpp/src/barretenberg/ecc/fields/prime_field.test.cpp

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,20 @@ TYPED_TEST(PrimeFieldTest, CompileTimeEquality)
117117
static_assert(!(a == f));
118118
}
119119

120+
TYPED_TEST(PrimeFieldTest, IsZeroOnModulusForm)
121+
{
122+
using F = TypeParam;
123+
124+
F modulus_form{ F::modulus.data[0], F::modulus.data[1], F::modulus.data[2], F::modulus.data[3] };
125+
EXPECT_TRUE(modulus_form.is_zero());
126+
127+
F prefix_match{ F::modulus.data[0], F::modulus.data[1], F::modulus.data[2], F::modulus.data[3] - 1 };
128+
EXPECT_FALSE(prefix_match.is_zero());
129+
130+
F first_limb_only{ F::modulus.data[0], 0, 0, 0 };
131+
EXPECT_FALSE(first_limb_only.is_zero());
132+
}
133+
120134
TYPED_TEST(PrimeFieldTest, CompileTimeSmallAddSubMul)
121135
{
122136
using F = TypeParam;

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

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,45 @@ template <typename G1> class TestAffineElement : public testing::Test {
190190
EXPECT_EQ(element(result) == expected, true);
191191
}
192192

193+
// Regression test for the large-modulus mixed-addition path: element +/- affine_element must
194+
// detect when the affine operand is the infinity sentinel (x = modulus, y = 0). Previously the
195+
// operator only checked whether `*this` was infinity, so adding the infinity sentinel to a
196+
// normal point fell through to the arithmetic and produced an off-curve garbage result.
197+
// operator-=(affine) inherits the bug via its `to_add{other.x, -other.y}` delegation.
198+
static void test_mixed_add_infinity_regression()
199+
{
200+
const element P = element::random_element();
201+
const affine_element Q_inf = affine_element::infinity();
202+
203+
// P (+/-) infinity == P, both as out-of-place and compound-assignment.
204+
EXPECT_EQ(P + Q_inf, P);
205+
EXPECT_EQ(P - Q_inf, P);
206+
{
207+
element acc = P;
208+
acc += Q_inf;
209+
EXPECT_EQ(acc, P);
210+
}
211+
{
212+
element acc = P;
213+
acc -= Q_inf;
214+
EXPECT_EQ(acc, P);
215+
}
216+
217+
// infinity (+/-) P == +/-P
218+
EXPECT_EQ(Q_inf + P, P);
219+
EXPECT_EQ(Q_inf - P, -P);
220+
221+
// *this = infinity, other = infinity must remain infinity (not become {modulus, 0, 1}).
222+
element inf_elem = element::zero();
223+
ASSERT_TRUE(inf_elem.is_point_at_infinity());
224+
EXPECT_TRUE((inf_elem + Q_inf).is_point_at_infinity());
225+
EXPECT_TRUE((inf_elem - Q_inf).is_point_at_infinity());
226+
227+
// The result of mixing a normal point with the infinity sentinel must remain on-curve.
228+
EXPECT_TRUE((P + Q_inf).on_curve());
229+
EXPECT_TRUE((P - Q_inf).on_curve());
230+
}
231+
193232
// Regression test to ensure that the point at infinity is not equal to its coordinate-wise reduction, which may lie
194233
// on the curve, depending on the y-coordinate.
195234
static void test_infinity_regression()
@@ -336,6 +375,13 @@ TYPED_TEST(TestAffineElement, AddAffine)
336375
TestFixture::test_add_affine();
337376
}
338377

378+
// Regression test for `element +/- affine_element` when the affine operand is the infinity sentinel.
379+
// Exercises both the large-modulus and small-modulus branches of `element::operator+=(affine)`.
380+
TYPED_TEST(TestAffineElement, MixedAddInfinityRegression)
381+
{
382+
TestFixture::test_mixed_add_infinity_regression();
383+
}
384+
339385
TYPED_TEST(TestAffineElement, ReadWrite)
340386
{
341387
TestFixture::test_read_and_write();
@@ -417,6 +463,25 @@ TYPED_TEST(TestAffineElement, MulWithEndomorphismMatchesMulWithoutEndomorphism)
417463
}
418464
}
419465

466+
// mul_const_time must agree with operator* on every input, including edge cases (0, 1, n-1, low and
467+
// high Hamming weight).
468+
TYPED_TEST(TestAffineElement, MulConstTimeMatchesOperatorMul)
469+
{
470+
using element_t = typename TypeParam::element;
471+
using Fr = typename TypeParam::Fr;
472+
element_t G(element_t::random_element());
473+
474+
// Edge-case scalars
475+
for (Fr s : { Fr::zero(), Fr::one(), -Fr::one(), Fr(2), Fr(uint256_t(1) << 128) }) {
476+
EXPECT_EQ(G.mul_const_time(s), G * s);
477+
}
478+
// Random scalars
479+
for (int i = 0; i < 50; ++i) {
480+
Fr s = Fr::random_element();
481+
EXPECT_EQ(G.mul_const_time(s), G * s);
482+
}
483+
}
484+
420485
// FrCodec is defined only for BN254 and Grumpkin (the two curves whose points appear in transcripts).
421486
TYPED_TEST(TestAffineElement, FrCodecRoundTrip)
422487
{

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,9 @@ template <class Fq, class Fr, class T> constexpr void affine_element<Fq, Fr, T>:
100100
x.data[1] = Fq::modulus.data[1];
101101
x.data[2] = Fq::modulus.data[2];
102102
x.data[3] = Fq::modulus.data[3];
103+
104+
// Clear y for memory hygiene
105+
y = Fq::zero();
103106
} else {
104107
(*this).x = Fq::zero();
105108
(*this).y = Fq::zero();

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,29 @@ template <class Fq, class Fr, class Params> class alignas(32) element {
8585
element operator*(const Fr& exponent) const noexcept;
8686
element operator*=(const Fr& exponent) noexcept;
8787

88+
/**
89+
* @brief Constant-time scalar multiplication intended for secret scalars (e.g. ECDSA / Schnorr nonces).
90+
*
91+
* Implementation: Montgomery ladder (Montgomery 1987 [1]; SCA-regular form: Joye & Yen,
92+
* CHES 2002 [2]) over a fixed iteration count, with Coron's first DPA countermeasure
93+
* (CHES 1999 [3]) applied to the scalar: k' = k + r * n for a fresh random 64-bit r sampled
94+
* per call. Since n * P = O in the prime-order subgroup, k' * P = k * P; the randomization
95+
* decorrelates the per-bit timing trace across signings with the same k.
96+
*
97+
* [1] P. L. Montgomery, "Speeding the Pollard and Elliptic Curve Methods of Factorization",
98+
* Mathematics of Computation 48 (1987), pp. 243-264.
99+
* [2] M. Joye and S.-M. Yen, "The Montgomery Powering Ladder", CHES 2002, LNCS 2523,
100+
* pp. 291-302.
101+
* [3] J.-S. Coron, "Resistance against Differential Power Analysis for Elliptic Curve
102+
* Cryptosystems", CHES 1999, LNCS 1717, pp. 292-302.
103+
*
104+
* @param engine Optional RNG for the blinding factor. If nullptr, uses the global RNG.
105+
*
106+
* @warning Slower than operator*. Use only when the scalar is secret. For public scalars (MSM,
107+
* public arithmetic), prefer operator*.
108+
*/
109+
element mul_const_time(const Fr& scalar, numeric::RNG* engine = nullptr) const noexcept;
110+
88111
// If you end up implementing this, congrats, you've solved the DL problem!
89112
// P.S. This is a joke, don't even attempt! 😂
90113
// constexpr Fr operator/(const element& other) noexcept {}

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

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -296,6 +296,38 @@ template <typename G_> class TestElement : public testing::Test {
296296
EXPECT_EQ(inf_element.is_point_at_infinity(), true);
297297
}
298298

299+
static void test_infinity_canonical_form()
300+
{
301+
// affine_element: infinity() (value-init then self_set_infinity) and set_infinity() applied
302+
// to a random point (copy of a non-trivial state then self_set_infinity) must match.
303+
const affine_element inf_from_factory = affine_element::infinity();
304+
const affine_element inf_from_random = affine_element(element::random_element()).set_infinity();
305+
306+
EXPECT_TRUE(inf_from_factory.is_point_at_infinity());
307+
EXPECT_TRUE(inf_from_random.is_point_at_infinity());
308+
309+
EXPECT_EQ(inf_from_factory.y, Fq::zero());
310+
EXPECT_EQ(inf_from_random.y, Fq::zero());
311+
EXPECT_EQ(inf_from_factory.x, inf_from_random.x);
312+
EXPECT_EQ(inf_from_factory.y, inf_from_random.y);
313+
314+
// element: infinity() (value-init), zero() (default-init, indeterminate y/z storage), and
315+
// set_infinity() applied to a random point must all match.
316+
const element inf_elem_factory = element::infinity();
317+
const element inf_elem_zero = element::zero();
318+
const element inf_elem_set = element::random_element().set_infinity();
319+
320+
EXPECT_TRUE(inf_elem_factory.is_point_at_infinity());
321+
EXPECT_TRUE(inf_elem_zero.is_point_at_infinity());
322+
EXPECT_TRUE(inf_elem_set.is_point_at_infinity());
323+
324+
for (const element& e : { inf_elem_factory, inf_elem_zero, inf_elem_set }) {
325+
EXPECT_EQ(e.y, Fq::zero());
326+
EXPECT_EQ(e.z, Fq::zero());
327+
EXPECT_EQ(e.x, inf_elem_factory.x);
328+
}
329+
}
330+
299331
static void test_derive_generators()
300332
{
301333
constexpr size_t num_generators = 128;
@@ -407,6 +439,11 @@ TYPED_TEST(TestElement, Infinity)
407439
TestFixture::test_infinity();
408440
}
409441

442+
TYPED_TEST(TestElement, InfinityCanonicalForm)
443+
{
444+
TestFixture::test_infinity_canonical_form();
445+
}
446+
410447
TYPED_TEST(TestElement, DeriveGenerators)
411448
{
412449
if constexpr (!std::is_same_v<typename TestFixture::G, bb::g2>) {

0 commit comments

Comments
 (0)