Skip to content

Commit

Permalink
Revert "ecdsa_impl: replace scalar if-checks with VERIFY_CHECKs in ec…
Browse files Browse the repository at this point in the history
…dsa_sig_sign"

This reverts commit 25e3cfb. The reverted
commit was probably based on the assumption that this is about the touched
checks cover the secret nonce k instead of r, which is the x-coord of the public
nonce. A signature with a zero r is invalid by the spec, so we should return 0
to make the caller retry with a different nonce. Overflow is not an issue.

Fixes dashpay#720.
  • Loading branch information
real-or-random committed Mar 31, 2020
1 parent 8f78e20 commit 93d343b
Showing 1 changed file with 8 additions and 4 deletions.
12 changes: 8 additions & 4 deletions src/ecdsa_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -288,10 +288,14 @@ static int secp256k1_ecdsa_sig_sign(const secp256k1_ecmult_gen_context *ctx, sec
secp256k1_fe_normalize(&r.y);
secp256k1_fe_get_b32(b, &r.x);
secp256k1_scalar_set_b32(sigr, b, &overflow);
/* These two conditions should be checked before calling */
VERIFY_CHECK(!secp256k1_scalar_is_zero(sigr));
VERIFY_CHECK(overflow == 0);

if (secp256k1_scalar_is_zero(sigr)) {
/* P.x = order is on the curve, so technically sig->r could end up zero, which would be an invalid signature.
* This branch is cryptographically unreachable as hitting it requires finding the discrete log of P.x = N.
*/
secp256k1_gej_clear(&rp);
secp256k1_ge_clear(&r);
return 0;
}
if (recid) {
/* The overflow condition is cryptographically unreachable as hitting it requires finding the discrete log
* of some P where P.x >= order, and only 1 in about 2^127 points meet this criteria.
Expand Down

0 comments on commit 93d343b

Please sign in to comment.