Split fe_set_b32 into reducing and normalizing variants

bitcoin-core · Feb 5, 2023 · 21c5b28 · 21c5b28
1 parent 727d50a
commit 21c5b28
Show file tree

Hide file tree

Showing 15 changed files with 78 additions and 37 deletions.
diff --git a/src/bench_internal.c b/src/bench_internal.c
@@ -65,10 +65,10 @@ static void bench_setup(void* arg) {
 
     secp256k1_scalar_set_b32(&data->scalar[0], init[0], NULL);
     secp256k1_scalar_set_b32(&data->scalar[1], init[1], NULL);
-    secp256k1_fe_set_b32(&data->fe[0], init[0]);
-    secp256k1_fe_set_b32(&data->fe[1], init[1]);
-    secp256k1_fe_set_b32(&data->fe[2], init[2]);
-    secp256k1_fe_set_b32(&data->fe[3], init[3]);
+    secp256k1_fe_set_b32_mod(&data->fe[0], init[0]);
+    secp256k1_fe_set_b32_mod(&data->fe[1], init[1]);
+    secp256k1_fe_set_b32_mod(&data->fe[2], init[2]);
+    secp256k1_fe_set_b32_mod(&data->fe[3], init[3]);
     CHECK(secp256k1_ge_set_xo_var(&data->ge[0], &data->fe[0], 0));
     CHECK(secp256k1_ge_set_xo_var(&data->ge[1], &data->fe[1], 1));
     secp256k1_gej_set_ge(&data->gej[0], &data->ge[0]);

diff --git a/src/ecdsa_impl.h b/src/ecdsa_impl.h
@@ -239,7 +239,8 @@ static int secp256k1_ecdsa_sig_verify(const secp256k1_scalar *sigr, const secp25
 }
 #else
     secp256k1_scalar_get_b32(c, sigr);
-    secp256k1_fe_set_b32(&xr, c);
+    /* we can ignore the fe_set_b32_limit return value, because we know the input is in range */
+    (void)secp256k1_fe_set_b32_limit(&xr, c);
 
     /** We now have the recomputed R point in pr, and its claimed x coordinate (modulo n)
      *  in xr. Naively, we would extract the x coordinate from pr (requiring a inversion modulo p),

diff --git a/src/eckey_impl.h b/src/eckey_impl.h
@@ -17,10 +17,10 @@
 static int secp256k1_eckey_pubkey_parse(secp256k1_ge *elem, const unsigned char *pub, size_t size) {
     if (size == 33 && (pub[0] == SECP256K1_TAG_PUBKEY_EVEN || pub[0] == SECP256K1_TAG_PUBKEY_ODD)) {
         secp256k1_fe x;
-        return secp256k1_fe_set_b32(&x, pub+1) && secp256k1_ge_set_xo_var(elem, &x, pub[0] == SECP256K1_TAG_PUBKEY_ODD);
+        return secp256k1_fe_set_b32_limit(&x, pub+1) && secp256k1_ge_set_xo_var(elem, &x, pub[0] == SECP256K1_TAG_PUBKEY_ODD);
     } else if (size == 65 && (pub[0] == SECP256K1_TAG_PUBKEY_UNCOMPRESSED || pub[0] == SECP256K1_TAG_PUBKEY_HYBRID_EVEN || pub[0] == SECP256K1_TAG_PUBKEY_HYBRID_ODD)) {
         secp256k1_fe x, y;
-        if (!secp256k1_fe_set_b32(&x, pub+1) || !secp256k1_fe_set_b32(&y, pub+33)) {
+        if (!secp256k1_fe_set_b32_limit(&x, pub+1) || !secp256k1_fe_set_b32_limit(&y, pub+33)) {
             return 0;
         }
         secp256k1_ge_set_xy(elem, &x, &y);

diff --git a/src/ecmult_gen_compute_table_impl.h b/src/ecmult_gen_compute_table_impl.h
@@ -31,7 +31,7 @@ static void secp256k1_ecmult_gen_compute_table(secp256k1_ge_storage* table, cons
         secp256k1_fe nums_x;
         secp256k1_ge nums_ge;
         int r;
-        r = secp256k1_fe_set_b32(&nums_x, nums_b32);
+        r = secp256k1_fe_set_b32_limit(&nums_x, nums_b32);
         (void)r;
         VERIFY_CHECK(r);
         r = secp256k1_ge_set_xo_var(&nums_ge, &nums_x, 0);

diff --git a/src/ecmult_gen_impl.h b/src/ecmult_gen_impl.h
@@ -108,7 +108,7 @@ static void secp256k1_ecmult_gen_blind(secp256k1_ecmult_gen_context *ctx, const
     memset(keydata, 0, sizeof(keydata));
     /* Accept unobservably small non-uniformity. */
     secp256k1_rfc6979_hmac_sha256_generate(&rng, nonce32, 32);
-    overflow = !secp256k1_fe_set_b32(&s, nonce32);
+    overflow = !secp256k1_fe_set_b32_limit(&s, nonce32);
     overflow |= secp256k1_fe_is_zero(&s);
     secp256k1_fe_cmov(&s, &secp256k1_fe_one, overflow);
     /* Randomize the projection to defend against multiplier sidechannels.

diff --git a/src/field.h b/src/field.h
@@ -75,10 +75,14 @@ static int secp256k1_fe_equal_var(const secp256k1_fe *a, const secp256k1_fe *b);
 /** Compare two field elements. Requires both inputs to be normalized */
 static int secp256k1_fe_cmp_var(const secp256k1_fe *a, const secp256k1_fe *b);
 
-/** Set a field element equal to 32-byte big endian value.
- *  Returns 1 if no overflow occurred, and then the output is normalized.
- *  Returns 0 if overflow occurred, and then the output is only weakly normalized. */
-static int secp256k1_fe_set_b32(secp256k1_fe *r, const unsigned char *a);
+/** Set a field element equal to 32-byte big endian value, reducing modulo the curve
+ *  order if need be. The output is weakly normalized. */
+static void secp256k1_fe_set_b32_mod(secp256k1_fe *r, const unsigned char *a);
+
+/** Set a field element equal to 32-byte big endian value. If the input is >= the
+ *  curve order, r is set to an indeterminate value and 0 is returned. Otherwise,
+ *  1 is returned and the output is normalized. */
+static int secp256k1_fe_set_b32_limit(secp256k1_fe *r, const unsigned char *a);
 
 /** Convert a field element to a 32-byte big endian value. Requires the input to be normalized */
 static void secp256k1_fe_get_b32(unsigned char *r, const secp256k1_fe *a);

diff --git a/src/field_10x26_impl.h b/src/field_10x26_impl.h
@@ -351,8 +351,7 @@ static int secp256k1_fe_cmp_var(const secp256k1_fe *a, const secp256k1_fe *b) {
     return 0;
 }
 
-static int secp256k1_fe_set_b32(secp256k1_fe *r, const unsigned char *a) {
-    int ret;
+static void secp256k1_fe_set_b32_mod(secp256k1_fe *r, const unsigned char *a) {
     r->n[0] = (uint32_t)a[31] | ((uint32_t)a[30] << 8) | ((uint32_t)a[29] << 16) | ((uint32_t)(a[28] & 0x3) << 24);
     r->n[1] = (uint32_t)((a[28] >> 2) & 0x3f) | ((uint32_t)a[27] << 6) | ((uint32_t)a[26] << 14) | ((uint32_t)(a[25] & 0xf) << 22);
     r->n[2] = (uint32_t)((a[25] >> 4) & 0xf) | ((uint32_t)a[24] << 4) | ((uint32_t)a[23] << 12) | ((uint32_t)(a[22] & 0x3f) << 20);
@@ -364,12 +363,29 @@ static int secp256k1_fe_set_b32(secp256k1_fe *r, const unsigned char *a) {
     r->n[8] = (uint32_t)a[5] | ((uint32_t)a[4] << 8) | ((uint32_t)a[3] << 16) | ((uint32_t)(a[2] & 0x3) << 24);
     r->n[9] = (uint32_t)((a[2] >> 2) & 0x3f) | ((uint32_t)a[1] << 6) | ((uint32_t)a[0] << 14);
 
-    ret = !((r->n[9] == 0x3FFFFFUL) & ((r->n[8] & r->n[7] & r->n[6] & r->n[5] & r->n[4] & r->n[3] & r->n[2]) == 0x3FFFFFFUL) & ((r->n[1] + 0x40UL + ((r->n[0] + 0x3D1UL) >> 26)) > 0x3FFFFFFUL));
 #ifdef VERIFY
     r->magnitude = 1;
-    r->normalized = ret;
+    r->normalized = 0;
     secp256k1_fe_verify(r);
 #endif
+}
+
+static int secp256k1_fe_set_b32_limit(secp256k1_fe *r, const unsigned char *a) {
+    int ret;
+
+    secp256k1_fe_set_b32_mod(r, a);
+    ret = !((r->n[9] == 0x3FFFFFUL) & ((r->n[8] & r->n[7] & r->n[6] & r->n[5] & r->n[4] & r->n[3] & r->n[2]) == 0x3FFFFFFUL) & ((r->n[1] + 0x40UL + ((r->n[0] + 0x3D1UL) >> 26)) > 0x3FFFFFFUL));
+
+#ifdef VERIFY
+    if (ret) {
+        r->magnitude = 1;
+        r->normalized = 1;
+        secp256k1_fe_verify(r);
+    } else {
+        r->n[4] ^= 1; /* change value, hopefully triggering errors if the value is used. */
+    }
+#endif
+
     return ret;
 }
 

diff --git a/src/field_5x52_impl.h b/src/field_5x52_impl.h
@@ -303,8 +303,7 @@ static int secp256k1_fe_cmp_var(const secp256k1_fe *a, const secp256k1_fe *b) {
     return 0;
 }
 
-static int secp256k1_fe_set_b32(secp256k1_fe *r, const unsigned char *a) {
-    int ret;
+static void secp256k1_fe_set_b32_mod(secp256k1_fe *r, const unsigned char *a) {
     r->n[0] = (uint64_t)a[31]
             | ((uint64_t)a[30] << 8)
             | ((uint64_t)a[29] << 16)
@@ -339,12 +338,30 @@ static int secp256k1_fe_set_b32(secp256k1_fe *r, const unsigned char *a) {
             | ((uint64_t)a[2] << 24)
             | ((uint64_t)a[1] << 32)
             | ((uint64_t)a[0] << 40);
-    ret = !((r->n[4] == 0x0FFFFFFFFFFFFULL) & ((r->n[3] & r->n[2] & r->n[1]) == 0xFFFFFFFFFFFFFULL) & (r->n[0] >= 0xFFFFEFFFFFC2FULL));
+
 #ifdef VERIFY
     r->magnitude = 1;
-    r->normalized = ret;
+    r->normalized = 0;
     secp256k1_fe_verify(r);
 #endif
+}
+
+static int secp256k1_fe_set_b32_limit(secp256k1_fe *r, const unsigned char *a) {
+    int ret;
+
+    secp256k1_fe_set_b32_mod(r, a);
+    ret = !((r->n[4] == 0x0FFFFFFFFFFFFULL) & ((r->n[3] & r->n[2] & r->n[1]) == 0xFFFFFFFFFFFFFULL) & (r->n[0] >= 0xFFFFEFFFFFC2FULL));
+
+#ifdef VERIFY
+    if (ret) {
+        r->magnitude = 1;
+        r->normalized = 1;
+        secp256k1_fe_verify(r);
+    } else {
+        r->n[2] ^= 1; /* change value, hopefully triggering errors if the value is used. */
+    }
+#endif
+
     return ret;
 }
 

diff --git a/src/modules/extrakeys/main_impl.h b/src/modules/extrakeys/main_impl.h
@@ -27,7 +27,7 @@ int secp256k1_xonly_pubkey_parse(const secp256k1_context* ctx, secp256k1_xonly_p
     memset(pubkey, 0, sizeof(*pubkey));
     ARG_CHECK(input32 != NULL);
 
-    if (!secp256k1_fe_set_b32(&x, input32)) {
+    if (!secp256k1_fe_set_b32_limit(&x, input32)) {
         return 0;
     }
     if (!secp256k1_ge_set_xo_var(&pk, &x, 0)) {

diff --git a/src/modules/extrakeys/tests_exhaustive_impl.h b/src/modules/extrakeys/tests_exhaustive_impl.h
@@ -47,7 +47,7 @@ static void test_exhaustive_extrakeys(const secp256k1_context *ctx, const secp25
         CHECK(secp256k1_memcmp_var(xonly_pubkey_bytes[i - 1], buf, 32) == 0);
 
         /* Compare the xonly_pubkey bytes against the precomputed group. */
-        secp256k1_fe_set_b32(&fe, xonly_pubkey_bytes[i - 1]);
+        secp256k1_fe_set_b32_mod(&fe, xonly_pubkey_bytes[i - 1]);
         CHECK(secp256k1_fe_equal_var(&fe, &group[i].x));
 
         /* Check the parity against the precomputed group. */

diff --git a/src/modules/recovery/main_impl.h b/src/modules/recovery/main_impl.h
@@ -98,7 +98,7 @@ static int secp256k1_ecdsa_sig_recover(const secp256k1_scalar *sigr, const secp2
     }
 
     secp256k1_scalar_get_b32(brx, sigr);
-    r = secp256k1_fe_set_b32(&fx, brx);
+    r = secp256k1_fe_set_b32_limit(&fx, brx);
     (void)r;
     VERIFY_CHECK(r); /* brx comes from a scalar, so is less than the order; certainly less than p */
     if (recid & 2) {

diff --git a/src/modules/schnorrsig/main_impl.h b/src/modules/schnorrsig/main_impl.h
@@ -232,7 +232,7 @@ int secp256k1_schnorrsig_verify(const secp256k1_context* ctx, const unsigned cha
     ARG_CHECK(msg != NULL || msglen == 0);
     ARG_CHECK(pubkey != NULL);
 
-    if (!secp256k1_fe_set_b32(&rx, &sig64[0])) {
+    if (!secp256k1_fe_set_b32_limit(&rx, &sig64[0])) {
         return 0;
     }
 

diff --git a/src/secp256k1.c b/src/secp256k1.c
@@ -247,8 +247,8 @@ static int secp256k1_pubkey_load(const secp256k1_context* ctx, secp256k1_ge* ge,
     } else {
         /* Otherwise, fall back to 32-byte big endian for X and Y. */
         secp256k1_fe x, y;
-        secp256k1_fe_set_b32(&x, pubkey->data);
-        secp256k1_fe_set_b32(&y, pubkey->data + 32);
+        secp256k1_fe_set_b32_mod(&x, pubkey->data);
+        secp256k1_fe_set_b32_mod(&y, pubkey->data + 32);
         secp256k1_ge_set_xy(ge, &x, &y);
     }
     ARG_CHECK(!secp256k1_fe_is_zero(&ge->x));

diff --git a/src/tests.c b/src/tests.c
@@ -100,7 +100,7 @@ static void random_field_element_test(secp256k1_fe *fe) {
     do {
         unsigned char b32[32];
         secp256k1_testrand256_test(b32);
-        if (secp256k1_fe_set_b32(fe, b32)) {
+        if (secp256k1_fe_set_b32_limit(fe, b32)) {
             break;
         }
     } while(1);
@@ -2901,7 +2901,7 @@ static void random_fe(secp256k1_fe *x) {
     unsigned char bin[32];
     do {
         secp256k1_testrand256(bin);
-        if (secp256k1_fe_set_b32(x, bin)) {
+        if (secp256k1_fe_set_b32_limit(x, bin)) {
             return;
         }
     } while(1);
@@ -2911,7 +2911,7 @@ static void random_fe_test(secp256k1_fe *x) {
     unsigned char bin[32];
     do {
         secp256k1_testrand256_test(bin);
-        if (secp256k1_fe_set_b32(x, bin)) {
+        if (secp256k1_fe_set_b32_limit(x, bin)) {
             return;
         }
     } while(1);
@@ -2965,7 +2965,7 @@ static void run_field_convert(void) {
     unsigned char b322[32];
     secp256k1_fe_storage fes2;
     /* Check conversions to fe. */
-    CHECK(secp256k1_fe_set_b32(&fe2, b32));
+    CHECK(secp256k1_fe_set_b32_limit(&fe2, b32));
     CHECK(secp256k1_fe_equal_var(&fe, &fe2));
     secp256k1_fe_from_storage(&fe2, &fes);
     CHECK(secp256k1_fe_equal_var(&fe, &fe2));
@@ -2992,7 +2992,8 @@ static void run_field_be32_overflow(void) {
         };
         unsigned char out[32];
         secp256k1_fe fe;
-        CHECK(secp256k1_fe_set_b32(&fe, zero_overflow) == 0);
+        CHECK(secp256k1_fe_set_b32_limit(&fe, zero_overflow) == 0);
+        secp256k1_fe_set_b32_mod(&fe, zero_overflow);
         CHECK(secp256k1_fe_normalizes_to_zero(&fe) == 1);
         secp256k1_fe_normalize(&fe);
         CHECK(secp256k1_fe_is_zero(&fe) == 1);
@@ -3014,7 +3015,8 @@ static void run_field_be32_overflow(void) {
         };
         unsigned char out[32];
         secp256k1_fe fe;
-        CHECK(secp256k1_fe_set_b32(&fe, one_overflow) == 0);
+        CHECK(secp256k1_fe_set_b32_limit(&fe, one_overflow) == 0);
+        secp256k1_fe_set_b32_mod(&fe, one_overflow);
         secp256k1_fe_normalize(&fe);
         CHECK(secp256k1_fe_cmp_var(&fe, &secp256k1_fe_one) == 0);
         secp256k1_fe_get_b32(out, &fe);
@@ -3036,7 +3038,8 @@ static void run_field_be32_overflow(void) {
         unsigned char out[32];
         secp256k1_fe fe;
         const secp256k1_fe fe_ff = SECP256K1_FE_CONST(0, 0, 0, 0, 0, 0, 0x01, 0x000003d0);
-        CHECK(secp256k1_fe_set_b32(&fe, ff_overflow) == 0);
+        CHECK(secp256k1_fe_set_b32_limit(&fe, ff_overflow) == 0);
+        secp256k1_fe_set_b32_mod(&fe, ff_overflow);
         secp256k1_fe_normalize(&fe);
         CHECK(secp256k1_fe_cmp_var(&fe, &fe_ff) == 0);
         secp256k1_fe_get_b32(out, &fe);
@@ -3611,7 +3614,7 @@ static void run_inverse_tests(void)
         b32[31] = i & 0xff;
         b32[30] = (i >> 8) & 0xff;
         secp256k1_scalar_set_b32(&x_scalar, b32, NULL);
-        secp256k1_fe_set_b32(&x_fe, b32);
+        secp256k1_fe_set_b32_mod(&x_fe, b32);
         for (var = 0; var <= 1; ++var) {
             test_inverse_scalar(NULL, &x_scalar, var);
             test_inverse_field(NULL, &x_fe, var);
@@ -3628,7 +3631,7 @@ static void run_inverse_tests(void)
         for (i = 0; i < 64 * COUNT; ++i) {
             (testrand ? secp256k1_testrand256_test : secp256k1_testrand256)(b32);
             secp256k1_scalar_set_b32(&x_scalar, b32, NULL);
-            secp256k1_fe_set_b32(&x_fe, b32);
+            secp256k1_fe_set_b32_mod(&x_fe, b32);
             for (var = 0; var <= 1; ++var) {
                 test_inverse_scalar(NULL, &x_scalar, var);
                 test_inverse_field(NULL, &x_fe, var);

diff --git a/src/tests_exhaustive.c b/src/tests_exhaustive.c
@@ -54,7 +54,7 @@ static void random_fe(secp256k1_fe *x) {
     unsigned char bin[32];
     do {
         secp256k1_testrand256(bin);
-        if (secp256k1_fe_set_b32(x, bin)) {
+        if (secp256k1_fe_set_b32_limit(x, bin)) {
             return;
         }
     } while(1);