From 809103663faffc05128bae6a418b819708f2da93 Mon Sep 17 00:00:00 2001 From: Jonas Nick Date: Mon, 7 Oct 2019 18:49:21 +0000 Subject: [PATCH 1/3] Return 0 if invalid seckey is given to ec_privkey_negate --- include/secp256k1.h | 6 ++++-- src/secp256k1.c | 6 +++++- src/tests.c | 27 +++++++++++++++++++++++++++ 3 files changed, 36 insertions(+), 3 deletions(-) diff --git a/include/secp256k1.h b/include/secp256k1.h index fc27626dd8..5cd9048bcd 100644 --- a/include/secp256k1.h +++ b/include/secp256k1.h @@ -577,9 +577,11 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_pubkey_create( /** Negates a private key in place. * - * Returns: 1 always + * Returns: 1 if seckey was successfully negated and 0 otherwise * Args: ctx: pointer to a context object - * In/Out: seckey: pointer to the 32-byte private key to be negated (cannot be NULL) + * In/Out: seckey: pointer to the 32-byte private key to be negated. The private key + * interpreted as an integer (most significant byte first) must be less than + * the curve order. (cannot be NULL) */ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_privkey_negate( const secp256k1_context* ctx, diff --git a/src/secp256k1.c b/src/secp256k1.c index a3f446e507..60f70c20b8 100644 --- a/src/secp256k1.c +++ b/src/secp256k1.c @@ -530,10 +530,14 @@ int secp256k1_ec_pubkey_create(const secp256k1_context* ctx, secp256k1_pubkey *p int secp256k1_ec_privkey_negate(const secp256k1_context* ctx, unsigned char *seckey) { secp256k1_scalar sec; + int overflow; VERIFY_CHECK(ctx != NULL); ARG_CHECK(seckey != NULL); - secp256k1_scalar_set_b32(&sec, seckey, NULL); + secp256k1_scalar_set_b32(&sec, seckey, &overflow); + if (overflow) { + return 0; + } secp256k1_scalar_negate(&sec, &sec); secp256k1_scalar_get_b32(seckey, &sec); diff --git a/src/tests.c b/src/tests.c index d408a5c30a..2dfe5bbffe 100644 --- a/src/tests.c +++ b/src/tests.c @@ -4104,6 +4104,30 @@ void run_eckey_edge_case_test(void) { secp256k1_context_set_illegal_callback(ctx, NULL, NULL); } +void run_eckey_arithmetic_test(void) { + unsigned char seckey[32]; + unsigned char seckey_tmp[32]; + + secp256k1_rand256_test(seckey); + memcpy(seckey_tmp, seckey, 32); + + /* Verify negation changes the key and changes it back */ + CHECK(secp256k1_ec_privkey_negate(ctx, seckey) == 1); + CHECK(memcmp(seckey, seckey_tmp, 32) != 0); + CHECK(secp256k1_ec_privkey_negate(ctx, seckey) == 1); + CHECK(memcmp(seckey, seckey_tmp, 32) == 0); + + /* Negating an overflowing seckey fails and the seckey is not modified. In + * this test, the seckey has 16 random bytes to ensure that + * ec_privkey_negate doesn't just set seckey to a constant value in case of + * failure.*/ + secp256k1_rand256_test(seckey); + memset(seckey, 0xFF, 16); + memcpy(seckey_tmp, seckey, 32); + CHECK(secp256k1_ec_privkey_negate(ctx, seckey) == 0); + CHECK(memcmp(seckey, seckey_tmp, 32) == 0); +} + void random_sign(secp256k1_scalar *sigr, secp256k1_scalar *sigs, const secp256k1_scalar *key, const secp256k1_scalar *msg, int *recid) { secp256k1_scalar nonce; do { @@ -5270,6 +5294,9 @@ int main(int argc, char **argv) { /* EC key edge cases */ run_eckey_edge_case_test(); + /* EC key arithmetic test */ + run_eckey_arithmetic_test(); + #ifdef ENABLE_MODULE_ECDH /* ecdh tests */ run_ecdh_tests(); From 1748318f54d9125cc8e58e673c270d4eccf925db Mon Sep 17 00:00:00 2001 From: Jonas Nick Date: Tue, 8 Oct 2019 09:11:16 +0000 Subject: [PATCH 2/3] Add test for boundary conditions of scalar_set_b32 with respect to overflows --- src/tests.c | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/src/tests.c b/src/tests.c index 2dfe5bbffe..d38e4d16a3 100644 --- a/src/tests.c +++ b/src/tests.c @@ -1097,16 +1097,42 @@ void run_scalar_tests(void) { #ifndef USE_NUM_NONE { - /* A scalar with value of the curve order should be 0. */ + /* Test secp256k1_scalar_set_b32 boundary conditions */ secp256k1_num order; - secp256k1_scalar zero; + secp256k1_scalar scalar; unsigned char bin[32]; + unsigned char bin_tmp[32]; int overflow = 0; + static const secp256k1_scalar all_zeros_minus_order = SECP256K1_SCALAR_CONST( + 0x00000000UL, 0x00000000UL, 0x00000000UL, 0x00000001UL, + 0x45512319UL, 0x50B75FC4UL, 0x402DA173UL, 0x2FC9BEBEUL + ); + + /* A scalar set to 0s should be 0. */ + memset(bin, 0, 32); + secp256k1_scalar_set_b32(&scalar, bin, &overflow); + CHECK(overflow == 0); + CHECK(secp256k1_scalar_is_zero(&scalar)); + + /* A scalar with value of the curve order should be 0. */ secp256k1_scalar_order_get_num(&order); secp256k1_num_get_bin(bin, 32, &order); - secp256k1_scalar_set_b32(&zero, bin, &overflow); + secp256k1_scalar_set_b32(&scalar, bin, &overflow); + CHECK(overflow == 1); + CHECK(secp256k1_scalar_is_zero(&scalar)); + + /* A scalar with value of the curve order minus one should not overflow. */ + bin[31] -= 1; + secp256k1_scalar_set_b32(&scalar, bin, &overflow); + CHECK(overflow == 0); + secp256k1_scalar_get_b32(bin_tmp, &scalar); + CHECK(memcmp(bin, bin_tmp, 32) == 0); + + /* A scalar set to all 1s should overflow. */ + memset(bin, 0xFF, 32); + secp256k1_scalar_set_b32(&scalar, bin, &overflow); CHECK(overflow == 1); - CHECK(secp256k1_scalar_is_zero(&zero)); + CHECK(secp256k1_scalar_eq(&scalar, &all_zeros_minus_order)); } #endif From 3f5512c81b17f1412d7992f8fe418a1ab629d955 Mon Sep 17 00:00:00 2001 From: Jonas Nick Date: Mon, 28 Oct 2019 13:06:01 +0000 Subject: [PATCH 3/3] Return 0 in privkey_negate for an all 0 seckey and move definition of valid secret keys to verify_seckey --- include/secp256k1.h | 11 +++++++---- src/secp256k1.c | 2 +- src/tests.c | 12 ++++++++++-- 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/include/secp256k1.h b/include/secp256k1.h index 5cd9048bcd..b67897bdae 100644 --- a/include/secp256k1.h +++ b/include/secp256k1.h @@ -549,7 +549,10 @@ SECP256K1_API int secp256k1_ecdsa_sign( const void *ndata ) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4); -/** Verify an ECDSA secret key. +/** Verify an ECDSA secret key. It is valid if it is not 0 and less than the + * secp256k1 curve order when interpreted as an integer (most significant byte + * first). The probability of choosing a 32-byte string at random which is an + * invalid secret key is negligible. * * Returns: 1: secret key is valid * 0: secret key is invalid @@ -579,9 +582,9 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_pubkey_create( * * Returns: 1 if seckey was successfully negated and 0 otherwise * Args: ctx: pointer to a context object - * In/Out: seckey: pointer to the 32-byte private key to be negated. The private key - * interpreted as an integer (most significant byte first) must be less than - * the curve order. (cannot be NULL) + * In/Out: seckey: pointer to the 32-byte private key to be negated. The private + * key should be valid according to secp256k1_ec_seckey_verify + * (cannot be NULL) */ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_privkey_negate( const secp256k1_context* ctx, diff --git a/src/secp256k1.c b/src/secp256k1.c index 60f70c20b8..c49308f4e2 100644 --- a/src/secp256k1.c +++ b/src/secp256k1.c @@ -535,7 +535,7 @@ int secp256k1_ec_privkey_negate(const secp256k1_context* ctx, unsigned char *sec ARG_CHECK(seckey != NULL); secp256k1_scalar_set_b32(&sec, seckey, &overflow); - if (overflow) { + if (overflow || secp256k1_scalar_is_zero(&sec)) { return 0; } secp256k1_scalar_negate(&sec, &sec); diff --git a/src/tests.c b/src/tests.c index d38e4d16a3..46761f0ba1 100644 --- a/src/tests.c +++ b/src/tests.c @@ -1103,7 +1103,8 @@ void run_scalar_tests(void) { unsigned char bin[32]; unsigned char bin_tmp[32]; int overflow = 0; - static const secp256k1_scalar all_zeros_minus_order = SECP256K1_SCALAR_CONST( + /* 2^256-1 - order */ + static const secp256k1_scalar all_ones_minus_order = SECP256K1_SCALAR_CONST( 0x00000000UL, 0x00000000UL, 0x00000000UL, 0x00000001UL, 0x45512319UL, 0x50B75FC4UL, 0x402DA173UL, 0x2FC9BEBEUL ); @@ -1132,7 +1133,7 @@ void run_scalar_tests(void) { memset(bin, 0xFF, 32); secp256k1_scalar_set_b32(&scalar, bin, &overflow); CHECK(overflow == 1); - CHECK(secp256k1_scalar_eq(&scalar, &all_zeros_minus_order)); + CHECK(secp256k1_scalar_eq(&scalar, &all_ones_minus_order)); } #endif @@ -4143,6 +4144,13 @@ void run_eckey_arithmetic_test(void) { CHECK(secp256k1_ec_privkey_negate(ctx, seckey) == 1); CHECK(memcmp(seckey, seckey_tmp, 32) == 0); + /* Negating all 0s fails */ + memset(seckey, 0, 32); + memset(seckey_tmp, 0, 32); + CHECK(secp256k1_ec_privkey_negate(ctx, seckey) == 0); + /* Check that seckey is not modified */ + CHECK(memcmp(seckey, seckey_tmp, 32) == 0); + /* Negating an overflowing seckey fails and the seckey is not modified. In * this test, the seckey has 16 random bytes to ensure that * ec_privkey_negate doesn't just set seckey to a constant value in case of