Merge pull request #1267 from davecgh/txscript_backport_sigencoding_updates

txscript: Cleanup strict signature enforcement.

txscript/engine.go

@@ -1,4 +1,5 @@
-// Copyright (c) 2013-2017 The btcsuite developers
+// Copyright (c) 2013-2018 The btcsuite developers
+// Copyright (c) 2015-2018 The Decred developers
 // Use of this source code is governed by an ISC
 // license that can be found in the LICENSE file.
 
@@ -633,119 +634,170 @@ func (vm *Engine) checkSignatureEncoding(sig []byte) error {
 	//   - S is the arbitrary length big-endian encoded number which
 	//     represents the S value of the signature.  The encoding rules are
 	//     identical as those for R.
+	const (
+		asn1SequenceID = 0x30
+		asn1IntegerID  = 0x02
 
-	// Minimum length is when both numbers are 1 byte each.
+		// minSigLen is the minimum length of a DER encoded signature and is
+		// when both R and S are 1 byte each.
+		//
 		// 0x30 + <1-byte> + 0x02 + 0x01 + <byte> + 0x2 + 0x01 + <byte>
-	if len(sig) < 8 {
+		minSigLen = 8
-		// Too short
-		str := fmt.Sprintf("malformed signature: too short: %d < 8",
-			len(sig))
-		return scriptError(ErrSigDER, str)
-	}
 
-	// Maximum length is when both numbers are 33 bytes each.  It is 33
+		// maxSigLen is the maximum length of a DER encoded signature and is
-	// bytes because a 256-bit integer requires 32 bytes and an additional
+		// when both R and S are 33 bytes each.  It is 33 bytes because a
-	// leading null byte might required if the high bit is set in the value.
+		// 256-bit integer requires 32 bytes and an additional leading null byte
+		// might required if the high bit is set in the value.
+		//
 		// 0x30 + <1-byte> + 0x02 + 0x21 + <33 bytes> + 0x2 + 0x21 + <33 bytes>
-	if len(sig) > 72 {
+		maxSigLen = 72
-		// Too long
+
-		str := fmt.Sprintf("malformed signature: too long: %d > 72",
+		// sequenceOffset is the byte offset within the signature of the
-			len(sig))
+		// expected ASN.1 sequence identifier.
-		return scriptError(ErrSigDER, str)
+		sequenceOffset = 0
+
+		// dataLenOffset is the byte offset within the signature of the expected
+		// total length of all remaining data in the signature.
+		dataLenOffset = 1
+
+		// rTypeOffset is the byte offset within the signature of the ASN.1
+		// identifier for R and is expected to indicate an ASN.1 integer.
+		rTypeOffset = 2
+
+		// rLenOffset is the byte offset within the signature of the length of
+		// R.
+		rLenOffset = 3
+
+		// rOffset is the byte offset within the signature of R.
+		rOffset = 4
+	)
+
+	// The signature must adhere to the minimum and maximum allowed length.
+	sigLen := len(sig)
+	if sigLen < minSigLen {
+		str := fmt.Sprintf("malformed signature: too short: %d < %d", sigLen,
+			minSigLen)
+		return scriptError(ErrSigTooShort, str)
 	}
-	if sig[0] != 0x30 {
+	if sigLen > maxSigLen {
-		// Wrong type
+		str := fmt.Sprintf("malformed signature: too long: %d > %d", sigLen,
-		str := fmt.Sprintf("malformed signature: format has wrong "+
+			maxSigLen)
-			"type: 0x%x", sig[0])
+		return scriptError(ErrSigTooLong, str)
-		return scriptError(ErrSigDER, str)
 	}
-	if int(sig[1]) != len(sig)-2 {
+
-		// Invalid length
+	// The signature must start with the ASN.1 sequence identifier.
+	if sig[sequenceOffset] != asn1SequenceID {
+		str := fmt.Sprintf("malformed signature: format has wrong type: %#x",
+			sig[sequenceOffset])
+		return scriptError(ErrSigInvalidSeqID, str)
+	}
+
+	// The signature must indicate the correct amount of data for all elements
+	// related to R and S.
+	if int(sig[dataLenOffset]) != sigLen-2 {
 		str := fmt.Sprintf("malformed signature: bad length: %d != %d",
-			sig[1], len(sig)-2)
+			sig[dataLenOffset], sigLen-2)
-		return scriptError(ErrSigDER, str)
+		return scriptError(ErrSigInvalidDataLen, str)
 	}
 
-	rLen := int(sig[3])
+	// Calculate the offsets of the elements related to S and ensure S is inside
-
+	// the signature.
-	// Make sure S is inside the signature.
+	//
-	if rLen+5 > len(sig) {
+	// rLen specifies the length of the big-endian encoded number which
-		return scriptError(ErrSigDER,
+	// represents the R value of the signature.
-			"malformed signature: S out of bounds")
+	//
+	// sTypeOffset is the offset of the ASN.1 identifier for S and, like its R
+	// counterpart, is expected to indicate an ASN.1 integer.
+	//
+	// sLenOffset and sOffset are the byte offsets within the signature of the
+	// length of S and S itself, respectively.
+	rLen := int(sig[rLenOffset])
+	sTypeOffset := rOffset + rLen
+	sLenOffset := sTypeOffset + 1
+	if sTypeOffset >= sigLen {
+		str := "malformed signature: S type indicator missing"
+		return scriptError(ErrSigMissingSTypeID, str)
+	}
+	if sLenOffset >= sigLen {
+		str := "malformed signature: S length missing"
+		return scriptError(ErrSigMissingSLen, str)
 	}
 
-	sLen := int(sig[rLen+5])
+	// The lengths of R and S must match the overall length of the signature.
-
+	//
-	// The length of the elements does not match the length of the
+	// sLen specifies the length of the big-endian encoded number which
-	// signature.
+	// represents the S value of the signature.
-	if rLen+sLen+6 != len(sig) {
+	sOffset := sLenOffset + 1
-		return scriptError(ErrSigDER,
+	sLen := int(sig[sLenOffset])
-			"malformed signature: invalid R length")
+	if sOffset+sLen != sigLen {
+		str := "malformed signature: invalid S length"
+		return scriptError(ErrSigInvalidSLen, str)
 	}
 
-	// R elements must be integers.
+	// R elements must be ASN.1 integers.
-	if sig[2] != 0x02 {
+	if sig[rTypeOffset] != asn1IntegerID {
-		return scriptError(ErrSigDER,
+		str := fmt.Sprintf("malformed signature: R integer marker: %#x != %#x",
-			"malformed signature: missing first integer marker")
+			sig[rTypeOffset], asn1IntegerID)
+		return scriptError(ErrSigInvalidRIntID, str)
 	}
 
 	// Zero-length integers are not allowed for R.
 	if rLen == 0 {
-		return scriptError(ErrSigDER,
+		str := "malformed signature: R length is zero"
-			"malformed signature: R length is zero")
+		return scriptError(ErrSigZeroRLen, str)
 	}
 
 	// R must not be negative.
-	if sig[4]&0x80 != 0 {
+	if sig[rOffset]&0x80 != 0 {
-		return scriptError(ErrSigDER,
+		str := "malformed signature: R is negative"
-			"malformed signature: R value is negative")
+		return scriptError(ErrSigNegativeR, str)
 	}
 
-	// Null bytes at the start of R are not allowed, unless R would
+	// Null bytes at the start of R are not allowed, unless R would otherwise be
-	// otherwise be interpreted as a negative number.
+	// interpreted as a negative number.
-	if rLen > 1 && sig[4] == 0x00 && sig[5]&0x80 == 0 {
+	if rLen > 1 && sig[rOffset] == 0x00 && sig[rOffset+1]&0x80 == 0 {
-		return scriptError(ErrSigDER,
+		str := "malformed signature: R value has too much padding"
-			"malformed signature: invalid R value")
+		return scriptError(ErrSigTooMuchRPadding, str)
 	}
 
-	// S elements must be integers.
+	// S elements must be ASN.1 integers.
-	if sig[rLen+4] != 0x02 {
+	if sig[sTypeOffset] != asn1IntegerID {
-		return scriptError(ErrSigDER,
+		str := fmt.Sprintf("malformed signature: S integer marker: %#x != %#x",
-			"malformed signature: missing second integer marker")
+			sig[sTypeOffset], asn1IntegerID)
+		return scriptError(ErrSigInvalidSIntID, str)
 	}
 
 	// Zero-length integers are not allowed for S.
 	if sLen == 0 {
-		return scriptError(ErrSigDER,
+		str := "malformed signature: S length is zero"
-			"malformed signature: S length is zero")
+		return scriptError(ErrSigZeroSLen, str)
 	}
 
 	// S must not be negative.
-	if sig[rLen+6]&0x80 != 0 {
+	if sig[sOffset]&0x80 != 0 {
-		return scriptError(ErrSigDER,
+		str := "malformed signature: S is negative"
-			"malformed signature: S value is negative")
+		return scriptError(ErrSigNegativeS, str)
 	}
 
-	// Null bytes at the start of S are not allowed, unless S would
+	// Null bytes at the start of S are not allowed, unless S would otherwise be
-	// otherwise be interpreted as a negative number.
+	// interpreted as a negative number.
-	if sLen > 1 && sig[rLen+6] == 0x00 && sig[rLen+7]&0x80 == 0 {
+	if sLen > 1 && sig[sOffset] == 0x00 && sig[sOffset+1]&0x80 == 0 {
-		return scriptError(ErrSigDER,
+		str := "malformed signature: S value has too much padding"
-			"malformed signature: invalid S value")
+		return scriptError(ErrSigTooMuchSPadding, str)
 	}
 
-	// Verify the S value is <= half the order of the curve.  This check is
+	// Verify the S value is <= half the order of the curve.  This check is done
-	// done because when it is higher, the complement modulo the order can
+	// because when it is higher, the complement modulo the order can be used
-	// be used instead which is a shorter encoding by 1 byte.  Further,
+	// instead which is a shorter encoding by 1 byte.  Further, without
-	// without enforcing this, it is possible to replace a signature in a
+	// enforcing this, it is possible to replace a signature in a valid
-	// valid transaction with the complement while still being a valid
+	// transaction with the complement while still being a valid signature that
-	// signature that verifies.  This would result in changing the
+	// verifies.  This would result in changing the transaction hash and thus is
-	// transaction hash and thus is source of malleability.
+	// a source of malleability.
 	if vm.hasFlag(ScriptVerifyLowS) {
-		sValue := new(big.Int).SetBytes(sig[rLen+6 : rLen+6+sLen])
+		sValue := new(big.Int).SetBytes(sig[sOffset : sOffset+sLen])
 		if sValue.Cmp(halfOrder) > 0 {
-			return scriptError(ErrSigHighS,
+			return scriptError(ErrSigHighS, "signature is not canonical due "+
-				"signature is not canonical due to "+
+				"to unnecessarily high S value")
-					"unnecessarily high S value")
 		}
 	}
 

txscript/error.go

@@ -175,9 +175,71 @@ const (
 	// one of the supported types.
 	ErrInvalidSigHashType
 
-	// ErrSigDER is returned when a signature is not a canonically-encoded
+	// ErrSigTooShort is returned when a signature that should be a
-	// DER signature.
+	// canonically-encoded DER signature is too short.
-	ErrSigDER
+	ErrSigTooShort
+
+	// ErrSigTooLong is returned when a signature that should be a
+	// canonically-encoded DER signature is too long.
+	ErrSigTooLong
+
+	// ErrSigInvalidSeqID is returned when a signature that should be a
+	// canonically-encoded DER signature does not have the expected ASN.1
+	// sequence ID.
+	ErrSigInvalidSeqID
+
+	// ErrSigInvalidDataLen is returned a signature that should be a
+	// canonically-encoded DER signature does not specify the correct number
+	// of remaining bytes for the R and S portions.
+	ErrSigInvalidDataLen
+
+	// ErrSigMissingSTypeID is returned a signature that should be a
+	// canonically-encoded DER signature does not provide the ASN.1 type ID
+	// for S.
+	ErrSigMissingSTypeID
+
+	// ErrSigMissingSLen is returned when a signature that should be a
+	// canonically-encoded DER signature does not provide the length of S.
+	ErrSigMissingSLen
+
+	// ErrSigInvalidSLen is returned a signature that should be a
+	// canonically-encoded DER signature does not specify the correct number
+	// of bytes for the S portion.
+	ErrSigInvalidSLen
+
+	// ErrSigInvalidRIntID is returned when a signature that should be a
+	// canonically-encoded DER signature does not have the expected ASN.1
+	// integer ID for R.
+	ErrSigInvalidRIntID
+
+	// ErrSigZeroRLen is returned when a signature that should be a
+	// canonically-encoded DER signature has an R length of zero.
+	ErrSigZeroRLen
+
+	// ErrSigNegativeR is returned when a signature that should be a
+	// canonically-encoded DER signature has a negative value for R.
+	ErrSigNegativeR
+
+	// ErrSigTooMuchRPadding is returned when a signature that should be a
+	// canonically-encoded DER signature has too much padding for R.
+	ErrSigTooMuchRPadding
+
+	// ErrSigInvalidSIntID is returned when a signature that should be a
+	// canonically-encoded DER signature does not have the expected ASN.1
+	// integer ID for S.
+	ErrSigInvalidSIntID
+
+	// ErrSigZeroSLen is returned when a signature that should be a
+	// canonically-encoded DER signature has an S length of zero.
+	ErrSigZeroSLen
+
+	// ErrSigNegativeS is returned when a signature that should be a
+	// canonically-encoded DER signature has a negative value for S.
+	ErrSigNegativeS
+
+	// ErrSigTooMuchSPadding is returned when a signature that should be a
+	// canonically-encoded DER signature has too much padding for S.
+	ErrSigTooMuchSPadding
 
 	// ErrSigHighS is returned when the ScriptVerifyLowS flag is set and the
 	// script contains any signatures whose S values are higher than the
@@ -314,7 +376,21 @@ var errorCodeStrings = map[ErrorCode]string{
 	ErrUnbalancedConditional:              "ErrUnbalancedConditional",
 	ErrMinimalData:                        "ErrMinimalData",
 	ErrInvalidSigHashType:                 "ErrInvalidSigHashType",
-	ErrSigDER:                             "ErrSigDER",
+	ErrSigTooShort:                        "ErrSigTooShort",
+	ErrSigTooLong:                         "ErrSigTooLong",
+	ErrSigInvalidSeqID:                    "ErrSigInvalidSeqID",
+	ErrSigInvalidDataLen:                  "ErrSigInvalidDataLen",
+	ErrSigMissingSTypeID:                  "ErrSigMissingSTypeID",
+	ErrSigMissingSLen:                     "ErrSigMissingSLen",
+	ErrSigInvalidSLen:                     "ErrSigInvalidSLen",
+	ErrSigInvalidRIntID:                   "ErrSigInvalidRIntID",
+	ErrSigZeroRLen:                        "ErrSigZeroRLen",
+	ErrSigNegativeR:                       "ErrSigNegativeR",
+	ErrSigTooMuchRPadding:                 "ErrSigTooMuchRPadding",
+	ErrSigInvalidSIntID:                   "ErrSigInvalidSIntID",
+	ErrSigZeroSLen:                        "ErrSigZeroSLen",
+	ErrSigNegativeS:                       "ErrSigNegativeS",
+	ErrSigTooMuchSPadding:                 "ErrSigTooMuchSPadding",
 	ErrSigHighS:                           "ErrSigHighS",
 	ErrNotPushOnly:                        "ErrNotPushOnly",
 	ErrSigNullDummy:                       "ErrSigNullDummy",

txscript/error_test.go

@@ -47,7 +47,21 @@ func TestErrorCodeStringer(t *testing.T) {
 		{ErrUnbalancedConditional, "ErrUnbalancedConditional"},
 		{ErrMinimalData, "ErrMinimalData"},
 		{ErrInvalidSigHashType, "ErrInvalidSigHashType"},
-		{ErrSigDER, "ErrSigDER"},
+		{ErrSigTooShort, "ErrSigTooShort"},
+		{ErrSigTooLong, "ErrSigTooLong"},
+		{ErrSigInvalidSeqID, "ErrSigInvalidSeqID"},
+		{ErrSigInvalidDataLen, "ErrSigInvalidDataLen"},
+		{ErrSigMissingSTypeID, "ErrSigMissingSTypeID"},
+		{ErrSigMissingSLen, "ErrSigMissingSLen"},
+		{ErrSigInvalidSLen, "ErrSigInvalidSLen"},
+		{ErrSigInvalidRIntID, "ErrSigInvalidRIntID"},
+		{ErrSigZeroRLen, "ErrSigZeroRLen"},
+		{ErrSigNegativeR, "ErrSigNegativeR"},
+		{ErrSigTooMuchRPadding, "ErrSigTooMuchRPadding"},
+		{ErrSigInvalidSIntID, "ErrSigInvalidSIntID"},
+		{ErrSigZeroSLen, "ErrSigZeroSLen"},
+		{ErrSigNegativeS, "ErrSigNegativeS"},
+		{ErrSigTooMuchSPadding, "ErrSigTooMuchSPadding"},
 		{ErrSigHighS, "ErrSigHighS"},
 		{ErrNotPushOnly, "ErrNotPushOnly"},
 		{ErrSigNullDummy, "ErrSigNullDummy"},

txscript/reference_test.go

@@ -211,7 +211,13 @@ func parseExpectedResult(expected string) ([]ErrorCode, error) {
 	case "PUBKEYTYPE":
 		return []ErrorCode{ErrPubKeyType}, nil
 	case "SIG_DER":
-		return []ErrorCode{ErrSigDER, ErrInvalidSigHashType}, nil
+		return []ErrorCode{ErrSigTooShort, ErrSigTooLong,
+			ErrSigInvalidSeqID, ErrSigInvalidDataLen, ErrSigMissingSTypeID,
+			ErrSigMissingSLen, ErrSigInvalidSLen,
+			ErrSigInvalidRIntID, ErrSigZeroRLen, ErrSigNegativeR,
+			ErrSigTooMuchRPadding, ErrSigInvalidSIntID,
+			ErrSigZeroSLen, ErrSigNegativeS, ErrSigTooMuchSPadding,
+			ErrInvalidSigHashType}, nil
 	case "EVAL_FALSE":
 		return []ErrorCode{ErrEvalFalse, ErrEmptyStack}, nil
 	case "EQUALVERIFY":