Fable was very fixated on the zero values
That's on me, I wanted to make this testable, the original version of Fable was this:
<details><summary>field: correct secp256k1_fe_equal's max magnitude for b (31 -> 30)</summary>
secp256k1_fe_equal(a, b) (src/field_impl.h) documents and asserts via
SECP256K1_FE_VERIFY_MAGNITUDE that b may have magnitude up to 31
(contract in src/field.h:166-171). But the body negates a (magnitude <= 1),
and secp256k1_fe_negate sets the result magnitude to m+1 = 2
(field_impl.h:303), then runs secp256k1_fe_add(&na, b), which requires
r->magnitude + a->magnitude <= 32 (field_impl.h:326). With na at magnitude 2,
b's true maximum is 30, not 31.
The precondition is therefore self-contradictory: b at magnitude 31 satisfies
the stated contract (SECP256K1_FE_VERIFY_MAGNITUDE(b, 31) passes) yet aborts the
VERIFY build deep inside secp256k1_fe_add. The bound has been latent since the
fe_add magnitude assertion was added: the contract comment that introduced "31"
(7d7d43c6dd, 2022) predates that assertion, when a magnitude-33 intermediate was
merely out-of-spec rather than an abort. This file is identical to
bitcoin-core/secp256k1 master, so the off-by-one exists upstream too.
Fix: tighten the asserted/documented maximum for b to its true value, 30. Every
production caller passes b as a low-magnitude group-element coordinate or computed
value (max SECP256K1_GE_X_MAGNITUDE_MAX = 4; the fe_sqrt verify path passes
magnitude <= 8), so no caller is affected by the tightening.
Repro of the boundary bug (pre-fix), standalone VERIFY build:
secp256k1_fe a, b;
secp256k1_fe_set_int(&a, 1); /* magnitude 1 = documented max for a */
secp256k1_fe_get_bounds(&b, 31); /* magnitude 31 = documented max for b */
secp256k1_fe_equal(&a, &b);
$ clang -DVERIFY -I. -Iinclude -Isrc repro.c \
src/precomputed_ecmult.c src/precomputed_ecmult_gen.c -o repro && ./repro
a.mag=1 b.mag=31 (about to call fe_equal)
./src/field_impl.h:326: test condition failed: r->magnitude + a->magnitude <= 32
-> SIGABRT (exit 134): an input the contract declares valid crashes in fe_add.
Post-fix, same standalone:
- b magnitude 31 is now rejected at fe_equal's OWN precondition:
./src/field_impl.h:167: test condition failed: a->magnitude <= m (SIGABRT)
- b magnitude 30 returns normally (exit 0).
Verifier: new narrow regression test run_fe_equal_magnitude (src/tests.c,
registered as CASE(fe_equal_magnitude)) exercises b at the exact maximum
magnitude 30 for both the equal and unequal cases. Bumping its magnitude-30
constructions to 31 reproduces the abort.
$ ninja -C cmake-build-debug tests # Debug build defines -DVERIFY
$ ./cmake-build-debug/bin/tests -t=fe_equal_magnitude -log=1
Test fe_equal_magnitude PASSED
$ ./cmake-build-debug/bin/tests -i=2 # full suite, exit 0
Non-VERIFY build also compiles, links and passes (#ifdef VERIFY guards the
magnitude field accesses).
Limitation: VERIFY-only. In non-VERIFY builds the assertions compile out and the
limb representation has enough slack that the magnitude-33 intermediate does not
overflow, so results stay correct; this is a contract/assertion off-by-one, not a
runtime or security defect. src/field.h is an internal header, so there is no
public-API or externally observable contract change.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---
src/field.h | 2 +-
src/field_impl.h | 2 +-
src/tests.c | 43 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/src/field.h b/src/field.h
index 945029ecd8..8b25d99b66 100644
--- a/src/field.h
+++ b/src/field.h
@@ -166,7 +166,7 @@ static int secp256k1_fe_is_odd(const secp256k1_fe *a);
/** Determine whether two field elements are equal.
*
* On input, a and b must be valid field elements with magnitudes not exceeding
- * 1 and 31, respectively.
+ * 1 and 30, respectively.
* Returns a = b (mod p).
*/
static int secp256k1_fe_equal(const secp256k1_fe *a, const secp256k1_fe *b);
diff --git a/src/field_impl.h b/src/field_impl.h
index 7aa7de431a..19af6aa26f 100644
--- a/src/field_impl.h
+++ b/src/field_impl.h
@@ -27,7 +27,7 @@ SECP256K1_INLINE static int secp256k1_fe_equal(const secp256k1_fe *a, const secp
SECP256K1_FE_VERIFY(a);
SECP256K1_FE_VERIFY(b);
SECP256K1_FE_VERIFY_MAGNITUDE(a, 1);
- SECP256K1_FE_VERIFY_MAGNITUDE(b, 31);
+ SECP256K1_FE_VERIFY_MAGNITUDE(b, 30);
secp256k1_fe_negate(&na, a, 1);
secp256k1_fe_add(&na, b);
diff --git a/src/tests.c b/src/tests.c
index 6c3cd39f2f..8b64a5fb05 100644
--- a/src/tests.c
+++ b/src/tests.c
@@ -3059,6 +3059,48 @@ static int fe_equal(const secp256k1_fe *a, const secp256k1_fe *b) {
return secp256k1_fe_equal(&an, &bn);
}
+/* secp256k1_fe_equal(a, b) accepts a with magnitude up to 1 and b with magnitude
+ * up to 30 (see the contract in src/field.h). It negates a (raising its magnitude
+ * to 2) and adds b, and secp256k1_fe_add requires the operand magnitudes to sum to
+ * at most 32, so b's true maximum magnitude is 30, not 31. This pins that exact
+ * boundary: at magnitude 30 the comparison must work and return the correct
+ * result. Bumping the magnitude-30 constructions below to 31 aborts inside
+ * secp256k1_fe_add (./src/field_impl.h: r->magnitude + a->magnitude <= 32) under
+ * VERIFY, which is the boundary bug this guards against. */
+static void run_fe_equal_magnitude(void) {
+ secp256k1_fe a, b, zero_mag29;
+ int i;
+
+ /* zero_mag29 = 0 (mod p) carried at magnitude 29; adding it to a magnitude-1
+ * element leaves the value unchanged but raises its magnitude to 30. */
+ secp256k1_fe_set_int(&zero_mag29, 0);
+ secp256k1_fe_negate(&zero_mag29, &zero_mag29, 0); /* 0 (mod p), magnitude 1 */
+ secp256k1_fe_mul_int_unchecked(&zero_mag29, 29); /* 0 (mod p), magnitude 29 */
+#ifdef VERIFY
+ CHECK(zero_mag29.magnitude == 29);
+#endif
+
+ for (i = 1; i <= 16; i++) {
+ secp256k1_fe_set_int(&a, i);
+
+ /* Equal case: b has the same value as a, carried at magnitude 30. */
+ secp256k1_fe_set_int(&b, i);
+ secp256k1_fe_add(&b, &zero_mag29);
+#ifdef VERIFY
+ CHECK(a.magnitude == 1 && b.magnitude == 30);
+#endif
+ CHECK(secp256k1_fe_equal(&a, &b));
+
+ /* Unequal case: b differs from a, still carried at magnitude 30. */
+ secp256k1_fe_set_int(&b, i + 1);
+ secp256k1_fe_add(&b, &zero_mag29);
+#ifdef VERIFY
+ CHECK(b.magnitude == 30);
+#endif
+ CHECK(!secp256k1_fe_equal(&a, &b));
+ }
+}
+
static void run_field_convert(void) {
static const unsigned char b32[32] = {
0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
@@ -7964,6 +8006,7 @@ static const struct tf_test_entry tests_scalar[] = {
static const struct tf_test_entry tests_field[] = {
CASE(field_half),
CASE(field_misc),
+ CASE(fe_equal_magnitude),
CASE(field_convert),
CASE(field_be32_overflow),
CASE(fe_mul),
</details>
