Merge commit from fork

Author: melekes

.changelog/unreleased/bug-fixes/0021-abc-panic-precommit-validator-index.md

@@ -0,0 +1,3 @@
+- `[consensus]` Do not panic if the validator index of a `Vote` message is out
+  of bounds, when vote extensions are enabled
+  ([\#ABC-0021](https://github.com/cometbft/cometbft/security/advisories/GHSA-p7mv-53f2-4cwj))

consensus/errors.go

@@ -0,0 +1,9 @@
+package consensus
+
+type ErrInvalidVote struct {
+	Reason string
+}
+
+func (e ErrInvalidVote) Error() string {
+	return "invalid vote: " + e.Reason
+}

consensus/reactor_test.go

@@ -26,6 +26,7 @@ import (
 	"github.com/cometbft/cometbft/libs/bytes"
 	"github.com/cometbft/cometbft/libs/json"
 	"github.com/cometbft/cometbft/libs/log"
+	cmtrand "github.com/cometbft/cometbft/libs/rand"
 	cmtsync "github.com/cometbft/cometbft/libs/sync"
 	mempl "github.com/cometbft/cometbft/mempool"
 	"github.com/cometbft/cometbft/p2p"
@@ -1126,3 +1127,39 @@ func TestMarshalJSONPeerState(t *testing.T) {
 			"block_parts":"0"}
 		}`, string(data))
 }
+
+func TestVoteMessageValidateBasic(t *testing.T) {
+	_, vss := randState(2)
+
+	randBytes := cmtrand.Bytes(tmhash.Size)
+	blockID := types.BlockID{
+		Hash: randBytes,
+		PartSetHeader: types.PartSetHeader{
+			Total: 1,
+			Hash:  randBytes,
+		},
+	}
+	vote := signVote(vss[1], cmtproto.PrecommitType, randBytes, blockID.PartSetHeader, true)
+
+	testCases := []struct {
+		malleateFn func(*VoteMessage)
+		expErr     string
+	}{
+		{func(_ *VoteMessage) {}, ""},
+		{func(msg *VoteMessage) { msg.Vote.ValidatorIndex = -1 }, "negative ValidatorIndex"},
+		// INVALID, but passes ValidateBasic, since the method does not know the number of active validators
+		{func(msg *VoteMessage) { msg.Vote.ValidatorIndex = 1000 }, ""},
+	}
+
+	for i, tc := range testCases {
+		t.Run(fmt.Sprintf("#%d", i), func(t *testing.T) {
+			msg := &VoteMessage{vote}
+
+			tc.malleateFn(msg)
+			err := msg.ValidateBasic()
+			if tc.expErr != "" && assert.Error(t, err) { //nolint:testifylint // require.Error doesn't work with the conditional here
+				assert.Contains(t, err.Error(), tc.expErr)
+			}
+		})
+	}
+}

consensus/state.go

@@ -2192,6 +2192,14 @@ func (cs *State) addVote(vote *types.Vote, peerID p2p.ID) (added bool, err error
 			// Here, we verify the signature of the vote extension included in the vote
 			// message.
 			_, val := cs.state.Validators.GetByIndex(vote.ValidatorIndex)
+			if val == nil { // TODO: we should disconnect from this malicious peer
+				valsCount := cs.state.Validators.Size()
+				cs.Logger.Info("Peer sent us vote with invalid ValidatorIndex",
+					"peer", peerID,
+					"validator_index", vote.ValidatorIndex,
+					"len_validators", valsCount)
+				return added, ErrInvalidVote{Reason: fmt.Sprintf("ValidatorIndex %d is out of bounds [0, %d)", vote.ValidatorIndex, valsCount)}
+			}
 			if err := vote.VerifyExtension(cs.state.ChainID, val.PubKey); err != nil {
 				return false, err
 			}

consensus/state_test.go

@@ -1938,6 +1938,33 @@ func TestVoteExtensionEnableHeight(t *testing.T) {
 	}
 }
 
+// TestStateDoesntCrashOnInvalidVote tests that the state does not crash when
+// receiving an invalid vote. In particular, one with the incorrect
+// ValidatorIndex.
+func TestStateDoesntCrashOnInvalidVote(t *testing.T) {
+	cs, vss := randState(2)
+	height, round := cs.Height, cs.Round
+	// create dummy peer
+	peer := p2pmock.NewPeer(nil)
+
+	startTestRound(cs, height, round)
+
+	vote := signVote(vss[1], cmtproto.PrecommitType, nil, types.PartSetHeader{}, true)
+	// Non-existent validator index
+	vote.ValidatorIndex = int32(len(vss))
+
+	voteMessage := &VoteMessage{vote}
+	assert.NotPanics(t, func() {
+		cs.handleMsg(msgInfo{voteMessage, peer.ID()})
+	})
+
+	added, err := cs.AddVote(vote, peer.ID())
+	assert.False(t, added)
+	assert.NoError(t, err)
+	// TODO: uncomment once we punish peer and return an error
+	// assert.Equal(t, ErrInvalidVote{Reason: "ValidatorIndex 2 is out of bounds [0, 2)"}, err)
+}
+
 // 4 vals, 3 Nil Precommits at P0
 // What we want:
 // P0 waits for timeoutPrecommit before starting next round