Skip to content

Commit d907ebc

Browse files
sipasipa
committed
Add bounds checking to field element setters
1 parent bb2cd94 commit d907ebc

File tree

10 files changed

+37
-24
lines changed

10 files changed

+37
-24
lines changed

‎src/ecdsa_impl.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ static int secp256k1_ecdsa_sig_recover(const secp256k1_ecdsa_sig_t *sig, secp256
108108
secp256k1_num_get_bin(brx, 32, &rx);
109109
secp256k1_num_free(&rx);
110110
secp256k1_fe_t fx;
111-
secp256k1_fe_set_b32(&fx, brx);
111+
VERIFY_CHECK(secp256k1_fe_set_b32(&fx, brx)); /* Either rx < n (and n < p), or rx + n < p (checked above). */
112112
secp256k1_ge_t x;
113113
if (!secp256k1_ge_set_xo(&x, &fx, recid & 1))
114114
return 0;

‎src/eckey_impl.h‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,12 @@
1717
static int secp256k1_eckey_pubkey_parse(secp256k1_ge_t *elem, const unsigned char *pub, int size) {
1818
if (size == 33 && (pub[0] == 0x02 || pub[0] == 0x03)) {
1919
secp256k1_fe_t x;
20-
secp256k1_fe_set_b32(&x, pub+1);
21-
return secp256k1_ge_set_xo(elem, &x, pub[0] == 0x03);
20+
return secp256k1_fe_set_b32(&x, pub+1) && secp256k1_ge_set_xo(elem, &x, pub[0] == 0x03);
2221
} else if (size == 65 && (pub[0] == 0x04 || pub[0] == 0x06 || pub[0] == 0x07)) {
2322
secp256k1_fe_t x, y;
24-
secp256k1_fe_set_b32(&x, pub+1);
25-
secp256k1_fe_set_b32(&y, pub+33);
23+
if (!secp256k1_fe_set_b32(&x, pub+1) || !secp256k1_fe_set_b32(&y, pub+33)) {
24+
return 0;
25+
}
2626
secp256k1_ge_set_xy(elem, &x, &y);
2727
if ((pub[0] == 0x06 || pub[0] == 0x07) && secp256k1_fe_is_odd(&y) != (pub[0] == 0x07))
2828
return 0;

‎src/ecmult_gen_impl.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ static void secp256k1_ecmult_gen_start(void) {
4545
{
4646
static const unsigned char nums_b32[32] = "The scalar for this x is unknown";
4747
secp256k1_fe_t nums_x;
48-
secp256k1_fe_set_b32(&nums_x, nums_b32);
48+
VERIFY_CHECK(secp256k1_fe_set_b32(&nums_x, nums_b32));
4949
secp256k1_ge_t nums_ge;
5050
VERIFY_CHECK(secp256k1_ge_set_xo(&nums_ge, &nums_x, 0));
5151
secp256k1_gej_set_ge(&nums_gej, &nums_ge);

‎src/field.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,8 @@ static int secp256k1_fe_is_odd(const secp256k1_fe_t *a);
5959
/** Compare two field elements. Requires both inputs to be normalized */
6060
static int secp256k1_fe_equal(const secp256k1_fe_t *a, const secp256k1_fe_t *b);
6161

62-
/** Set a field element equal to 32-byte big endian value. Resulting field element is normalized. */
63-
static void secp256k1_fe_set_b32(secp256k1_fe_t *r, const unsigned char *a);
62+
/** Set a field element equal to 32-byte big endian value. If succesful, the resulting field element is normalized. */
63+
static int secp256k1_fe_set_b32(secp256k1_fe_t *r, const unsigned char *a);
6464

6565
/** Convert a field element to a 32-byte big endian value. Requires the input to be normalized */
6666
static void secp256k1_fe_get_b32(unsigned char *r, const secp256k1_fe_t *a);
@@ -109,6 +109,6 @@ static void secp256k1_fe_inv_all_var(size_t len, secp256k1_fe_t r[len], const se
109109
static void secp256k1_fe_get_hex(char *r, int *rlen, const secp256k1_fe_t *a);
110110

111111
/** Convert a hexadecimal string to a field element. */
112-
static void secp256k1_fe_set_hex(secp256k1_fe_t *r, const char *a, int alen);
112+
static int secp256k1_fe_set_hex(secp256k1_fe_t *r, const char *a, int alen);
113113

114114
#endif

‎src/field_10x26_impl.h‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ SECP256K1_INLINE static int secp256k1_fe_equal(const secp256k1_fe_t *a, const se
152152
| (t[5]^u[5]) | (t[6]^u[6]) | (t[7]^u[7]) | (t[8]^u[8]) | (t[9]^u[9])) == 0;
153153
}
154154

155-
static void secp256k1_fe_set_b32(secp256k1_fe_t *r, const unsigned char *a) {
155+
static int secp256k1_fe_set_b32(secp256k1_fe_t *r, const unsigned char *a) {
156156
r->n[0] = r->n[1] = r->n[2] = r->n[3] = r->n[4] = 0;
157157
r->n[5] = r->n[6] = r->n[7] = r->n[8] = r->n[9] = 0;
158158
for (int i=0; i<32; i++) {
@@ -162,11 +162,15 @@ static void secp256k1_fe_set_b32(secp256k1_fe_t *r, const unsigned char *a) {
162162
r->n[limb] |= (uint32_t)((a[31-i] >> (2*j)) & 0x3) << shift;
163163
}
164164
}
165+
if (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) {
166+
return 0;
167+
}
165168
#ifdef VERIFY
166169
r->magnitude = 1;
167170
r->normalized = 1;
168171
secp256k1_fe_verify(r);
169172
#endif
173+
return 1;
170174
}
171175

172176
/** Convert a field element to a 32-byte big endian value. Requires the input to be normalized */

‎src/field_5x52_impl.h‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ SECP256K1_INLINE static int secp256k1_fe_equal(const secp256k1_fe_t *a, const se
150150
return ((t[0]^u[0]) | (t[1]^u[1]) | (t[2]^u[2]) | (t[3]^u[3]) | (t[4]^u[4])) == 0;
151151
}
152152

153-
static void secp256k1_fe_set_b32(secp256k1_fe_t *r, const unsigned char *a) {
153+
static int secp256k1_fe_set_b32(secp256k1_fe_t *r, const unsigned char *a) {
154154
r->n[0] = r->n[1] = r->n[2] = r->n[3] = r->n[4] = 0;
155155
for (int i=0; i<32; i++) {
156156
for (int j=0; j<2; j++) {
@@ -159,11 +159,15 @@ static void secp256k1_fe_set_b32(secp256k1_fe_t *r, const unsigned char *a) {
159159
r->n[limb] |= (uint64_t)((a[31-i] >> (4*j)) & 0xF) << shift;
160160
}
161161
}
162+
if (r->n[4] == 0x0FFFFFFFFFFFFULL && (r->n[3] & r->n[2] & r->n[1]) == 0xFFFFFFFFFFFFFULL && r->n[0] >= 0xFFFFEFFFFFC2FULL) {
163+
return 0;
164+
}
162165
#ifdef VERIFY
163166
r->magnitude = 1;
164167
r->normalized = 1;
165168
secp256k1_fe_verify(r);
166169
#endif
170+
return 1;
167171
}
168172

169173
/** Convert a field element to a 32-byte big endian value. Requires the input to be normalized */

‎src/field_gmp_impl.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,14 +75,15 @@ SECP256K1_INLINE static int secp256k1_fe_equal(const secp256k1_fe_t *a, const se
7575
return ret;
7676
}
7777

78-
static void secp256k1_fe_set_b32(secp256k1_fe_t *r, const unsigned char *a) {
78+
static int secp256k1_fe_set_b32(secp256k1_fe_t *r, const unsigned char *a) {
7979
for (int i=0; i<FIELD_LIMBS+1; i++)
8080
r->n[i] = 0;
8181
for (int i=0; i<256; i++) {
8282
int limb = i/GMP_NUMB_BITS;
8383
int shift = i%GMP_NUMB_BITS;
8484
r->n[limb] |= (mp_limb_t)((a[31-i/8] >> (i%8)) & 0x1) << shift;
8585
}
86+
return (mpn_cmp(r->n, secp256k1_field_p, FIELD_LIMBS) < 0);
8687
}
8788

8889
/** Convert a field element to a 32-byte big endian value. Requires the input to be normalized */

‎src/field_impl.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ static void secp256k1_fe_get_hex(char *r, int *rlen, const secp256k1_fe_t *a) {
4141
r[64] = 0x00;
4242
}
4343

44-
static void secp256k1_fe_set_hex(secp256k1_fe_t *r, const char *a, int alen) {
44+
static int secp256k1_fe_set_hex(secp256k1_fe_t *r, const char *a, int alen) {
4545
unsigned char tmp[32] = {};
4646
static const int cvt[256] = {0, 0, 0, 0, 0, 0, 0,0,0,0,0,0,0,0,0,0,
4747
0, 0, 0, 0, 0, 0, 0,0,0,0,0,0,0,0,0,0,
@@ -63,7 +63,7 @@ static void secp256k1_fe_set_hex(secp256k1_fe_t *r, const char *a, int alen) {
6363
if (alen > i*2)
6464
tmp[32 - alen/2 + i] = (cvt[(unsigned char)a[2*i]] << 4) + cvt[(unsigned char)a[2*i+1]];
6565
}
66-
secp256k1_fe_set_b32(r, tmp);
66+
return secp256k1_fe_set_b32(r, tmp);
6767
}
6868

6969
static int secp256k1_fe_sqrt(secp256k1_fe_t *r, const secp256k1_fe_t *a) {
@@ -212,7 +212,7 @@ static void secp256k1_fe_inv_var(secp256k1_fe_t *r, const secp256k1_fe_t *a) {
212212
secp256k1_num_set_bin(&n, b, 32);
213213
secp256k1_num_mod_inverse(&n, &n, &secp256k1_fe_consts->p);
214214
secp256k1_num_get_bin(b, 32, &n);
215-
secp256k1_fe_set_b32(r, b);
215+
VERIFY_CHECK(secp256k1_fe_set_b32(r, b));
216216
#else
217217
#error "Please select field inverse implementation"
218218
#endif

‎src/group_impl.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -498,11 +498,11 @@ static void secp256k1_ge_start(void) {
498498
secp256k1_num_set_bin(&ret->a1b2, secp256k1_ge_consts_a1b2, sizeof(secp256k1_ge_consts_a1b2));
499499
secp256k1_num_set_bin(&ret->a2, secp256k1_ge_consts_a2, sizeof(secp256k1_ge_consts_a2));
500500
secp256k1_num_set_bin(&ret->b1, secp256k1_ge_consts_b1, sizeof(secp256k1_ge_consts_b1));
501-
secp256k1_fe_set_b32(&ret->beta, secp256k1_ge_consts_beta);
501+
VERIFY_CHECK(secp256k1_fe_set_b32(&ret->beta, secp256k1_ge_consts_beta));
502502
#endif
503503
secp256k1_fe_t g_x, g_y;
504-
secp256k1_fe_set_b32(&g_x, secp256k1_ge_consts_g_x);
505-
secp256k1_fe_set_b32(&g_y, secp256k1_ge_consts_g_y);
504+
VERIFY_CHECK(secp256k1_fe_set_b32(&g_x, secp256k1_ge_consts_g_x));
505+
VERIFY_CHECK(secp256k1_fe_set_b32(&g_y, secp256k1_ge_consts_g_y));
506506
secp256k1_ge_set_xy(&ret->g, &g_x, &g_y);
507507
secp256k1_ge_consts = ret;
508508
}

‎src/tests.c‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ void random_field_element_test(secp256k1_fe_t *fe) {
3838
secp256k1_num_set_bin(&num, b32, 32);
3939
if (secp256k1_num_cmp(&num, &secp256k1_fe_consts->p) >= 0)
4040
continue;
41-
secp256k1_fe_set_b32(fe, b32);
41+
VERIFY_CHECK(secp256k1_fe_set_b32(fe, b32));
4242
break;
4343
} while(1);
4444
}
@@ -440,8 +440,12 @@ void run_scalar_tests(void) {
440440

441441
void random_fe(secp256k1_fe_t *x) {
442442
unsigned char bin[32];
443-
secp256k1_rand256(bin);
444-
secp256k1_fe_set_b32(x, bin);
443+
do {
444+
secp256k1_rand256(bin);
445+
if (secp256k1_fe_set_b32(x, bin)) {
446+
return;
447+
}
448+
} while(1);
445449
}
446450

447451
void random_fe_non_zero(secp256k1_fe_t *nz) {
@@ -697,8 +701,8 @@ void run_ge(void) {
697701

698702
void run_ecmult_chain(void) {
699703
/* random starting point A (on the curve) */
700-
secp256k1_fe_t ax; secp256k1_fe_set_hex(&ax, "8b30bbe9ae2a990696b22f670709dff3727fd8bc04d3362c6c7bf458e2846004", 64);
701-
secp256k1_fe_t ay; secp256k1_fe_set_hex(&ay, "a357ae915c4a65281309edf20504740f0eb3343990216b4f81063cb65f2f7e0f", 64);
704+
secp256k1_fe_t ax; VERIFY_CHECK(secp256k1_fe_set_hex(&ax, "8b30bbe9ae2a990696b22f670709dff3727fd8bc04d3362c6c7bf458e2846004", 64));
705+
secp256k1_fe_t ay; VERIFY_CHECK(secp256k1_fe_set_hex(&ay, "a357ae915c4a65281309edf20504740f0eb3343990216b4f81063cb65f2f7e0f", 64));
702706
secp256k1_gej_t a; secp256k1_gej_set_xy(&a, &ax, &ay);
703707
/* two random initial factors xn and gn */
704708
secp256k1_num_t xn;
@@ -759,7 +763,7 @@ void test_point_times_order(const secp256k1_gej_t *point) {
759763
}
760764

761765
void run_point_times_order(void) {
762-
secp256k1_fe_t x; secp256k1_fe_set_hex(&x, "02", 2);
766+
secp256k1_fe_t x; VERIFY_CHECK(secp256k1_fe_set_hex(&x, "02", 2));
763767
for (int i=0; i<500; i++) {
764768
secp256k1_ge_t p;
765769
if (secp256k1_ge_set_xo(&p, &x, 1)) {

0 commit comments

Comments
 (0)