From 49bf0774bc6097433a4bd02ec950cbdc49d0976c Mon Sep 17 00:00:00 2001
From: Emmanuel T Odeke <emmanuel@orijtech.com>
Date: Mon, 26 Apr 2021 09:40:27 -0700
Subject: [PATCH] crypto/types: fix negative index accesses in
 CompactUnmarshal,GetIndex,SetIndex (#9196)

Fixes unchecked negative index access that'd cause panics, in CompactBitArray's:
* CompactUnmarshal, which blindly used the result of binary.Uvarint
* GetIndex
* SetIndex

Fixes #9164
Fixes #9165
---
 crypto/types/compact_bit_array.go      |  7 +++++--
 crypto/types/compact_bit_array_test.go | 20 ++++++++++++++++++++
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/crypto/types/compact_bit_array.go b/crypto/types/compact_bit_array.go
index 9ac23212ae42..02d02e811bf9 100644
--- a/crypto/types/compact_bit_array.go
+++ b/crypto/types/compact_bit_array.go
@@ -54,7 +54,7 @@ func (bA *CompactBitArray) GetIndex(i int) bool {
 	if bA == nil {
 		return false
 	}
-	if i >= bA.Count() {
+	if i < 0 || i >= bA.Count() {
 		return false
 	}
 
@@ -68,7 +68,7 @@ func (bA *CompactBitArray) SetIndex(i int, v bool) bool {
 		return false
 	}
 
-	if i >= bA.Count() {
+	if i < 0 || i >= bA.Count() {
 		return false
 	}
 
@@ -262,6 +262,9 @@ func CompactUnmarshal(bz []byte) (*CompactBitArray, error) {
 	}
 
 	size, n := binary.Uvarint(bz)
+	if n < 0 || n >= len(bz) {
+		return nil, fmt.Errorf("compact bit array: n=%d is out of range of len(bz)=%d", n, len(bz))
+	}
 	bz = bz[n:]
 
 	if len(bz) != int(size+7)/8 {
diff --git a/crypto/types/compact_bit_array_test.go b/crypto/types/compact_bit_array_test.go
index 05bc73eb3498..11984729ac28 100644
--- a/crypto/types/compact_bit_array_test.go
+++ b/crypto/types/compact_bit_array_test.go
@@ -184,6 +184,17 @@ func TestCompactMarshalUnmarshal(t *testing.T) {
 	}
 }
 
+// Ensure that CompactUnmarshal does not blindly try to slice using
+// a negative/out of bounds index of size returned from binary.Uvarint.
+// See issue https://github.com/cosmos/cosmos-sdk/issues/9165
+func TestCompactMarshalUnmarshalReturnsErrorOnInvalidSize(t *testing.T) {
+	malicious := []byte{0xd7, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x01, 0x24, 0x28}
+	cba, err := CompactUnmarshal(malicious)
+	require.Error(t, err)
+	require.Nil(t, cba)
+	require.Contains(t, err.Error(), "n=-11 is out of range of len(bz)=13")
+}
+
 func TestCompactBitArrayNumOfTrueBitsBefore(t *testing.T) {
 	testCases := []struct {
 		marshalledBA   string
@@ -227,6 +238,15 @@ func TestCompactBitArrayGetSetIndex(t *testing.T) {
 			val := (r.Int63() % 2) == 0
 			bA.SetIndex(index, val)
 			require.Equal(t, val, bA.GetIndex(index), "bA.SetIndex(%d, %v) failed on bit array: %s", index, val, copy)
+
+			// Ensure that passing in negative indices to .SetIndex and .GetIndex do not
+			// panic. See issue https://github.com/cosmos/cosmos-sdk/issues/9164.
+			// To intentionally use negative indices, We want only values that aren't 0.
+			if index == 0 {
+				continue
+			}
+			require.False(t, bA.SetIndex(-index, val))
+			require.False(t, bA.GetIndex(-index))
 		}
 	}
 }