This PR unifies handling of invalid secret keys by introducing a new function scalar_set_b32_secret which returns false if the b32 overflows or is 0. By using this in privkey_{negate, tweak_add, tweak_mul} these function will now return 0 if the secret key is invalid which matches the behavior of ecdsa_sign and pubkey_create.
Instead of deciding whether to zeroize the secret key on failure, I only added documentation for now that the value is undefined on failure.
1148+ /* A scalar with value of the curve order minus one should not overflow. */
1149+ bin[31] -= 1;
1150+ secp256k1_scalar_set_b32(&scalar, bin, &overflow);
1151+ CHECK(overflow == 0);
1152+ secp256k1_scalar_get_b32(bin_tmp, &scalar);
1153+ CHECK(memcmp(bin, bin_tmp, 32) == 0);
scalar_is_zero.
601@@ -599,9 +602,10 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_pubkey_negate(
602
603 /** Tweak a private key by adding tweak to it.
604 * Returns: 0 if the tweak was out of range (chance of around 1 in 2^128 for
605- * uniformly random 32-byte arrays, or if the resulting private key
606- * would be invalid (only when the tweak is the complement of the
607- * private key). 1 otherwise.
608+ * uniformly random 32-byte arrays, or if the given private key is
609+ * invalid according to secp256k1_ec_seckey_verify, or if the resulting
610+ * private key would be invalid (only when the tweak is the complement
Maybe “negated private key” instead of “complement”?
Is this really the only case? What about overflows?
) after “32-byte arrays”. (This was a preexisting problem but we should fix it now.)
617@@ -614,9 +618,10 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_privkey_tweak_add(
618
619 /** Tweak a public key by adding tweak times the generator to it.
620 * Returns: 0 if the tweak was out of range (chance of around 1 in 2^128 for
621- * uniformly random 32-byte arrays, or if the resulting public key
622- * would be invalid (only when the tweak is the complement of the
623- * corresponding private key). 1 otherwise.
624+ * uniformly random 32-byte arrays, or if the given private key is
625+ * invalid according to secp256k1_ec_seckey_verify, or if the resulting
626+ * public key would be invalid (only when the tweak is the complement
) issue here
525@@ -526,7 +526,9 @@ int secp256k1_ec_privkey_negate(const secp256k1_context* ctx, unsigned char *sec
526 VERIFY_CHECK(ctx != NULL);
527 ARG_CHECK(seckey != NULL);
528
529- secp256k1_scalar_set_b32(&sec, seckey, NULL);
530+ if(!secp256k1_scalar_set_b32_seckey(&sec, seckey)) {
583@@ -584,7 +584,8 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_pubkey_create(
584 * secp256k1_ec_seckey_verify. 1 otherwise
585 * Args: ctx: pointer to a context object
586 * In/Out: seckey: pointer to the 32-byte private key to be negated. The private
587- * key should be valid according to secp256k1_ec_seckey_verify
588+ * key should be valid according to secp256k1_ec_seckey_verify.
589+ * Value becomes undefined if this function returns 0.
@real-or-random I added a commit that rephrases the docs. Let me know if you think that’s better.
(Also, I replaced secp256k1_rand256_test with secp256k1_rand256 because the former is apparently not random, may produce invalid seckeys and travis randomly hit that in one of the test configurations).
682+ * In/Out: seckey: pointer to a 32-byte secret key. If this function returns 0,
683+ * seckey will be some unspecified value. (cannot be NULL).
684+ * In: tweak: pointer to a 32-byte tweak. Must be in the same range as secret
685+ * keys (see secp256k1_ec_seckey_verify). For uniformly random
686+ * 32-byte arrays the chance of being out of range is
687+ * negligible (around 1 in 2^128). (cannot be NULL)
while the changes are negligible, if one is aimed to write a deterministic code (like in the case of rust wrapper), it may be good to cover this case as well.
It is possible to add to the docs that in case tweak<Z_p, the function will return 0?
Also, do you see any value of differentiating between tweak<Z_p and point at infinity cases? Right now both will return 0, but from the end-user perspective the first case requires change of the tweaking factor, while the second may be solved just by using a different public key…
while the changes are negligible, if one is aimed to write a deterministic code (like in the case of rust wrapper), it may be good to cover this case as well.
Believe me, you don’t care about a failure probability of 2^-128. It’s astronomically small. It’s much smaller than the probability of a hardware error (e.g., RAM returns wrong value). You can simply abort in this case.
3993@@ -3994,6 +3994,16 @@ void run_eckey_edge_case_test(void) {
3994 CHECK(secp256k1_ec_pubkey_tweak_mul(ctx, &pubkey, ctmp2) == 0);
3995 CHECK(memcmp(&pubkey, zeros, sizeof(pubkey)) == 0);
3996 memcpy(&pubkey, &pubkey2, sizeof(pubkey));
3997+ /* Overflowing key zeroizes */
3998+ memcpy(ctmp, orderc, 32);
3999+ memset(ctmp2, 0, 32);
4000+ ctmp2[31] = 0x01;
4001+ CHECK(secp256k1_ec_seckey_verify(ctx, ctmp2) == 1);
4002+ CHECK(secp256k1_ec_privkey_tweak_add(ctx, ctmp, ctmp2) == 0);
ctmp and ctmp2 swapped? Or maybe there is little value in this since both of these are invalid.
ecdsa_sign’s uses of secp256k1_scalar_set_b32_seckey a little bit.
550@@ -551,7 +551,10 @@ SECP256K1_API int secp256k1_ecdsa_sign(
551 const void *ndata
552 ) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4);
553
554-/** Verify an ECDSA secret key.
555+/** Verify an ECDSA secret key. It is valid if it is not 0 and less than the
550@@ -551,7 +551,10 @@ SECP256K1_API int secp256k1_ecdsa_sign(
551 const void *ndata
552 ) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4);
553
554-/** Verify an ECDSA secret key.
555+/** Verify an ECDSA secret key. It is valid if it is not 0 and less than the
556+ * secp256k1 curve order when interpreted as an integer (most significant byte
557+ * first). The probability of choosing a 32-byte string at random which is an
630+ * function returns 0, seckey will be some unspecified
631+ * value. (cannot be NULL)
632+ * In: tweak: pointer to a 32-byte tweak. Must be in the same range as secret
633+ * keys (see secp256k1_ec_seckey_verify). For uniformly random
634+ * 32-byte arrays the chance of being out of range is
635+ * negligible (around 1 in 2^128). (cannot be NULL)
662+ * In/Out: pubkey: pointer to a public key object. If this function returns 0,
663+ * pubkey will be invalid. (cannot be NULL).
664+ * In: tweak: pointer to a 32-byte tweak. Must be in the same range as secret
665+ * keys (see secp256k1_ec_seckey_verify). For uniformly random
666+ * 32-byte arrays the chance of being out of range is
667+ * negligible (around 1 in 2^128). (cannot be NULL)
470@@ -471,7 +471,7 @@ int secp256k1_ecdsa_sign(const secp256k1_context* ctx, secp256k1_ecdsa_signature
471 secp256k1_scalar r, s;
472 secp256k1_scalar sec, non, msg;
473 int ret = 0;
474- int overflow = 0;
475+ int is_sec_valid;
Neat! This really improves the logic in the signing function.
Now actually with the duplicate code, we should do this in the recover module as well… Not sure in which order to perform the changes. :/ Do you have a suggestion?
4002 CHECK(secp256k1_ec_pubkey_tweak_mul(ctx, &pubkey, ctmp2) == 0);
4003 CHECK(memcmp(&pubkey, zeros, sizeof(pubkey)) == 0);
4004 memcpy(&pubkey, &pubkey2, sizeof(pubkey));
4005- /* Overflowing key tweak zeroizes. */
4006+ /* If seckey_tweak_add or seckey_tweak_mul are called with an overflowing
4007+ seckey, the seckey is zeroized. */
CHECK(secp256k1_ec_seckey_verify(ctx, ctmp) == 0);
54@@ -55,6 +55,12 @@ static void secp256k1_scalar_order_get_num(secp256k1_num *r) {
55 }
56 #endif
57
58+static int secp256k1_scalar_set_b32_seckey(secp256k1_scalar *r, const unsigned char *bin) {
59+ int overflow;
60+ secp256k1_scalar_set_b32(r, bin, &overflow);
61+ return (!overflow) & (!secp256k1_scalar_is_zero(r));
598+ const secp256k1_context* ctx,
599+ unsigned char *seckey
600+) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2);
601+
602+/** Same as secp256k1_ec_seckey_negate, but DEPRECATED. Will be removed in
603+ * future versions. */
seckey variants? For example https://github.com/bitcoin-core/secp256k1/blob/master/src/valgrind_ctime_test.c#L76
612@@ -599,57 +613,85 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_pubkey_negate(
613 secp256k1_pubkey *pubkey
614 ) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2);
615
616-/** Tweak a private key by adding tweak to it.
617- * Returns: 0 if the tweak was out of range (chance of around 1 in 2^128 for
618- * uniformly random 32-byte arrays, or if the resulting private key
619- * would be invalid (only when the tweak is the complement of the
620- * private key). 1 otherwise.
621+/** Tweak a secret key by adding tweak to it.
53@@ -54,10 +54,7 @@ static int secp256k1_eckey_pubkey_serialize(secp256k1_ge *elem, unsigned char *p
54
55 static int secp256k1_eckey_privkey_tweak_add(secp256k1_scalar *key, const secp256k1_scalar *tweak) {
56 secp256k1_scalar_add(key, key, tweak);
57- if (secp256k1_scalar_is_zero(key)) {
58- return 0;
59- }
60- return 1;
652- * Returns: 0 if the tweak was out of range (chance of around 1 in 2^128 for
653- * uniformly random 32-byte arrays, or if the resulting public key
654- * would be invalid (only when the tweak is the complement of the
655- * corresponding private key). 1 otherwise.
656+ * Returns: 0 if the resulting public key would be invalid (only when the tweak
657+ * is the negation of the corresponding secret key). 1 otherwise.
588+ * Returns: 0 if the given secret key is invalid according to
589+ * secp256k1_ec_seckey_verify. 1 otherwise
590 * Args: ctx: pointer to a context object
591- * In/Out: seckey: pointer to the 32-byte private key to be negated (cannot be NULL)
592+ * In/Out: seckey: pointer to the 32-byte secret key to be negated. The secret
593+ * key should be valid according to secp256k1_ec_seckey_verify.
623+ * is the negation of the secret key). 1 otherwise.
624 * Args: ctx: pointer to a context object (cannot be NULL).
625- * In/Out: seckey: pointer to a 32-byte private key.
626- * In: tweak: pointer to a 32-byte tweak.
627- */
628+ * In/Out: seckey: pointer to a 32-byte secret key. The secret key should be
617- * Returns: 0 if the tweak was out of range (chance of around 1 in 2^128 for
618- * uniformly random 32-byte arrays, or if the resulting private key
619- * would be invalid (only when the tweak is the complement of the
620- * private key). 1 otherwise.
621+/** Tweak a secret key by adding tweak to it.
622+ * Returns: 0 if the resulting secret key would be invalid (only when the tweak
627- */
628+ * In/Out: seckey: pointer to a 32-byte secret key. The secret key should be
629+ * valid according to secp256k1_ec_seckey_verify. If this
630+ * function returns 0, seckey will be some unspecified
631+ * value. (cannot be NULL)
632+ * In: tweak: pointer to a 32-byte tweak. Must be in the same range as secret
Sorry for the many comments now, I probably could have pointed out some of those things earlier.
Since there are invalid tweaks, do we need a tweak_verify function?
571- return 1;
572+ return ret;
573+}
574+
575+int secp256k1_ec_privkey_negate(const secp256k1_context* ctx, unsigned char *seckey) {
576+ return secp256k1_ec_seckey_negate(ctx, seckey);
608@@ -605,6 +609,10 @@ int secp256k1_ec_privkey_tweak_add(const secp256k1_context* ctx, unsigned char *
609 return ret;
610 }
611
612+int secp256k1_ec_privkey_tweak_add(const secp256k1_context* ctx, unsigned char *seckey, const unsigned char *tweak) {
613+ return secp256k1_ec_seckey_tweak_add(ctx, seckey, tweak);
656@@ -649,6 +657,10 @@ int secp256k1_ec_privkey_tweak_mul(const secp256k1_context* ctx, unsigned char *
657 return ret;
658 }
659
660+int secp256k1_ec_privkey_tweak_mul(const secp256k1_context* ctx, unsigned char *seckey, const unsigned char *tweak) {
661+ return secp256k1_ec_seckey_tweak_mul(ctx, seckey, tweak);
625- * In/Out: seckey: pointer to a 32-byte private key.
626- * In: tweak: pointer to a 32-byte tweak.
627- */
628+/** Tweak a secret key by adding tweak to it.
629+ *
630+ * Returns: 0 if the arguments are invalid or resulting secret key would be
0 * Returns: 0 if the arguments are invalid or the resulting secret key would be
Do this by adding a newline after the first sentence and aligning the rest.
In particular, mention that the functions return 0 if seckey or tweak are
invalid (as opposed to saying "should" or "must" be valid).