From f812519df0496e525dbb110e40a8ffc46c5ef021 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Wed, 1 Apr 2020 00:25:01 -0500 Subject: [PATCH] schnorr: Rework signature parsing. This reworks the Schnorr signature parsing code to be more consistent with the ecdsa parsing code and pave the way for eventually being able to remove the reliance on big ints. It also adds a full set of signature parsing tests that include 100% coverage of all branches in the parsing code and test for the explicit reason for the failure to ensure that each test is actually testing the intended condition. In particular, the parsing code has been changed as follows: - Reject signatures that have the r and/or s components out of the allowed ranges for their respective type - Make use of the specialized mod N scalar and field val types - Introduce new error codes for signature errors and return them --- dcrec/secp256k1/schnorr/error.go | 20 +++++++ dcrec/secp256k1/schnorr/error_test.go | 4 ++ dcrec/secp256k1/schnorr/signature.go | 59 +++++++++++++++------ dcrec/secp256k1/schnorr/signature_test.go | 63 +++++++++++++++++++++++ 4 files changed, 131 insertions(+), 15 deletions(-) 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.