Skip to content

Commit 3353d3c

Browse files
sipasipa
committed
Merge #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
2 parents 006ddc1 + 5b32602 commit 3353d3c

16 files changed

+68
-39
lines changed

Diff for: ‎src/bench_internal.c

+4-4
Original file line numberDiff line numberDiff line change
@@ -65,10 +65,10 @@ static void bench_setup(void* arg) {
6565

6666
secp256k1_scalar_set_b32(&data->scalar[0], init[0], NULL);
6767
secp256k1_scalar_set_b32(&data->scalar[1], init[1], NULL);
68-
secp256k1_fe_set_b32(&data->fe[0], init[0]);
69-
secp256k1_fe_set_b32(&data->fe[1], init[1]);
70-
secp256k1_fe_set_b32(&data->fe[2], init[2]);
71-
secp256k1_fe_set_b32(&data->fe[3], init[3]);
68+
secp256k1_fe_set_b32_limit(&data->fe[0], init[0]);
69+
secp256k1_fe_set_b32_limit(&data->fe[1], init[1]);
70+
secp256k1_fe_set_b32_limit(&data->fe[2], init[2]);
71+
secp256k1_fe_set_b32_limit(&data->fe[3], init[3]);
7272
CHECK(secp256k1_ge_set_xo_var(&data->ge[0], &data->fe[0], 0));
7373
CHECK(secp256k1_ge_set_xo_var(&data->ge[1], &data->fe[1], 1));
7474
secp256k1_gej_set_ge(&data->gej[0], &data->ge[0]);

Diff for: ‎src/ecdsa_impl.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -239,7 +239,8 @@ static int secp256k1_ecdsa_sig_verify(const secp256k1_scalar *sigr, const secp25
239239
}
240240
#else
241241
secp256k1_scalar_get_b32(c, sigr);
242-
secp256k1_fe_set_b32(&xr, c);
242+
/* we can ignore the fe_set_b32_limit return value, because we know the input is in range */
243+
(void)secp256k1_fe_set_b32_limit(&xr, c);
243244

244245
/** We now have the recomputed R point in pr, and its claimed x coordinate (modulo n)
245246
* in xr. Naively, we would extract the x coordinate from pr (requiring a inversion modulo p),

Diff for: ‎src/eckey_impl.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,10 @@
1717
static int secp256k1_eckey_pubkey_parse(secp256k1_ge *elem, const unsigned char *pub, size_t size) {
1818
if (size == 33 && (pub[0] == SECP256K1_TAG_PUBKEY_EVEN || pub[0] == SECP256K1_TAG_PUBKEY_ODD)) {
1919
secp256k1_fe x;
20-
return secp256k1_fe_set_b32(&x, pub+1) && secp256k1_ge_set_xo_var(elem, &x, pub[0] == SECP256K1_TAG_PUBKEY_ODD);
20+
return secp256k1_fe_set_b32_limit(&x, pub+1) && secp256k1_ge_set_xo_var(elem, &x, pub[0] == SECP256K1_TAG_PUBKEY_ODD);
2121
} else if (size == 65 && (pub[0] == SECP256K1_TAG_PUBKEY_UNCOMPRESSED || pub[0] == SECP256K1_TAG_PUBKEY_HYBRID_EVEN || pub[0] == SECP256K1_TAG_PUBKEY_HYBRID_ODD)) {
2222
secp256k1_fe x, y;
23-
if (!secp256k1_fe_set_b32(&x, pub+1) || !secp256k1_fe_set_b32(&y, pub+33)) {
23+
if (!secp256k1_fe_set_b32_limit(&x, pub+1) || !secp256k1_fe_set_b32_limit(&y, pub+33)) {
2424
return 0;
2525
}
2626
secp256k1_ge_set_xy(elem, &x, &y);

Diff for: ‎src/ecmult_gen_compute_table_impl.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ static void secp256k1_ecmult_gen_compute_table(secp256k1_ge_storage* table, cons
3131
secp256k1_fe nums_x;
3232
secp256k1_ge nums_ge;
3333
int r;
34-
r = secp256k1_fe_set_b32(&nums_x, nums_b32);
34+
r = secp256k1_fe_set_b32_limit(&nums_x, nums_b32);
3535
(void)r;
3636
VERIFY_CHECK(r);
3737
r = secp256k1_ge_set_xo_var(&nums_ge, &nums_x, 0);

Diff for: ‎src/ecmult_gen_impl.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ static void secp256k1_ecmult_gen_blind(secp256k1_ecmult_gen_context *ctx, const
108108
memset(keydata, 0, sizeof(keydata));
109109
/* Accept unobservably small non-uniformity. */
110110
secp256k1_rfc6979_hmac_sha256_generate(&rng, nonce32, 32);
111-
overflow = !secp256k1_fe_set_b32(&s, nonce32);
111+
overflow = !secp256k1_fe_set_b32_limit(&s, nonce32);
112112
overflow |= secp256k1_fe_is_zero(&s);
113113
secp256k1_fe_cmov(&s, &secp256k1_fe_one, overflow);
114114
/* Randomize the projection to defend against multiplier sidechannels.

Diff for: ‎src/field.h

+12-7
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,8 @@ static const secp256k1_fe secp256k1_const_beta = SECP256K1_FE_CONST(
8585
# define secp256k1_fe_is_zero secp256k1_fe_impl_is_zero
8686
# define secp256k1_fe_is_odd secp256k1_fe_impl_is_odd
8787
# define secp256k1_fe_cmp_var secp256k1_fe_impl_cmp_var
88-
# define secp256k1_fe_set_b32 secp256k1_fe_impl_set_b32
88+
# define secp256k1_fe_set_b32_mod secp256k1_fe_impl_set_b32_mod
89+
# define secp256k1_fe_set_b32_limit secp256k1_fe_impl_set_b32_limit
8990
# define secp256k1_fe_get_b32 secp256k1_fe_impl_get_b32
9091
# define secp256k1_fe_negate secp256k1_fe_impl_negate
9192
# 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);
189190
*/
190191
static int secp256k1_fe_cmp_var(const secp256k1_fe *a, const secp256k1_fe *b);
191192

192-
/** Set a field element equal to a provided 32-byte big endian value.
193+
/** Set a field element equal to a provided 32-byte big endian value, reducing it.
193194
*
194195
* On input, r does not need to be initalized. a must be a pointer to an initialized 32-byte array.
195-
* On output, r = a (mod p). It will have magnitude 1, and if (a < p), it will be normalized.
196-
* If not, it will only be weakly normalized. Returns whether (a < p).
196+
* On output, r = a (mod p). It will have magnitude 1, and not be normalized.
197+
*/
198+
static void secp256k1_fe_set_b32_mod(secp256k1_fe *r, const unsigned char *a);
199+
200+
/** Set a field element equal to a provided 32-byte big endian value, checking for overflow.
197201
*
198-
* Note that this function is unusual in that the normalization of the output depends on the
199-
* run-time value of a.
202+
* On input, r does not need to be initalized. a must be a pointer to an initialized 32-byte array.
203+
* On output, r = a if (a < p), it will be normalized with magnitude 1, and 1 is returned.
204+
* If a >= p, 0 is returned, and r will be made invalid (and must not be used without overwriting).
200205
*/
201-
static int secp256k1_fe_set_b32(secp256k1_fe *r, const unsigned char *a);
206+
static int secp256k1_fe_set_b32_limit(secp256k1_fe *r, const unsigned char *a);
202207

203208
/** Convert a field element to 32-byte big endian byte array.
204209
* On input, a must be a valid normalized field element, and r a pointer to a 32-byte array.

Diff for: ‎src/field_10x26_impl.h

+4-1
Original file line numberDiff line numberDiff line change
@@ -290,7 +290,7 @@ static int secp256k1_fe_impl_cmp_var(const secp256k1_fe *a, const secp256k1_fe *
290290
return 0;
291291
}
292292

293-
static int secp256k1_fe_impl_set_b32(secp256k1_fe *r, const unsigned char *a) {
293+
static void secp256k1_fe_impl_set_b32_mod(secp256k1_fe *r, const unsigned char *a) {
294294
r->n[0] = (uint32_t)a[31] | ((uint32_t)a[30] << 8) | ((uint32_t)a[29] << 16) | ((uint32_t)(a[28] & 0x3) << 24);
295295
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);
296296
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) {
301301
r->n[7] = (uint32_t)((a[9] >> 6) & 0x3) | ((uint32_t)a[8] << 2) | ((uint32_t)a[7] << 10) | ((uint32_t)a[6] << 18);
302302
r->n[8] = (uint32_t)a[5] | ((uint32_t)a[4] << 8) | ((uint32_t)a[3] << 16) | ((uint32_t)(a[2] & 0x3) << 24);
303303
r->n[9] = (uint32_t)((a[2] >> 2) & 0x3f) | ((uint32_t)a[1] << 6) | ((uint32_t)a[0] << 14);
304+
}
304305

306+
static int secp256k1_fe_impl_set_b32_limit(secp256k1_fe *r, const unsigned char *a) {
307+
secp256k1_fe_impl_set_b32_mod(r, a);
305308
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));
306309
}
307310

Diff for: ‎src/field_5x52_impl.h

+5-1
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,7 @@ static int secp256k1_fe_impl_cmp_var(const secp256k1_fe *a, const secp256k1_fe *
236236
return 0;
237237
}
238238

239-
static int secp256k1_fe_impl_set_b32(secp256k1_fe *r, const unsigned char *a) {
239+
static void secp256k1_fe_impl_set_b32_mod(secp256k1_fe *r, const unsigned char *a) {
240240
r->n[0] = (uint64_t)a[31]
241241
| ((uint64_t)a[30] << 8)
242242
| ((uint64_t)a[29] << 16)
@@ -271,6 +271,10 @@ static int secp256k1_fe_impl_set_b32(secp256k1_fe *r, const unsigned char *a) {
271271
| ((uint64_t)a[2] << 24)
272272
| ((uint64_t)a[1] << 32)
273273
| ((uint64_t)a[0] << 40);
274+
}
275+
276+
static int secp256k1_fe_impl_set_b32_limit(secp256k1_fe *r, const unsigned char *a) {
277+
secp256k1_fe_impl_set_b32_mod(r, a);
274278
return !((r->n[4] == 0x0FFFFFFFFFFFFULL) & ((r->n[3] & r->n[2] & r->n[1]) == 0xFFFFFFFFFFFFFULL) & (r->n[0] >= 0xFFFFEFFFFFC2FULL));
275279
}
276280

Diff for: ‎src/field_impl.h

+18-5
Original file line numberDiff line numberDiff line change
@@ -260,13 +260,26 @@ SECP256K1_INLINE static int secp256k1_fe_cmp_var(const secp256k1_fe *a, const se
260260
return secp256k1_fe_impl_cmp_var(a, b);
261261
}
262262

263-
static int secp256k1_fe_impl_set_b32(secp256k1_fe *r, const unsigned char *a);
264-
SECP256K1_INLINE static int secp256k1_fe_set_b32(secp256k1_fe *r, const unsigned char *a) {
265-
int ret = secp256k1_fe_impl_set_b32(r, a);
263+
static void secp256k1_fe_impl_set_b32_mod(secp256k1_fe *r, const unsigned char *a);
264+
SECP256K1_INLINE static void secp256k1_fe_set_b32_mod(secp256k1_fe *r, const unsigned char *a) {
265+
secp256k1_fe_impl_set_b32_mod(r, a);
266266
r->magnitude = 1;
267-
r->normalized = ret;
267+
r->normalized = 0;
268268
secp256k1_fe_verify(r);
269-
return ret;
269+
}
270+
271+
static int secp256k1_fe_impl_set_b32_limit(secp256k1_fe *r, const unsigned char *a);
272+
SECP256K1_INLINE static int secp256k1_fe_set_b32_limit(secp256k1_fe *r, const unsigned char *a) {
273+
if (secp256k1_fe_impl_set_b32_limit(r, a)) {
274+
r->magnitude = 1;
275+
r->normalized = 1;
276+
secp256k1_fe_verify(r);
277+
return 1;
278+
} else {
279+
/* Mark the output field element as invalid. */
280+
r->magnitude = -1;
281+
return 0;
282+
}
270283
}
271284

272285
static void secp256k1_fe_impl_get_b32(unsigned char *r, const secp256k1_fe *a);

Diff for: ‎src/modules/extrakeys/main_impl.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ int secp256k1_xonly_pubkey_parse(const secp256k1_context* ctx, secp256k1_xonly_p
2828
memset(pubkey, 0, sizeof(*pubkey));
2929
ARG_CHECK(input32 != NULL);
3030

31-
if (!secp256k1_fe_set_b32(&x, input32)) {
31+
if (!secp256k1_fe_set_b32_limit(&x, input32)) {
3232
return 0;
3333
}
3434
if (!secp256k1_ge_set_xo_var(&pk, &x, 0)) {

Diff for: ‎src/modules/extrakeys/tests_exhaustive_impl.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ static void test_exhaustive_extrakeys(const secp256k1_context *ctx, const secp25
4747
CHECK(secp256k1_memcmp_var(xonly_pubkey_bytes[i - 1], buf, 32) == 0);
4848

4949
/* Compare the xonly_pubkey bytes against the precomputed group. */
50-
secp256k1_fe_set_b32(&fe, xonly_pubkey_bytes[i - 1]);
50+
secp256k1_fe_set_b32_mod(&fe, xonly_pubkey_bytes[i - 1]);
5151
CHECK(secp256k1_fe_equal_var(&fe, &group[i].x));
5252

5353
/* Check the parity against the precomputed group. */

Diff for: ‎src/modules/recovery/main_impl.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ static int secp256k1_ecdsa_sig_recover(const secp256k1_scalar *sigr, const secp2
9898
}
9999

100100
secp256k1_scalar_get_b32(brx, sigr);
101-
r = secp256k1_fe_set_b32(&fx, brx);
101+
r = secp256k1_fe_set_b32_limit(&fx, brx);
102102
(void)r;
103103
VERIFY_CHECK(r); /* brx comes from a scalar, so is less than the order; certainly less than p */
104104
if (recid & 2) {

Diff for: ‎src/modules/schnorrsig/main_impl.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,7 @@ int secp256k1_schnorrsig_verify(const secp256k1_context* ctx, const unsigned cha
232232
ARG_CHECK(msg != NULL || msglen == 0);
233233
ARG_CHECK(pubkey != NULL);
234234

235-
if (!secp256k1_fe_set_b32(&rx, &sig64[0])) {
235+
if (!secp256k1_fe_set_b32_limit(&rx, &sig64[0])) {
236236
return 0;
237237
}
238238

Diff for: ‎src/secp256k1.c

+2-2
Original file line numberDiff line numberDiff line change
@@ -247,8 +247,8 @@ static int secp256k1_pubkey_load(const secp256k1_context* ctx, secp256k1_ge* ge,
247247
} else {
248248
/* Otherwise, fall back to 32-byte big endian for X and Y. */
249249
secp256k1_fe x, y;
250-
secp256k1_fe_set_b32(&x, pubkey->data);
251-
secp256k1_fe_set_b32(&y, pubkey->data + 32);
250+
secp256k1_fe_set_b32_mod(&x, pubkey->data);
251+
secp256k1_fe_set_b32_mod(&y, pubkey->data + 32);
252252
secp256k1_ge_set_xy(ge, &x, &y);
253253
}
254254
ARG_CHECK(!secp256k1_fe_is_zero(&ge->x));

Diff for: ‎src/tests.c

+12-9
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ static void random_field_element_test(secp256k1_fe *fe) {
9090
do {
9191
unsigned char b32[32];
9292
secp256k1_testrand256_test(b32);
93-
if (secp256k1_fe_set_b32(fe, b32)) {
93+
if (secp256k1_fe_set_b32_limit(fe, b32)) {
9494
break;
9595
}
9696
} while(1);
@@ -2957,7 +2957,7 @@ static void random_fe(secp256k1_fe *x) {
29572957
unsigned char bin[32];
29582958
do {
29592959
secp256k1_testrand256(bin);
2960-
if (secp256k1_fe_set_b32(x, bin)) {
2960+
if (secp256k1_fe_set_b32_limit(x, bin)) {
29612961
return;
29622962
}
29632963
} while(1);
@@ -2967,7 +2967,7 @@ static void random_fe_test(secp256k1_fe *x) {
29672967
unsigned char bin[32];
29682968
do {
29692969
secp256k1_testrand256_test(bin);
2970-
if (secp256k1_fe_set_b32(x, bin)) {
2970+
if (secp256k1_fe_set_b32_limit(x, bin)) {
29712971
return;
29722972
}
29732973
} while(1);
@@ -3021,7 +3021,7 @@ static void run_field_convert(void) {
30213021
unsigned char b322[32];
30223022
secp256k1_fe_storage fes2;
30233023
/* Check conversions to fe. */
3024-
CHECK(secp256k1_fe_set_b32(&fe2, b32));
3024+
CHECK(secp256k1_fe_set_b32_limit(&fe2, b32));
30253025
CHECK(secp256k1_fe_equal_var(&fe, &fe2));
30263026
secp256k1_fe_from_storage(&fe2, &fes);
30273027
CHECK(secp256k1_fe_equal_var(&fe, &fe2));
@@ -3043,7 +3043,8 @@ static void run_field_be32_overflow(void) {
30433043
static const unsigned char zero[32] = { 0x00 };
30443044
unsigned char out[32];
30453045
secp256k1_fe fe;
3046-
CHECK(secp256k1_fe_set_b32(&fe, zero_overflow) == 0);
3046+
CHECK(secp256k1_fe_set_b32_limit(&fe, zero_overflow) == 0);
3047+
secp256k1_fe_set_b32_mod(&fe, zero_overflow);
30473048
CHECK(secp256k1_fe_normalizes_to_zero(&fe) == 1);
30483049
secp256k1_fe_normalize(&fe);
30493050
CHECK(secp256k1_fe_is_zero(&fe) == 1);
@@ -3065,7 +3066,8 @@ static void run_field_be32_overflow(void) {
30653066
};
30663067
unsigned char out[32];
30673068
secp256k1_fe fe;
3068-
CHECK(secp256k1_fe_set_b32(&fe, one_overflow) == 0);
3069+
CHECK(secp256k1_fe_set_b32_limit(&fe, one_overflow) == 0);
3070+
secp256k1_fe_set_b32_mod(&fe, one_overflow);
30693071
secp256k1_fe_normalize(&fe);
30703072
CHECK(secp256k1_fe_cmp_var(&fe, &secp256k1_fe_one) == 0);
30713073
secp256k1_fe_get_b32(out, &fe);
@@ -3087,7 +3089,8 @@ static void run_field_be32_overflow(void) {
30873089
unsigned char out[32];
30883090
secp256k1_fe fe;
30893091
const secp256k1_fe fe_ff = SECP256K1_FE_CONST(0, 0, 0, 0, 0, 0, 0x01, 0x000003d0);
3090-
CHECK(secp256k1_fe_set_b32(&fe, ff_overflow) == 0);
3092+
CHECK(secp256k1_fe_set_b32_limit(&fe, ff_overflow) == 0);
3093+
secp256k1_fe_set_b32_mod(&fe, ff_overflow);
30913094
secp256k1_fe_normalize(&fe);
30923095
CHECK(secp256k1_fe_cmp_var(&fe, &fe_ff) == 0);
30933096
secp256k1_fe_get_b32(out, &fe);
@@ -3673,7 +3676,7 @@ static void run_inverse_tests(void)
36733676
b32[31] = i & 0xff;
36743677
b32[30] = (i >> 8) & 0xff;
36753678
secp256k1_scalar_set_b32(&x_scalar, b32, NULL);
3676-
secp256k1_fe_set_b32(&x_fe, b32);
3679+
secp256k1_fe_set_b32_mod(&x_fe, b32);
36773680
for (var = 0; var <= 1; ++var) {
36783681
test_inverse_scalar(NULL, &x_scalar, var);
36793682
test_inverse_field(NULL, &x_fe, var);
@@ -3690,7 +3693,7 @@ static void run_inverse_tests(void)
36903693
for (i = 0; i < 64 * COUNT; ++i) {
36913694
(testrand ? secp256k1_testrand256_test : secp256k1_testrand256)(b32);
36923695
secp256k1_scalar_set_b32(&x_scalar, b32, NULL);
3693-
secp256k1_fe_set_b32(&x_fe, b32);
3696+
secp256k1_fe_set_b32_mod(&x_fe, b32);
36943697
for (var = 0; var <= 1; ++var) {
36953698
test_inverse_scalar(NULL, &x_scalar, var);
36963699
test_inverse_field(NULL, &x_fe, var);

Diff for: ‎src/tests_exhaustive.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ static void random_fe(secp256k1_fe *x) {
6060
unsigned char bin[32];
6161
do {
6262
secp256k1_testrand256(bin);
63-
if (secp256k1_fe_set_b32(x, bin)) {
63+
if (secp256k1_fe_set_b32_limit(x, bin)) {
6464
return;
6565
}
6666
} while(1);

0 commit comments

Comments
 (0)