btcec/schnorr: don't check message length, add test vectors

[aakselrod]

Change Description

Per bitcoin/bips@200f9b2 there is no longer a requirement to check message length when signing or verifying BIP-0340 Schnorr signatures. This commit updates the signing and verification algorithm and adds test vectors from the BIP for arbitrary-length messages.

Steps to Test

Check the added test vectors against the ones in BIP-0340, then run the unit tests for the btcec/schnorr directory. The tests already automatically run in CI.

Pull Request Checklist

Testing

• Your PR passes all CI checks.
• Tests covering the positive and negative (error paths) are included.
• Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• The change is not insubstantial. Typo fixes are not accepted to fight bot spam.
• The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• Commits follow the Ideal Git Commit Structure.
• Any new logging statements use an appropriate subsystem and logging level.

📝 Please see our Contribution Guidelines for further guidance.

[aakselrod]

Currently I've commented out the code paths I've eliminated, but happy to fully erase them if that's preferred. I can also renumber the signing/verification algorithm steps if desired.

Alternatively, I could also gate this behind a functional option so the length check is still done by default unless the option is included. The option could pass in an expected length, or just allow arbitrary length if specified.

The desired application is that I'm writing a btcec-based implementation of the ChillDKG BIP which requires a 4-byte message to be signed.

An aside: there's also a requirement in ChillDKG to use different tags for the tagged hashes than what's specified by BIP-0340. I'll address that in a followup PR by passing in the alternate tags as functional options and make it clear that those options aren't for use with Bitcoin consensus/transaction signing.

[Lrifton92]

The spec side of this is right. I checked BIP-340's "Messages of Arbitrary Size" section — "The signature scheme specified in this BIP accepts byte strings of arbitrary size as input messages" — along with the 2023-04 changelog entry allowing it. I also diffed the four added vectors against bip-0340/test-vectors.csv on bips master: byte-identical to indices 15, 16, 17 and 18 (sizes 0, 1, 17, 100). No issue there.

However, I don't think this can land as-is. Removing the length check makes a pre-existing, non-BIP-340 nonce derivation reachable, and the consequence is private key recovery from two signatures.

Blocking: nonce reuse -> key recovery on the default (RFC6979) signing path

Sign() only follows BIP-340 nonce derivation when CustomNonce is passed. The default path falls through to the RFC6979 loop:

k := btcec.NonceRFC6979(
        privKeyBytes[:], hash, rfc6979ExtraDataV0[:], nil, iteration,
)

NonceRFC6979 normalizes its hash argument to exactly 32 bytes before HMAC'ing it. From github.com/decred/dcrd/dcrec/secp256k1/v4@v4.4.0/nonce.go:135-141 (the version pinned in btcec/go.mod):

if len(hash) > hashLen {
        hash = hash[:hashLen]
}
offset := privKeyLen - len(privKey) // Zero left padding if needed.
offset += copy(keyBuf[offset:], privKey)
offset += hashLen - len(hash) // Zero left padding if needed.
offset += copy(keyBuf[offset:], hash)

So the nonce depends only on the first 32 bytes of the message (zero-left-padded if shorter), while the challenge e in schnorrSign commits to the entire message. Any two distinct messages that collapse to the same 32-byte RFC6979 view therefore share k, hence share R, and d = (s1 - s2) / (e1 - e2) falls out immediately.

Until this PR that was unreachable: the len(hash) != scalarSize check in Sign rejected every input that could trigger it. Commenting it out makes it reachable with default options and no warning.

I ran this against this branch (9021273). Three independent collision classes, each recovering the full key. Secret key is vector 15's 0340...0340, signatures produced by the default Sign() with no options:

case	m1	m2	shared R	key recovered
truncation	ab*40	ab*32	d1137e64f9a7707f33be3fd94af711a636e10ba2f55a5904e6172d20d5fe89f6	yes, exact
common 32-byte prefix	cd*32 || 01	cd*32 || 02	9b1968c4f26fa978a3131a29b093bf7d8472928b6d747560b917c533482fc16b	yes, exact
zero left-padding	11 (1 byte)	00*31 || 11 (32 bytes)	6247e30a769f0c845ee735e518faf1b706fbdfed24fb7072f57270405bfed60c	yes, exact

Output for the common-prefix case:

m1=cdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcd01 (33 bytes)
m2=cdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcd02 (33 bytes)
R1=9b1968c4f26fa978a3131a29b093bf7d8472928b6d747560b917c533482fc16b
R2=9b1968c4f26fa978a3131a29b093bf7d8472928b6d747560b917c533482fc16b
recovered d' = 0340034003400340034003400340034003400340034003400340034003400340
actual  d    = 0340034003400340034003400340034003400340034003400340034003400340

Recovery is just d = (s1 - s2) / (e1 - e2) with e = H(R.x || P.x || m) recomputed per message; no search involved.

The common-prefix class is the dangerous one in practice: once arbitrary-length messages are allowed, callers sign structured/serialized payloads that share a fixed header, and 32 bytes of shared prefix is a very low bar. This also bears on the stated ChillDKG use case — a 4-byte message collides with the 32-byte message that is its zero-left-padded form, under the same key.

BIP-340 is explicit that this is not merely a determinism nit: "No matter which method is used to generate the rand value, the value must be a fresh uniformly random 32-byte string which is not even partially predictable for the attacker. For nonces without randomness, this implies that the same inputs must not be presented in another context."

Minimal fix that keeps every existing 32-byte signature bit-identical (no test vector churn, no change for the txscript callers) — pre-hash only when the message is not already a 32-byte digest, so the nonce stays bound to the whole message:

// NonceRFC6979 only absorbs the first 32 bytes of the message (and
// zero-left-pads shorter ones), while the BIP-340 challenge commits to
// the full message. Pre-hash anything that is not already a 32-byte
// digest so that the nonce stays bound to the entire message and
// distinct messages cannot collide onto the same nonce.
nonceMsg := hash
if len(nonceMsg) != scalarSize {
        h := sha256.Sum256(hash)
        nonceMsg = h[:]
}

and feed nonceMsg to NonceRFC6979. Alternatively, return an error when len(hash) != scalarSize and no CustomNonce was supplied — more conservative, but it pushes the burden onto callers.

Either way I'd add a regression test asserting that Sign produces distinct R values for two messages sharing a 32-byte prefix. That's the assertion that keeps this from silently regressing.

Test coverage gap

TestSchnorrSign only takes the CustomNonce path unless test.rfc6979 is set, and the only vector with rfc6979: true is the 32-byte one. So all four new vectors exercise BIP-340 aux-rand nonce derivation — which is correct, and which I confirmed does hash the full message (TaggedHash(TagBIP0340Nonce, t[:], pubKeyBytes[1:], hash)). The vulnerable default path is never exercised with a non-32-byte message, which is why CI is green here. go test ./schnorr/... passes on this branch.

Stale invariant doc

btcec/schnorr/signature.go:245, on schnorrSign, still reads:

// WARNING: The hash MUST be 32 bytes and both the nonce and private keys must
// NOT be 0.  Since this is an internal use function, these preconditions MUST
// be satisfied by the caller.

That precondition no longer holds and, given the issue above, it's the one place a reader would look to convince themselves the change is safe. Worth rewriting rather than leaving it contradicting the code.

Scope check (not a problem, for the record)

I traced the in-tree callers and the consensus path is unaffected either way: txscript/sign.go:88 and :159 pass sigHash from calcTaprootSignatureHashRaw, which returns chainhash.TaggedHash(...)[:], always 32 bytes; the verify side (txscript/sigvalidate.go:144, :149, :355) does the same. musig2 types its message as [32]byte (musig2/sign.go:256, musig2/context.go:610), so it can't reach the new path at all. The blast radius is external/library callers, not block validation.

On your open questions

Deleting the dead branches rather than commenting them out, and renumbering the algorithm steps, would be my preference — commented-out code with a "skipped as no longer required" note reads as unfinished, and the step numbers in the reproduced algorithm no longer match the BIP.

I wouldn't gate this behind a functional option. The length check isn't the real invariant being protected; the nonce derivation is. Fix the nonce derivation and arbitrary-length messages can be supported unconditionally, matching the BIP.

Also note this branch is ~175 commits behind master and still imports github.com/btcsuite/btcd/chaincfg/chainhash, which master has moved to github.com/btcsuite/btcd/chainhash/v2. It'll need a rebase.

[aakselrod]

Thanks for the in-depth review, and great catch on the nonce collision! I've rebased and added a test that fails with the examples you provided; next commit will fix the issue and address the rest of the comments. I do prefer pre-hashing for non-32-byte messages, which will make things easier on the caller.

Also note that in the specific ChillDKG use case, the BIP specifies a nonce construction, so the CustomNonce option is always used.

[aakselrod]

Seems good now, so I'm going to squash and update the commit message.

[aakselrod]

Failure looks like an unrelated flake

[Lrifton92]

Re-reviewed at 41437ee. The nonce derivation is now bound to the whole message on the default path (nonceMsg = chainhash.HashB(hash) when len(hash) != 32), and existing 32-byte inputs are untouched so the RFC6979 vector and every txscript/musig2 caller stay bit-identical.

I re-ran my key-recovery harness against this head: for all three collision classes (truncation, shared 32-byte prefix, zero left-padding) Sign() now yields distinct R and d = (s1 - s2)/(e1 - e2) no longer recovers the key. As a control I reverted just the pre-hash line: the harness recovers the key 3/3 and TestSchnorrSignArbitraryLengthNoNonceCollision fails 3/3, so the new regression test does fail for the right reason. go test ./schnorr/... (incl. musig2) passes on this branch, which is now rebased on chainhash/v2.

The rpctest failure is TestSyncManagerRaceCorruption in integration/, outside the btcec module — unrelated to this change, agreed it looks like a flake.

One leftover doc nit inline, non-blocking. LGTM ✅

[Lrifton92]

Re-approving on 5605f0f — the steps 5-7 reference in signature.go is fixed and the squashed commit still carries the nonce-collision test vectors. LGTM.

[allocz]

This change was also discussed in #2546, but changing the implementation breaks the semantics of the API, as we currently do not allow messages of arbitrary length.

[aakselrod]

In that case, maybe it would be better to actually gate this with a functional option similar to the next commit in #2590 (which provides a use case for signing arbitrary-length messages). I'll wait for a review from a project maintainer to make that decision, but I could easily bring forward the infrastructure (VerifyCustom and passing signing options to schnorrVerify) from that commit and use it to gate this as well. Alternatively, I could allow it only with a custom nonce.

Copying and adapting the signing code from the schnorr package into the threshold subpackage is an option but seems like it would cause maintenance headaches in the future.