diff --git a/barretenberg/cpp/src/barretenberg/crypto/ecdsa/ecdsa_impl.hpp b/barretenberg/cpp/src/barretenberg/crypto/ecdsa/ecdsa_impl.hpp index 33cec9c4331..06420e60ffc 100644 --- a/barretenberg/cpp/src/barretenberg/crypto/ecdsa/ecdsa_impl.hpp +++ b/barretenberg/cpp/src/barretenberg/crypto/ecdsa/ecdsa_impl.hpp @@ -28,7 +28,11 @@ signature construct_signature(const std::string& message, const key_pair Fr r_fr = Fr::serialize_from_buffer(&sig.r[0]); Fr s_fr = (z + r_fr * account.private_key) / k; - Fr::serialize_to_buffer(s_fr, &sig.s[0]); + // Ensure that the value of s is "low", i.e. s := min{ s_fr, (|Fr| - s_fr) } + const bool is_s_low = (uint256_t(s_fr) < (uint256_t(Fr::modulus) / 2)); + uint256_t s_uint256 = is_s_low ? uint256_t(s_fr) : (uint256_t(Fr::modulus) - uint256_t(s_fr)); + + Fr::serialize_to_buffer(Fr(s_uint256), &sig.s[0]); // compute recovery_id: given R = (x, y) // 0: y is even && x < |Fr| @@ -39,9 +43,10 @@ signature construct_signature(const std::string& message, const key_pair Fq r_fq = Fq(R.x); bool is_r_finite = (uint256_t(r_fq) == uint256_t(r_fr)); bool y_parity = uint256_t(R.y).get_bit(0); + bool recovery_bit = y_parity ^ is_s_low; constexpr uint8_t offset = 27; - int value = offset + y_parity + static_cast(2) * !is_r_finite; + int value = offset + recovery_bit + static_cast(2) * !is_r_finite; ASSERT(value <= UINT8_MAX); sig.v = static_cast(value); return sig; @@ -54,6 +59,7 @@ typename G1::affine_element recover_public_key(const std::string& message, const uint256_t r_uint; uint256_t s_uint; uint8_t v_uint; + uint256_t mod = uint256_t(Fr::modulus); const auto* r_buf = &sig.r[0]; const auto* s_buf = &sig.s[0]; @@ -63,13 +69,18 @@ typename G1::affine_element recover_public_key(const std::string& message, const read(v_buf, v_uint); // We need to check that r and s are in Field according to specification - if ((r_uint >= Fr::modulus) || (s_uint >= Fr::modulus)) { + if ((r_uint >= mod) || (s_uint >= mod)) { throw_or_abort("r or s value exceeds the modulus"); } if ((r_uint == 0) || (s_uint == 0)) { throw_or_abort("r or s value is zero"); } + // Check that the s value is less than |Fr| / 2 + if (s_uint * 2 > mod) { + throw_or_abort("s value is not less than curve order by 2"); + } + // Check that v must either be in {27, 28, 29, 30} Fr r = Fr(r_uint); Fr s = Fr(s_uint); @@ -94,7 +105,7 @@ typename G1::affine_element recover_public_key(const std::string& message, const // Negate the y-coordinate point of R based on the parity of v bool y_parity_R = uint256_t(point_R.y).get_bit(0); - if ((v_uint & 1) == y_parity_R) { + if ((v_uint & 1) ^ y_parity_R) { point_R.y = -point_R.y; } @@ -119,6 +130,7 @@ bool verify_signature(const std::string& message, const typename G1::affine_elem using serialize::read; uint256_t r_uint; uint256_t s_uint; + uint256_t mod = uint256_t(Fr::modulus); if (!public_key.on_curve()) { return false; } @@ -127,12 +139,18 @@ bool verify_signature(const std::string& message, const typename G1::affine_elem read(r_buf, r_uint); read(s_buf, s_uint); // We need to check that r and s are in Field according to specification - if ((r_uint >= Fr::modulus) || (s_uint >= Fr::modulus)) { + if ((r_uint >= mod) || (s_uint >= mod)) { return false; } if ((r_uint == 0) || (s_uint == 0)) { return false; } + + // Check that the s value is less than |Fr| / 2 + if (s_uint * 2 > mod) { + throw_or_abort("s value is not less than curve order by 2"); + } + Fr r = Fr(r_uint); Fr s = Fr(s_uint); diff --git a/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa_impl.hpp b/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa_impl.hpp index 96e07e4d9c5..3f01b97f31a 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa_impl.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa_impl.hpp @@ -74,6 +74,10 @@ bool_t verify_signature(const stdlib::byte_array& message, r.assert_is_not_equal(Fr::zero()); s.assert_is_not_equal(Fr::zero()); + // s should be less than |Fr| / 2 + // Read more about this at: https://www.derpturkey.com/inherent-malleability-of-ecdsa-signatures/amp/ + s.assert_less_than((Fr::modulus + 1) / 2); + Fr u1 = z / s; Fr u2 = r / s; @@ -143,6 +147,10 @@ bool_t verify_signature_prehashed_message_noassert(const stdlib::byte_ r.assert_is_not_equal(Fr::zero()); s.assert_is_not_equal(Fr::zero()); + // s should be less than |Fr| / 2 + // Read more about this at: https://www.derpturkey.com/inherent-malleability-of-ecdsa-signatures/amp/ + s.assert_less_than((Fr::modulus + 1) / 2); + Fr u1 = z / s; Fr u2 = r / s; diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield.hpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield.hpp index 4b12a20eff9..b400baa05a1 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield.hpp @@ -248,6 +248,7 @@ template class bigfield { bigfield conditional_select(const bigfield& other, const bool_t& predicate) const; void assert_is_in_field() const; + void assert_less_than(const uint256_t upper_limit) const; void assert_equal(const bigfield& other) const; void assert_is_not_equal(const bigfield& other) const; diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield.test.cpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield.test.cpp index 2ec05ce652d..bcb0e4ce883 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield.test.cpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield.test.cpp @@ -708,6 +708,53 @@ template class stdlib_bigfield : public testing::Test { EXPECT_EQ(result, true); } + static void test_assert_less_than_success() + { + auto composer = Composer(); + size_t num_repetitions = 10; + constexpr size_t num_bits = 200; + constexpr uint256_t bit_mask = (uint256_t(1) << num_bits) - 1; + for (size_t i = 0; i < num_repetitions; ++i) { + + fq inputs[4]{ uint256_t(fq::random_element()) && bit_mask, + uint256_t(fq::random_element()) && bit_mask, + uint256_t(fq::random_element()) && bit_mask, + uint256_t(fq::random_element()) && bit_mask }; + + fq_ct a(witness_ct(&composer, fr(uint256_t(inputs[0]).slice(0, fq_ct::NUM_LIMB_BITS * 2))), + witness_ct(&composer, + fr(uint256_t(inputs[0]).slice(fq_ct::NUM_LIMB_BITS * 2, fq_ct::NUM_LIMB_BITS * 4)))); + fq_ct b(witness_ct(&composer, fr(uint256_t(inputs[1]).slice(0, fq_ct::NUM_LIMB_BITS * 2))), + witness_ct(&composer, + fr(uint256_t(inputs[1]).slice(fq_ct::NUM_LIMB_BITS * 2, fq_ct::NUM_LIMB_BITS * 4)))); + + fq_ct c = a; + fq expected = inputs[0]; + for (size_t i = 0; i < 16; ++i) { + c = b * b + c; + expected = inputs[1] * inputs[1] + expected; + } + // fq_ct c = a + a + a + a - b - b - b - b; + c.assert_less_than(bit_mask + 1); + uint256_t result = (c.get_value().lo); + EXPECT_EQ(result, uint256_t(expected)); + EXPECT_EQ(c.get_value().get_msb() < num_bits, true); + } + bool result = composer.check_circuit(); + EXPECT_EQ(result, true); + // Checking edge conditions + fq random_input = fq::random_element(); + fq_ct a(witness_ct(&composer, fr(uint256_t(random_input).slice(0, fq_ct::NUM_LIMB_BITS * 2))), + witness_ct(&composer, + fr(uint256_t(random_input).slice(fq_ct::NUM_LIMB_BITS * 2, fq_ct::NUM_LIMB_BITS * 4)))); + + a.assert_less_than(random_input + 1); + EXPECT_EQ(composer.check_circuit(), true); + + a.assert_less_than(random_input); + EXPECT_EQ(composer.check_circuit(), false); + } + static void test_byte_array_constructors() { auto composer = Composer(); @@ -866,6 +913,10 @@ TYPED_TEST(stdlib_bigfield, assert_is_in_field_succes) { TestFixture::test_assert_is_in_field_success(); } +TYPED_TEST(stdlib_bigfield, assert_less_than_success) +{ + TestFixture::test_assert_less_than_success(); +} TYPED_TEST(stdlib_bigfield, byte_array_constructors) { TestFixture::test_byte_array_constructors(); diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield_impl.hpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield_impl.hpp index 2a569e3d33e..1529ff5300d 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield_impl.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield_impl.hpp @@ -1664,6 +1664,71 @@ template void bigfield::assert_is_in_field() cons } } +template void bigfield::assert_less_than(const uint256_t upper_limit) const +{ + // TODO(kesha): Merge this with assert_is_in_field + // Warning: this assumes we have run circuit construction at least once in debug mode where large non reduced + // constants are allowed via ASSERT + if (is_constant()) { + return; + } + ASSERT(upper_limit != 0); + // The circuit checks that limit - this >= 0, so if we are doing a less_than comparison, we need to subtract 1 from + // the limit + uint256_t strict_upper_limit = upper_limit - uint256_t(1); + self_reduce(); // this method in particular enforces limb vals are <2^b - needed for logic described above + uint256_t value = get_value().lo; + + const uint256_t upper_limit_value_0 = strict_upper_limit.slice(0, NUM_LIMB_BITS); + const uint256_t upper_limit_value_1 = strict_upper_limit.slice(NUM_LIMB_BITS, NUM_LIMB_BITS * 2); + const uint256_t upper_limit_value_2 = strict_upper_limit.slice(NUM_LIMB_BITS * 2, NUM_LIMB_BITS * 3); + const uint256_t upper_limit_value_3 = strict_upper_limit.slice(NUM_LIMB_BITS * 3, NUM_LIMB_BITS * 4); + + bool borrow_0_value = value.slice(0, NUM_LIMB_BITS) > upper_limit_value_0; + bool borrow_1_value = + (value.slice(NUM_LIMB_BITS, NUM_LIMB_BITS * 2) + uint256_t(borrow_0_value)) > (upper_limit_value_1); + bool borrow_2_value = + (value.slice(NUM_LIMB_BITS * 2, NUM_LIMB_BITS * 3) + uint256_t(borrow_1_value)) > (upper_limit_value_2); + + field_t upper_limit_0(context, upper_limit_value_0); + field_t upper_limit_1(context, upper_limit_value_1); + field_t upper_limit_2(context, upper_limit_value_2); + field_t upper_limit_3(context, upper_limit_value_3); + bool_t borrow_0(witness_t(context, borrow_0_value)); + bool_t borrow_1(witness_t(context, borrow_1_value)); + bool_t borrow_2(witness_t(context, borrow_2_value)); + // The way we use borrows here ensures that we are checking that upper_limit - binary_basis > 0. + // We check that the result in each limb is > 0. + // If the modulus part in this limb is smaller, we simply borrow the value from the higher limb. + // The prover can rearrange the borrows the way they like. The important thing is that the borrows are + // constrained. + field_t r0 = upper_limit_0 - binary_basis_limbs[0].element + static_cast>(borrow_0) * shift_1; + field_t r1 = upper_limit_1 - binary_basis_limbs[1].element + static_cast>(borrow_1) * shift_1 - + static_cast>(borrow_0); + field_t r2 = upper_limit_2 - binary_basis_limbs[2].element + static_cast>(borrow_2) * shift_1 - + static_cast>(borrow_1); + field_t r3 = upper_limit_3 - binary_basis_limbs[3].element - static_cast>(borrow_2); + r0 = r0.normalize(); + r1 = r1.normalize(); + r2 = r2.normalize(); + r3 = r3.normalize(); + if constexpr (C::type == ComposerType::PLOOKUP) { + context->decompose_into_default_range(r0.witness_index, static_cast(NUM_LIMB_BITS)); + context->decompose_into_default_range(r1.witness_index, static_cast(NUM_LIMB_BITS)); + context->decompose_into_default_range(r2.witness_index, static_cast(NUM_LIMB_BITS)); + context->decompose_into_default_range(r3.witness_index, static_cast(NUM_LIMB_BITS)); + } else { + context->decompose_into_base4_accumulators( + r0.witness_index, static_cast(NUM_LIMB_BITS), "bigfield: assert_less_than range constraint 1."); + context->decompose_into_base4_accumulators( + r1.witness_index, static_cast(NUM_LIMB_BITS), "bigfield: assert_less_than range constraint 2."); + context->decompose_into_base4_accumulators( + r2.witness_index, static_cast(NUM_LIMB_BITS), "bigfield: assert_less_than range constraint 3."); + context->decompose_into_base4_accumulators( + r3.witness_index, static_cast(NUM_LIMB_BITS), "bigfield: assert_less_than range constraint 4."); + } +} + // check elements are equal mod p by proving their integer difference is a multiple of p. // This relies on the minus operator for a-b increasing a by a multiple of p large enough so diff is non-negative template void bigfield::assert_equal(const bigfield& other) const