diff --git a/dcrec/secp256k1/schnorr/error.go b/dcrec/secp256k1/schnorr/error.go index cb71091e..ddbb8e0a 100644 --- a/dcrec/secp256k1/schnorr/error.go +++ b/dcrec/secp256k1/schnorr/error.go @@ -61,6 +61,22 @@ const ( // threshold signature failed to have a matching R value. ErrNonmatchingR + // ErrSigTooShort is returned when a signature that should be a Schnorr + // signature is too short. + ErrSigTooShort + + // ErrSigTooLong is returned when a signature that should be a Schnorr + // signature is too long. + ErrSigTooLong + + // ErrSigRTooBig is returned when a signature has r with a value that is + // greater than or equal to the prime of the field underlying the group. + ErrSigRTooBig + + // ErrSigSTooBig is returned when a signature has s with a value that is + // greater than or equal to the group order. + ErrSigSTooBig + // numErrorCodes is the maximum error code number used in tests. This entry // MUST be the last entry in the enum. numErrorCodes @@ -80,6 +96,10 @@ var errorCodeStrings = map[ErrorCode]string{ ErrBadNonce: "ErrBadNonce", ErrZeroSigS: "ErrZeroSigS", ErrNonmatchingR: "ErrNonmatchingR", + ErrSigTooShort: "ErrSigTooShort", + ErrSigTooLong: "ErrSigTooLong", + ErrSigRTooBig: "ErrSigRTooBig", + ErrSigSTooBig: "ErrSigSTooBig", } // String returns the ErrorCode as a human-readable name. diff --git a/dcrec/secp256k1/schnorr/error_test.go b/dcrec/secp256k1/schnorr/error_test.go index 51bb81a5..831ad555 100644 --- a/dcrec/secp256k1/schnorr/error_test.go +++ b/dcrec/secp256k1/schnorr/error_test.go @@ -27,6 +27,10 @@ func TestErrorCodeStringer(t *testing.T) { {ErrBadNonce, "ErrBadNonce"}, {ErrZeroSigS, "ErrZeroSigS"}, {ErrNonmatchingR, "ErrNonmatchingR"}, + {ErrSigTooShort, "ErrSigTooShort"}, + {ErrSigTooLong, "ErrSigTooLong"}, + {ErrSigRTooBig, "ErrSigRTooBig"}, + {ErrSigSTooBig, "ErrSigSTooBig"}, {0xffff, "Unknown ErrorCode (65535)"}, } diff --git a/dcrec/secp256k1/schnorr/signature.go b/dcrec/secp256k1/schnorr/signature.go index ddcc5893..5607133c 100644 --- a/dcrec/secp256k1/schnorr/signature.go +++ b/dcrec/secp256k1/schnorr/signature.go @@ -66,24 +66,53 @@ func (sig Signature) Serialize() []byte { return all } -func parseSig(sigStr []byte) (*Signature, error) { - if len(sigStr) != SignatureSize { - return nil, fmt.Errorf("bad signature size; have %v, want %v", - len(sigStr), SignatureSize) +// ParseSignature parses a signature according to the EC-Schnorr-DCRv0 +// specification and enforces the following additional restrictions specific to +// secp256k1: +// +// - 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 +func ParseSignature(sig []byte) (*Signature, error) { + // The signature must be the correct length. + sigLen := len(sig) + if sigLen < SignatureSize { + str := fmt.Sprintf("malformed signature: too short: %d < %d", sigLen, + SignatureSize) + return nil, signatureError(ErrSigTooShort, str) + } + if sigLen > SignatureSize { + str := fmt.Sprintf("malformed signature: too long: %d > %d", sigLen, + SignatureSize) + return nil, signatureError(ErrSigTooLong, str) } - rBytes := copyBytes(sigStr[0:32]) - r := encodedBytesToBigInt(rBytes) - sBytes := copyBytes(sigStr[32:64]) - s := encodedBytesToBigInt(sBytes) + // The signature is validly encoded at this point, however, enforce + // additional restrictions to ensure r is in the range [0, p-1], and s is in + // the range [0, n-1] since valid Schnorr signatures are required to be in + // that range per spec. + // + // Notice that rejecting these values here is not strictly required because + // they are also checked when verifying the signature, but there really + // isn't a good reason not to fail early here on signatures that do not + // conform to the spec. + var r secp256k1.FieldVal + if overflow := r.SetByteSlice(sig[0:32]); overflow { + str := "invalid signature: r >= field prime" + return nil, signatureError(ErrSigRTooBig, str) + } + var s secp256k1.ModNScalar + if overflow := s.SetByteSlice(sig[32:64]); overflow { + str := "invalid signature: s >= group order" + return nil, signatureError(ErrSigSTooBig, str) + } - return &Signature{r, s}, nil -} - -// ParseSignature parses a signature in BER format for the curve type `curve' -// into a Signature type, performing some basic sanity checks. -func ParseSignature(sigStr []byte) (*Signature, error) { - return parseSig(sigStr) + // Return the signature. + var rBytes, sBytes [scalarSize]byte + r.PutBytes(&rBytes) + s.PutBytes(&sBytes) + rBig := encodedBytesToBigInt(&rBytes) + sBig := encodedBytesToBigInt(&sBytes) + return &Signature{rBig, sBig}, nil } // IsEqual compares this Signature instance to the one passed, returning true diff --git a/dcrec/secp256k1/schnorr/signature_test.go b/dcrec/secp256k1/schnorr/signature_test.go index b7058e4c..e7f27975 100644 --- a/dcrec/secp256k1/schnorr/signature_test.go +++ b/dcrec/secp256k1/schnorr/signature_test.go @@ -15,6 +15,69 @@ import ( "github.com/decred/dcrd/dcrec/secp256k1/v3" ) +// TestSignatureParsing ensures that signatures are properly parsed including +// error paths. +func TestSignatureParsing(t *testing.T) { + tests := []struct { + name string // test description + sig string // hex encoded signature to parse + err error // expected error + }{{ + name: "valid signature 1", + sig: "c6ec70969d8367538c442f8e13eb20ff0c9143690f31cd3a384da54dd29ec0aa" + + "4b78a1b0d6b4186195d42a85614d3befd9f12ed26542d0dd1045f38c98b4a405", + err: nil, + }, { + name: "valid signature 2", + sig: "adc21db084fa1765f9372c2021fb298720f3d13e6d844e2dff751a2d46a69277" + + "0b989e316f7faf308a5f4a7343c0569465287cf6bff457250d6dacbb361f6e63", + err: nil, + }, { + name: "empty", + sig: "", + err: ErrSigTooShort, + }, { + name: "too short by one byte", + sig: "adc21db084fa1765f9372c2021fb298720f3d13e6d844e2dff751a2d46a69277" + + "0b989e316f7faf308a5f4a7343c0569465287cf6bff457250d6dacbb361f6e", + err: ErrSigTooShort, + }, { + name: "too long by one byte", + sig: "adc21db084fa1765f9372c2021fb298720f3d13e6d844e2dff751a2d46a69277" + + "0b989e316f7faf308a5f4a7343c0569465287cf6bff457250d6dacbb361f6e6300", + err: ErrSigTooLong, + }, { + name: "r == p", + sig: "fffffffffffffffffffffffffffffffffffffffffffffffffffffffefffffc2f" + + "181522ec8eca07de4860a4acdd12909d831cc56cbbac4622082221a8768d1d09", + err: ErrSigRTooBig, + }, { + name: "r > p", + sig: "fffffffffffffffffffffffffffffffffffffffffffffffffffffffefffffc30" + + "181522ec8eca07de4860a4acdd12909d831cc56cbbac4622082221a8768d1d09", + err: ErrSigRTooBig, + }, { + name: "s == n", + sig: "4e45e16932b8af514961a1d3a1a25fdf3f4f7732e9d624c6c61548ab5fb8cd41" + + "fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141", + err: ErrSigSTooBig, + }, { + name: "s > n", + sig: "4e45e16932b8af514961a1d3a1a25fdf3f4f7732e9d624c6c61548ab5fb8cd41" + + "fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364142", + err: ErrSigSTooBig, + }} + + for _, test := range tests { + _, err := ParseSignature(hexToBytes(test.sig)) + if !errors.Is(err, test.err) { + t.Errorf("%s mismatched err -- got %v, want %v", test.name, err, + test.err) + continue + } + } +} + // TestSchnorrSignAndVerify ensures the Schnorr signing function produces the // expected signatures for a selected set of private keys, messages, and nonces // that have been independently verified with the Sage computer algebra system.