consensus: reject oversized proposals

Validate proposal part counts and add tests.

Author: maxim-inj

CHANGELOG.md

@@ -15,6 +15,8 @@ encouraged to upgrade as soon as possible.
   Cap `SynchronyParams.MessageDelay` to 24hrs.
   Cap `SynchronyParams.Precision` to 30 sec.
   ([\#4815](https://github.com/cometbft/cometbft/issues/4815))
+- `[consensus]` Reject oversized proposals
+  ([\#5324](https://github.com/cometbft/cometbft/pull/5324))
 - `[crypto/bls12381]` Fix JSON marshal of private key
   ([\#4772](https://github.com/cometbft/cometbft/pull/4772))
 - `[crypto/bls12381]` Modify `Sign`, `Verify` to use `dstMinPk`
@@ -1210,4 +1212,3 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/cosmos).
 ## Previous changes
 
 For changes released before the creation of CometBFT, please refer to the Tendermint Core [CHANGELOG.md](https://github.com/tendermint/tendermint/blob/a9feb1c023e172b542c972605311af83b777855b/CHANGELOG.md).
-

internal/consensus/byzantine_test.go

@@ -593,3 +593,87 @@ func (br *ByzantineReactor) Receive(e p2p.Envelope) {
 }
 
 func (*ByzantineReactor) InitPeer(peer p2p.Peer) p2p.Peer { return peer }
+
+// Large/oversized proposals should be rejected.
+func TestRejectOversizedProposals(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	n := 2
+	css, cleanup := randConsensusNet(t, n, "consensus_reactor_test", newMockTickerFunc(false), newKVStore)
+	defer cleanup()
+
+	switches := make([]*p2p.Switch, n)
+	p2pLogger := consensusLogger().With("module", "p2p")
+	for i := 0; i < n; i++ {
+		switches[i] = p2p.MakeSwitch(
+			config.P2P,
+			i,
+			func(_ int, sw *p2p.Switch) *p2p.Switch {
+				return sw
+			})
+		switches[i].SetLogger(p2pLogger.With("validator", i))
+	}
+
+	reactors := make([]p2p.Reactor, n)
+	for i := 0; i < n; i++ {
+		conR := NewReactor(css[i], false)
+		defer func() { require.NoError(t, conR.Stop()) }()
+
+		conR.SetLogger(consensusLogger().With("validator", i))
+		reactors[i] = conR
+	}
+
+	p2p.MakeConnectedSwitches(config.P2P, n, func(i int, _ *p2p.Switch) *p2p.Switch {
+		switches[i].AddReactor("CONSENSUS", reactors[i])
+		return switches[i]
+	}, p2p.Connect2Switches)
+
+	peers := switches[0].Peers().Copy()
+	require.NotEmpty(t, peers)
+	targetPeer := peers[0]
+
+	height := int64(1)
+	round := int32(0)
+	cs := css[0]
+
+	block, err := cs.createProposalBlock(ctx)
+	require.NoError(t, err)
+
+	blockParts, err := block.MakePartSet(types.BlockPartSizeBytes)
+	require.NoError(t, err)
+
+	// create oversized proposal
+	propBlockID := types.BlockID{Hash: block.Hash(), PartSetHeader: blockParts.Header()}
+	propBlockID.PartSetHeader.Total = 4294967295
+
+	proposal := types.NewProposal(height, round, -1, propBlockID, block.Header.Time)
+	p := proposal.ToProto()
+	if err := cs.privValidator.SignProposal(cs.state.ChainID, p); err != nil {
+		t.Error(err)
+	}
+	proposal.Signature = p.Signature
+
+	success := targetPeer.Send(p2p.Envelope{
+		ChannelID: DataChannel,
+		Message:   &cmtcons.Proposal{Proposal: *proposal.ToProto()},
+	})
+	require.True(t, success)
+
+	select {
+	case e := <-css[1].peerMsgQueue:
+		// if we receive a message here, the peer incorrectly accepted the
+		// oversized proposal
+		if _, receivedProposal := e.Msg.(*ProposalMessage); receivedProposal {
+			assert.Fail(t, "peer incorrectly accepted oversized proposal")
+			return
+		}
+		// invalid state, we received some other unexpected message type, fail
+		// the test
+		assert.Fail(t, "received unexpected message type on peer msg queue, expected *ProposalMessage")
+	case <-ctx.Done():
+	case <-time.After(500 * time.Millisecond):
+		// timeout after 500ms if nothing has happened and assume peer rejected
+		// the proposal
+	}
+}

internal/consensus/reactor.go

@@ -332,6 +332,15 @@ func (conR *Reactor) Receive(e p2p.Envelope) {
 		}
 		switch msg := msg.(type) {
 		case *ProposalMessage:
+			conR.conS.mtx.RLock()
+			maxBytes := conR.conS.state.ConsensusParams.Block.MaxBytes
+			conR.conS.mtx.RUnlock()
+			if err := msg.Proposal.ValidateBlockSize(maxBytes); err != nil {
+				conR.Logger.Error("Rejecting oversized proposal", "peer", e.Src, "height", msg.Proposal.Height)
+				conR.Switch.StopPeerForError(e.Src, ErrProposalTooManyParts)
+				return
+			}
+
 			ps.SetHasProposal(msg.Proposal)
 			conR.conS.peerMsgQueue <- msgInfo{msg, e.Src.ID(), cmttime.Now()}
 		case *ProposalPOLMessage:
@@ -1793,6 +1802,12 @@ func (m *ProposalMessage) ValidateBasic() error {
 	return m.Proposal.ValidateBasic()
 }
 
+// ValidateBlockSize validates the proposal's block size against a maximum. If
+// -1 is passed, types.MaxBlockSizeBytes will be used as the maximum.
+func (m *ProposalMessage) ValidateBlockSize(maxBlockSizeBytes int64) error {
+	return m.Proposal.ValidateBlockSize(maxBlockSizeBytes)
+}
+
 // String returns a string representation.
 func (m *ProposalMessage) String() string {
 	return fmt.Sprintf("[Proposal %v]", m.Proposal)

types/proposal.go

@@ -82,6 +82,21 @@ func (p *Proposal) ValidateBasic() error {
 	return nil
 }
 
+// ValidateBlockSize ensures a proposal block is not larger than a maximum size,
+// based on the total number of parts reported in the PartSetHeader. If -1 is
+// passed as the maxBlockSizeBytes, MaxBlockSizeBytes will be used.
+func (p *Proposal) ValidateBlockSize(maxBlockSizeBytes int64) error {
+	if maxBlockSizeBytes == -1 {
+		maxBlockSizeBytes = int64(MaxBlockSizeBytes)
+	}
+	totalParts := int64(p.BlockID.PartSetHeader.Total)
+	maxParts := (maxBlockSizeBytes-1)/int64(BlockPartSizeBytes) + 1
+	if totalParts > maxParts {
+		return fmt.Errorf("proposal has too many parts %d (max: %d)", totalParts, maxParts)
+	}
+	return nil
+}
+
 // IsTimely validates that the proposal timestamp is 'timely' according to the
 // proposer-based timestamp algorithm. To evaluate if a proposal is timely, its
 // timestamp is compared to the local time of the validator when it receives

types/proposal_test.go

@@ -221,6 +221,33 @@ func TestProposalProtoBuf(t *testing.T) {
 	}
 }
 
+func TestProposalValidateBlockSize(t *testing.T) {
+	testCases := []struct {
+		testName     string
+		maxBlockSize int64
+		proposal     *Proposal
+		expectPass   bool
+	}{
+		{"10 chunk max, 5 chunk proposal, success", int64(10 * BlockPartSizeBytes), NewProposal(0, 0, 0, BlockID{PartSetHeader: PartSetHeader{Total: 5}}, cmttime.Now()), true},
+		{"10 chunk max, 20 chunk proposal, fail", int64(10 * BlockPartSizeBytes), NewProposal(0, 0, 0, BlockID{PartSetHeader: PartSetHeader{Total: 20}}, cmttime.Now()), false},
+		{"10 chunk max, max uint32 chunk proposal, fail", int64(10 * BlockPartSizeBytes), NewProposal(0, 0, 0, BlockID{PartSetHeader: PartSetHeader{Total: math.MaxUint32}}, cmttime.Now()), false},
+		{"-1 chunk max, max uint32 chunk proposal, fail", -1, NewProposal(0, 0, 0, BlockID{PartSetHeader: PartSetHeader{Total: math.MaxUint32}}, cmttime.Now()), false},
+		{"0 chunk max, max uint32 chunk proposal, fail", -1, NewProposal(0, 0, 0, BlockID{PartSetHeader: PartSetHeader{Total: math.MaxUint32}}, cmttime.Now()), false},
+		{"total parts equals chunk max, success", -1, NewProposal(0, 0, 0, BlockID{PartSetHeader: PartSetHeader{Total: 1600}}, cmttime.Now()), true},
+	}
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.testName, func(t *testing.T) {
+			err := tc.proposal.ValidateBlockSize(tc.maxBlockSize)
+			if tc.expectPass {
+				require.NoError(t, err)
+			} else {
+				require.Error(t, err)
+			}
+		})
+	}
+}
+
 func TestProposalIsTimely(t *testing.T) {
 	timestamp, err := time.Parse(time.RFC3339, "2019-03-13T23:00:00Z")
 	require.NoError(t, err)

types/validator_set_test.go

@@ -477,13 +477,14 @@ func TestValidatorSetFromProtoReturnsErrorOnOverflow(t *testing.T) {
 	pubKey := ed25519.GenPrivKey().PubKey()
 	pkProto, err := cryptoenc.PubKeyToProto(pubKey)
 	require.NoError(t, err)
+	pkProtoPtr := &pkProto
 
 	protoVals := &cmtproto.ValidatorSet{
 		Validators: []*cmtproto.Validator{
-			{Address: pubKey.Address(), PubKey: pkProto, VotingPower: math.MaxInt64, ProposerPriority: 0},
-			{Address: pubKey.Address(), PubKey: pkProto, VotingPower: math.MaxInt64, ProposerPriority: 0},
+			{Address: pubKey.Address(), PubKey: pkProtoPtr, VotingPower: math.MaxInt64, ProposerPriority: 0},
+			{Address: pubKey.Address(), PubKey: pkProtoPtr, VotingPower: math.MaxInt64, ProposerPriority: 0},
 		},
-		Proposer: &cmtproto.Validator{Address: pubKey.Address(), PubKey: pkProto, VotingPower: math.MaxInt64, ProposerPriority: 0},
+		Proposer: &cmtproto.Validator{Address: pubKey.Address(), PubKey: pkProtoPtr, VotingPower: math.MaxInt64, ProposerPriority: 0},
 	}
 
 	_, err = ValidatorSetFromProto(protoVals)