Merge bitcoin-core/secp256k1#1207: Split fe_set_b32 into reducing and…

… normalizing variants

5b32602 Split fe_set_b32 into reducing and normalizing variants (Pieter Wuille)

Pull request description:

  Follow-up to #1205.

  This splits the `secp256k1_fe_set_b32` function into two variants:
  * `secp256k1_fe_set_b32_mod`, which returns `void`, reduces modulo the curve order, and only promises weakly normalized output.
  * `secp256k1_fe_set_b32_limit`, which returns `int` indicating success/failure, and only promises valid output in case the input is in range (but guarantees it's strongly normalized in this case).

  This removes one of the few cases in the codebase where normalization status depends on runtime values, making it fixed at compile-time instead.

ACKs for top commit:
  real-or-random:
    ACK 5b32602
  jonasnick:
    ACK 5b32602

Tree-SHA512: 4b93502272638c6ecdef4d74afa629e7ee540c0a20b377dccedbe567857b56c4684fad3af4b4293ed7ba35fed4aa5d0beaacdd77a903f44f24e8d87305919b61

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_limit(&data->fe[0], init[0]);
+    secp256k1_fe_set_b32_limit(&data->fe[1], init[1]);
+    secp256k1_fe_set_b32_limit(&data->fe[2], init[2]);
+    secp256k1_fe_set_b32_limit(&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]);

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),

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);

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);

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.

src/field.h

@@ -85,7 +85,8 @@ static const secp256k1_fe secp256k1_const_beta = SECP256K1_FE_CONST(
 #  define secp256k1_fe_is_zero secp256k1_fe_impl_is_zero
 #  define secp256k1_fe_is_odd secp256k1_fe_impl_is_odd
 #  define secp256k1_fe_cmp_var secp256k1_fe_impl_cmp_var
-#  define secp256k1_fe_set_b32 secp256k1_fe_impl_set_b32
+#  define secp256k1_fe_set_b32_mod secp256k1_fe_impl_set_b32_mod
+#  define secp256k1_fe_set_b32_limit secp256k1_fe_impl_set_b32_limit
 #  define secp256k1_fe_get_b32 secp256k1_fe_impl_get_b32
 #  define secp256k1_fe_negate secp256k1_fe_impl_negate
 #  define secp256k1_fe_mul_int secp256k1_fe_impl_mul_int
@@ -189,16 +190,20 @@ static int secp256k1_fe_equal_var(const secp256k1_fe *a, const secp256k1_fe *b);
  */
 static int secp256k1_fe_cmp_var(const secp256k1_fe *a, const secp256k1_fe *b);
 
-/** Set a field element equal to a provided 32-byte big endian value.
+/** Set a field element equal to a provided 32-byte big endian value, reducing it.
  *
  * On input, r does not need to be initalized. a must be a pointer to an initialized 32-byte array.
- * On output, r = a (mod p). It will have magnitude 1, and if (a < p), it will be normalized.
- * If not, it will only be weakly normalized. Returns whether (a < p).
+ * On output, r = a (mod p). It will have magnitude 1, and not be normalized.
+ */
+static void secp256k1_fe_set_b32_mod(secp256k1_fe *r, const unsigned char *a);
+
+/** Set a field element equal to a provided 32-byte big endian value, checking for overflow.
  *
- * Note that this function is unusual in that the normalization of the output depends on the
- * run-time value of a.
+ * On input, r does not need to be initalized. a must be a pointer to an initialized 32-byte array.
+ * On output, r = a if (a < p), it will be normalized with magnitude 1, and 1 is returned.
+ * If a >= p, 0 is returned, and r will be made invalid (and must not be used without overwriting).
  */
-static int secp256k1_fe_set_b32(secp256k1_fe *r, const unsigned char *a);
+static int secp256k1_fe_set_b32_limit(secp256k1_fe *r, const unsigned char *a);
 
 /** Convert a field element to 32-byte big endian byte array.
  * On input, a must be a valid normalized field element, and r a pointer to a 32-byte array.

src/field_10x26_impl.h

@@ -290,7 +290,7 @@ static int secp256k1_fe_impl_cmp_var(const secp256k1_fe *a, const secp256k1_fe *
     return 0;
 }
 
-static int secp256k1_fe_impl_set_b32(secp256k1_fe *r, const unsigned char *a) {
+static void secp256k1_fe_impl_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);
@@ -301,7 +301,10 @@ static int secp256k1_fe_impl_set_b32(secp256k1_fe *r, const unsigned char *a) {
     r->n[7] = (uint32_t)((a[9] >> 6) & 0x3) | ((uint32_t)a[8] << 2) | ((uint32_t)a[7] << 10) | ((uint32_t)a[6] << 18);
     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);
+}
 
+static int secp256k1_fe_impl_set_b32_limit(secp256k1_fe *r, const unsigned char *a) {
+    secp256k1_fe_impl_set_b32_mod(r, a);
     return !((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));
 }
 

src/field_5x52_impl.h

@@ -236,7 +236,7 @@ static int secp256k1_fe_impl_cmp_var(const secp256k1_fe *a, const secp256k1_fe *
     return 0;
 }
 
-static int secp256k1_fe_impl_set_b32(secp256k1_fe *r, const unsigned char *a) {
+static void secp256k1_fe_impl_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)
@@ -271,6 +271,10 @@ static int secp256k1_fe_impl_set_b32(secp256k1_fe *r, const unsigned char *a) {
             | ((uint64_t)a[2] << 24)
             | ((uint64_t)a[1] << 32)
             | ((uint64_t)a[0] << 40);
+}
+
+static int secp256k1_fe_impl_set_b32_limit(secp256k1_fe *r, const unsigned char *a) {
+    secp256k1_fe_impl_set_b32_mod(r, a);
     return !((r->n[4] == 0x0FFFFFFFFFFFFULL) & ((r->n[3] & r->n[2] & r->n[1]) == 0xFFFFFFFFFFFFFULL) & (r->n[0] >= 0xFFFFEFFFFFC2FULL));
 }
 

src/field_impl.h

@@ -260,13 +260,26 @@ SECP256K1_INLINE static int secp256k1_fe_cmp_var(const secp256k1_fe *a, const se
     return secp256k1_fe_impl_cmp_var(a, b);
 }
 
-static int secp256k1_fe_impl_set_b32(secp256k1_fe *r, const unsigned char *a);
-SECP256K1_INLINE static int secp256k1_fe_set_b32(secp256k1_fe *r, const unsigned char *a) {
-    int ret = secp256k1_fe_impl_set_b32(r, a);
+static void secp256k1_fe_impl_set_b32_mod(secp256k1_fe *r, const unsigned char *a);
+SECP256K1_INLINE static void secp256k1_fe_set_b32_mod(secp256k1_fe *r, const unsigned char *a) {
+    secp256k1_fe_impl_set_b32_mod(r, a);
     r->magnitude = 1;
-    r->normalized = ret;
+    r->normalized = 0;
     secp256k1_fe_verify(r);
-    return ret;
+}
+
+static int secp256k1_fe_impl_set_b32_limit(secp256k1_fe *r, const unsigned char *a);
+SECP256K1_INLINE static int secp256k1_fe_set_b32_limit(secp256k1_fe *r, const unsigned char *a) {
+    if (secp256k1_fe_impl_set_b32_limit(r, a)) {
+        r->magnitude = 1;
+        r->normalized = 1;
+        secp256k1_fe_verify(r);
+        return 1;
+    } else {
+        /* Mark the output field element as invalid. */
+        r->magnitude = -1;
+        return 0;
+    }
 }
 
 static void secp256k1_fe_impl_get_b32(unsigned char *r, const secp256k1_fe *a);

src/modules/extrakeys/main_impl.h

@@ -28,7 +28,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)) {

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. */

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) {

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;
     }
 

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));

src/tests.c

@@ -90,7 +90,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);
@@ -2957,7 +2957,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);
@@ -2967,7 +2967,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);
@@ -3021,7 +3021,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));
@@ -3043,7 +3043,8 @@ static void run_field_be32_overflow(void) {
         static const unsigned char zero[32] = { 0x00 };
         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);
@@ -3065,7 +3066,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);
@@ -3087,7 +3089,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);
@@ -3673,7 +3676,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);
@@ -3690,7 +3693,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);

src/tests_exhaustive.c

@@ -60,7 +60,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);