diff --git a/include/secp256k1.h b/include/secp256k1.h index 1678406610e68..816374a4674fd 100644 --- a/include/secp256k1.h +++ b/include/secp256k1.h @@ -579,9 +579,12 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_pubkey_create( /** Negates a private key in place. * - * Returns: 1 always + * Returns: 0 if the given private key is invalid according to + * secp256k1_ec_seckey_verify. 1 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, @@ -601,9 +604,10 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_pubkey_negate( /** Tweak a private key by adding tweak to it. * Returns: 0 if the tweak was out of range (chance of around 1 in 2^128 for - * uniformly random 32-byte arrays, or if the resulting private key - * would be invalid (only when the tweak is the complement of the - * private key). 1 otherwise. + * uniformly random 32-byte arrays, or if the given private key is + * invalid according to secp256k1_ec_seckey_verify, or if the resulting + * private key would be invalid (only when the tweak is the complement + * of the private key). 1 otherwise. * Args: ctx: pointer to a context object (cannot be NULL). * In/Out: seckey: pointer to a 32-byte private key. * In: tweak: pointer to a 32-byte tweak. @@ -616,9 +620,10 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_privkey_tweak_add( /** Tweak a public key by adding tweak times the generator to it. * Returns: 0 if the tweak was out of range (chance of around 1 in 2^128 for - * uniformly random 32-byte arrays, or if the resulting public key - * would be invalid (only when the tweak is the complement of the - * corresponding private key). 1 otherwise. + * uniformly random 32-byte arrays, or if the given private key is + * invalid according to secp256k1_ec_seckey_verify, or if the resulting + * public key would be invalid (only when the tweak is the complement + * of the corresponding private key). 1 otherwise. * Args: ctx: pointer to a context object initialized for validation * (cannot be NULL). * In/Out: pubkey: pointer to a public key object. diff --git a/src/eckey_impl.h b/src/eckey_impl.h index 00e4a86fc5bd7..e2e72d93039a7 100644 --- a/src/eckey_impl.h +++ b/src/eckey_impl.h @@ -54,10 +54,7 @@ static int secp256k1_eckey_pubkey_serialize(secp256k1_ge *elem, unsigned char *p static int secp256k1_eckey_privkey_tweak_add(secp256k1_scalar *key, const secp256k1_scalar *tweak) { secp256k1_scalar_add(key, key, tweak); - if (secp256k1_scalar_is_zero(key)) { - return 0; - } - return 1; + return !secp256k1_scalar_is_zero(key); } static int secp256k1_eckey_pubkey_tweak_add(const secp256k1_ecmult_context *ctx, secp256k1_ge *key, const secp256k1_scalar *tweak) { diff --git a/src/secp256k1.c b/src/secp256k1.c index 20cf3c7e05b57..d205e3072e508 100644 --- a/src/secp256k1.c +++ b/src/secp256k1.c @@ -556,15 +556,17 @@ 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 ret = 0; VERIFY_CHECK(ctx != NULL); ARG_CHECK(seckey != NULL); - secp256k1_scalar_set_b32(&sec, seckey, NULL); + ret = secp256k1_scalar_set_b32_seckey(&sec, seckey); + secp256k1_scalar_cmov(&sec, &secp256k1_scalar_zero, !ret); secp256k1_scalar_negate(&sec, &sec); secp256k1_scalar_get_b32(seckey, &sec); secp256k1_scalar_clear(&sec); - return 1; + return ret; } int secp256k1_ec_pubkey_negate(const secp256k1_context* ctx, secp256k1_pubkey *pubkey) { @@ -592,9 +594,9 @@ int secp256k1_ec_privkey_tweak_add(const secp256k1_context* ctx, unsigned char * ARG_CHECK(tweak != NULL); secp256k1_scalar_set_b32(&term, tweak, &overflow); - secp256k1_scalar_set_b32(&sec, seckey, NULL); + ret = secp256k1_scalar_set_b32_seckey(&sec, seckey); - ret = (!overflow) & secp256k1_eckey_privkey_tweak_add(&sec, &term); + ret &= (!overflow) & secp256k1_eckey_privkey_tweak_add(&sec, &term); secp256k1_scalar_cmov(&sec, &secp256k1_scalar_zero, !ret); secp256k1_scalar_get_b32(seckey, &sec); @@ -637,8 +639,8 @@ int secp256k1_ec_privkey_tweak_mul(const secp256k1_context* ctx, unsigned char * ARG_CHECK(tweak != NULL); secp256k1_scalar_set_b32(&factor, tweak, &overflow); - secp256k1_scalar_set_b32(&sec, seckey, NULL); - ret = (!overflow) & secp256k1_eckey_privkey_tweak_mul(&sec, &factor); + ret = secp256k1_scalar_set_b32_seckey(&sec, seckey); + ret &= (!overflow) & secp256k1_eckey_privkey_tweak_mul(&sec, &factor); secp256k1_scalar_cmov(&sec, &secp256k1_scalar_zero, !ret); secp256k1_scalar_get_b32(seckey, &sec); diff --git a/src/tests.c b/src/tests.c index ef166e4afecd1..534f5bbfd6290 100644 --- a/src/tests.c +++ b/src/tests.c @@ -4000,7 +4000,20 @@ void run_eckey_edge_case_test(void) { CHECK(secp256k1_ec_pubkey_tweak_mul(ctx, &pubkey, ctmp2) == 0); CHECK(memcmp(&pubkey, zeros, sizeof(pubkey)) == 0); memcpy(&pubkey, &pubkey2, sizeof(pubkey)); - /* Overflowing key tweak zeroizes. */ + /* If seckey_tweak_add or seckey_tweak_mul are called with an overflowing + seckey, the seckey is zeroized. */ + memcpy(ctmp, orderc, 32); + memset(ctmp2, 0, 32); + ctmp2[31] = 0x01; + CHECK(secp256k1_ec_seckey_verify(ctx, ctmp2) == 1); + CHECK(secp256k1_ec_seckey_verify(ctx, ctmp) == 0); + CHECK(secp256k1_ec_privkey_tweak_add(ctx, ctmp, ctmp2) == 0); + CHECK(memcmp(zeros, ctmp, 32) == 0); + memcpy(ctmp, orderc, 32); + CHECK(secp256k1_ec_privkey_tweak_mul(ctx, ctmp, ctmp2) == 0); + CHECK(memcmp(zeros, ctmp, 32) == 0); + /* If seckey_tweak_add or seckey_tweak_mul are called with an overflowing + tweak, the seckey is zeroized. */ memcpy(ctmp, orderc, 32); ctmp[31] = 0x40; CHECK(secp256k1_ec_privkey_tweak_add(ctx, ctmp, orderc) == 0); @@ -4011,13 +4024,20 @@ void run_eckey_edge_case_test(void) { CHECK(memcmp(zeros, ctmp, 32) == 0); memcpy(ctmp, orderc, 32); ctmp[31] = 0x40; + /* If pubkey_tweak_add or pubkey_tweak_mul are called with an overflowing + tweak, the pubkey is zeroized. */ CHECK(secp256k1_ec_pubkey_tweak_add(ctx, &pubkey, orderc) == 0); CHECK(memcmp(&pubkey, zeros, sizeof(pubkey)) == 0); memcpy(&pubkey, &pubkey2, sizeof(pubkey)); CHECK(secp256k1_ec_pubkey_tweak_mul(ctx, &pubkey, orderc) == 0); CHECK(memcmp(&pubkey, zeros, sizeof(pubkey)) == 0); memcpy(&pubkey, &pubkey2, sizeof(pubkey)); - /* Private key tweaks results in a key of zero. */ + /* If the resulting key in secp256k1_ec_seckey_tweak_add and + * secp256k1_ec_pubkey_tweak_add is 0 the functions fail and in the latter + * case the pubkey is zeroized. */ + memcpy(ctmp, orderc, 32); + ctmp[31] = 0x40; + memset(ctmp2, 0, 32); ctmp2[31] = 1; CHECK(secp256k1_ec_privkey_tweak_add(ctx, ctmp2, ctmp) == 0); CHECK(memcmp(zeros, ctmp2, 32) == 0); @@ -4157,6 +4177,36 @@ void run_eckey_edge_case_test(void) { secp256k1_context_set_illegal_callback(ctx, NULL, NULL); } +void run_eckey_negate_test(void) { + unsigned char seckey[32]; + unsigned char seckey_tmp[32]; + + random_scalar_order_b32(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 zeroed. 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. */ + random_scalar_order_b32(seckey); + memset(seckey, 0xFF, 16); + memset(seckey_tmp, 0, 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 { @@ -5347,6 +5397,9 @@ int main(int argc, char **argv) { /* EC key edge cases */ run_eckey_edge_case_test(); + /* EC key arithmetic test */ + run_eckey_negate_test(); + #ifdef ENABLE_MODULE_ECDH /* ecdh tests */ run_ecdh_tests();