diff --git a/include/secp256k1.h b/include/secp256k1.h index fc27626dd8..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 @@ -577,9 +580,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 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 a3f446e507..c49308f4e2 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 || secp256k1_scalar_is_zero(&sec)) { + 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..46761f0ba1 100644 --- a/src/tests.c +++ b/src/tests.c @@ -1097,16 +1097,43 @@ 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; + /* 2^256-1 - order */ + static const secp256k1_scalar all_ones_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_ones_minus_order)); } #endif @@ -4104,6 +4131,37 @@ 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 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 + * 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 +5328,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();