From 93d343bfc5323e56f6a60cb41d60b96368cc09c7 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Tue, 31 Mar 2020 14:28:48 +0200 Subject: [PATCH 1/2] Revert "ecdsa_impl: replace scalar if-checks with VERIFY_CHECKs in ecdsa_sig_sign" This reverts commit 25e3cfbf9b52d2f5afa543f967a73aa8850d2038. 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 #720. --- src/ecdsa_impl.h | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/ecdsa_impl.h b/src/ecdsa_impl.h index 359c621..a944239 100644 --- a/src/ecdsa_impl.h +++ b/src/ecdsa_impl.h @@ -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. From 37ed51a7eafa5fd9955ceb66213251cdd8b8b8c4 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Tue, 31 Mar 2020 14:57:19 +0200 Subject: [PATCH 2/2] Make ecdsa_sig_sign constant-time again after reverting 25e3cfb --- src/ecdsa_impl.h | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/ecdsa_impl.h b/src/ecdsa_impl.h index a944239..5f54b59 100644 --- a/src/ecdsa_impl.h +++ b/src/ecdsa_impl.h @@ -288,14 +288,6 @@ 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); - 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. @@ -314,7 +306,10 @@ static int secp256k1_ecdsa_sig_sign(const secp256k1_ecmult_gen_context *ctx, sec if (recid) { *recid ^= high; } - return !secp256k1_scalar_is_zero(sigs); + /* P.x = order is on the curve, so technically sig->r could end up being zero, which would be an invalid signature. + * This is cryptographically unreachable as hitting it requires finding the discrete log of P.x = N. + */ + return !secp256k1_scalar_is_zero(sigr) & !secp256k1_scalar_is_zero(sigs); } #endif /* SECP256K1_ECDSA_IMPL_H */