fix to ProposalPOLMessage poisoning (CON-222)

sei-tendermint/internal/consensus/common_test.go

@@ -301,8 +301,8 @@ func validatePrevote(
 
 	address := pubKey.Address()
 
-	vote := prevotes.GetByAddress(address)
-	require.NotNil(t, vote, "Failed to find prevote from validator")
+	vote, ok := prevotes.GetByAddress(address)
+	require.True(t, ok, "Failed to find prevote from validator")
 
 	if blockHash == nil {
 		require.Nil(t, vote.BlockID.Hash, "Expected prevote to be for nil, got %X", vote.BlockID.Hash)
@@ -319,8 +319,8 @@ func validateLastPrecommit(ctx context.Context, t *testing.T, cs *State, privVal
 	require.NoError(t, err)
 	address := pv.Address()
 
-	vote := votes.GetByAddress(address)
-	require.NotNil(t, vote)
+	vote, ok := votes.GetByAddress(address)
+	require.True(t, ok)
 
 	require.True(t, bytes.Equal(vote.BlockID.Hash, blockHash),
 		"Expected precommit to be for %X, got %X", blockHash, vote.BlockID.Hash)
@@ -343,8 +343,8 @@ func validatePrecommit(
 	require.NoError(t, err)
 	address := pv.Address()
 
-	vote := precommits.GetByAddress(address)
-	require.NotNil(t, vote, "Failed to find precommit from validator")
+	vote, ok := precommits.GetByAddress(address)
+	require.True(t, ok, "Failed to find precommit from validator")
 
 	if votedBlockHash == nil {
 		require.Nil(t, vote.BlockID.Hash, "Expected precommit to be for nil")

sei-tendermint/internal/consensus/invalid_test.go

@@ -11,17 +11,147 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/sei-protocol/sei-chain/sei-tendermint/crypto"
+	cstypes "github.com/sei-protocol/sei-chain/sei-tendermint/internal/consensus/types"
 	"github.com/sei-protocol/sei-chain/sei-tendermint/internal/eventbus"
 	"github.com/sei-protocol/sei-chain/sei-tendermint/internal/p2p"
+	"github.com/sei-protocol/sei-chain/sei-tendermint/libs/bits"
 	"github.com/sei-protocol/sei-chain/sei-tendermint/libs/bytes"
 	tmrand "github.com/sei-protocol/sei-chain/sei-tendermint/libs/rand"
 	tmtime "github.com/sei-protocol/sei-chain/sei-tendermint/libs/time"
 	"github.com/sei-protocol/sei-chain/sei-tendermint/libs/utils"
 	tmcons "github.com/sei-protocol/sei-chain/sei-tendermint/proto/tendermint/consensus"
 	tmproto "github.com/sei-protocol/sei-chain/sei-tendermint/proto/tendermint/types"
 	"github.com/sei-protocol/sei-chain/sei-tendermint/types"
+	"github.com/sei-protocol/sei-chain/sei-tendermint/version"
 )
 
+// Test checking that if peer sends a ProposalPOLMessage with a bitarray with bad length,
+// the node will handle it gracefully.
+func TestGossipVotesForHeightPoisonedProposalPOL(t *testing.T) {
+	ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second)
+	defer cancel()
+
+	cfg := configSetup(t)
+	states, cleanup := makeConsensusState(ctx, t, cfg, 2, "consensus_reactor_test", newMockTickerFunc(true))
+	t.Cleanup(cleanup)
+
+	rts := setup(ctx, t, 2, states, 1)
+
+	var nodeIDs []types.NodeID
+	for _, node := range rts.network.Nodes() {
+		nodeIDs = append(nodeIDs, node.NodeID)
+	}
+	require.Len(t, nodeIDs, 2)
+
+	reactor := rts.reactors[nodeIDs[0]]
+	peerID := nodeIDs[1]
+	state := reactor.state.GetState()
+	reactor.SwitchToConsensus(ctx, state, false)
+
+	require.Eventually(t, func() bool {
+		_, ok := reactor.GetPeerState(peerID)
+		return ok
+	}, time.Hour, 50*time.Millisecond)
+
+	valSet, privVals := types.RandValidatorSet(4, 1)
+	proposerPubKey, err := privVals[0].GetPubKey(ctx)
+	require.NoError(t, err)
+
+	proposal := types.NewProposal(
+		1,
+		1,
+		0,
+		types.BlockID{
+			Hash: crypto.CRandBytes(crypto.HashSize),
+			PartSetHeader: types.PartSetHeader{
+				Total: 1,
+				Hash:  crypto.CRandBytes(crypto.HashSize),
+			},
+		},
+		time.Now(),
+		nil,
+		types.Header{
+			Version:         version.Consensus{Block: version.BlockProtocol},
+			Height:          1,
+			ProposerAddress: proposerPubKey.Address(),
+		},
+		&types.Commit{},
+		nil,
+		proposerPubKey.Address(),
+	)
+	proposal.Signature = makeSig("invalid-signature")
+
+	require.NoError(t, reactor.handleStateMessage(p2p.RecvMsg[*tmcons.Message]{
+		From: peerID,
+		Message: MsgToProto(&NewRoundStepMessage{
+			HRS: cstypes.HRS{
+				Height: 1,
+				Round:  1,
+				Step:   cstypes.RoundStepPrevote,
+			},
+			SecondsSinceStartTime: 1,
+			LastCommitRound:       -1,
+		}),
+	}))
+
+	require.NoError(t, reactor.handleDataMessage(ctx, p2p.RecvMsg[*tmcons.Message]{
+		From: peerID,
+		Message: MsgToProto(&ProposalMessage{
+			Proposal: proposal,
+		}),
+	}))
+
+	require.NoError(t, reactor.handleDataMessage(ctx, p2p.RecvMsg[*tmcons.Message]{
+		From: peerID,
+		Message: MsgToProto(&ProposalPOLMessage{
+			Height:           1,
+			ProposalPOLRound: 0,
+			ProposalPOL:      bits.NewBitArray(1),
+		}),
+	}))
+
+	ps, ok := reactor.GetPeerState(peerID)
+	require.True(t, ok)
+	prs := ps.GetRoundState()
+	require.Equal(t, int64(1), prs.Height)
+	require.Equal(t, int32(1), prs.Round)
+	require.Equal(t, int32(0), prs.ProposalPOLRound)
+	require.Equal(t, 1, prs.ProposalPOL.Size())
+
+	voteSet := cstypes.NewHeightVoteSet("test-chain", 1, valSet)
+	voteSet.SetRound(1)
+
+	voter := newValidatorStub(privVals[1], 1)
+	voter.Height = 1
+	voter.Round = 0
+
+	vote := signVote(ctx, t, voter, tmproto.PrevoteType, "test-chain", types.BlockID{
+		Hash: crypto.CRandBytes(crypto.HashSize),
+		PartSetHeader: types.PartSetHeader{
+			Total: 1,
+			Hash:  crypto.CRandBytes(crypto.HashSize),
+		},
+	})
+	added, err := voteSet.AddVote(vote, "")
+	require.NoError(t, err)
+	require.True(t, added)
+
+	rs := &cstypes.RoundState{
+		HRS: cstypes.HRS{
+			Height: 1,
+			Round:  1,
+			Step:   cstypes.RoundStepPrevote,
+		},
+		Votes: voteSet,
+	}
+
+	// Gossip should take into consideration that PeerState might contain
+	// invalid length bitarrays.
+	for range 10 {
+		reactor.gossipVotesForHeight(rs, ps.GetRoundState(), ps)
+	}
+}
+
 func TestReactorInvalidPrecommit(t *testing.T) {
 	t.Skip("test doesn't check anything useful")
 	ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second)
@@ -128,7 +258,10 @@ func invalidDoPrevoteFunc(
 		require.NoError(t, err)
 
 		addr := pubKey.Address()
-		valIndex, _ := cs.roundState.Validators().GetByAddress(addr)
+		valIndex, _, ok := cs.roundState.Validators().GetByAddress(addr)
+		if !ok {
+			panic("missing validator")
+		}
 
 		// precommit a random block
 		blockHash := bytes.HexBytes(tmrand.Bytes(32))

sei-tendermint/internal/consensus/msgs.go

@@ -131,7 +131,7 @@ func (m *ProposalMessage) String() string {
 	return fmt.Sprintf("[Proposal %v]", m.Proposal)
 }
 
-// ProposalPOLMessage is sent when a previous proposal is re-proposed.
+// ProposalPOLMessage is sent when a node needs POL round votes.
 type ProposalPOLMessage struct {
 	Height           int64
 	ProposalPOLRound int32

sei-tendermint/internal/consensus/peer_state.go

@@ -93,12 +93,6 @@ func (ps *PeerState) SetHasProposal(proposal *types.Proposal) {
 		return
 	}
 
-	// Check memory limits before acquiring lock or setting any state
-	if proposal.BlockID.PartSetHeader.Total > types.MaxBlockPartsCount {
-		logger.Debug("PartSetHeader.Total exceeds maximum", "total", proposal.BlockID.PartSetHeader.Total, "max", types.MaxBlockPartsCount)
-		return
-	}
-
 	ps.mtx.Lock()
 	defer ps.mtx.Unlock()
 
@@ -187,12 +181,8 @@ func (ps *PeerState) PickVoteToSend(votes types.VoteSetReader) (*types.Vote, boo
 	}
 
 	if index, ok := votes.BitArray().Sub(psVotes).PickRandom(); ok {
-		vote := votes.GetByIndex(int32(index)) //nolint:gosec // index is bounded by validator set size which fits in int32
-		if vote != nil {
-			return vote, true
-		}
+		return votes.GetByIndex(int32(index)) //nolint:gosec // index is bounded by validator set size which fits in int32
 	}
-
 	return nil, false
 }
 

sei-tendermint/internal/consensus/reactor.go

@@ -468,12 +468,15 @@ func (r *Reactor) pickSendVote(ps *PeerState, votes types.VoteSetReader) bool {
 		}
 		logger.Debug("sending vote message", "ps", string(psJson), "vote", vote)
 	}
-	r.channels.vote.Send(MsgToProto(&VoteMessage{Vote: vote}), ps.peerID)
 
+	// SetHasVote can fail because ps state (in particular the bitarrays)
+	// is not verified and it depends what peer has sent us.
 	if err := ps.SetHasVote(vote); err != nil {
-		panic(fmt.Errorf("ps.SetHasVote(): %w", err))
+		logger.Info("ps.SetHasVote()", "peerID", ps.peerID, "err", err)
+		return false
 	}
 
+	r.channels.vote.Send(MsgToProto(&VoteMessage{Vote: vote}), ps.peerID)
 	return true
 }
 

sei-tendermint/internal/consensus/state.go

@@ -34,7 +34,6 @@ import (
 // Consensus sentinel errors
 var (
 	ErrInvalidProposalSignature     = errors.New("error invalid proposal signature")
-	ErrInvalidProposalPOLRound      = errors.New("error invalid proposal POL round")
 	ErrAddingVote                   = errors.New("error adding vote")
 	ErrSignatureFoundInPastBlocks   = errors.New("found signature from the same key")
 	ErrInvalidProposalPartSetHeader = errors.New("error invalid proposal part set header")
@@ -2045,7 +2044,7 @@ func (cs *State) RecordMetrics(height int64, block *types.Block) {
 
 	for _, ev := range block.Evidence {
 		if dve, ok := ev.(*types.DuplicateVoteEvidence); ok {
-			if _, val := cs.roundState.Validators().GetByAddress(dve.VoteA.ValidatorAddress); val != nil {
+			if _, val, ok := cs.roundState.Validators().GetByAddress(dve.VoteA.ValidatorAddress); ok {
 				byzantineValidatorsCount++
 				byzantineValidatorsPower += val.VotingPower
 			}
@@ -2103,7 +2102,10 @@ func (cs *State) defaultSetProposal(proposal *types.Proposal, recvTime time.Time
 	if cs.roundState.Proposal() != nil || proposal == nil {
 		return nil
 	}
-
+	// Preemptively re-verify the proposal.
+	if err := proposal.ValidateBasic(); err != nil {
+		return err
+	}
 	// Does not apply
 	if proposal.Height != cs.roundState.Height() || proposal.Round != cs.roundState.Round() {
 		return nil
@@ -2125,12 +2127,6 @@ func (cs *State) defaultSetProposal(proposal *types.Proposal, recvTime time.Time
 		}
 	}
 
-	// Verify POLRound, which must be -1 or in range [0, proposal.Round).
-	if proposal.POLRound < -1 ||
-		(proposal.POLRound >= 0 && proposal.POLRound >= proposal.Round) {
-		return ErrInvalidProposalPOLRound
-	}
-
 	p := proposal.ToProto()
 	// Verify signature
 	if err := cs.roundState.Validators().GetProposer().PubKey.Verify(
@@ -2491,7 +2487,10 @@ func (cs *State) addVote(
 	}
 	if vote.Round == cs.roundState.Round() {
 		vals := cs.state.Validators
-		_, val := vals.GetByIndex(vote.ValidatorIndex)
+		_, val, ok := vals.GetByIndex(vote.ValidatorIndex)
+		if !ok {
+			panic(fmt.Errorf("validator index %v out of range", vote.ValidatorIndex))
+		}
 		cs.metrics.MarkVoteReceived(vote.Type, val.VotingPower, vals.TotalVotingPower())
 	}
 
@@ -2619,8 +2618,10 @@ func (cs *State) signVote(
 	}
 
 	addr := privValidatorPubKey.Address()
-	valIdx, _ := cs.roundState.Validators().GetByAddress(addr)
-
+	valIdx, _, ok := cs.roundState.Validators().GetByAddress(addr)
+	if !ok {
+		panic(fmt.Errorf("validator %v not in committee", addr))
+	}
 	vote := &types.Vote{
 		ValidatorAddress: addr,
 		ValidatorIndex:   valIdx,
@@ -2748,7 +2749,10 @@ func (cs *State) calculatePrevoteMessageDelayMetrics() {
 
 	var votingPowerSeen int64
 	for _, v := range pl {
-		_, val := cs.roundState.Validators().GetByAddress(v.ValidatorAddress)
+		_, val, ok := cs.roundState.Validators().GetByAddress(v.ValidatorAddress)
+		if !ok {
+			panic(fmt.Errorf("validator %v not in committee", v.ValidatorAddress))
+		}
 		votingPowerSeen += val.VotingPower
 		if votingPowerSeen >= cs.roundState.Validators().TotalVotingPower()*2/3+1 {
 			cs.metrics.QuorumPrevoteDelay.With("proposer_address", cs.roundState.Validators().GetProposer().Address.String()).Set(v.Timestamp.Sub(cs.roundState.Proposal().Timestamp).Seconds())

sei-tendermint/internal/consensus/types/peer_round_state.go

@@ -31,6 +31,7 @@ type PeerRoundState struct {
 
 	StartTime time.Time // Estimated start of round 0 at this height
 
+	// WARNING: this only partially validated, so logic accessing it should be conservative.
 	Proposal                   bool // True if peer has proposal for this round
 	ProposalBlockPartSetHeader types.PartSetHeader
 	ProposalBlockParts         *bits.BitArray

sei-tendermint/internal/consensus/types/round_state.go

@@ -400,15 +400,18 @@ type RoundStateSimple struct {
 	Proposer          types.ValidatorInfo `json:"proposer"`
 }
 
-// Compress the RoundState to RoundStateSimple
+// Compress the RoundState to RoundStateSimple.
 func (rs *RoundState) RoundStateSimple() RoundStateSimple {
 	votesJSON, err := rs.Votes.MarshalJSON()
 	if err != nil {
 		panic(err)
 	}
 
 	addr := rs.Validators.GetProposer().Address
-	idx, _ := rs.Validators.GetByAddress(addr)
+	idx, _, ok := rs.Validators.GetByAddress(addr)
+	if !ok {
+		panic(fmt.Errorf("validator %v not in committee", addr))
+	}
 
 	return RoundStateSimple{
 		HeightRoundStep:   fmt.Sprintf("%d/%d/%d", rs.Height, rs.Round, rs.Step),
@@ -427,8 +430,10 @@ func (rs *RoundState) RoundStateSimple() RoundStateSimple {
 // NewRoundEvent returns the RoundState with proposer information as an event.
 func (rs *RoundState) NewRoundEvent() types.EventDataNewRound {
 	addr := rs.Validators.GetProposer().Address
-	idx, _ := rs.Validators.GetByAddress(addr)
-
+	idx, _, ok := rs.Validators.GetByAddress(addr)
+	if !ok {
+		panic(fmt.Errorf("validator %v not in committee", addr))
+	}
 	return types.EventDataNewRound{
 		Height: rs.Height,
 		Round:  rs.Round,

sei-tendermint/internal/evidence/verify.go

@@ -71,8 +71,10 @@ func (evpool *Pool) verify(ctx context.Context, evidence types.Evidence) error {
 			return types.NewErrInvalidEvidence(evidence, err)
 		}
 
-		_, val := valSet.GetByAddress(ev.VoteA.ValidatorAddress)
-
+		_, val, ok := valSet.GetByAddress(ev.VoteA.ValidatorAddress)
+		if !ok {
+			return fmt.Errorf("validator %v not in committee", ev.VoteA.ValidatorAddress)
+		}
 		if err := ev.ValidateABCI(val, valSet, evTime); err != nil {
 			ev.GenerateABCI(val, valSet, evTime)
 			if addErr := evpool.addPendingEvidence(ctx, ev); addErr != nil {
@@ -202,8 +204,8 @@ func VerifyLightClientAttack(e *types.LightClientAttackEvidence, commonHeader, t
 //   - the block ID's must be different
 //   - The signatures must both be valid
 func VerifyDuplicateVote(e *types.DuplicateVoteEvidence, chainID string, valSet *types.ValidatorSet) error {
-	_, val := valSet.GetByAddress(e.VoteA.ValidatorAddress)
-	if val == nil {
+	_, val, ok := valSet.GetByAddress(e.VoteA.ValidatorAddress)
+	if !ok {
 		return fmt.Errorf("address %X was not a validator at height %d", e.VoteA.ValidatorAddress, e.Height())
 	}
 	pubKey := val.PubKey