Add in Stronger Length Checks (#9758)

* add changes

* radek's review

Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>

beacon-chain/cache/depositcache/deposits_cache.go

@@ -136,7 +136,10 @@ func (dc *DepositCache) InsertFinalizedDeposits(ctx context.Context, eth1Deposit
 			log.WithError(err).Error("Could not hash deposit data. Finalized deposit cache not updated.")
 			return
 		}
-		depositTrie.Insert(depHash[:], insertIndex)
+		if err = depositTrie.Insert(depHash[:], insertIndex); err != nil {
+			log.WithError(err).Error("Could not insert deposit hash")
+			return
+		}
 		insertIndex++
 	}
 

beacon-chain/powchain/deposit_test.go

@@ -220,7 +220,7 @@ func TestProcessDeposit_IncompleteDeposit(t *testing.T) {
 	factor := params.BeaconConfig().MaxEffectiveBalance / params.BeaconConfig().EffectiveBalanceIncrement
 	// deposit till 31e9
 	for i := 0; i < int(factor-1); i++ {
-		trie.Insert(dataRoot[:], i)
+		assert.NoError(t, trie.Insert(dataRoot[:], i))
 
 		trieRoot := trie.HashTreeRoot()
 		eth1Data.DepositRoot = trieRoot[:]

beacon-chain/powchain/log_processing.go

@@ -138,7 +138,9 @@ func (s *Service) ProcessDepositLog(ctx context.Context, depositLog gethTypes.Lo
 	if s.depositTrie.NumOfItems() != int(index) {
 		return errors.Errorf("invalid deposit index received: wanted %d but got %d", s.depositTrie.NumOfItems(), index)
 	}
-	s.depositTrie.Insert(depositHash[:], int(index))
+	if err = s.depositTrie.Insert(depositHash[:], int(index)); err != nil {
+		return err
+	}
 
 	deposit := &ethpb.Deposit{
 		Data: depositData,

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

@@ -697,7 +697,9 @@ func (vs *Server) depositTrie(ctx context.Context, canonicalEth1Data *ethpb.Eth1
 		if err != nil {
 			return nil, errors.Wrap(err, "could not hash deposit data")
 		}
-		depositTrie.Insert(depHash[:], int(insertIndex))
+		if err = depositTrie.Insert(depHash[:], int(insertIndex)); err != nil {
+			return nil, err
+		}
 		insertIndex++
 	}
 	valid, err := vs.validateDepositTrie(depositTrie, canonicalEth1Data)

beacon-chain/rpc/prysm/v1alpha1/validator/proposer_test.go

@@ -486,7 +486,7 @@ func TestProposer_PendingDeposits_OutsideEth1FollowWindow(t *testing.T) {
 		depositHash, err := dp.Deposit.Data.HashTreeRoot()
 		require.NoError(t, err, "Unable to determine hashed value of deposit")
 
-		depositTrie.Insert(depositHash[:], int(dp.Index))
+		assert.NoError(t, depositTrie.Insert(depositHash[:], int(dp.Index)))
 		assert.NoError(t, depositCache.InsertDeposit(ctx, dp.Deposit, dp.Eth1BlockHeight, dp.Index, depositTrie.HashTreeRoot()))
 	}
 	for _, dp := range recentDeposits {
@@ -621,7 +621,7 @@ func TestProposer_PendingDeposits_FollowsCorrectEth1Block(t *testing.T) {
 		depositHash, err := dp.Deposit.Data.HashTreeRoot()
 		require.NoError(t, err, "Unable to determine hashed value of deposit")
 
-		depositTrie.Insert(depositHash[:], int(dp.Index))
+		assert.NoError(t, depositTrie.Insert(depositHash[:], int(dp.Index)))
 		assert.NoError(t, depositCache.InsertDeposit(ctx, dp.Deposit, dp.Eth1BlockHeight, dp.Index, depositTrie.HashTreeRoot()))
 	}
 	for _, dp := range recentDeposits {
@@ -720,7 +720,7 @@ func TestProposer_PendingDeposits_CantReturnBelowStateEth1DepositIndex(t *testin
 		depositHash, err := dp.Deposit.Data.HashTreeRoot()
 		require.NoError(t, err, "Unable to determine hashed value of deposit")
 
-		depositTrie.Insert(depositHash[:], int(dp.Index))
+		assert.NoError(t, depositTrie.Insert(depositHash[:], int(dp.Index)))
 		assert.NoError(t, depositCache.InsertDeposit(ctx, dp.Deposit, uint64(dp.Index), dp.Index, depositTrie.HashTreeRoot()))
 	}
 	for _, dp := range recentDeposits {
@@ -816,7 +816,7 @@ func TestProposer_PendingDeposits_CantReturnMoreThanMax(t *testing.T) {
 		depositHash, err := dp.Deposit.Data.HashTreeRoot()
 		require.NoError(t, err, "Unable to determine hashed value of deposit")
 
-		depositTrie.Insert(depositHash[:], int(dp.Index))
+		assert.NoError(t, depositTrie.Insert(depositHash[:], int(dp.Index)))
 		assert.NoError(t, depositCache.InsertDeposit(ctx, dp.Deposit, height.Uint64(), dp.Index, depositTrie.HashTreeRoot()))
 	}
 	for _, dp := range recentDeposits {
@@ -910,7 +910,7 @@ func TestProposer_PendingDeposits_CantReturnMoreThanDepositCount(t *testing.T) {
 		depositHash, err := dp.Deposit.Data.HashTreeRoot()
 		require.NoError(t, err, "Unable to determine hashed value of deposit")
 
-		depositTrie.Insert(depositHash[:], int(dp.Index))
+		assert.NoError(t, depositTrie.Insert(depositHash[:], int(dp.Index)))
 		assert.NoError(t, depositCache.InsertDeposit(ctx, dp.Deposit, uint64(dp.Index), dp.Index, depositTrie.HashTreeRoot()))
 	}
 	for _, dp := range recentDeposits {
@@ -1019,7 +1019,7 @@ func TestProposer_DepositTrie_UtilizesCachedFinalizedDeposits(t *testing.T) {
 		depositHash, err := dp.Deposit.Data.HashTreeRoot()
 		require.NoError(t, err, "Unable to determine hashed value of deposit")
 
-		depositTrie.Insert(depositHash[:], int(dp.Index))
+		assert.NoError(t, depositTrie.Insert(depositHash[:], int(dp.Index)))
 		assert.NoError(t, depositCache.InsertDeposit(ctx, dp.Deposit, dp.Eth1BlockHeight, dp.Index, depositTrie.HashTreeRoot()))
 	}
 	for _, dp := range recentDeposits {
@@ -1129,7 +1129,7 @@ func TestProposer_DepositTrie_RebuildTrie(t *testing.T) {
 		depositHash, err := dp.Deposit.Data.HashTreeRoot()
 		require.NoError(t, err, "Unable to determine hashed value of deposit")
 
-		depositTrie.Insert(depositHash[:], int(dp.Index))
+		assert.NoError(t, depositTrie.Insert(depositHash[:], int(dp.Index)))
 		assert.NoError(t, depositCache.InsertDeposit(ctx, dp.Deposit, dp.Eth1BlockHeight, dp.Index, depositTrie.HashTreeRoot()))
 	}
 	for _, dp := range recentDeposits {
@@ -1191,17 +1191,17 @@ func TestProposer_ValidateDepositTrie(t *testing.T) {
 			eth1dataCreator: func() *ethpb.Eth1Data {
 				trie, err := trie.NewTrie(params.BeaconConfig().DepositContractTreeDepth)
 				assert.NoError(t, err)
-				trie.Insert([]byte{'a'}, 0)
-				trie.Insert([]byte{'b'}, 1)
-				trie.Insert([]byte{'c'}, 2)
+				assert.NoError(t, trie.Insert([]byte{'a'}, 0))
+				assert.NoError(t, trie.Insert([]byte{'b'}, 1))
+				assert.NoError(t, trie.Insert([]byte{'c'}, 2))
 				return &ethpb.Eth1Data{DepositRoot: []byte{'B'}, DepositCount: 3, BlockHash: []byte{}}
 			},
 			trieCreator: func() *trie.SparseMerkleTrie {
 				trie, err := trie.NewTrie(params.BeaconConfig().DepositContractTreeDepth)
 				assert.NoError(t, err)
-				trie.Insert([]byte{'a'}, 0)
-				trie.Insert([]byte{'b'}, 1)
-				trie.Insert([]byte{'c'}, 2)
+				assert.NoError(t, trie.Insert([]byte{'a'}, 0))
+				assert.NoError(t, trie.Insert([]byte{'b'}, 1))
+				assert.NoError(t, trie.Insert([]byte{'c'}, 2))
 				return trie
 			},
 			success: false,
@@ -1211,18 +1211,18 @@ func TestProposer_ValidateDepositTrie(t *testing.T) {
 			eth1dataCreator: func() *ethpb.Eth1Data {
 				trie, err := trie.NewTrie(params.BeaconConfig().DepositContractTreeDepth)
 				assert.NoError(t, err)
-				trie.Insert([]byte{'a'}, 0)
-				trie.Insert([]byte{'b'}, 1)
-				trie.Insert([]byte{'c'}, 2)
+				assert.NoError(t, trie.Insert([]byte{'a'}, 0))
+				assert.NoError(t, trie.Insert([]byte{'b'}, 1))
+				assert.NoError(t, trie.Insert([]byte{'c'}, 2))
 				rt := trie.HashTreeRoot()
 				return &ethpb.Eth1Data{DepositRoot: rt[:], DepositCount: 3, BlockHash: []byte{}}
 			},
 			trieCreator: func() *trie.SparseMerkleTrie {
 				trie, err := trie.NewTrie(params.BeaconConfig().DepositContractTreeDepth)
 				assert.NoError(t, err)
-				trie.Insert([]byte{'a'}, 0)
-				trie.Insert([]byte{'b'}, 1)
-				trie.Insert([]byte{'c'}, 2)
+				assert.NoError(t, trie.Insert([]byte{'a'}, 0))
+				assert.NoError(t, trie.Insert([]byte{'b'}, 1))
+				assert.NoError(t, trie.Insert([]byte{'c'}, 2))
 				return trie
 			},
 			success: true,
@@ -1960,7 +1960,7 @@ func TestProposer_Deposits_ReturnsEmptyList_IfLatestEth1DataEqGenesisEth1Block(t
 		depositHash, err := dp.Deposit.Data.HashTreeRoot()
 		require.NoError(t, err, "Unable to determine hashed value of deposit")
 
-		depositTrie.Insert(depositHash[:], int(dp.Index))
+		assert.NoError(t, depositTrie.Insert(depositHash[:], int(dp.Index)))
 		assert.NoError(t, depositCache.InsertDeposit(ctx, dp.Deposit, uint64(dp.Index), dp.Index, depositTrie.HashTreeRoot()))
 	}
 	for _, dp := range recentDeposits {

beacon-chain/rpc/prysm/v1alpha1/validator/status_test.go

@@ -532,7 +532,7 @@ func TestActivationStatus_OK(t *testing.T) {
 	assert.NoError(t, depositCache.InsertDeposit(ctx, dep, 10 /*blockNum*/, 0, depositTrie.HashTreeRoot()))
 
 	dep = deposits[2]
-	depositTrie.Insert(dep.Data.Signature, 15)
+	assert.NoError(t, depositTrie.Insert(dep.Data.Signature, 15))
 	assert.NoError(t, depositCache.InsertDeposit(context.Background(), dep, 0, 1, depositTrie.HashTreeRoot()))
 
 	vs := &Server{
@@ -736,7 +736,7 @@ func TestMultipleValidatorStatus_Pubkeys(t *testing.T) {
 	dep := deposits[0]
 	assert.NoError(t, depositCache.InsertDeposit(ctx, dep, 10 /*blockNum*/, 0, depositTrie.HashTreeRoot()))
 	dep = deposits[2]
-	depositTrie.Insert(dep.Data.Signature, 15)
+	assert.NoError(t, depositTrie.Insert(dep.Data.Signature, 15))
 	assert.NoError(t, depositCache.InsertDeposit(context.Background(), dep, 0, 1, depositTrie.HashTreeRoot()))
 
 	vs := &Server{

container/trie/BUILD.bazel

@@ -27,6 +27,7 @@ go_test(
         "//crypto/hash:go_default_library",
         "//encoding/bytesutil:go_default_library",
         "//proto/prysm/v1alpha1:go_default_library",
+        "//testing/assert:go_default_library",
         "//testing/require:go_default_library",
         "@com_github_ethereum_go_ethereum//accounts/abi/bind:go_default_library",
     ],

container/trie/sparse_merkle.go

@@ -93,7 +93,10 @@ func (m *SparseMerkleTrie) HashTreeRoot() [32]byte {
 }
 
 // Insert an item into the trie.
-func (m *SparseMerkleTrie) Insert(item []byte, index int) {
+func (m *SparseMerkleTrie) Insert(item []byte, index int) error {
+	if index < 0 {
+		return fmt.Errorf("negative index provided: %d", index)
+	}
 	for index >= len(m.branches[0]) {
 		m.branches[0] = append(m.branches[0], ZeroHashes[0][:])
 	}
@@ -132,10 +135,14 @@ func (m *SparseMerkleTrie) Insert(item []byte, index int) {
 		}
 		currentIndex = parentIdx
 	}
+	return nil
 }
 
 // MerkleProof computes a proof from a trie's branches using a Merkle index.
 func (m *SparseMerkleTrie) MerkleProof(index int) ([][]byte, error) {
+	if index < 0 {
+		return nil, fmt.Errorf("merkle index is negative: %d", index)
+	}
 	merkleIndex := uint(index)
 	leaves := m.branches[0]
 	if index >= len(leaves) {

container/trie/sparse_merkle_test.go

@@ -11,6 +11,7 @@ import (
 	"github.com/prysmaticlabs/prysm/crypto/hash"
 	"github.com/prysmaticlabs/prysm/encoding/bytesutil"
 	ethpb "github.com/prysmaticlabs/prysm/proto/prysm/v1alpha1"
+	"github.com/prysmaticlabs/prysm/testing/assert"
 	"github.com/prysmaticlabs/prysm/testing/require"
 )
 
@@ -112,6 +113,24 @@ func TestMerkleTrie_VerifyMerkleProof(t *testing.T) {
 	require.Equal(t, false, trie.VerifyMerkleBranch(root[:], []byte("buzz"), 3, proof, params.BeaconConfig().DepositContractTreeDepth))
 }
 
+func TestMerkleTrie_NegativeIndexes(t *testing.T) {
+	items := [][]byte{
+		[]byte("A"),
+		[]byte("B"),
+		[]byte("C"),
+		[]byte("D"),
+		[]byte("E"),
+		[]byte("F"),
+		[]byte("G"),
+		[]byte("H"),
+	}
+	m, err := trie.GenerateTrieFromItems(items, params.BeaconConfig().DepositContractTreeDepth)
+	require.NoError(t, err)
+	_, err = m.MerkleProof(-1)
+	require.ErrorContains(t, "merkle index is negative", err)
+	require.ErrorContains(t, "negative index provided", m.Insert([]byte{'J'}, -1))
+}
+
 func TestMerkleTrie_VerifyMerkleProof_TrieUpdated(t *testing.T) {
 	items := [][]byte{
 		{1},
@@ -128,7 +147,7 @@ func TestMerkleTrie_VerifyMerkleProof_TrieUpdated(t *testing.T) {
 	require.Equal(t, true, trie.VerifyMerkleBranch(root[:], items[0], 0, proof, depth))
 
 	// Now we update the trie.
-	m.Insert([]byte{5}, 3)
+	assert.NoError(t, m.Insert([]byte{5}, 3))
 	proof, err = m.MerkleProof(3)
 	require.NoError(t, err)
 	root = m.HashTreeRoot()
@@ -140,7 +159,7 @@ func TestMerkleTrie_VerifyMerkleProof_TrieUpdated(t *testing.T) {
 	}
 
 	// Now we update the trie at an index larger than the number of items.
-	m.Insert([]byte{6}, 15)
+	assert.NoError(t, m.Insert([]byte{6}, 15))
 }
 
 func TestRoundtripProto_OK(t *testing.T) {
@@ -209,7 +228,7 @@ func BenchmarkInsertTrie_Optimized(b *testing.B) {
 	someItem := bytesutil.ToBytes32([]byte("hello-world"))
 	b.StartTimer()
 	for i := 0; i < b.N; i++ {
-		tr.Insert(someItem[:], i%numDeposits)
+		require.NoError(b, tr.Insert(someItem[:], i%numDeposits))
 	}
 }
 

contracts/deposit/deposit_tree_test.go

@@ -44,7 +44,7 @@ func TestDepositTrieRoot_OK(t *testing.T) {
 		item, err := data.HashTreeRoot()
 		require.NoError(t, err)
 
-		localTrie.Insert(item[:], i)
+		assert.NoError(t, localTrie.Insert(item[:], i))
 		depRoot, err = testAcc.Contract.GetDepositRoot(&bind.CallOpts{})
 		require.NoError(t, err)
 		assert.Equal(t, depRoot, localTrie.HashTreeRoot(), "Local deposit trie root and contract deposit trie root are not equal for index %d", i)
@@ -84,7 +84,7 @@ func TestDepositTrieRoot_Fail(t *testing.T) {
 		item, err := data.HashTreeRoot()
 		require.NoError(t, err)
 
-		localTrie.Insert(item[:], i)
+		assert.NoError(t, localTrie.Insert(item[:], i))
 
 		depRoot, err = testAcc.Contract.GetDepositRoot(&bind.CallOpts{})
 		require.NoError(t, err)

encoding/bytesutil/bytes.go

@@ -16,6 +16,9 @@ var hexRegex = regexp.MustCompile("^0x[0-9a-fA-F]+$")
 // ToBytes returns integer x to bytes in little-endian format at the specified length.
 // Spec defines similar method uint_to_bytes(n: uint) -> bytes, which is equivalent to ToBytes(n, 8).
 func ToBytes(x uint64, length int) []byte {
+	if length < 0 {
+		length = 0
+	}
 	makeLength := length
 	if length < 8 {
 		makeLength = 8
@@ -70,6 +73,9 @@ func Bytes32(x uint64) []byte {
 // FromBytes4 returns an integer which is stored in the little-endian format(4, 'little')
 // from a byte array.
 func FromBytes4(x []byte) uint64 {
+	if len(x) < 4 {
+		return 0
+	}
 	empty4bytes := make([]byte, 4)
 	return binary.LittleEndian.Uint64(append(x[:4], empty4bytes...))
 }
@@ -77,6 +83,9 @@ func FromBytes4(x []byte) uint64 {
 // FromBytes8 returns an integer which is stored in the little-endian format(8, 'little')
 // from a byte array.
 func FromBytes8(x []byte) uint64 {
+	if len(x) < 8 {
+		return 0
+	}
 	return binary.LittleEndian.Uint64(x)
 }
 
@@ -134,6 +143,9 @@ func ToBool(x byte) bool {
 // FromBytes2 returns an integer which is stored in the little-endian format(2, 'little')
 // from a byte array.
 func FromBytes2(x []byte) uint16 {
+	if len(x) < 2 {
+		return 0
+	}
 	return binary.LittleEndian.Uint16(x[:2])
 }
 
@@ -172,9 +184,11 @@ func Trunc(x []byte) []byte {
 
 // ToLowInt64 returns the lowest 8 bytes interpreted as little endian.
 func ToLowInt64(x []byte) int64 {
-	if len(x) > 8 {
-		x = x[:8]
+	if len(x) < 8 {
+		return 0
 	}
+	// Use the first 8 bytes.
+	x = x[:8]
 	return int64(binary.LittleEndian.Uint64(x))
 }
 
@@ -249,7 +263,7 @@ func SetBit(b []byte, i int) []byte {
 // Returns the original bitlist if the index `i`
 // is out of range.
 func ClearBit(b []byte, i int) []byte {
-	if i >= len(b)*8 {
+	if i >= len(b)*8 || i < 0 {
 		return b
 	}
 
@@ -288,6 +302,9 @@ func HighestBitIndexAt(b []byte, index int) (int, error) {
 	if b == nil || bLength == 0 {
 		return 0, errors.New("input list can't be empty or nil")
 	}
+	if index < 0 {
+		return 0, errors.Errorf("index is negative: %d", index)
+	}
 
 	start := index / 8
 	if start >= bLength {

encoding/bytesutil/bytes_test.go

@@ -471,3 +471,29 @@ func TestSafeCopy2dBytes(t *testing.T) {
 		})
 	}
 }
+
+func TestBytesInvalidInputs(t *testing.T) {
+	defer func() {
+		if r := recover(); r != nil {
+			t.Errorf("Test panicked: %v", r)
+		}
+	}()
+	rawBytes := bytesutil.ToBytes(100, -10)
+	assert.DeepEqual(t, []byte{}, rawBytes)
+
+	_, err := bytesutil.HighestBitIndexAt([]byte{'A', 'B', 'C'}, -5)
+	assert.ErrorContains(t, "index is negative", err)
+
+	// There should be no panic
+	_ = bytesutil.ClearBit([]byte{'C', 'D', 'E'}, -7)
+	res := bytesutil.FromBytes4([]byte{})
+	assert.Equal(t, res, uint64(0))
+	newRes := bytesutil.FromBytes2([]byte{})
+	assert.Equal(t, newRes, uint16(0))
+	res = bytesutil.FromBytes8([]byte{})
+	assert.Equal(t, res, uint64(0))
+
+	intRes := bytesutil.ToLowInt64([]byte{})
+	assert.Equal(t, intRes, int64(0))
+
+}

testing/util/deposits.go

@@ -69,7 +69,9 @@ func DeterministicDepositsAndKeys(numDeposits uint64) ([]*ethpb.Deposit, []bls.S
 				return nil, nil, errors.Wrap(err, "could not tree hash deposit data")
 			}
 
-			t.Insert(hashedDeposit[:], int(numExisting+i))
+			if err = t.Insert(hashedDeposit[:], int(numExisting+i)); err != nil {
+				return nil, nil, err
+			}
 		}
 	}
 
@@ -138,7 +140,9 @@ func DepositsWithBalance(balances []uint64) ([]*ethpb.Deposit, *trie.SparseMerkl
 			return nil, nil, errors.Wrap(err, "could not tree hash deposit data")
 		}
 
-		sparseTrie.Insert(hashedDeposit[:], int(i))
+		if err = sparseTrie.Insert(hashedDeposit[:], int(i)); err != nil {
+			return nil, nil, err
+		}
 	}
 
 	depositTrie, _, err := DepositTrieSubset(sparseTrie, int(numDeposits))
@@ -364,7 +368,9 @@ func DeterministicDepositsAndKeysSameValidator(numDeposits uint64) ([]*ethpb.Dep
 				return nil, nil, errors.Wrap(err, "could not tree hash deposit data")
 			}
 
-			t.Insert(hashedDeposit[:], int(numExisting+i))
+			if err = t.Insert(hashedDeposit[:], int(numExisting+i)); err != nil {
+				return nil, nil, err
+			}
 		}
 	}