seedhammer/dcrd
seedhammer/dcrd
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

secp256k1: Remove BER signature parsing.

Browse Source 
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.
This commit is contained in:
Dave Collins 2020-02-20 02:04:30 -06:00
parent 6335a6bb78
commit ce39748e76
No known key found for this signature in database
GPG Key ID: B8904D9D9C93D1F2
3 changed files with 23 additions and 93 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

47
dcrec/secp256k1/signature.go
View File

@ -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

61
dcrec/secp256k1/signature_test.go
View File

@ -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,

8
txscript/sigcache_test.go
View File

@ -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 " +