Merge bitcoin/bitcoin#22934: Add verification to `Sign`, `SignCompact` and `SignSchnorr`

79fd28cacbbcb86ea03d3d468845001f84a76de3 Adds verification step to Schnorr and ECDSA signing (amadeuszpawlik)

Pull request description:

  As detailed in #22435, BIP340 defines that during Schnorr signing a verification should be done. This is so that potentially corrupt signage does not leak information about private keys used during the process. This is not followed today as no such verification step is being done. The same is valid for ECDSA signing functions `Sign` and `SignCompact`.
  This PR adds this missing verification step to `SignSchnorr`, `Sign` and `SignCompact`.

ACKs for top commit:
  sipa:
    utACK 79fd28cacbbcb86ea03d3d468845001f84a76de3
  laanwj:
    Code review ACK 79fd28cacbbcb86ea03d3d468845001f84a76de3
  theStack:
    re-ACK 79fd28cacbbcb86ea03d3d468845001f84a76de3

Tree-SHA512: 8fefa26caea577ae8631cc16c4e2f4cc6cfa1c7cf51d45a4a34165636ee290950617a17a19b4237c6f7a841db0e40fd5c36ad12ef43da82507c0e9fb9375ab82

1 files changed, 24 insertions, 3 deletions

diff --git a/src/key.cpp b/src/key.cpp
index 2e42c0718d..7688254515 100644
--- a/src/key.cpp
+++ b/src/key.cpp
@@ -229,6 +229,12 @@ bool CKey::Sign(const uint256 &hash, std::vector<unsigned char>& vchSig, bool gr
     assert(ret);
     secp256k1_ecdsa_signature_serialize_der(secp256k1_context_sign, vchSig.data(), &nSigLen, &sig);
     vchSig.resize(nSigLen);
+    // Additional verification step to prevent using a potentially corrupted signature
+    secp256k1_pubkey pk;
+    ret = secp256k1_ec_pubkey_create(secp256k1_context_sign, &pk, begin());
+    assert(ret);
+    ret = secp256k1_ecdsa_verify(GetVerifyContext(), &sig, hash.begin(), &pk);
+    assert(ret);
     return true;
 }
 
@@ -251,13 +257,21 @@ bool CKey::SignCompact(const uint256 &hash, std::vector<unsigned char>& vchSig)
         return false;
     vchSig.resize(CPubKey::COMPACT_SIGNATURE_SIZE);
     int rec = -1;
-    secp256k1_ecdsa_recoverable_signature sig;
-    int ret = secp256k1_ecdsa_sign_recoverable(secp256k1_context_sign, &sig, hash.begin(), begin(), secp256k1_nonce_function_rfc6979, nullptr);
+    secp256k1_ecdsa_recoverable_signature rsig;
+    int ret = secp256k1_ecdsa_sign_recoverable(secp256k1_context_sign, &rsig, hash.begin(), begin(), secp256k1_nonce_function_rfc6979, nullptr);
     assert(ret);
-    ret = secp256k1_ecdsa_recoverable_signature_serialize_compact(secp256k1_context_sign, &vchSig[1], &rec, &sig);
+    ret = secp256k1_ecdsa_recoverable_signature_serialize_compact(secp256k1_context_sign, &vchSig[1], &rec, &rsig);
     assert(ret);
     assert(rec != -1);
     vchSig[0] = 27 + rec + (fCompressed ? 4 : 0);
+    // Additional verification step to prevent using a potentially corrupted signature
+    secp256k1_pubkey epk, rpk;
+    ret = secp256k1_ec_pubkey_create(secp256k1_context_sign, &epk, begin());
+    assert(ret);
+    ret = secp256k1_ecdsa_recover(GetVerifyContext(), &rpk, &rsig, hash.begin());
+    assert(ret);
+    ret = secp256k1_ec_pubkey_cmp(GetVerifyContext(), &epk, &rpk);
+    assert(ret == 0);
     return true;
 }
 
@@ -275,6 +289,13 @@ bool CKey::SignSchnorr(const uint256& hash, Span<unsigned char> sig, const uint2
         if (!secp256k1_keypair_xonly_tweak_add(GetVerifyContext(), &keypair, tweak.data())) return false;
     }
     bool ret = secp256k1_schnorrsig_sign(secp256k1_context_sign, sig.data(), hash.data(), &keypair, aux ? (unsigned char*)aux->data() : nullptr);
+    if (ret) {
+        // Additional verification step to prevent using a potentially corrupted signature
+        secp256k1_xonly_pubkey pubkey_verify;
+        ret = secp256k1_keypair_xonly_pub(GetVerifyContext(), &pubkey_verify, nullptr, &keypair);
+        ret &= secp256k1_schnorrsig_verify(GetVerifyContext(), sig.data(), hash.begin(), 32, &pubkey_verify);
+    }
+    if (!ret) memory_cleanse(sig.data(), sig.size());
     memory_cleanse(&keypair, sizeof(keypair));
     return ret;
 }