wire: Reject non-canonically encoded varints.

The Bitcoin wire protocol includes several fields with their lengths
encoded according to a variable length integer encoding scheme that does
not enforce a unique encoding for all numbers.

This can lead to a situation where deserializing and re-serializing the
same data can result in different bytes.  There are no currently known
issues due to this, but it is safer to reject such subtle differences as
they could potentially lead to exploits.

Consequently, this commit modifies the varint decoding function to error
when the value is not canonically encoded which effectively means that
all messages with varints that are not canonically encoded will now be
rejected.  This will not cause issues with old client versions in
regards to blocks and transactions since the data is deserialized into
memory and then reserialized before being relayed thereby effectively
erasing any non-canonical encodings.

Also, new tests have been added to ensure non-canonical encodings are
properly rejected and exercise the new code, and the default user agent
version for wire has been bumped to version 0.2.1 to differentiate the
new behavior.

The equivalent logic was implemented in Bitcoin Core by PR 2884.

wire/common.go

@@ -17,6 +17,11 @@ import (
 // Maximum payload size for a variable length integer.
 const MaxVarIntPayload = 9
 
+// errNonCanonicalVarInt is the common format string used for non-canonically
+// encoded variable length integer errors.
+var errNonCanonicalVarInt = "non-canonical varint %x - discriminant %x must " +
+	"encode a value greater than %x"
+
 // readElement reads the next sequence of bytes from r using little endian
 // depending on the concrete type of element pointed to.
 func readElement(r io.Reader, element interface{}) error {
@@ -336,6 +341,14 @@ func readVarInt(r io.Reader, pver uint32) (uint64, error) {
 		}
 		rv = binary.LittleEndian.Uint64(b[:])
 
+		// The encoding is not canonical if the value could have been
+		// encoded using fewer bytes.
+		min := uint64(0x100000000)
+		if rv < min {
+			return 0, messageError("readVarInt", fmt.Sprintf(
+				errNonCanonicalVarInt, rv, discriminant, min))
+		}
+
 	case 0xfe:
 		_, err := io.ReadFull(r, b[0:4])
 		if err != nil {
@@ -343,6 +356,14 @@ func readVarInt(r io.Reader, pver uint32) (uint64, error) {
 		}
 		rv = uint64(binary.LittleEndian.Uint32(b[:]))
 
+		// The encoding is not canonical if the value could have been
+		// encoded using fewer bytes.
+		min := uint64(0x10000)
+		if rv < min {
+			return 0, messageError("readVarInt", fmt.Sprintf(
+				errNonCanonicalVarInt, rv, discriminant, min))
+		}
+
 	case 0xfd:
 		_, err := io.ReadFull(r, b[0:2])
 		if err != nil {
@@ -350,6 +371,14 @@ func readVarInt(r io.Reader, pver uint32) (uint64, error) {
 		}
 		rv = uint64(binary.LittleEndian.Uint16(b[:]))
 
+		// The encoding is not canonical if the value could have been
+		// encoded using fewer bytes.
+		min := uint64(0xfd)
+		if rv < min {
+			return 0, messageError("readVarInt", fmt.Sprintf(
+				errNonCanonicalVarInt, rv, discriminant, min))
+		}
+
 	default:
 		rv = uint64(discriminant)
 	}

wire/common_test.go

@@ -354,6 +354,62 @@ func TestVarIntWireErrors(t *testing.T) {
 	}
 }
 
+// TestVarIntNonCanonical ensures variable length integers that are not encoded
+// canonically return the expected error.
+func TestVarIntNonCanonical(t *testing.T) {
+	pver := wire.ProtocolVersion
+
+	tests := []struct {
+		name string // Test name for easier identification
+		in   []byte // Value to decode
+		pver uint32 // Protocol version for wire encoding
+	}{
+		{
+			"0 encoded with 3 bytes", []byte{0xfd, 0x00, 0x00},
+			pver,
+		},
+		{
+			"max single-byte value encoded with 3 bytes",
+			[]byte{0xfd, 0xfc, 0x00}, pver,
+		},
+		{
+			"0 encoded with 5 bytes",
+			[]byte{0xfe, 0x00, 0x00, 0x00, 0x00}, pver,
+		},
+		{
+			"max three-byte value encoded with 5 bytes",
+			[]byte{0xfe, 0xff, 0xff, 0x00, 0x00}, pver,
+		},
+		{
+			"0 encoded with 9 bytes",
+			[]byte{0xff, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
+			pver,
+		},
+		{
+			"max five-byte value encoded with 9 bytes",
+			[]byte{0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x00},
+			pver,
+		},
+	}
+
+	t.Logf("Running %d tests", len(tests))
+	for i, test := range tests {
+		// Decode from wire format.
+		rbuf := bytes.NewReader(test.in)
+		val, err := wire.TstReadVarInt(rbuf, test.pver)
+		if _, ok := err.(*wire.MessageError); !ok {
+			t.Errorf("readVarInt #%d (%s) unexpected error %v", i,
+				test.name, err)
+			continue
+		}
+		if val != 0 {
+			t.Errorf("readVarInt #%d (%s)\n got: %d want: 0", i,
+				test.name, val)
+			continue
+		}
+	}
+}
+
 // TestVarIntWire tests the serialize size for variable length integers.
 func TestVarIntSerializeSize(t *testing.T) {
 	tests := []struct {

wire/msgversion.go

@@ -18,7 +18,7 @@ import (
 const MaxUserAgentLen = 2000
 
 // DefaultUserAgent for wire in the stack
-const DefaultUserAgent = "/btcwire:0.2.0/"
+const DefaultUserAgent = "/btcwire:0.2.1/"
 
 // MsgVersion implements the Message interface and represents a bitcoin version
 // message.  It is used for a peer to advertise itself as soon as an outbound