CVE-2020-15091: Improper Verification of Cryptographic Signature (CWE-347)
value: TenderMint from version 0.33.0 and before version 0.33.6 allows block proposers to include signatures for the wrong block. This may happen naturally if you start a network, have it run for some time and restart it (**without changing chainID**). A malicious block proposer (even with a minimal amount of stake) can use this vulnerability to completely halt the network. This issue is fixed in Tendermint 0.33.6 which checks all the signatures are for the block with 2/3+ majority before creating a commit.
CWE description: The product does not verify, or incorrectly verifies, the cryptographic signature for data.
CVSS v3 base score: 6.5
Published: 2020-07-02T17:15Z
Repository: https://github.com/tendermint/tendermint
Language: Go
Files changed: consensus/invalid_test.go, types/vote_set.go
Commit date: 2020-07-02 15:41:49 +0200
Commit message: consensus: Do not allow signatures for a wrong block in commits

Closes #4926

The dump consensus state had this:

      "last_commit": {
        "votes": [
          "Vote{0:04CBBF43CA3E 385085/00/2(Precommit) 1B73DA9FC4C8 42C97B86D89D @ 2020-05-27T06:46:51.042392895Z}",
          "Vote{1:055799E028FA 385085/00/2(Precommit) 652B08AD61EA 0D507D7FA3AB @ 2020-06-28T04:57:29.20793209Z}",
          "Vote{2:056024CFA910 385085/00/2(Precommit) 652B08AD61EA C8E95532A4C3 @ 2020-06-28T04:57:29.452696998Z}",
          "Vote{3:0741C95814DA 385085/00/2(Precommit) 652B08AD61EA 36D567615F7C @ 2020-06-28T04:57:29.279788593Z}",

Note there's a precommit in there from the first val from May (2020-05-27) while the rest are from today (2020-06-28). It suggests there's a validator from an old instance of the network at this height (they're using the same chain-id!). Obviously a single bad validator shouldn't be an issue. But the Commit refactor work introduced a bug.

When we propose a block, we get the block.LastCommit by calling MakeCommit on the set of precommits we saw for the last height. This set may include precommits for a different block, and hence the block.LastCommit we propose may include precommits that aren't actually for the last block (but of course +2/3 will be). Before v0.33, we just skipped over these precommits during verification. But in v0.33, we expect all signatures for a blockID to be for the same block ID! Thus we end up proposing a block that we can't verify.

Diff: diff --git a/consensus/invalid_test.go b/consensus/invalid_test.go
new file mode 100644
index 000000000..13b872f3b
--- /dev/null
+++ b/consensus/invalid_test.go
@@ -0,0 +1,97 @@
+package consensus
+
+import (
+	"testing"
+
+	"github.com/tendermint/tendermint/libs/bytes"
+	"github.com/tendermint/tendermint/libs/log"
+	tmrand "github.com/tendermint/tendermint/libs/rand"
+	"github.com/tendermint/tendermint/p2p"
+	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
+	"github.com/tendermint/tendermint/types"
+)
+
+//----------------------------------------------
+// byzantine failures
+
+// one byz val sends a precommit for a random block at each height
+// Ensure a testnet makes blocks
+func TestReactorInvalidPrecommit(t *testing.T) {
+	N := 4
+	css, cleanup := randConsensusNet(N, "consensus_reactor_test", newMockTickerFunc(true), newCounter)
+	defer cleanup()
+
+	for i := 0; i < 4; i++ {
+		ticker := NewTimeoutTicker()
+		ticker.SetLogger(css[i].Logger)
+		css[i].SetTimeoutTicker(ticker)
+
+	}
+
+	reactors, blocksSubs, eventBuses := startConsensusNet(t, css, N)
+
+	// this val sends a random precommit at each height
+	byzValIdx := 0
+	byzVal := css[byzValIdx]
+	byzR := reactors[byzValIdx]
+
+	// update the doPrevote function to just send a valid precommit for a random block
+	// and otherwise disable the priv validator
+	byzVal.mtx.Lock()
+	pv := byzVal.privValidator
+	byzVal.doPrevote = func(height int64, round int32) {
+		invalidDoPrevoteFunc(t, height, round, byzVal, byzR.Switch, pv)
+	}
+	byzVal.mtx.Unlock()
+	defer stopConsensusNet(log.TestingLogger(), reactors, eventBuses)
+
+	// wait for a bunch of blocks
+	// TODO: make this tighter by ensuring the halt happens by block 2
+	for i := 0; i < 10; i++ {
+		timeoutWaitGroup(t, N, func(j int) {
+			<-blocksSubs[j].Out()
+		}, css)
+	}
+}
+
+func invalidDoPrevoteFunc(t *testing.T, height int64, round int32, cs *State, sw *p2p.Switch, pv types.PrivValidator) {
+	// routine to:
+	// - precommit for a random block
+	// - send precommit to all peers
+	// - disable privValidator (so we don't do normal precommits)
+	go func() {
+		cs.mtx.Lock()
+		cs.privValidator = pv
+		pubKey, err := cs.privValidator.GetPubKey()
+		if err != nil {
+			panic(err)
+		}
+		addr := pubKey.Address()
+		valIndex, _ := cs.Validators.GetByAddress(addr)
+
+		// precommit a random block
+		blockHash := bytes.HexBytes(tmrand.Bytes(32))
+		precommit := &types.Vote{
+			ValidatorAddress: addr,
+			ValidatorIndex:   valIndex,
+			Height:           cs.Height,
+			Round:            cs.Round,
+			Timestamp:        cs.voteTime(),
+			Type:             tmproto.PrecommitType,
+			BlockID: types.BlockID{
+				Hash:          blockHash,
+				PartSetHeader: types.PartSetHeader{Total: 1, Hash: tmrand.Bytes(32)}},
+		}
+		p := precommit.ToProto()
+		cs.privValidator.SignVote(cs.state.ChainID, p)
+		precommit.Signature = p.Signature
+		cs.privValidator = nil // disable priv val so we don't do normal votes
+		cs.mtx.Unlock()
+
+		peers := sw.Peers().List()
+		for _, peer := range peers {
+			cs.Logger.Info("Sending bad vote", "block", blockHash, "peer", peer)
+			peer.Send(VoteChannel, MustEncode(&VoteMessage{precommit}))
+		}
+	}()
+}
diff --git a/types/vote_set.go b/types/vote_set.go
index 5c0545900..c61a3d698 100644
--- a/types/vote_set.go
+++ b/types/vote_set.go
@@ -547,13 +547,15 @@ func (voteSet *VoteSet) sumTotalFrac() (int64, int64, float64) {
 }
 
 //--------------------------------------------------------------------------------
 // Commit
 
-// MakeCommit constructs a Commit from the VoteSet.
-// Panics if the vote type is not PrecommitType or if
-// there's no +2/3 votes for a single block.
+// MakeCommit constructs a Commit from the VoteSet. It only includes precommits
+// for the block, which has 2/3+ majority, and nil.
+//
+// Panics if the vote type is not PrecommitType or if there's no +2/3 votes for
+// a single block.
 func (voteSet *VoteSet) MakeCommit() *Commit {
 	if voteSet.signedMsgType != tmproto.PrecommitType {
 		panic("Cannot MakeCommit() unless VoteSet.Type is PrecommitType")
 	}
 	voteSet.mtx.Lock()
@@ -565,11 +567,16 @@ func (voteSet *VoteSet) MakeCommit() *Commit {
 	}
 
 	// For every validator, get the precommit
 	commitSigs := make([]CommitSig, len(voteSet.votes))
 	for i, v := range voteSet.votes {
-		commitSigs[i] = v.CommitSig()
+		commitSig := v.CommitSig()
+		// if block ID exists but doesn't match, exclude sig
+		if commitSig.ForBlock() && !v.BlockID.Equals(*voteSet.maj23) {
+			commitSig = NewCommitSigAbsent()
+		}
+		commitSigs[i] = commitSig
 	}
 
 	return NewCommit(voteSet.GetHeight(), voteSet.GetRound(), *voteSet.maj23, commitSigs)
 }
 

Vulnerable code: // MakeCommit constructs a Commit from the VoteSet.
// Panics if the vote type is not PrecommitType or if
// there's no +2/3 votes for a single block.
		commitSigs[i] = v.CommitSig()
Fixed code: package consensus

import (
	"testing"

	"github.com/tendermint/tendermint/libs/bytes"
	"github.com/tendermint/tendermint/libs/log"
	tmrand "github.com/tendermint/tendermint/libs/rand"
	"github.com/tendermint/tendermint/p2p"
	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
	"github.com/tendermint/tendermint/types"
)

//----------------------------------------------
// byzantine failures

// one byz val sends a precommit for a random block at each height
// Ensure a testnet makes blocks
func TestReactorInvalidPrecommit(t *testing.T) {
	N := 4
	css, cleanup := randConsensusNet(N, "consensus_reactor_test", newMockTickerFunc(true), newCounter)
	defer cleanup()

	for i := 0; i < 4; i++ {
		ticker := NewTimeoutTicker()
		ticker.SetLogger(css[i].Logger)
		css[i].SetTimeoutTicker(ticker)

	}

	reactors, blocksSubs, eventBuses := startConsensusNet(t, css, N)

	// this val sends a random precommit at each height
	byzValIdx := 0
	byzVal := css[byzValIdx]
	byzR := reactors[byzValIdx]

	// update the doPrevote function to just send a valid precommit for a random block
	// and otherwise disable the priv validator
	byzVal.mtx.Lock()
	pv := byzVal.privValidator
	byzVal.doPrevote = func(height int64, round int32) {
		invalidDoPrevoteFunc(t, height, round, byzVal, byzR.Switch, pv)
	}
	byzVal.mtx.Unlock()
	defer stopConsensusNet(log.TestingLogger(), reactors, eventBuses)

	// wait for a bunch of blocks
	// TODO: make this tighter by ensuring the halt happens by block 2
	for i := 0; i < 10; i++ {
		timeoutWaitGroup(t, N, func(j int) {
			<-blocksSubs[j].Out()
		}, css)
	}
}

func invalidDoPrevoteFunc(t *testing.T, height int64, round int32, cs *State, sw *p2p.Switch, pv types.PrivValidator) {
	// routine to:
	// - precommit for a random block
	// - send precommit to all peers
	// - disable privValidator (so we don't do normal precommits)
	go func() {
		cs.mtx.Lock()
		cs.privValidator = pv
		pubKey, err := cs.privValidator.GetPubKey()
		if err != nil {
			panic(err)
		}
		addr := pubKey.Address()
		valIndex, _ := cs.Validators.GetByAddress(addr)

		// precommit a random block
		blockHash := bytes.HexBytes(tmrand.Bytes(32))
		precommit := &types.Vote{
			ValidatorAddress: addr,
			ValidatorIndex:   valIndex,
			Height:           cs.Height,
			Round:            cs.Round,
			Timestamp:        cs.voteTime(),
			Type:             tmproto.PrecommitType,
			BlockID: types.BlockID{
				Hash:          blockHash,
				PartSetHeader: types.PartSetHeader{Total: 1, Hash: tmrand.Bytes(32)}},
		}
		p := precommit.ToProto()
		cs.privValidator.SignVote(cs.state.ChainID, p)
		precommit.Signature = p.Signature
		cs.privValidator = nil // disable priv val so we don't do normal votes
		cs.mtx.Unlock()

		peers := sw.Peers().List()
		for _, peer := range peers {
			cs.Logger.Info("Sending bad vote", "block", blockHash, "peer", peer)
			peer.Send(VoteChannel, MustEncode(&VoteMessage{precommit}))
		}
	}()
}
// MakeCommit constructs a Commit from the VoteSet. It only includes precommits
// for the block, which has 2/3+ majority, and nil.
//
// Panics if the vote type is not PrecommitType or if there's no +2/3 votes for
// a single block.
		commitSig := v.CommitSig()
		// if block ID exists but doesn't match, exclude sig
		if commitSig.ForBlock() && !v.BlockID.Equals(*voteSet.maj23) {
			commitSig = NewCommitSigAbsent()
		}
		commitSigs[i] = commitSig