Skip to content

Commit

Permalink
Merge bitcoin-core/secp256k1#1207: Split fe_set_b32 into reducing and…
Browse files Browse the repository at this point in the history
… normalizing variants

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

Pull request description:

  Follow-up to bitcoin#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
  • Loading branch information
sipa committed May 11, 2023
2 parents 006ddc1 + 5b32602 commit 3353d3c
Show file tree
Hide file tree
Showing 16 changed files with 68 additions and 39 deletions.
8 changes: 4 additions & 4 deletions src/bench_internal.c
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Expand Down
3 changes: 2 additions & 1 deletion src/ecdsa_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
4 changes: 2 additions & 2 deletions src/eckey_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/ecmult_gen_compute_table_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/ecmult_gen_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
19 changes: 12 additions & 7 deletions src/field.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
5 changes: 4 additions & 1 deletion src/field_10x26_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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));
}

Expand Down
6 changes: 5 additions & 1 deletion src/field_5x52_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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));
}

Expand Down
23 changes: 18 additions & 5 deletions src/field_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/modules/extrakeys/main_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down
2 changes: 1 addition & 1 deletion src/modules/extrakeys/tests_exhaustive_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down
2 changes: 1 addition & 1 deletion src/modules/recovery/main_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion src/modules/schnorrsig/main_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
4 changes: 2 additions & 2 deletions src/secp256k1.c
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
21 changes: 12 additions & 9 deletions src/tests.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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));
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/tests_exhaustive.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit 3353d3c

Please sign in to comment.