fix overflow-related out-of-bounds panic in decodeBytes() (#347)

CHANGELOG.md

@@ -1,10 +1,16 @@
 # Changelog
 
+## 0.15.2 (December 14, 2020)
+
+### Bug Fixes
+
+- [\#347](https://github.com/cosmos/iavl/pull/347) Fix another integer overflow in `decodeBytes()` that can cause panics for certain inputs. The `ValueOp` and `AbsenceOp` proof decoders are vulnerable to this via malicious inputs since 0.15.0.
+
 ## 0.15.1 (December 13, 2020)
 
 ### Bug Fixes
 
-- [\#340](https://github.com/cosmos/iavl/pull/340) Fix integer overflow in `decodeBytes()` that can cause out-of-memory errors on 32-bit machines for certain inputs.
+[\#340](https://github.com/cosmos/iavl/pull/340) Fix integer overflow in `decodeBytes()` that can cause panics on 64-bit systems and out-of-memory issues on 32-bit systems. The `ValueOp` and `AbsenceOp` proof decoders are vulnerable to this via malicious inputs. The bug was introduced in 0.15.0.
 
 ## 0.15.0 (November 23, 2020)
 

encoding.go

@@ -16,18 +16,25 @@ func decodeBytes(bz []byte) ([]byte, int, error) {
 	if err != nil {
 		return nil, n, err
 	}
-	// ^uint(0) >> 1 will help determine the max int value variably on 32-bit and 64-bit machines.
-	if uint64(n)+s >= uint64(^uint(0)>>1) {
-		return nil, n, fmt.Errorf("invalid out of range length %v decoding []byte", uint64(n)+s)
-	}
+	// Make sure size doesn't overflow. ^uint(0) >> 1 will help determine the
+	// max int value variably on 32-bit and 64-bit machines. We also doublecheck
+	// that size is positive.
 	size := int(s)
-	if len(bz) < n+size {
+	if s >= uint64(^uint(0)>>1) || size < 0 {
+		return nil, n, fmt.Errorf("invalid out of range length %v decoding []byte", s)
+	}
+	// Make sure end index doesn't overflow. We know n>0 from decodeUvarint().
+	end := n + size
+	if end < n {
+		return nil, n, fmt.Errorf("invalid out of range length %v decoding []byte", size)
+	}
+	// Make sure the end index is within bounds.
+	if len(bz) < end {
 		return nil, n, fmt.Errorf("insufficient bytes decoding []byte of length %v", size)
 	}
 	bz2 := make([]byte, size)
-	copy(bz2, bz[n:n+size])
-	n += size
-	return bz2, n, nil
+	copy(bz2, bz[n:end])
+	return bz2, end, nil
 }
 
 // decodeUvarint decodes a varint-encoded unsigned integer from a byte slice, returning it and the

encoding_test.go

@@ -0,0 +1,88 @@
+package iavl
+
+import (
+	"encoding/binary"
+	"math"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestDecodeBytes(t *testing.T) {
+	bz := []byte{0, 1, 2, 3, 4, 5, 6, 7}
+	testcases := map[string]struct {
+		bz           []byte
+		lengthPrefix uint64
+		expect       []byte
+		expectErr    bool
+	}{
+		"full":                      {bz, 8, bz, false},
+		"empty":                     {bz, 0, []byte{}, false},
+		"partial":                   {bz, 3, []byte{0, 1, 2}, false},
+		"out of bounds":             {bz, 9, nil, true},
+		"empty input":               {[]byte{}, 0, []byte{}, false},
+		"empty input out of bounds": {[]byte{}, 1, nil, true},
+
+		// The following will always fail, since the byte slice is only 8 bytes,
+		// but we're making sure they don't panic due to overflow issues. See:
+		// https://github.com/cosmos/iavl/issues/339
+		"max int32":     {bz, uint64(math.MaxInt32), nil, true},
+		"max int32 -1":  {bz, uint64(math.MaxInt32) - 1, nil, true},
+		"max int32 -10": {bz, uint64(math.MaxInt32) - 10, nil, true},
+		"max int32 +1":  {bz, uint64(math.MaxInt32) + 1, nil, true},
+		"max int32 +10": {bz, uint64(math.MaxInt32) + 10, nil, true},
+
+		"max int32*2":     {bz, uint64(math.MaxInt32) * 2, nil, true},
+		"max int32*2 -1":  {bz, uint64(math.MaxInt32)*2 - 1, nil, true},
+		"max int32*2 -10": {bz, uint64(math.MaxInt32)*2 - 10, nil, true},
+		"max int32*2 +1":  {bz, uint64(math.MaxInt32)*2 + 1, nil, true},
+		"max int32*2 +10": {bz, uint64(math.MaxInt32)*2 + 10, nil, true},
+
+		"max uint32":     {bz, uint64(math.MaxUint32), nil, true},
+		"max uint32 -1":  {bz, uint64(math.MaxUint32) - 1, nil, true},
+		"max uint32 -10": {bz, uint64(math.MaxUint32) - 10, nil, true},
+		"max uint32 +1":  {bz, uint64(math.MaxUint32) + 1, nil, true},
+		"max uint32 +10": {bz, uint64(math.MaxUint32) + 10, nil, true},
+
+		"max uint32*2":     {bz, uint64(math.MaxUint32) * 2, nil, true},
+		"max uint32*2 -1":  {bz, uint64(math.MaxUint32)*2 - 1, nil, true},
+		"max uint32*2 -10": {bz, uint64(math.MaxUint32)*2 - 10, nil, true},
+		"max uint32*2 +1":  {bz, uint64(math.MaxUint32)*2 + 1, nil, true},
+		"max uint32*2 +10": {bz, uint64(math.MaxUint32)*2 + 10, nil, true},
+
+		"max int64":     {bz, uint64(math.MaxInt64), nil, true},
+		"max int64 -1":  {bz, uint64(math.MaxInt64) - 1, nil, true},
+		"max int64 -10": {bz, uint64(math.MaxInt64) - 10, nil, true},
+		"max int64 +1":  {bz, uint64(math.MaxInt64) + 1, nil, true},
+		"max int64 +10": {bz, uint64(math.MaxInt64) + 10, nil, true},
+
+		"max uint64":     {bz, uint64(math.MaxUint64), nil, true},
+		"max uint64 -1":  {bz, uint64(math.MaxUint64) - 1, nil, true},
+		"max uint64 -10": {bz, uint64(math.MaxUint64) - 10, nil, true},
+	}
+	for name, tc := range testcases {
+		tc := tc
+		t.Run(name, func(t *testing.T) {
+			// Generate an input slice.
+			buf := make([]byte, binary.MaxVarintLen64)
+			varintBytes := binary.PutUvarint(buf, tc.lengthPrefix)
+			buf = append(buf[:varintBytes], tc.bz...)
+
+			// Attempt to decode it.
+			b, n, err := decodeBytes(buf)
+			if tc.expectErr {
+				require.Error(t, err)
+				require.Equal(t, varintBytes, n)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, uint64(n), uint64(varintBytes)+tc.lengthPrefix)
+				require.Equal(t, tc.bz[:tc.lengthPrefix], b)
+			}
+		})
+	}
+}
+
+func TestDecodeBytes_invalidVarint(t *testing.T) {
+	_, _, err := decodeBytes([]byte{0xff})
+	require.Error(t, err)
+}