From 5e52a6ec558f789b642a231c257f8754b97637bc Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 30 Jun 2020 11:03:58 +0400 Subject: [PATCH 1/4] types: verify commit fully Since the light client work introduced in v0.33 it appears full nodes are no longer fully verifying commit signatures during block execution - they stop after +2/3. See in VerifyCommit: https://github.com/tendermint/tendermint/blob/0c7fd316eb006c0afc13996c00ac8bde1078b32c/types/validator_set.go#L700-L703 This means proposers can propose blocks that contain valid +2/3 signatures and then the rest of the signatures can be whatever they want. They can claim that all the other validators signed just by including a CommitSig with arbitrary signature data. While this doesn't seem to impact safety of Tendermint per se, it means that Commits may contain a lot of invalid data. This is already true of blocks, since they can include invalid txs filled with garbage, but in that case the application knows they they are invalid and can punish the proposer. But since applications dont verify commit signatures directly (they trust tendermint to do that), they won't be able to detect it. This can impact incentivization logic in the application that depends on the LastCommitInfo sent in BeginBlock, which includes which validators signed. For instance, Gaia incentivizes proposers with a bonus for including more than +2/3 of the signatures. But a proposer can now claim that bonus just by including arbitrary data for the final -1/3 of validators without actually waiting for their signatures. There may be other tricks that can be played because of this. In general, the full node should be a fully verifying machine. While it's true that the light client can avoid verifying all signatures by stopping after +2/3, the full node can not. Thus the light client and full node should use distinct VerifyCommit functions if one is going to stop after +2/3 or otherwise perform less validation (for instance light clients can also skip verifying votes for nil while full nodes can not). See a commit with a bad signature that verifies here: 56367fd. From what I can tell, Tendermint will go on to think this commit is valid and forward this data to the app, so the app will think the second validator actually signed when it clearly did not. --- lite2/client.go | 4 +- lite2/verifier.go | 6 +- types/validator_set.go | 82 +++++++++++++--- types/validator_set_test.go | 182 ++++++++++++++++++++++++++---------- 4 files changed, 206 insertions(+), 68 deletions(-) diff --git a/lite2/client.go b/lite2/client.go index 08561a2c9..03c7f7a70 100644 --- a/lite2/client.go +++ b/lite2/client.go @@ -387,7 +387,7 @@ func (c *Client) initializeWithTrustOptions(options TrustOptions) error { } // Ensure that +2/3 of validators signed correctly. - err = vals.VerifyCommit(c.chainID, h.Commit.BlockID, h.Height, h.Commit) + err = vals.VerifyCommitLight(c.chainID, h.Commit.BlockID, h.Height, h.Commit) if err != nil { return fmt.Errorf("invalid commit: %w", err) } @@ -954,7 +954,7 @@ func (c *Client) compareNewHeaderWithWitnesses(h *types.SignedHeader) error { } if !bytes.Equal(h.Hash(), altH.Hash()) { - if err = c.latestTrustedVals.VerifyCommitTrusting(c.chainID, altH.Commit.BlockID, + if err = c.latestTrustedVals.VerifyCommitLightTrusting(c.chainID, altH.Commit.BlockID, altH.Height, altH.Commit, c.trustLevel); err != nil { c.logger.Error("Witness sent us incorrect header", "err", err, "witness", witness) witnessesToRemove = append(witnessesToRemove, i) diff --git a/lite2/verifier.go b/lite2/verifier.go index 1ef54677b..d754e9e7b 100644 --- a/lite2/verifier.go +++ b/lite2/verifier.go @@ -57,7 +57,7 @@ func VerifyNonAdjacent( } // Ensure that +`trustLevel` (default 1/3) or more of last trusted validators signed correctly. - err := trustedVals.VerifyCommitTrusting(chainID, untrustedHeader.Commit.BlockID, untrustedHeader.Height, + err := trustedVals.VerifyCommitLightTrusting(chainID, untrustedHeader.Commit.BlockID, untrustedHeader.Height, untrustedHeader.Commit, trustLevel) if err != nil { switch e := err.(type) { @@ -73,7 +73,7 @@ func VerifyNonAdjacent( // NOTE: this should always be the last check because untrustedVals can be // intentionally made very large to DOS the light client. not the case for // VerifyAdjacent, where validator set is known in advance. - if err := untrustedVals.VerifyCommit(chainID, untrustedHeader.Commit.BlockID, untrustedHeader.Height, + if err := untrustedVals.VerifyCommitLight(chainID, untrustedHeader.Commit.BlockID, untrustedHeader.Height, untrustedHeader.Commit); err != nil { return ErrInvalidHeader{err} } @@ -128,7 +128,7 @@ func VerifyAdjacent( } // Ensure that +2/3 of new validators signed correctly. - if err := untrustedVals.VerifyCommit(chainID, untrustedHeader.Commit.BlockID, untrustedHeader.Height, + if err := untrustedVals.VerifyCommitLight(chainID, untrustedHeader.Commit.BlockID, untrustedHeader.Height, untrustedHeader.Commit); err != nil { return ErrInvalidHeader{err} } diff --git a/types/validator_set.go b/types/validator_set.go index ecfe865a3..f4f11fcc6 100644 --- a/types/validator_set.go +++ b/types/validator_set.go @@ -627,6 +627,12 @@ func (vals *ValidatorSet) UpdateWithChangeSet(changes []*Validator) error { } // VerifyCommit verifies +2/3 of the set had signed the given commit. +// +// It checks all the signatures! While it's safe to exit as soon as we have +// 2/3+ signatures, doing so would impact incentivization logic in the ABCI +// application that depends on the LastCommitInfo sent in BeginBlock, which +// includes which validators signed. For instance, Gaia incentivizes proposers +// with a bonus for including more than +2/3 of the signatures. func (vals *ValidatorSet) VerifyCommit(chainID string, blockID BlockID, height int64, commit *Commit) error { @@ -661,6 +667,58 @@ func (vals *ValidatorSet) VerifyCommit(chainID string, blockID BlockID, // It's OK that the BlockID doesn't match. We include stray // signatures (~votes for nil) to measure validator availability. // } + } + + if got, needed := talliedVotingPower, votingPowerNeeded; got <= needed { + return ErrNotEnoughVotingPowerSigned{Got: got, Needed: needed} + } + + return nil +} + +/////////////////////////////////////////////////////////////////////////////// +// LIGHT CLIENT VERIFICATION METHODS +/////////////////////////////////////////////////////////////////////////////// + +// VerifyCommitLight verifies +2/3 of the set had signed the given commit. +// +// This method is primarily used by the light client and does not check all the +// signatures. +func (vals *ValidatorSet) VerifyCommitLight(chainID string, blockID BlockID, + height int64, commit *Commit) error { + + if vals.Size() != len(commit.Signatures) { + return NewErrInvalidCommitSignatures(vals.Size(), len(commit.Signatures)) + } + + // Validate Height and BlockID. + if height != commit.Height { + return NewErrInvalidCommitHeight(height, commit.Height) + } + if !blockID.Equals(commit.BlockID) { + return fmt.Errorf("invalid commit -- wrong block ID: want %v, got %v", + blockID, commit.BlockID) + } + + talliedVotingPower := int64(0) + votingPowerNeeded := vals.TotalVotingPower() * 2 / 3 + for idx, commitSig := range commit.Signatures { + // No need to verify absent or nil votes. + if !commitSig.ForBlock() { + continue + } + + // The vals and commit have a 1-to-1 correspondance. + // This means we don't need the validator address or to do any lookup. + val := vals.Validators[idx] + + // Validate signature. + voteSignBytes := commit.VoteSignBytes(chainID, idx) + if !val.PubKey.VerifyBytes(voteSignBytes, commitSig.Signature) { + return fmt.Errorf("wrong signature (#%d): %X", idx, commitSig.Signature) + } + + talliedVotingPower += val.VotingPower // return as soon as +2/3 of the signatures are verified if talliedVotingPower > votingPowerNeeded { @@ -668,7 +726,6 @@ func (vals *ValidatorSet) VerifyCommit(chainID string, blockID BlockID, } } - // talliedVotingPower <= needed, thus return error return ErrNotEnoughVotingPowerSigned{Got: talliedVotingPower, Needed: votingPowerNeeded} } @@ -748,14 +805,17 @@ func (vals *ValidatorSet) VerifyFutureCommit(newSet *ValidatorSet, chainID strin return nil } -// VerifyCommitTrusting verifies that trustLevel ([1/3, 1]) of the validator -// set signed this commit. +// VerifyCommitLightTrusting verifies that trustLevel of the validator set signed +// this commit. +// +// This method is primarily used by the light client and does not check all the +// signatures. // // NOTE the given validators do not necessarily correspond to the validator set // for this commit, but there may be some intersection. // // Panics if trustLevel is invalid. -func (vals *ValidatorSet) VerifyCommitTrusting(chainID string, blockID BlockID, +func (vals *ValidatorSet) VerifyCommitLightTrusting(chainID string, blockID BlockID, height int64, commit *Commit, trustLevel tmmath.Fraction) error { // sanity check @@ -781,8 +841,9 @@ func (vals *ValidatorSet) VerifyCommitTrusting(chainID string, blockID BlockID, votingPowerNeeded := totalVotingPowerMulByNumerator / trustLevel.Denominator for idx, commitSig := range commit.Signatures { - if commitSig.Absent() { - continue // OK, some signatures can be absent. + // No need to verify absent or nil votes. + if !commitSig.ForBlock() { + continue } // We don't know the validators that committed this block, so we have to @@ -803,14 +864,7 @@ func (vals *ValidatorSet) VerifyCommitTrusting(chainID string, blockID BlockID, return errors.Errorf("wrong signature (#%d): %X", idx, commitSig.Signature) } - // Good! - if blockID.Equals(commitSig.BlockID(commit.BlockID)) { - talliedVotingPower += val.VotingPower - } - // else { - // It's OK that the BlockID doesn't match. We include stray - // signatures (~votes for nil) to measure validator availability. - // } + talliedVotingPower += val.VotingPower if talliedVotingPower > votingPowerNeeded { return nil diff --git a/types/validator_set_test.go b/types/validator_set_test.go index b28f138d8..059d043df 100644 --- a/types/validator_set_test.go +++ b/types/validator_set_test.go @@ -17,7 +17,6 @@ import ( "github.com/tendermint/tendermint/crypto/ed25519" tmmath "github.com/tendermint/tendermint/libs/math" tmrand "github.com/tendermint/tendermint/libs/rand" - tmtime "github.com/tendermint/tendermint/types/time" ) func TestValidatorSetBasic(t *testing.T) { @@ -584,62 +583,147 @@ func TestSafeSubClip(t *testing.T) { //------------------------------------------------------------------- -func TestValidatorSetVerifyCommit(t *testing.T) { - privKey := ed25519.GenPrivKey() - pubKey := privKey.PubKey() - v1 := NewValidator(pubKey, 1000) - vset := NewValidatorSet([]*Validator{v1}) - - // good +// Check VerifyCommit, VerifyCommitLight and VerifyCommitLightTrusting basic +// verification. +func TestValidatorSet_VerifyCommit_All(t *testing.T) { var ( - chainID = "mychainID" - blockID = makeBlockIDRandom() - height = int64(5) + privKey = ed25519.GenPrivKey() + pubKey = privKey.PubKey() + v1 = NewValidator(pubKey, 1000) + vset = NewValidatorSet([]*Validator{v1}) + + chainID = "Lalande21185" ) - vote := &Vote{ - ValidatorAddress: v1.Address, - ValidatorIndex: 0, - Height: height, - Round: 0, - Timestamp: tmtime.Now(), - Type: PrecommitType, - BlockID: blockID, - } + + vote := examplePrecommit() + vote.ValidatorAddress = pubKey.Address() sig, err := privKey.Sign(vote.SignBytes(chainID)) assert.NoError(t, err) vote.Signature = sig - commit := NewCommit(vote.Height, vote.Round, blockID, []CommitSig{vote.CommitSig()}) - // bad - var ( - badChainID = "notmychainID" - badBlockID = BlockID{Hash: []byte("goodbye")} - badHeight = height + 1 - badCommit = NewCommit(badHeight, 0, blockID, []CommitSig{{BlockIDFlag: BlockIDFlagAbsent}}) - ) + commit := NewCommit(vote.Height, vote.Round, vote.BlockID, []CommitSig{vote.CommitSig()}) - // test some error cases - // TODO: test more cases! - cases := []struct { - chainID string - blockID BlockID - height int64 - commit *Commit + vote2 := *vote + sig2, err := privKey.Sign(vote2.SignBytes("EpsilonEridani")) + require.NoError(t, err) + vote2.Signature = sig2 + + testCases := []struct { + description string + chainID string + blockID BlockID + height int64 + commit *Commit + expErr bool }{ - {badChainID, blockID, height, commit}, - {chainID, badBlockID, height, commit}, - {chainID, blockID, badHeight, commit}, - {chainID, blockID, height, badCommit}, + {"good", chainID, vote.BlockID, vote.Height, commit, false}, + + {"wrong signature (#0)", "EpsilonEridani", vote.BlockID, vote.Height, commit, true}, + {"wrong block ID", chainID, makeBlockIDRandom(), vote.Height, commit, true}, + {"wrong height", chainID, vote.BlockID, vote.Height - 1, commit, true}, + + {"wrong set size: 1 vs 0", chainID, vote.BlockID, vote.Height, + NewCommit(vote.Height, vote.Round, vote.BlockID, []CommitSig{}), true}, + + {"wrong set size: 1 vs 2", chainID, vote.BlockID, vote.Height, + NewCommit(vote.Height, vote.Round, vote.BlockID, + []CommitSig{vote.CommitSig(), {BlockIDFlag: BlockIDFlagAbsent}}), true}, + + {"insufficient voting power: got 0, needed more than 666", chainID, vote.BlockID, vote.Height, + NewCommit(vote.Height, vote.Round, vote.BlockID, []CommitSig{{BlockIDFlag: BlockIDFlagAbsent}}), true}, + + {"wrong signature (#0)", chainID, vote.BlockID, vote.Height, + NewCommit(vote.Height, vote.Round, vote.BlockID, []CommitSig{vote2.CommitSig()}), true}, } - for i, c := range cases { - err := vset.VerifyCommit(c.chainID, c.blockID, c.height, c.commit) - assert.NotNil(t, err, i) + for _, tc := range testCases { + tc := tc + t.Run(tc.description, func(t *testing.T) { + err := vset.VerifyCommit(tc.chainID, tc.blockID, tc.height, tc.commit) + if tc.expErr { + if assert.Error(t, err, "VerifyCommit") { + assert.Contains(t, err.Error(), tc.description, "VerifyCommit") + } + } else { + assert.NoError(t, err, "VerifyCommit") + } + + err = vset.VerifyCommitLight(tc.chainID, tc.blockID, tc.height, tc.commit) + if tc.expErr { + if assert.Error(t, err, "VerifyCommitLight") { + assert.Contains(t, err.Error(), tc.description, "VerifyCommitLight") + } + } else { + assert.NoError(t, err, "VerifyCommitLight") + } + + }) + } +} + +func TestValidatorSet_VerifyCommit_CheckAllSignatures(t *testing.T) { + var ( + chainID = "test_chain_id" + h = int64(3) + blockID = makeBlockIDRandom() + ) + + voteSet, valSet, vals := randVoteSet(h, 0, PrecommitType, 4, 10) + commit, err := MakeCommit(blockID, h, 0, voteSet, vals, time.Now()) + require.NoError(t, err) + + // malleate 4th signature + vote := voteSet.GetByIndex(3) + err = vals[3].SignVote("CentaurusA", vote) + require.NoError(t, err) + commit.Signatures[3] = vote.CommitSig() + + err = valSet.VerifyCommit(chainID, blockID, h, commit) + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "wrong signature (#3)") } +} - // test a good one - err = vset.VerifyCommit(chainID, blockID, height, commit) - assert.Nil(t, err) +func TestValidatorSet_VerifyCommitLight_ReturnsAsSoonAsMajorityOfVotingPowerSigned(t *testing.T) { + var ( + chainID = "test_chain_id" + h = int64(3) + blockID = makeBlockIDRandom() + ) + + voteSet, valSet, vals := randVoteSet(h, 0, PrecommitType, 4, 10) + commit, err := MakeCommit(blockID, h, 0, voteSet, vals, time.Now()) + require.NoError(t, err) + + // malleate 4th signature (3 signatures are enough for 2/3+) + vote := voteSet.GetByIndex(3) + err = vals[3].SignVote("CentaurusA", vote) + require.NoError(t, err) + commit.Signatures[3] = vote.CommitSig() + + err = valSet.VerifyCommitLight(chainID, blockID, h, commit) + assert.NoError(t, err) +} + +func TestValidatorSet_VerifyCommitLightTrusting_ReturnsAsSoonAsTrustLevelOfVotingPowerSigned(t *testing.T) { + var ( + chainID = "test_chain_id" + h = int64(3) + blockID = makeBlockIDRandom() + ) + + voteSet, valSet, vals := randVoteSet(h, 0, PrecommitType, 4, 10) + commit, err := MakeCommit(blockID, h, 0, voteSet, vals, time.Now()) + require.NoError(t, err) + + // malleate 3rd signature (2 signatures are enough for 1/3+ trust level) + vote := voteSet.GetByIndex(2) + err = vals[2].SignVote("CentaurusA", vote) + require.NoError(t, err) + commit.Signatures[2] = vote.CommitSig() + + err = valSet.VerifyCommitLightTrusting(chainID, blockID, h, commit, tmmath.Fraction{Numerator: 1, Denominator: 3}) + assert.NoError(t, err) } func TestEmptySet(t *testing.T) { @@ -1326,7 +1410,7 @@ func TestValSetUpdateOverflowRelated(t *testing.T) { } } -func TestVerifyCommitTrusting(t *testing.T) { +func TestValidatorSet_VerifyCommitLightTrusting(t *testing.T) { var ( blockID = makeBlockIDRandom() voteSet, originalValset, vals = randVoteSet(1, 1, PrecommitType, 6, 1) @@ -1357,7 +1441,7 @@ func TestVerifyCommitTrusting(t *testing.T) { } for _, tc := range testCases { - err = tc.valSet.VerifyCommitTrusting("test_chain_id", blockID, commit.Height, commit, + err = tc.valSet.VerifyCommitLightTrusting("test_chain_id", blockID, commit.Height, commit, tmmath.Fraction{Numerator: 1, Denominator: 3}) if tc.err { assert.Error(t, err) @@ -1367,7 +1451,7 @@ func TestVerifyCommitTrusting(t *testing.T) { } } -func TestVerifyCommitTrustingErrorsOnOverflow(t *testing.T) { +func TestValidatorSet_VerifyCommitLightTrustingErrorsOnOverflow(t *testing.T) { var ( blockID = makeBlockIDRandom() voteSet, valSet, vals = randVoteSet(1, 1, PrecommitType, 1, MaxTotalVotingPower) @@ -1375,7 +1459,7 @@ func TestVerifyCommitTrustingErrorsOnOverflow(t *testing.T) { ) require.NoError(t, err) - err = valSet.VerifyCommitTrusting("test_chain_id", blockID, commit.Height, commit, + err = valSet.VerifyCommitLightTrusting("test_chain_id", blockID, commit.Height, commit, tmmath.Fraction{Numerator: 25, Denominator: 55}) if assert.Error(t, err) { assert.Contains(t, err.Error(), "int64 overflow") From 8ccfdb96d05f8e5f563dd5afa6db3c703f3afa6f Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 30 Jun 2020 11:43:51 +0400 Subject: [PATCH 2/4] 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. --- consensus/invalid_test.go | 94 +++++++++++++++++++++++++++++++++++++++ types/vote_set.go | 15 +++++-- 2 files changed, 105 insertions(+), 4 deletions(-) create mode 100644 consensus/invalid_test.go diff --git a/consensus/invalid_test.go b/consensus/invalid_test.go new file mode 100644 index 000000000..1ae0a69ec --- /dev/null +++ b/consensus/invalid_test.go @@ -0,0 +1,94 @@ +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" + "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 int) { + 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 int, 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: types.PrecommitType, + BlockID: types.BlockID{ + Hash: blockHash, + PartsHeader: types.PartSetHeader{Total: 1, Hash: tmrand.Bytes(32)}}, + } + cs.privValidator.SignVote(cs.state.ChainID, precommit) + 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, cdc.MustMarshalBinaryBare(&VoteMessage{precommit})) + } + }() +} diff --git a/types/vote_set.go b/types/vote_set.go index 82698fe51..6ff6e02a5 100644 --- a/types/vote_set.go +++ b/types/vote_set.go @@ -547,9 +547,11 @@ 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 != PrecommitType { panic("Cannot MakeCommit() unless VoteSet.Type is PrecommitType") @@ -565,7 +567,12 @@ 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) From cefeab0fbbbf999035cd065e2142f30714cce3bf Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 30 Jun 2020 15:45:28 +0400 Subject: [PATCH 3/4] update changelog and bump version --- CHANGELOG.md | 60 +++++++++++++++++++++++++++++++++++++++++++- CHANGELOG_PENDING.md | 2 +- version/version.go | 2 +- 3 files changed, 61 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d6bd16ecf..6ef2dd62d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,8 +1,66 @@ # Changelog +## v0.33.6 + +*July 1, 2020* + +This security release fixes: + +### Denial of service + +Tendermint 0.33.0 and above allow 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**). Correct block +proposers will accidentally include signatures for the wrong block if they see +these signatures, and then commits won't validate, making all proposed blocks +invalid. A malicious validator (even with a minimal amount of stake) can use +this vulnerability to completely halt the network. + +Tendermint 0.33.6 checks all the signatures are for the block with 2/3+ +majority before creating a commit. + +### False Witness + +Tendermint 0.33.1 and above are no longer fully verifying commit signatures +during block execution - they stop after 2/3+. This means proposers can propose +blocks that contain valid +2/3 signatures and then the rest of the signatures +can be whatever they want. They can claim that all the other validators signed +just by including a CommitSig with arbitrary signature data. While this doesn't +seem to impact safety of Tendermint per se, it means that Commits may contain a +lot of invalid data. + +_This is already true of blocks, since they can include invalid txs filled +with garbage, but in that case the application knows they they are invalid and +can punish the proposer. But since applications dont verify commit signatures +directly (they trust tendermint to do that), they won't be able to detect it._ + +This can impact incentivization logic in the application that depends on the +LastCommitInfo sent in BeginBlock, which includes which validators signed. For +instance, Gaia incentivizes proposers with a bonus for including more than +2/3 +of the signatures. But a proposer can now claim that bonus just by including +arbitrary data for the final -1/3 of validators without actually waiting for +their signatures. There may be other tricks that can be played because of this. + +Tendermint 0.33.6 verifies all the signatures during block execution. + +_the light client does not check nil votes and exits as soon as 2/3+ of the +signatures are checked_ + +**All clients are recommended to upgrade** + +Special thanks to @njmurarka for reporting this. + +Friendly reminder, we have a [bug bounty +program](https://hackerone.com/tendermint). + +### SECURITY: + +- [consensus] Do not allow signatures for a wrong block in commits (@ebuchman) +- [consensus] Verify all the signatures during block execution (@melekes) + ## v.0.33.5 -Special thanks to our external contributor on this release: @tau3 +Special thanks to our external contributor on this release: @tau3 Friendly reminder: We have a [bug bounty program](https://hackerone.com/tendermint). diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 53e8e5ca3..ca710ba8c 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -1,4 +1,4 @@ -## v0.33.6 +## v0.33.7 \*\* diff --git a/version/version.go b/version/version.go index 7dc5ad2f7..06012e15f 100644 --- a/version/version.go +++ b/version/version.go @@ -21,7 +21,7 @@ const ( // XXX: Don't change the name of this variable or you will break // automation :) - TMCoreSemVer = "0.33.5" + TMCoreSemVer = "0.33.6" // ABCISemVer is the semantic version of the ABCI library ABCISemVer = "0.16.2" From 606d0a89ccabbd3e59cff521f9f4d875cc366ac9 Mon Sep 17 00:00:00 2001 From: Tess Rinearson Date: Thu, 2 Jul 2020 15:21:01 +0200 Subject: [PATCH 4/4] changelog: tweak 0.33.6 entry --- CHANGELOG.md | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ef2dd62d..bdb3e1c6d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## v0.33.6 -*July 1, 2020* +*July 2, 2020* This security release fixes: @@ -10,29 +10,31 @@ This security release fixes: Tendermint 0.33.0 and above allow 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**). Correct block -proposers will accidentally include signatures for the wrong block if they see -these signatures, and then commits won't validate, making all proposed blocks -invalid. A malicious validator (even with a minimal amount of stake) can use -this vulnerability to completely halt the network. - -Tendermint 0.33.6 checks all the signatures are for the block with 2/3+ +some time and restart it **without changing the chainID**. (It is a +[misconfiguration](https://docs.tendermint.com/master/tendermint-core/using-tendermint.html) +to reuse chainIDs.) Correct block proposers will accidentally include signatures +for the wrong block if they see these signatures, and then commits won't validate, +making all proposed blocks invalid. A malicious validator (even with a minimal +amount of stake) can use this vulnerability to completely halt the network. + +Tendermint 0.33.6 checks all the signatures are for the block with +2/3 majority before creating a commit. ### False Witness Tendermint 0.33.1 and above are no longer fully verifying commit signatures -during block execution - they stop after 2/3+. This means proposers can propose +during block execution - they stop after +2/3. This means proposers can propose blocks that contain valid +2/3 signatures and then the rest of the signatures can be whatever they want. They can claim that all the other validators signed just by including a CommitSig with arbitrary signature data. While this doesn't seem to impact safety of Tendermint per se, it means that Commits may contain a lot of invalid data. -_This is already true of blocks, since they can include invalid txs filled -with garbage, but in that case the application knows they they are invalid and -can punish the proposer. But since applications dont verify commit signatures -directly (they trust tendermint to do that), they won't be able to detect it._ +_This was already true of blocks, since they could include invalid txs filled +with garbage, but in that case the application knew that they are invalid and +could punish the proposer. But since applications didn't--and don't-- +verify commit signatures directly (they trust Tendermint to do that), +they won't be able to detect it._ This can impact incentivization logic in the application that depends on the LastCommitInfo sent in BeginBlock, which includes which validators signed. For @@ -43,12 +45,12 @@ their signatures. There may be other tricks that can be played because of this. Tendermint 0.33.6 verifies all the signatures during block execution. -_the light client does not check nil votes and exits as soon as 2/3+ of the -signatures are checked_ +_Please note that the light client does not check nil votes and exits as soon +as 2/3+ of the signatures are checked._ -**All clients are recommended to upgrade** +**All clients are recommended to upgrade.** -Special thanks to @njmurarka for reporting this. +Special thanks to @njmurarka at Bluzelle Networks for reporting this. Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermint).