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

amadeuszpawlik · 2021-09-09T20:05:27Z

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 · 2021-09-10T09:48:46Z

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 · 2021-09-13T17:42:59Z

Concept ACK

sipa

Concept ACK, two small comments.

Any reason not to do the same for ECDSA?

src/key.cpp

amadeuszpawlik · 2021-09-15T15:04:18Z

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

benthecarman · 2021-09-16T15:10:39Z

Please squash your commits

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

laanwj · 2021-09-27T11:32:39Z

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 · 2021-09-30T08:19:35Z

@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 · 2021-09-30T17:23:56Z

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

theStack

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).

src/key.cpp

amadeuszpawlik · 2021-10-04T16:44:31Z

@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

Light code-review ACK c9a920e
(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 · 2021-10-05T07:44:44Z

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

src/key.cpp

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.

sipa · 2021-11-02T21:23:39Z

utACK 79fd28c

theStack

re-ACK 79fd28c

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,

bitcoin/src/secp256k1/include/secp256k1_recovery.h

Lines 90 to 93 in 24abd83

    
           /** Recover an ECDSA public key from a signature. 
        
            * 
        
            *  Returns: 1: public key successfully recovered (which guarantees a correct signature). 
        
            *           0: otherwise.

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 · 2021-11-09T13:11:38Z

Code review ACK 79fd28c

…ignSchnorr` 79fd28c Adds verification step to Schnorr and ECDSA signing (amadeuszpawlik) Pull request description: As detailed in bitcoin#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 79fd28c laanwj: Code review ACK 79fd28c theStack: re-ACK 79fd28c Tree-SHA512: 8fefa26caea577ae8631cc16c4e2f4cc6cfa1c7cf51d45a4a34165636ee290950617a17a19b4237c6f7a841db0e40fd5c36ad12ef43da82507c0e9fb9375ab82

DrahtBot added Build system Utils/log/libs labels Sep 9, 2021

bitcoin deleted a comment from ah5884 Sep 10, 2021

maflcko removed the Build system label Sep 10, 2021

DrahtBot mentioned this pull request Sep 10, 2021

build: tighter Univalue integration, remove --with-system-univalue #22646

Merged

sipa reviewed Sep 13, 2021

View reviewed changes

src/key.cpp Outdated Show resolved Hide resolved

src/key.cpp Outdated Show resolved Hide resolved

amadeuszpawlik changed the title ~~Add verification to SignSchnorr~~ Add verification to Sign, SignCompact and SignSchnorr Sep 15, 2021

amadeuszpawlik force-pushed the schnorr_sig branch 4 times, most recently from 67e3138 to 454be06 Compare September 20, 2021 19:55

amadeuszpawlik force-pushed the schnorr_sig branch from 454be06 to 9ed9ef2 Compare September 30, 2021 17:20

theStack reviewed Oct 2, 2021

View reviewed changes

src/key.cpp Outdated Show resolved Hide resolved

amadeuszpawlik force-pushed the schnorr_sig branch 2 times, most recently from 96ec9d8 to c9a920e Compare October 4, 2021 15:09

theStack approved these changes Oct 4, 2021

View reviewed changes

This was referenced Oct 28, 2021

Update libsecp256k1 subtree to current master #23383

Merged

Taproot wallet test vectors (generation+tests) #23394

Closed

sipa reviewed Nov 1, 2021

View reviewed changes

src/key.cpp Outdated Show resolved Hide resolved

amadeuszpawlik force-pushed the schnorr_sig branch from c9a920e to ed16d80 Compare November 2, 2021 15:36

amadeuszpawlik force-pushed the schnorr_sig branch from ed16d80 to 79fd28c Compare November 2, 2021 16:19

theStack approved these changes Nov 4, 2021

View reviewed changes

nkohen mentioned this pull request Nov 9, 2021

Schnorr sig implementation diverges from the BIP340 standard bitcoin-s/bitcoin-s#3806

Closed

laanwj merged commit cb4adbd into bitcoin:master Nov 9, 2021

nkohen mentioned this pull request Mar 24, 2022

Added validation to signature methods to avoid corruption edge cases bitcoin-s/bitcoin-s#4214

Merged

sipa mentioned this pull request Jun 8, 2022

Schnorr sig implementation diverges from the BIP340 standard #22435

Closed

bitcoin locked and limited conversation to collaborators Nov 9, 2022

	/** Recover an ECDSA public key from a signature.
	*
	* Returns: 1: public key successfully recovered (which guarantees a correct signature).
	* 0: otherwise.

Add verification to Sign, SignCompact and SignSchnorr #22934

Add verification to Sign, SignCompact and SignSchnorr #22934

Uh oh!

Conversation

amadeuszpawlik commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

benthecarman commented Sep 13, 2021

Uh oh!

sipa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

amadeuszpawlik commented Sep 15, 2021

Uh oh!

benthecarman commented Sep 16, 2021

Uh oh!

laanwj commented Sep 27, 2021

Uh oh!

practicalswift commented Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amadeuszpawlik commented Sep 30, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amadeuszpawlik commented Oct 4, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

amadeuszpawlik commented Oct 5, 2021

Uh oh!

Uh oh!

sipa commented Nov 2, 2021

Uh oh!

theStack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Nov 9, 2021

Uh oh!

Uh oh!

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

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

amadeuszpawlik commented Sep 9, 2021 •

edited

Loading

DrahtBot commented Sep 10, 2021 •

edited

Loading

practicalswift commented Sep 30, 2021 •

edited

Loading

theStack left a comment •

edited

Loading