From d907ebc0e386ea17a96d34cd3008be9207b6f94f Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Mon, 24 Nov 2014 12:38:05 +0100 Subject: [PATCH 1/2] Add bounds checking to field element setters --- src/ecdsa_impl.h | 2 +- src/eckey_impl.h | 8 ++++---- src/ecmult_gen_impl.h | 2 +- src/field.h | 6 +++--- src/field_10x26_impl.h | 6 +++++- src/field_5x52_impl.h | 6 +++++- src/field_gmp_impl.h | 3 ++- src/field_impl.h | 6 +++--- src/group_impl.h | 6 +++--- src/tests.c | 16 ++++++++++------ 10 files changed, 37 insertions(+), 24 deletions(-) diff --git a/src/ecdsa_impl.h b/src/ecdsa_impl.h index 4c05ec39f8..dadf21550d 100644 --- a/src/ecdsa_impl.h +++ b/src/ecdsa_impl.h @@ -108,7 +108,7 @@ static int secp256k1_ecdsa_sig_recover(const secp256k1_ecdsa_sig_t *sig, secp256 secp256k1_num_get_bin(brx, 32, &rx); secp256k1_num_free(&rx); secp256k1_fe_t fx; - secp256k1_fe_set_b32(&fx, brx); + VERIFY_CHECK(secp256k1_fe_set_b32(&fx, brx)); /* Either rx < n (and n < p), or rx + n < p (checked above). */ secp256k1_ge_t x; if (!secp256k1_ge_set_xo(&x, &fx, recid & 1)) return 0; diff --git a/src/eckey_impl.h b/src/eckey_impl.h index 290b1f0900..bddf51a1a0 100644 --- a/src/eckey_impl.h +++ b/src/eckey_impl.h @@ -17,12 +17,12 @@ static int secp256k1_eckey_pubkey_parse(secp256k1_ge_t *elem, const unsigned char *pub, int size) { if (size == 33 && (pub[0] == 0x02 || pub[0] == 0x03)) { secp256k1_fe_t x; - secp256k1_fe_set_b32(&x, pub+1); - return secp256k1_ge_set_xo(elem, &x, pub[0] == 0x03); + return secp256k1_fe_set_b32(&x, pub+1) && secp256k1_ge_set_xo(elem, &x, pub[0] == 0x03); } else if (size == 65 && (pub[0] == 0x04 || pub[0] == 0x06 || pub[0] == 0x07)) { secp256k1_fe_t x, y; - secp256k1_fe_set_b32(&x, pub+1); - secp256k1_fe_set_b32(&y, pub+33); + if (!secp256k1_fe_set_b32(&x, pub+1) || !secp256k1_fe_set_b32(&y, pub+33)) { + return 0; + } secp256k1_ge_set_xy(elem, &x, &y); if ((pub[0] == 0x06 || pub[0] == 0x07) && secp256k1_fe_is_odd(&y) != (pub[0] == 0x07)) return 0; diff --git a/src/ecmult_gen_impl.h b/src/ecmult_gen_impl.h index 07859ab04b..d0101195da 100644 --- a/src/ecmult_gen_impl.h +++ b/src/ecmult_gen_impl.h @@ -45,7 +45,7 @@ static void secp256k1_ecmult_gen_start(void) { { static const unsigned char nums_b32[32] = "The scalar for this x is unknown"; secp256k1_fe_t nums_x; - secp256k1_fe_set_b32(&nums_x, nums_b32); + VERIFY_CHECK(secp256k1_fe_set_b32(&nums_x, nums_b32)); secp256k1_ge_t nums_ge; VERIFY_CHECK(secp256k1_ge_set_xo(&nums_ge, &nums_x, 0)); secp256k1_gej_set_ge(&nums_gej, &nums_ge); diff --git a/src/field.h b/src/field.h index c7feead900..b56a0a95e5 100644 --- a/src/field.h +++ b/src/field.h @@ -59,8 +59,8 @@ static int secp256k1_fe_is_odd(const secp256k1_fe_t *a); /** Compare two field elements. Requires both inputs to be normalized */ static int secp256k1_fe_equal(const secp256k1_fe_t *a, const secp256k1_fe_t *b); -/** Set a field element equal to 32-byte big endian value. Resulting field element is normalized. */ -static void secp256k1_fe_set_b32(secp256k1_fe_t *r, const unsigned char *a); +/** Set a field element equal to 32-byte big endian value. If succesful, the resulting field element is normalized. */ +static int secp256k1_fe_set_b32(secp256k1_fe_t *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_t *a); @@ -109,6 +109,6 @@ static void secp256k1_fe_inv_all_var(size_t len, secp256k1_fe_t r[len], const se static void secp256k1_fe_get_hex(char *r, int *rlen, const secp256k1_fe_t *a); /** Convert a hexadecimal string to a field element. */ -static void secp256k1_fe_set_hex(secp256k1_fe_t *r, const char *a, int alen); +static int secp256k1_fe_set_hex(secp256k1_fe_t *r, const char *a, int alen); #endif diff --git a/src/field_10x26_impl.h b/src/field_10x26_impl.h index c0f1be0b2d..de12d563a6 100644 --- a/src/field_10x26_impl.h +++ b/src/field_10x26_impl.h @@ -152,7 +152,7 @@ SECP256K1_INLINE static int secp256k1_fe_equal(const secp256k1_fe_t *a, const se | (t[5]^u[5]) | (t[6]^u[6]) | (t[7]^u[7]) | (t[8]^u[8]) | (t[9]^u[9])) == 0; } -static void secp256k1_fe_set_b32(secp256k1_fe_t *r, const unsigned char *a) { +static int secp256k1_fe_set_b32(secp256k1_fe_t *r, const unsigned char *a) { r->n[0] = r->n[1] = r->n[2] = r->n[3] = r->n[4] = 0; r->n[5] = r->n[6] = r->n[7] = r->n[8] = r->n[9] = 0; 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) { r->n[limb] |= (uint32_t)((a[31-i] >> (2*j)) & 0x3) << shift; } } + 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) { + return 0; + } #ifdef VERIFY r->magnitude = 1; r->normalized = 1; secp256k1_fe_verify(r); #endif + return 1; } /** Convert a field element to a 32-byte big endian value. Requires the input to be normalized */ diff --git a/src/field_5x52_impl.h b/src/field_5x52_impl.h index d1b06d05a4..e1e7ee1d6c 100644 --- a/src/field_5x52_impl.h +++ b/src/field_5x52_impl.h @@ -150,7 +150,7 @@ SECP256K1_INLINE static int secp256k1_fe_equal(const secp256k1_fe_t *a, const se return ((t[0]^u[0]) | (t[1]^u[1]) | (t[2]^u[2]) | (t[3]^u[3]) | (t[4]^u[4])) == 0; } -static void secp256k1_fe_set_b32(secp256k1_fe_t *r, const unsigned char *a) { +static int secp256k1_fe_set_b32(secp256k1_fe_t *r, const unsigned char *a) { r->n[0] = r->n[1] = r->n[2] = r->n[3] = r->n[4] = 0; for (int i=0; i<32; i++) { 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) { r->n[limb] |= (uint64_t)((a[31-i] >> (4*j)) & 0xF) << shift; } } + if (r->n[4] == 0x0FFFFFFFFFFFFULL && (r->n[3] & r->n[2] & r->n[1]) == 0xFFFFFFFFFFFFFULL && r->n[0] >= 0xFFFFEFFFFFC2FULL) { + return 0; + } #ifdef VERIFY r->magnitude = 1; r->normalized = 1; secp256k1_fe_verify(r); #endif + return 1; } /** Convert a field element to a 32-byte big endian value. Requires the input to be normalized */ diff --git a/src/field_gmp_impl.h b/src/field_gmp_impl.h index af4728e5b4..31045229ed 100644 --- a/src/field_gmp_impl.h +++ b/src/field_gmp_impl.h @@ -75,7 +75,7 @@ SECP256K1_INLINE static int secp256k1_fe_equal(const secp256k1_fe_t *a, const se return ret; } -static void secp256k1_fe_set_b32(secp256k1_fe_t *r, const unsigned char *a) { +static int secp256k1_fe_set_b32(secp256k1_fe_t *r, const unsigned char *a) { for (int i=0; in[i] = 0; for (int i=0; i<256; i++) { @@ -83,6 +83,7 @@ static void secp256k1_fe_set_b32(secp256k1_fe_t *r, const unsigned char *a) { int shift = i%GMP_NUMB_BITS; r->n[limb] |= (mp_limb_t)((a[31-i/8] >> (i%8)) & 0x1) << shift; } + return (mpn_cmp(r->n, secp256k1_field_p, FIELD_LIMBS) < 0); } /** Convert a field element to a 32-byte big endian value. Requires the input to be normalized */ diff --git a/src/field_impl.h b/src/field_impl.h index 3a31e1844e..f26648733a 100644 --- a/src/field_impl.h +++ b/src/field_impl.h @@ -41,7 +41,7 @@ static void secp256k1_fe_get_hex(char *r, int *rlen, const secp256k1_fe_t *a) { r[64] = 0x00; } -static void secp256k1_fe_set_hex(secp256k1_fe_t *r, const char *a, int alen) { +static int secp256k1_fe_set_hex(secp256k1_fe_t *r, const char *a, int alen) { unsigned char tmp[32] = {}; static const int cvt[256] = {0, 0, 0, 0, 0, 0, 0,0,0,0,0,0,0,0,0,0, 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) { if (alen > i*2) tmp[32 - alen/2 + i] = (cvt[(unsigned char)a[2*i]] << 4) + cvt[(unsigned char)a[2*i+1]]; } - secp256k1_fe_set_b32(r, tmp); + return secp256k1_fe_set_b32(r, tmp); } 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) { secp256k1_num_set_bin(&n, b, 32); secp256k1_num_mod_inverse(&n, &n, &secp256k1_fe_consts->p); secp256k1_num_get_bin(b, 32, &n); - secp256k1_fe_set_b32(r, b); + VERIFY_CHECK(secp256k1_fe_set_b32(r, b)); #else #error "Please select field inverse implementation" #endif diff --git a/src/group_impl.h b/src/group_impl.h index 1edbc6e099..b0bc3f9949 100644 --- a/src/group_impl.h +++ b/src/group_impl.h @@ -498,11 +498,11 @@ static void secp256k1_ge_start(void) { secp256k1_num_set_bin(&ret->a1b2, secp256k1_ge_consts_a1b2, sizeof(secp256k1_ge_consts_a1b2)); secp256k1_num_set_bin(&ret->a2, secp256k1_ge_consts_a2, sizeof(secp256k1_ge_consts_a2)); secp256k1_num_set_bin(&ret->b1, secp256k1_ge_consts_b1, sizeof(secp256k1_ge_consts_b1)); - secp256k1_fe_set_b32(&ret->beta, secp256k1_ge_consts_beta); + VERIFY_CHECK(secp256k1_fe_set_b32(&ret->beta, secp256k1_ge_consts_beta)); #endif secp256k1_fe_t g_x, g_y; - secp256k1_fe_set_b32(&g_x, secp256k1_ge_consts_g_x); - secp256k1_fe_set_b32(&g_y, secp256k1_ge_consts_g_y); + VERIFY_CHECK(secp256k1_fe_set_b32(&g_x, secp256k1_ge_consts_g_x)); + VERIFY_CHECK(secp256k1_fe_set_b32(&g_y, secp256k1_ge_consts_g_y)); secp256k1_ge_set_xy(&ret->g, &g_x, &g_y); secp256k1_ge_consts = ret; } diff --git a/src/tests.c b/src/tests.c index 5d9b8344d9..022bcb615e 100644 --- a/src/tests.c +++ b/src/tests.c @@ -38,7 +38,7 @@ void random_field_element_test(secp256k1_fe_t *fe) { secp256k1_num_set_bin(&num, b32, 32); if (secp256k1_num_cmp(&num, &secp256k1_fe_consts->p) >= 0) continue; - secp256k1_fe_set_b32(fe, b32); + VERIFY_CHECK(secp256k1_fe_set_b32(fe, b32)); break; } while(1); } @@ -440,8 +440,12 @@ void run_scalar_tests(void) { void random_fe(secp256k1_fe_t *x) { unsigned char bin[32]; - secp256k1_rand256(bin); - secp256k1_fe_set_b32(x, bin); + do { + secp256k1_rand256(bin); + if (secp256k1_fe_set_b32(x, bin)) { + return; + } + } while(1); } void random_fe_non_zero(secp256k1_fe_t *nz) { @@ -697,8 +701,8 @@ void run_ge(void) { void run_ecmult_chain(void) { /* random starting point A (on the curve) */ - secp256k1_fe_t ax; secp256k1_fe_set_hex(&ax, "8b30bbe9ae2a990696b22f670709dff3727fd8bc04d3362c6c7bf458e2846004", 64); - secp256k1_fe_t ay; secp256k1_fe_set_hex(&ay, "a357ae915c4a65281309edf20504740f0eb3343990216b4f81063cb65f2f7e0f", 64); + secp256k1_fe_t ax; VERIFY_CHECK(secp256k1_fe_set_hex(&ax, "8b30bbe9ae2a990696b22f670709dff3727fd8bc04d3362c6c7bf458e2846004", 64)); + secp256k1_fe_t ay; VERIFY_CHECK(secp256k1_fe_set_hex(&ay, "a357ae915c4a65281309edf20504740f0eb3343990216b4f81063cb65f2f7e0f", 64)); secp256k1_gej_t a; secp256k1_gej_set_xy(&a, &ax, &ay); /* two random initial factors xn and gn */ secp256k1_num_t xn; @@ -759,7 +763,7 @@ void test_point_times_order(const secp256k1_gej_t *point) { } void run_point_times_order(void) { - secp256k1_fe_t x; secp256k1_fe_set_hex(&x, "02", 2); + secp256k1_fe_t x; VERIFY_CHECK(secp256k1_fe_set_hex(&x, "02", 2)); for (int i=0; i<500; i++) { secp256k1_ge_t p; if (secp256k1_ge_set_xo(&p, &x, 1)) { From 99f0728f236680a63e8ed30ec52dca5bbdfd705d Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Mon, 24 Nov 2014 21:29:25 +0100 Subject: [PATCH 2/2] Fix secp256k1_num_set_bin handling of 0 --- src/num_gmp_impl.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/num_gmp_impl.h b/src/num_gmp_impl.h index e45a59e0cd..e7eae86763 100644 --- a/src/num_gmp_impl.h +++ b/src/num_gmp_impl.h @@ -71,6 +71,10 @@ static void secp256k1_num_set_bin(secp256k1_num_t *r, const unsigned char *a, un VERIFY_CHECK(alen > 0); VERIFY_CHECK(alen <= 64); int len = mpn_set_str(r->data, a, alen, 256); + if (len == 0) { + r->data[0] = 0; + len = 1; + } VERIFY_CHECK(len <= NUM_LIMBS*2); r->limbs = len; r->neg = 0;