From ce39748e760aecb60dc8417fe1ad6799bccd4f10 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Thu, 20 Feb 2020 02:04:30 -0600 Subject: [PATCH] secp256k1: Remove BER signature parsing. This removes the ability to parse signatures encoded with the more lax Basic Encoding Rules (BER) since they have never been valid on the Decred blockchain and the existing parsing code for them really didn't follow the spec anyway given it enforced additional length restrictions that the BER spec technically allows. --- dcrec/secp256k1/signature.go | 47 ++++++++---------------- dcrec/secp256k1/signature_test.go | 61 ++----------------------------- txscript/sigcache_test.go | 8 ++-- 3 files changed, 23 insertions(+), 93 deletions(-) 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 " +