diff --git a/dcrec/secp256k1/signature.go b/dcrec/secp256k1/signature.go index 16d07f4e..a70c6638 100644 --- a/dcrec/secp256k1/signature.go +++ b/dcrec/secp256k1/signature.go @@ -262,10 +262,9 @@ func (sig *Signature) IsEqual(otherSig *Signature) bool { } // parseSig attempts to parse the provided raw signature bytes into a Signature -// struct. The der flag specifies whether or not to enforce the more strict -// Distinguished Encoding Rules (DER) of the ASN.1 spec versus the more lax -// Basic Encoding Rules (BER). -func parseSig(sigStr []byte, der bool) (*Signature, error) { +// struct while enforcing the more strict Distinguished Encoding Rules (DER) +// format per section 10 of [ISO/IEC 8825-1]. +func parseSig(sigStr []byte) (*Signature, error) { // Originally this code used encoding/asn1 in order to parse the // signature, but a number of problems were found with this approach. // Despite the fact that signatures are stored as DER, the difference @@ -293,11 +292,9 @@ func parseSig(sigStr []byte, der bool) (*Signature, error) { // length of remaining message siglen := sigStr[index] index++ - if int(siglen+2) > len(sigStr) { + if int(siglen+2) != len(sigStr) { return nil, errors.New("malformed signature: bad length") } - // trim the slice we're working on so we only look at what matters. - sigStr = sigStr[:siglen+2] // 0x02 if sigStr[index] != 0x02 { @@ -317,13 +314,11 @@ func parseSig(sigStr []byte, der bool) (*Signature, error) { // Then R itself. rBytes := sigStr[index : index+rLen] - if der { - switch err := canonicalPadding(rBytes); err { - case errNegativeValue: - return nil, errors.New("signature R is negative") - case errExcessivelyPaddedValue: - return nil, errors.New("signature R is excessively padded") - } + switch err := canonicalPadding(rBytes); err { + case errNegativeValue: + return nil, errors.New("signature R is negative") + case errExcessivelyPaddedValue: + return nil, errors.New("signature R is excessively padded") } signature.r = new(big.Int).SetBytes(rBytes) index += rLen @@ -343,13 +338,11 @@ func parseSig(sigStr []byte, der bool) (*Signature, error) { // Then S itself. sBytes := sigStr[index : index+sLen] - if der { - switch err := canonicalPadding(sBytes); err { - case errNegativeValue: - return nil, errors.New("signature S is negative") - case errExcessivelyPaddedValue: - return nil, errors.New("signature S is excessively padded") - } + switch err := canonicalPadding(sBytes); err { + case errNegativeValue: + return nil, errors.New("signature S is negative") + case errExcessivelyPaddedValue: + return nil, errors.New("signature S is excessively padded") } signature.s = new(big.Int).SetBytes(sBytes) index += sLen @@ -380,18 +373,10 @@ func parseSig(sigStr []byte, der bool) (*Signature, error) { return signature, nil } -// ParseSignature parses a signature in the Basic Encoding Rules (BER) format -// into a Signature type, performing some basic sanity checks. If parsing -// according to the more strict DER format is needed, use ParseDERSignature. -func ParseSignature(sigStr []byte) (*Signature, error) { - return parseSig(sigStr, false) -} - // ParseDERSignature parses a signature in the Distinguished Encoding Rules -// (DER) format of the ASN.1 spec into a Signature type. If parsing according -// to the less strict BER format is needed, use ParseSignature. +// (DER) format per section 10 of [ISO/IEC 8825-1]. func ParseDERSignature(sigStr []byte) (*Signature, error) { - return parseSig(sigStr, true) + return parseSig(sigStr) } // canonicalPadding checks whether a big-endian encoded integer could diff --git a/dcrec/secp256k1/signature_test.go b/dcrec/secp256k1/signature_test.go index bd23fffb..54f31677 100644 --- a/dcrec/secp256k1/signature_test.go +++ b/dcrec/secp256k1/signature_test.go @@ -40,12 +40,11 @@ func hexToBytes(s string) []byte { } // TestSignatureParsing ensures that signatures are properly parsed according -// to both BER and DER rules. The error paths are tested as well. +// to DER rules. The error paths are tested as well. func TestSignatureParsing(t *testing.T) { tests := []struct { name string sig []byte - der bool isValid bool }{{ // signatures from bitcoin blockchain tx @@ -54,7 +53,6 @@ func TestSignatureParsing(t *testing.T) { sig: hexToBytes("304402204e45e16932b8af514961a1d3a1a25fdf3f4f7732e9d6" + "24c6c61548ab5fb8cd410220181522ec8eca07de4860a4acdd12909d831cc56c" + "bbac4622082221a8768d1d09"), - der: true, isValid: true, }, { name: "empty", @@ -65,152 +63,99 @@ func TestSignatureParsing(t *testing.T) { sig: hexToBytes("314402204e45e16932b8af514961a1d3a1a25fdf3f4f7732e9d6" + "24c6c61548ab5fb8cd410220181522ec8eca07de4860a4acdd12909d831cc56c" + "bbac4622082221a8768d1d09"), - der: true, isValid: false, }, { name: "bad 1st int marker magic", sig: hexToBytes("304403204e45e16932b8af514961a1d3a1a25fdf3f4f7732e9d6" + "24c6c61548ab5fb8cd410220181522ec8eca07de4860a4acdd12909d831cc56c" + "bbac4622082221a8768d1d09"), - der: true, isValid: false, }, { name: "bad 2nd int marker", sig: hexToBytes("304402204e45e16932b8af514961a1d3a1a25fdf3f4f7732e9d6" + "24c6c61548ab5fb8cd410320181522ec8eca07de4860a4acdd12909d831cc56c" + "bbac4622082221a8768d1d09"), - der: true, isValid: false, }, { name: "short len", sig: hexToBytes("304302204e45e16932b8af514961a1d3a1a25fdf3f4f7732e9d6" + "24c6c61548ab5fb8cd410220181522ec8eca07de4860a4acdd12909d831cc56c" + "bbac4622082221a8768d1d09"), - der: true, isValid: false, }, { name: "long len", sig: hexToBytes("304502204e45e16932b8af514961a1d3a1a25fdf3f4f7732e9d6" + "24c6c61548ab5fb8cd410220181522ec8eca07de4860a4acdd12909d831cc56c" + "bbac4622082221a8768d1d09"), - der: true, isValid: false, }, { name: "long X", sig: hexToBytes("304402424e45e16932b8af514961a1d3a1a25fdf3f4f7732e9d6" + "24c6c61548ab5fb8cd410220181522ec8eca07de4860a4acdd12909d831cc56c" + "bbac4622082221a8768d1d09"), - der: true, isValid: false, }, { name: "long Y", sig: hexToBytes("304402204e45e16932b8af514961a1d3a1a25fdf3f4f7732e9d6" + "24c6c61548ab5fb8cd410221181522ec8eca07de4860a4acdd12909d831cc56c" + "bbac4622082221a8768d1d09"), - der: true, isValid: false, }, { name: "short Y", sig: hexToBytes("304402204e45e16932b8af514961a1d3a1a25fdf3f4f7732e9d6" + "24c6c61548ab5fb8cd410219181522ec8eca07de4860a4acdd12909d831cc56c" + "bbac4622082221a8768d1d09"), - der: true, isValid: false, }, { name: "trailing crap", sig: hexToBytes("304402204e45e16932b8af514961a1d3a1a25fdf3f4f7732e9d6" + "24c6c61548ab5fb8cd410220181522ec8eca07de4860a4acdd12909d831cc56c" + "bbac4622082221a8768d1d0901"), - der: true, - - // This test is now passing (used to be failing) because there are - // signatures in the blockchain that have trailing zero bytes before - // the hashtype. So ParseSignature was fixed to permit buffers with - // trailing nonsense after the actual signature. - isValid: true, + isValid: false, }, { name: "X == N DER", sig: hexToBytes("30440220fffffffffffffffffffffffffffffffebaaedce6af48" + "a03bbfd25e8cd03641410220181522ec8eca07de4860a4acdd12909d831cc56c" + "bbac4622082221a8768d1d09"), - der: true, - isValid: false, - }, { - name: "X == N BER", - sig: hexToBytes("30440220fffffffffffffffffffffffffffffffebaaedce6af48" + - "a03bbfd25e8cd03641420220181522ec8eca07de4860a4acdd12909d831cc56c" + - "bbac4622082221a8768d1d09"), - der: false, isValid: false, }, { name: "Y == N", sig: hexToBytes("304402204e45e16932b8af514961a1d3a1a25fdf3f4f7732e9d6" + "24c6c61548ab5fb8cd410220fffffffffffffffffffffffffffffffebaaedce6" + "af48a03bbfd25e8cd0364141"), - der: true, isValid: false, }, { name: "Y > N", sig: hexToBytes("304402204e45e16932b8af514961a1d3a1a25fdf3f4f7732e9d6" + "24c6c61548ab5fb8cd410220fffffffffffffffffffffffffffffffebaaedce6" + "af48a03bbfd25e8cd0364142"), - der: false, isValid: false, }, { name: "0 len X", sig: hexToBytes("302402000220181522ec8eca07de4860a4acdd12909d831cc56c" + "bbac4622082221a8768d1d09"), - der: true, isValid: false, }, { name: "0 len Y", sig: hexToBytes("302402204e45e16932b8af514961a1d3a1a25fdf3f4f7732e9d6" + "24c6c61548ab5fb8cd410200"), - der: true, isValid: false, }, { name: "extra R padding", sig: hexToBytes("30450221004e45e16932b8af514961a1d3a1a25fdf3f4f7732e9" + "d624c6c61548ab5fb8cd410220181522ec8eca07de4860a4acdd12909d831cc5" + "6cbbac4622082221a8768d1d09"), - der: true, isValid: false, }, { name: "extra S padding.", sig: hexToBytes("304502204e45e16932b8af514961a1d3a1a25fdf3f4f7732e9d6" + "24c6c61548ab5fb8cd41022100181522ec8eca07de4860a4acdd12909d831cc5" + "6cbbac4622082221a8768d1d09"), - der: true, - isValid: false, - }, { - // Standard checks (in BER format, without checking for 'canonical' DER - // signatures) don't test for negative numbers here because there isn't - // a way that is the same between openssl and go that will mark a number - // as negative. The Go ASN.1 parser marks numbers as negative when - // openssl does not (it doesn't handle negative numbers that I can tell - // at all. When not parsing DER signatures, which is done by by bitcoind - // when accepting transactions into its mempool, we otherwise only check - // for the coordinates being zero. - name: "X == 0", - sig: hexToBytes("30250201000220181522ec8eca07de4860a4acdd12909d831cc5" + - "6cbbac4622082221a8768d1d09"), - der: false, - isValid: false, - }, { - name: "Y == 0", - sig: hexToBytes("302502204e45e16932b8af514961a1d3a1a25fdf3f4f7732e9d6" + - "24c6c61548ab5fb8cd41020100"), - der: false, isValid: false, }} for _, test := range tests { - var err error - if test.der { - _, err = ParseDERSignature(test.sig) - } else { - _, err = ParseSignature(test.sig) - } + _, err := ParseDERSignature(test.sig) if err != nil { if test.isValid { t.Errorf("%s signature failed when shouldn't %v", test.name, diff --git a/txscript/sigcache_test.go b/txscript/sigcache_test.go index 13a45d6a..491a39d4 100644 --- a/txscript/sigcache_test.go +++ b/txscript/sigcache_test.go @@ -47,7 +47,7 @@ func TestSigCacheAddExists(t *testing.T) { sigCache.Add(*msg1, sig1, key1) // The previously added triplet should now be found within the sigcache. - sig1Copy, _ := secp256k1.ParseSignature(sig1.Serialize()) + sig1Copy, _ := secp256k1.ParseDERSignature(sig1.Serialize()) key1Copy, _ := secp256k1.ParsePubKey(key1.SerializeCompressed()) if !sigCache.Exists(*msg1, sig1Copy, key1Copy) { t.Errorf("previously added item not found in signature cache") @@ -70,7 +70,7 @@ func TestSigCacheAddEvictEntry(t *testing.T) { } sigCache.Add(*msg, sig, key) - sigCopy, _ := secp256k1.ParseSignature(sig.Serialize()) + sigCopy, _ := secp256k1.ParseDERSignature(sig.Serialize()) keyCopy, _ := secp256k1.ParsePubKey(key.SerializeCompressed()) if !sigCache.Exists(*msg, sigCopy, keyCopy) { t.Errorf("previously added item not found in signature " + @@ -99,7 +99,7 @@ func TestSigCacheAddEvictEntry(t *testing.T) { } // The entry added above should be found within the sigcache. - sigNewCopy, _ := secp256k1.ParseSignature(sigNew.Serialize()) + sigNewCopy, _ := secp256k1.ParseDERSignature(sigNew.Serialize()) keyNewCopy, _ := secp256k1.ParsePubKey(keyNew.SerializeCompressed()) if !sigCache.Exists(*msgNew, sigNewCopy, keyNewCopy) { t.Fatalf("previously added item not found in signature cache") @@ -122,7 +122,7 @@ func TestSigCacheAddMaxEntriesZeroOrNegative(t *testing.T) { sigCache.Add(*msg1, sig1, key1) // The generated triplet should not be found. - sig1Copy, _ := secp256k1.ParseSignature(sig1.Serialize()) + sig1Copy, _ := secp256k1.ParseDERSignature(sig1.Serialize()) key1Copy, _ := secp256k1.ParsePubKey(key1.SerializeCompressed()) if sigCache.Exists(*msg1, sig1Copy, key1Copy) { t.Errorf("previously added signature found in sigcache, but " +