Add verification to `Sign`, `SignCompact` and `SignSchnorr` #22934

amadeuszpawlik commented at 8:05 PM on September 9, 2021: contributor

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.

DrahtBot added the label Build system on Sep 9, 2021

DrahtBot added the label Utils/log/libs on Sep 9, 2021

fanquake deleted a comment on Sep 10, 2021

MarcoFalke removed the label Build system on Sep 10, 2021

DrahtBot commented at 9:48 AM on September 10, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23394 (Taproot wallet test vectors (generation+tests) by sipa)
#23383 (Update libsecp256k1 subtree to current master by sipa)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

benthecarman commented at 5:42 PM on September 13, 2021: contributor

Concept ACK

in src/key.cpp:301 in b16428cd79 outdated

 279 | +    if(ret){
 280 | +        secp256k1_xonly_pubkey pubkey_verify;
 281 | +        ret = secp256k1_keypair_xonly_pub(GetVerifyContext(), &pubkey_verify, nullptr, &keypair);
 282 | +        ret &= secp256k1_schnorrsig_verify(GetVerifyContext(), sig.data(), hash.begin(), 32, &pubkey_verify);
 283 | +    }
 284 | +#endif

sipa commented at 5:48 PM on September 13, 2021:

For API safeness it's probably better to clear the output sig if ret is false.

amadeuszpawlik commented at 2:17 PM on September 14, 2021:

Like this?

fanquake commented at 3:21 AM on September 20, 2021:

How about using memory_cleanse().

amadeuszpawlik commented at 7:55 PM on September 20, 2021:

thanks

in src/key.cpp:279 in b16428cd79 outdated

 274 | @@ -275,6 +275,13 @@ bool CKey::SignSchnorr(const uint256& hash, Span<unsigned char> sig, const uint2
 275 |          if (!secp256k1_keypair_xonly_tweak_add(GetVerifyContext(), &keypair, tweak.data())) return false;
 276 |      }
 277 |      bool ret = secp256k1_schnorrsig_sign(secp256k1_context_sign, sig.data(), hash.data(), &keypair, aux ? (unsigned char*)aux->data() : nullptr);
 278 | +#ifdef ENABLE_SCHNORR_VERIFY
 279 | +    if(ret){

sipa commented at 5:48 PM on September 13, 2021:

Style nit: spaces before and after (ret).

amadeuszpawlik commented at 2:15 PM on September 14, 2021:

resolved, thanks

sipa commented at 5:49 PM on September 13, 2021: member

Concept ACK, two small comments.

Any reason not to do the same for ECDSA?

amadeuszpawlik renamed this:
~~Add verification to `SignSchnorr`~~
Add verification to `Sign`, `SignCompact` and `SignSchnorr`
on Sep 15, 2021

amadeuszpawlik commented at 3:04 PM on September 15, 2021: contributor

As per @sipa's suggestion, I added verification step to ECDSA signing functions too. Changed the disable flag name to reflect that.

benthecarman commented at 3:10 PM on September 16, 2021: contributor

Please squash your commits

https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

amadeuszpawlik force-pushed on Sep 18, 2021

amadeuszpawlik force-pushed on Sep 19, 2021

amadeuszpawlik force-pushed on Sep 20, 2021

laanwj commented at 11:32 AM on September 27, 2021: member

at the same time allowing for disabling the extra verification in case this extra step is computationally too expensive (--disable-verify-signing).

Does this ever come up in practice? I mean, yes, the secp256k1 library is used on some very low-end hardware. However Bitcoin Core's minimum specifications are a lot higher. The amount of signatures generated is also fairly low, compared to the number of verifications done, so it's important that verification is fast. Does it add that much overhead?

But maybe I'm missing something.

practicalswift commented at 8:19 AM on September 30, 2021: contributor

@amadeuszpawlik Do you have any measurements suggesting that --disable-verify-signing might be needed in practice for performance reasons? If not I think it would be better to not introduce that option :)

amadeuszpawlik force-pushed on Sep 30, 2021

amadeuszpawlik commented at 5:23 PM on September 30, 2021: contributor

@laanwj @practicalswift I got this idea from the original issue: #22435, but I agree with you both and have thus dropped the "disable option".

in src/key.cpp:295 in 9ed9ef2b61 outdated

 286 | @@ -275,6 +287,12 @@ bool CKey::SignSchnorr(const uint256& hash, Span<unsigned char> sig, const uint2
 287 |          if (!secp256k1_keypair_xonly_tweak_add(GetVerifyContext(), &keypair, tweak.data())) return false;
 288 |      }
 289 |      bool ret = secp256k1_schnorrsig_sign(secp256k1_context_sign, sig.data(), hash.data(), &keypair, aux ? (unsigned char*)aux->data() : nullptr);
 290 | +    if (ret) {
 291 | +        secp256k1_xonly_pubkey pubkey_verify;
 292 | +        ret = secp256k1_keypair_xonly_pub(GetVerifyContext(), &pubkey_verify, nullptr, &keypair);
 293 | +        ret &= secp256k1_schnorrsig_verify(GetVerifyContext(), sig.data(), hash.begin(), 32, &pubkey_verify);
 294 | +    }
 295 | +    if (!ret) memory_cleanse(&sig, sig.size()*sizeof(unsigned char));

theStack commented at 8:27 PM on October 2, 2021:

IMHO this cleans the wrong location, since you pass the address of the Span object, not the address it points to. Also, I think it's fine to simply pass the size without multiplying by sizeof(unsigned char) which is obviously 1. (We also use .size() on std::vector<unsigned char> objects frequently without this multiplication).

    if (!ret) memory_cleanse(sig.data(), sig.size());

theStack commented at 8:34 PM on October 2, 2021: member

Concept ACK

Left a code-review comment below. As an additional improvement, I'd suggest to add a comment like "// Additional verification step to prevent using a potentially corrupted signature" in each function before your introduced code to increase readability.

To fix the failed CI run on windows, rebasing on master should help (to include #23089).

amadeuszpawlik force-pushed on Oct 4, 2021

amadeuszpawlik commented at 4:44 PM on October 4, 2021: contributor

@theStack I don't think #23089 is it CirrusCI: TestFramework (ERROR): Unexpected exception caught during testing File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\wallet_send.py", line 506, in run_test ext_utxo = ext_fund.listunspent(addresses=[addr])[0] IndexError: list index out of range Can't reproduce on linux though

theStack approved

theStack commented at 8:22 PM on October 4, 2021: member

Light code-review ACK c9a920e378b1c8556ea4a3ca8d2cff01c7855484 (LGTM, though I don't have strong experience with correct secp256k1 usage, hence only "light") @amadeuszpawlik: Weird that it still fails on the Windows CI. I think it's unlikely that it's connected to this PR; if your introduced code failed in the course of a wallet send RPC, an assertion would be thrown (crashing bitcoind) and the "generate" RPC call above the line where the test assertion fails would not have succeeded.

amadeuszpawlik commented at 7:44 AM on October 5, 2021: contributor

@theStack Agree, I reran the Win64 build and it went through just fine 👍

in src/key.cpp:272 in c9a920e378 outdated

 270 | +    // Additional verification step to prevent using a potentially corrupted signature
 271 | +    secp256k1_pubkey pk;
 272 | +    secp256k1_ecdsa_signature sig;
 273 | +    ret = secp256k1_ec_pubkey_create(secp256k1_context_sign, &pk, begin());
 274 | +    assert(ret);
 275 | +    secp256k1_ecdsa_recoverable_signature_convert(GetVerifyContext(), &sig, &rsig);

sipa commented at 5:45 PM on November 1, 2021:

Instead, you could invoke the recovery operation itself, and verify that the decoded public key matches pk. That would verify that the recovery id in the signature is correct as well (secp256k1_ecdsa_recoverable_signature_convert throws it away).

amadeuszpawlik commented at 3:29 PM on November 2, 2021:

Hi @sipa, thanks a lot for the input. Let's see if I understand you right: instead of computing the pk in secp256k1_ec_pubkey_create, we recover the pk by running secp256k1_ecdsa_recover (which gives us the additional checks that you mention). The secp256k1_ecdsa_recoverable_signature_convert stays because we need a normal signature to do the verification - correct?

sipa commented at 3:46 PM on November 2, 2021:

No, the other way around.

You still use secp256k1_ec_pubkey_create to compute the expected public key. But then instead of verifying it against the normal signature extracted from sig, you run recovery on the message and sig, which gives you the signing public key. Then you can compare the expected pk with the signing one. There is no explicit signature verification in this case; instead you use the recovery operation (which is stricter than verification).

amadeuszpawlik commented at 4:21 PM on November 2, 2021:

Perfect, thanks!

amadeuszpawlik force-pushed on Nov 2, 2021

Adds verification step to Schnorr and ECDSA signing

As defined in BIP340, a verification step should be executed after
`secp256k1_schnorrsig_sign` to ensure that a potentially corrupted
signature isn't used; using corrupted signatures could reveal
information about the private key used. This applies to ECSDA as
well.

Additionally clears schnorr signature if signing failed.

79fd28cacb

amadeuszpawlik force-pushed on Nov 2, 2021

sipa commented at 9:23 PM on November 2, 2021: member

utACK 79fd28cacbbcb86ea03d3d468845001f84a76de3

theStack approved

theStack commented at 6:48 PM on November 4, 2021: member

re-ACK 79fd28cacbbcb86ea03d3d468845001f84a76de3

Checked that compared to my last ACK, the verificiation verification step in CKey::SignCompact is now done via ECDSA public key recovery (secp256k1 function secp256k1_ecdsa_recover), which according to the interface documentation "guarantees a correct signature" if successful, https://github.com/bitcoin/bitcoin/blob/24abd8312ec1caa04f9b3bd92cd960e28ca91e17/src/secp256k1/include/secp256k1_recovery.h#L90-L93 plus a comparison (secp256k1_ec_pubkey_cmp) between the recovered and the created public key (secp256k1_ec_pubkey_create). Also ran unit tests locally.

laanwj commented at 1:11 PM on November 9, 2021: member

Code review ACK 79fd28cacbbcb86ea03d3d468845001f84a76de3

laanwj merged this on Nov 9, 2021

laanwj closed this on Nov 9, 2021

sidhujag referenced this in commit 8d4ffa82f2 on Nov 9, 2021

DrahtBot locked this on Nov 9, 2022