txscript: Cleanup strict signature enforcement.

This cleans up the code for handling the checksig and checkmultisig
opcode strict signatures to explicitly call out any semantics that are
likely not obvious and improve readability.

It also introduce new distinct errors for each condition which can
result in a signature being rejected due to not following the strict
encoding requirements and updates reference test adaptor accordingly.

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
-	// 0x30 + <1-byte> + 0x02 + 0x01 + <byte> + 0x2 + 0x01 + <byte>
+		// when both R and S are 1 byte each.
-	if len(sig) < 8 {
+		//
-		// Too short
+		// 0x30 + <1-byte> + 0x02 + 0x01 + <byte> + 0x2 + 0x01 + <byte>
-		str := fmt.Sprintf("malformed signature: too short: %d < 8",
+		minSigLen = 8
-			len(sig))
+
-		return scriptError(ErrSigDER, str)
+		// maxSigLen is the maximum length of a DER encoded signature and is
+		// when both R and S are 33 bytes each.  It is 33 bytes because a
+		// 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>
+		maxSigLen = 72
+
+		// sequenceOffset is the byte offset within the signature of the
+		// expected ASN.1 sequence identifier.
+		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 sigLen > maxSigLen {
+		str := fmt.Sprintf("malformed signature: too long: %d > %d", sigLen,
+			maxSigLen)
+		return scriptError(ErrSigTooLong, str)
 	}
 
-	// Maximum length is when both numbers are 33 bytes each.  It is 33
+	// The signature must start with the ASN.1 sequence identifier.
-	// bytes because a 256-bit integer requires 32 bytes and an additional
+	if sig[sequenceOffset] != asn1SequenceID {
-	// leading null byte might required if the high bit is set in the value.
+		str := fmt.Sprintf("malformed signature: format has wrong type: %#x",
-	// 0x30 + <1-byte> + 0x02 + 0x21 + <33 bytes> + 0x2 + 0x21 + <33 bytes>
+			sig[sequenceOffset])
-	if len(sig) > 72 {
+		return scriptError(ErrSigInvalidSeqID, str)
-		// Too long
-		str := fmt.Sprintf("malformed signature: too long: %d > 72",
-			len(sig))
-		return scriptError(ErrSigDER, str)
 	}
-	if sig[0] != 0x30 {
+
-		// Wrong type
+	// The signature must indicate the correct amount of data for all elements
-		str := fmt.Sprintf("malformed signature: format has wrong "+
+	// related to R and S.
-			"type: 0x%x", sig[0])
+	if int(sig[dataLenOffset]) != sigLen-2 {
-		return scriptError(ErrSigDER, str)
-	}
-	if int(sig[1]) != len(sig)-2 {
-		// Invalid length
 		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":