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:
0c7fd316eb/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

@@ -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)

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}
 	}

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

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")