Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return 0 if invalid seckey is given to ec_privkey_negate #668

Closed
wants to merge 3 commits into from
+78 −8
Closed

Return 0 if invalid seckey is given to ec_privkey_negate #668

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8091036
Return 0 if invalid seckey is given to ec_privkey_negate
jonasnick Oct 7, 2019
1748318
Add test for boundary conditions of scalar_set_b32 with respect to ov…
jonasnick Oct 8, 2019
3f5512c
Return 0 in privkey_negate for an all 0 seckey and move definition of…
jonasnick Oct 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions include/secp256k1.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

less than the curve order and not be 0?

(Actually we should just document what a valid private key is, e.g., here https://github.com/bitcoin-core/secp256k1/blob/master/include/secp256k1.h#L552)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't fail for 0 though. I think ideally we document what's a valid secret key is (as you said) in the doc of seckey_verify and then just refer to that instead of having to restate it over and over. I can rewrite this PR to do that.

*/
SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_privkey_negate(
const secp256k1_context* ctx,
Expand Down
6 changes: 5 additions & 1 deletion src/secp256k1.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
27 changes: 27 additions & 0 deletions src/tests.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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();
Expand Down