btcec/schnorr: reject s >= group order in ParseSignature

[Lrifton92]

Change Description

schnorr.ParseSignature documents that it enforces the BIP-340 requirement that the s component lie in the range [0, n-1], alongside the range check it already performs on r:

// - The r component must be in the valid range for secp256k1 field elements
// - The s component must be in the valid range for secp256k1 scalars

The r check is implemented correctly (overflow → ErrSigRTooBig), but for s the overflow return value of ModNScalar.SetByteSlice was discarded:

var s btcec.ModNScalar
s.SetByteSlice(sig[32:64])

Because SetByteSlice reduces its input modulo n and reports the overflow via its return value, an encoding with s >= n was silently reduced modulo n and accepted instead of being rejected. This diverges from:

• the function's own documented contract (and the inline comment stating s is enforced to be in [0, n-1]);
• BIP-340, whose verification algorithm requires "fail if s >= n";
• the reference implementation in decred/dcrd/dcrec/secp256k1/schnorr — of which this file is a port — which rejects the same input with ErrSigSTooBig. The ErrSigSTooBig error kind is already defined in the imported schnorr package but was unused here, which is what tipped me off that the check was dropped during the port.

The fix mirrors the existing r check exactly:

var s btcec.ModNScalar
if overflow := s.SetByteSlice(sig[32:64]); overflow {
	str := "invalid signature: s >= group order"
	return nil, signatureError(ecdsa_schnorr.ErrSigSTooBig, str)
}

ParseSignature is on the taproot signature-validation path (txscript key-path and script-path spends), so this also brings btcd's strictness in line with Bitcoin Core, which rejects non-canonical (s >= n) Schnorr encodings.

Test

Adds TestParseSignatureComponentRange, reusing the reference s == n and s > n vectors. The test fails before this change (both encodings are accepted, returning nil) and passes after. Verified by reverting the one-line fix while keeping the test:

--- FAIL: TestParseSignatureComponentRange
    signature_test.go: s == n: mismatched err -- got <nil>, want ErrSigSTooBig
    signature_test.go: s > n:  mismatched err -- got <nil>, want ErrSigSTooBig

go build ./... && go test ./... and go vet ./... are green across the whole btcec module.

Notes

This is intentionally minimal and independent of the other open schnorr PRs (#2546, #2501) — neither touches the s-range check in ParseSignature (I checked their diffs). Happy to rebase or fold this into #2546 if a maintainer prefers.

[allocz]

Nice catch. In fact BIP-340 fails the verification if s is >= N, as we can see here.

Confirmed that the tests are passing and reverting signature.go causes the test to fail.

[Lrifton92]

Thanks for the review @allocz — and for confirming the fix against BIP-340.

Addressed both formatting notes in the test:

• Re-wrapped the t.Errorf call so no line exceeds 80 columns at tab width 8 (my editor was rendering tabs narrower — good catch).
• Switched the table to the expanded struct-literal layout you showed (each case in its own { ... }, block).

gofmt/go vet clean and the full btcec/schnorr/... suite passes. The signature.go fix itself is unchanged.

[Lrifton92]

Thanks @allocz — you're right, the wrapped continuation still exceeded 80 columns. I've now unwrapped the sig strings entirely (single line each, as you suggested you'd approve), and re-ran gofmt so the struct fields realign cleanly. Test still passes. Ready for another look.