From 98ac46b37d42410563f08de589cfe3c8e9b7a0b1 Mon Sep 17 00:00:00 2001 From: Josh Rickmar Date: Thu, 24 Oct 2013 16:13:27 -0400 Subject: [PATCH] Add ParseDERSignature. This change adds an additional signature parsing function which performs additional checks to verify the signature is serialized in a valid DER (and thus, unique) format, instead of allowing the less strict BER signatures that ParseSignature will happily accept. Added additional tests and updated test coverage to reflect changes. --- signature.go | 61 ++++++++++++++++++++++++++++++++++++++---- signature_test.go | 67 ++++++++++++++++++++++++++++++++++++++++++----- test_coverage.txt | 13 +++++---- 3 files changed, 125 insertions(+), 16 deletions(-) diff --git a/signature.go b/signature.go index 594bb06f..5340fb0b 100644 --- a/signature.go +++ b/signature.go @@ -11,15 +11,19 @@ import ( "math/big" ) +// Errors returned by canonicalPadding. +var ( + errNegativeValue = errors.New("value may be interpreted as negative") + errExcessivelyPaddedValue = errors.New("value is excessively padded") +) + // Signature is a type representing an ecdsa signature. type Signature struct { R *big.Int S *big.Int } -// ParseSignature parses a signature in DER format for the curve type `curve' -// into a Signature type, perfoming some basic sanity checks. -func ParseSignature(sigStr []byte, curve elliptic.Curve) (*Signature, error) { +func parseSig(sigStr []byte, curve elliptic.Curve, der bool) (*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 @@ -69,7 +73,16 @@ func ParseSignature(sigStr []byte, curve elliptic.Curve) (*Signature, error) { } // Then R itself. - signature.R = new(big.Int).SetBytes(sigStr[index : index+rLen]) + 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") + } + } + signature.R = new(big.Int).SetBytes(rBytes) index += rLen // 0x02. length already checked in previous if. if sigStr[index] != 0x02 { @@ -86,7 +99,16 @@ func ParseSignature(sigStr []byte, curve elliptic.Curve) (*Signature, error) { } // Then S itself. - signature.S = new(big.Int).SetBytes(sigStr[index : index+sLen]) + 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") + } + } + signature.S = new(big.Int).SetBytes(sBytes) index += sLen // sanity check length parsing @@ -114,3 +136,32 @@ func ParseSignature(sigStr []byte, curve elliptic.Curve) (*Signature, error) { return signature, nil } + +// ParseSignature parses a signature in BER format for the curve type `curve' +// into a Signature type, perfoming some basic sanity checks. If parsing +// according to the more strict DER format is needed, use ParseDERSignature. +func ParseSignature(sigStr []byte, curve elliptic.Curve) (*Signature, error) { + return parseSig(sigStr, curve, false) +} + +// ParseDERSignature parses a signature in DER format for the curve type +// `curve` into a Signature type. If parsing according to the less strict +// BER format is needed, use ParseSignature. +func ParseDERSignature(sigStr []byte, curve elliptic.Curve) (*Signature, error) { + return parseSig(sigStr, curve, true) +} + +// canonicalPadding checks whether a big-endian encoded integer could +// possibly be misinterpreted as a negative number (even though OpenSSL +// treats all numbers as unsigned), or if there is any unnecessary +// leading zero padding. +func canonicalPadding(b []byte) error { + switch { + case b[0]&0x80 == 0x80: + return errNegativeValue + case len(b) > 1 && b[0] == 0x00 && b[1]&0x80 != 0x80: + return errExcessivelyPaddedValue + default: + return nil + } +} diff --git a/signature_test.go b/signature_test.go index a64e3cfd..130e49ed 100644 --- a/signature_test.go +++ b/signature_test.go @@ -12,6 +12,7 @@ import ( type signatureTest struct { name string sig []byte + der bool isValid bool } @@ -29,6 +30,7 @@ var signatureTests = []signatureTest{ 0x9d, 0x83, 0x1c, 0xc5, 0x6c, 0xbb, 0xac, 0x46, 0x22, 0x08, 0x22, 0x21, 0xa8, 0x76, 0x8d, 0x1d, 0x09, }, + der: true, isValid: true, }, signatureTest{ @@ -47,6 +49,7 @@ var signatureTests = []signatureTest{ 0x9d, 0x83, 0x1c, 0xc5, 0x6c, 0xbb, 0xac, 0x46, 0x22, 0x08, 0x22, 0x21, 0xa8, 0x76, 0x8d, 0x1d, 0x09, }, + der: true, isValid: false, }, signatureTest{ @@ -60,6 +63,7 @@ var signatureTests = []signatureTest{ 0x9d, 0x83, 0x1c, 0xc5, 0x6c, 0xbb, 0xac, 0x46, 0x22, 0x08, 0x22, 0x21, 0xa8, 0x76, 0x8d, 0x1d, 0x09, }, + der: true, isValid: false, }, signatureTest{ @@ -73,6 +77,7 @@ var signatureTests = []signatureTest{ 0x9d, 0x83, 0x1c, 0xc5, 0x6c, 0xbb, 0xac, 0x46, 0x22, 0x08, 0x22, 0x21, 0xa8, 0x76, 0x8d, 0x1d, 0x09, }, + der: true, isValid: false, }, signatureTest{ @@ -86,6 +91,7 @@ var signatureTests = []signatureTest{ 0x9d, 0x83, 0x1c, 0xc5, 0x6c, 0xbb, 0xac, 0x46, 0x22, 0x08, 0x22, 0x21, 0xa8, 0x76, 0x8d, 0x1d, 0x09, }, + der: true, isValid: false, }, signatureTest{ @@ -99,6 +105,7 @@ var signatureTests = []signatureTest{ 0x9d, 0x83, 0x1c, 0xc5, 0x6c, 0xbb, 0xac, 0x46, 0x22, 0x08, 0x22, 0x21, 0xa8, 0x76, 0x8d, 0x1d, 0x09, }, + der: true, isValid: false, }, signatureTest{ @@ -112,6 +119,7 @@ var signatureTests = []signatureTest{ 0x9d, 0x83, 0x1c, 0xc5, 0x6c, 0xbb, 0xac, 0x46, 0x22, 0x08, 0x22, 0x21, 0xa8, 0x76, 0x8d, 0x1d, 0x09, }, + der: true, isValid: false, }, signatureTest{ @@ -125,6 +133,7 @@ var signatureTests = []signatureTest{ 0x9d, 0x83, 0x1c, 0xc5, 0x6c, 0xbb, 0xac, 0x46, 0x22, 0x08, 0x22, 0x21, 0xa8, 0x76, 0x8d, 0x1d, 0x09, }, + der: true, isValid: false, }, signatureTest{ @@ -138,6 +147,7 @@ var signatureTests = []signatureTest{ 0x9d, 0x83, 0x1c, 0xc5, 0x6c, 0xbb, 0xac, 0x46, 0x22, 0x08, 0x22, 0x21, 0xa8, 0x76, 0x8d, 0x1d, 0x09, }, + der: true, isValid: false, }, signatureTest{ @@ -151,6 +161,7 @@ var signatureTests = []signatureTest{ 0x9d, 0x83, 0x1c, 0xc5, 0x6c, 0xbb, 0xac, 0x46, 0x22, 0x08, 0x22, 0x21, 0xa8, 0x76, 0x8d, 0x1d, 0x09, 0x01, }, + der: true, // This test is now passing (used to be failing) because there // are signatures in the blockchain that have trailing zero @@ -170,6 +181,7 @@ var signatureTests = []signatureTest{ 0x9d, 0x83, 0x1c, 0xc5, 0x6c, 0xbb, 0xac, 0x46, 0x22, 0x08, 0x22, 0x21, 0xa8, 0x76, 0x8d, 0x1d, 0x09, }, + der: true, isValid: false, }, signatureTest{ @@ -183,6 +195,7 @@ var signatureTests = []signatureTest{ 0x9d, 0x83, 0x1c, 0xc5, 0x6c, 0xbb, 0xac, 0x46, 0x22, 0x08, 0x22, 0x21, 0xa8, 0x76, 0x8d, 0x1d, 0x09, }, + der: false, isValid: false, }, signatureTest{ @@ -196,6 +209,7 @@ var signatureTests = []signatureTest{ 0xFE, 0xBA, 0xAE, 0xDC, 0xE6, 0xAF, 0x48, 0xA0, 0x3B, 0xBF, 0xD2, 0x5E, 0x8C, 0xD0, 0x36, 0x41, 0x41, }, + der: true, isValid: false, }, signatureTest{ @@ -209,6 +223,7 @@ var signatureTests = []signatureTest{ 0xFE, 0xBA, 0xAE, 0xDC, 0xE6, 0xAF, 0x48, 0xA0, 0x3B, 0xBF, 0xD2, 0x5E, 0x8C, 0xD0, 0x36, 0x41, 0x42, }, + der: false, isValid: false, }, signatureTest{ @@ -219,6 +234,7 @@ var signatureTests = []signatureTest{ 0xbb, 0xac, 0x46, 0x22, 0x08, 0x22, 0x21, 0xa8, 0x76, 0x8d, 0x1d, 0x09, }, + der: true, isValid: false, }, signatureTest{ @@ -229,13 +245,45 @@ var signatureTests = []signatureTest{ 0x24, 0xc6, 0xc6, 0x15, 0x48, 0xab, 0x5f, 0xb8, 0xcd, 0x41, 0x02, 0x00, }, + der: true, isValid: false, }, - // We 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. - // therefore we only check for the coordinates being zero. + signatureTest{ + name: "extra R padding.", + sig: []byte{0x30, 0x45, 0x02, 0x21, 0x00, 0x4e, 0x45, 0xe1, 0x69, + 0x32, 0xb8, 0xaf, 0x51, 0x49, 0x61, 0xa1, 0xd3, 0xa1, + 0xa2, 0x5f, 0xdf, 0x3f, 0x4f, 0x77, 0x32, 0xe9, 0xd6, + 0x24, 0xc6, 0xc6, 0x15, 0x48, 0xab, 0x5f, 0xb8, 0xcd, + 0x41, 0x02, 0x20, 0x18, 0x15, 0x22, 0xec, 0x8e, 0xca, + 0x07, 0xde, 0x48, 0x60, 0xa4, 0xac, 0xdd, 0x12, 0x90, + 0x9d, 0x83, 0x1c, 0xc5, 0x6c, 0xbb, 0xac, 0x46, 0x22, + 0x08, 0x22, 0x21, 0xa8, 0x76, 0x8d, 0x1d, 0x09, + }, + der: true, + isValid: false, + }, + signatureTest{ + name: "extra S padding.", + sig: []byte{0x30, 0x45, 0x02, 0x20, 0x4e, 0x45, 0xe1, 0x69, + 0x32, 0xb8, 0xaf, 0x51, 0x49, 0x61, 0xa1, 0xd3, 0xa1, + 0xa2, 0x5f, 0xdf, 0x3f, 0x4f, 0x77, 0x32, 0xe9, 0xd6, + 0x24, 0xc6, 0xc6, 0x15, 0x48, 0xab, 0x5f, 0xb8, 0xcd, + 0x41, 0x02, 0x21, 0x00, 0x18, 0x15, 0x22, 0xec, 0x8e, 0xca, + 0x07, 0xde, 0x48, 0x60, 0xa4, 0xac, 0xdd, 0x12, 0x90, + 0x9d, 0x83, 0x1c, 0xc5, 0x6c, 0xbb, 0xac, 0x46, 0x22, + 0x08, 0x22, 0x21, 0xa8, 0x76, 0x8d, 0x1d, 0x09, + }, + 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. signatureTest{ name: "X == 0", sig: []byte{0x30, 0x25, 0x02, 0x01, 0x00, 0x02, 0x20, 0x18, @@ -244,6 +292,7 @@ var signatureTests = []signatureTest{ 0x6c, 0xbb, 0xac, 0x46, 0x22, 0x08, 0x22, 0x21, 0xa8, 0x76, 0x8d, 0x1d, 0x09, }, + der: false, isValid: false, }, signatureTest{ @@ -254,13 +303,19 @@ var signatureTests = []signatureTest{ 0x24, 0xc6, 0xc6, 0x15, 0x48, 0xab, 0x5f, 0xb8, 0xcd, 0x41, 0x02, 0x01, 0x00, }, + der: false, isValid: false, }, } func TestSignatures(t *testing.T) { for _, test := range signatureTests { - _, err := btcec.ParseSignature(test.sig, btcec.S256()) + var err error + if test.der { + _, err = btcec.ParseDERSignature(test.sig, btcec.S256()) + } else { + _, err = btcec.ParseSignature(test.sig, btcec.S256()) + } if err != nil { if test.isValid { t.Errorf("%s signature failed when shouldn't %v", diff --git a/test_coverage.txt b/test_coverage.txt index 4e0a3273..316be9bd 100644 --- a/test_coverage.txt +++ b/test_coverage.txt @@ -1,23 +1,26 @@ -github.com/conformal/btcec/signature.go ParseSignature 100.00% (41/41) +github.com/conformal/btcec/signature.go parseSig 100.00% (51/51) github.com/conformal/btcec/btcec.go KoblitzCurve.doubleJacobian 100.00% (21/21) github.com/conformal/btcec/btcec.go KoblitzCurve.ScalarMult 100.00% (9/9) github.com/conformal/btcec/pubkey.go PublicKey.SerializeHybrid 100.00% (8/8) -github.com/conformal/btcec/btcec.go KoblitzCurve.IsOnCurve 100.00% (7/7) -github.com/conformal/btcec/pubkey.go PublicKey.SerializeCompressed 100.00% (7/7) github.com/conformal/btcec/btcec.go initS256 100.00% (7/7) +github.com/conformal/btcec/pubkey.go PublicKey.SerializeCompressed 100.00% (7/7) +github.com/conformal/btcec/btcec.go KoblitzCurve.IsOnCurve 100.00% (7/7) github.com/conformal/btcec/pubkey.go PublicKey.SerializeUncompressed 100.00% (5/5) github.com/conformal/btcec/btcec.go zForAffine 100.00% (4/4) +github.com/conformal/btcec/signature.go canonicalPadding 100.00% (4/4) github.com/conformal/btcec/btcec.go KoblitzCurve.QPlus1Div4 100.00% (3/3) github.com/conformal/btcec/btcec.go KoblitzCurve.Add 100.00% (3/3) github.com/conformal/btcec/btcec.go S256 100.00% (2/2) +github.com/conformal/btcec/btcec.go initAll 100.00% (1/1) github.com/conformal/btcec/btcec.go KoblitzCurve.ScalarBaseMult 100.00% (1/1) github.com/conformal/btcec/pubkey.go isOdd 100.00% (1/1) -github.com/conformal/btcec/btcec.go initAll 100.00% (1/1) +github.com/conformal/btcec/signature.go ParseSignature 100.00% (1/1) +github.com/conformal/btcec/signature.go ParseDERSignature 100.00% (1/1) github.com/conformal/btcec/btcec.go KoblitzCurve.Params 100.00% (1/1) github.com/conformal/btcec/pubkey.go ParsePubKey 96.88% (31/32) github.com/conformal/btcec/btcec.go KoblitzCurve.addJacobian 91.67% (55/60) github.com/conformal/btcec/btcec.go KoblitzCurve.affineFromJacobian 90.00% (9/10) github.com/conformal/btcec/btcec.go KoblitzCurve.Double 0.00% (0/2) -github.com/conformal/btcec ------------------------------- 96.00% (216/225) +github.com/conformal/btcec ------------------------------- 96.27% (232/241)