From 29f8bb5dbf51091f6397c8772fa871e916f55e34 Mon Sep 17 00:00:00 2001 From: lklimek <842586+lklimek@users.noreply.github.com> Date: Mon, 12 Aug 2024 17:51:12 +0700 Subject: [PATCH] fix: non-active validators can't verify evidence signatures (#865) * fix: don't verify evidence signatures on non-validators * fix: don't accept evidence on non-validator hosts * chore: really drop msg --- internal/evidence/pool.go | 7 ++++++ internal/evidence/pool_test.go | 1 + internal/evidence/reactor.go | 13 +++++++++++ internal/evidence/verify.go | 37 +++++++++++++++++++------------- internal/evidence/verify_test.go | 4 ++-- 5 files changed, 45 insertions(+), 17 deletions(-) diff --git a/internal/evidence/pool.go b/internal/evidence/pool.go index 52dab70139..8d8a9970c0 100644 --- a/internal/evidence/pool.go +++ b/internal/evidence/pool.go @@ -544,6 +544,13 @@ func (evpool *Pool) updateState(state sm.State) { evpool.state = state } +// hasPublicKeys returns true if we have public keys of the current validator set. +func (evpool *Pool) hasPublicKeys() bool { + evpool.mtx.Lock() + defer evpool.mtx.Unlock() + return evpool.state.Validators.HasPublicKeys +} + // processConsensusBuffer converts all the duplicate votes witnessed from consensus // into DuplicateVoteEvidence. It sets the evidence timestamp to the block height // from the most recently committed block. diff --git a/internal/evidence/pool_test.go b/internal/evidence/pool_test.go index d243ef8aff..652af1ac7e 100644 --- a/internal/evidence/pool_test.go +++ b/internal/evidence/pool_test.go @@ -528,6 +528,7 @@ func initializeValidatorState( ThresholdPublicKey: validator.PubKey, QuorumType: quorumType, QuorumHash: quorumHash, + HasPublicKeys: true, } return initializeStateFromValidatorSet(t, valSet, height) diff --git a/internal/evidence/reactor.go b/internal/evidence/reactor.go index 54dd75a810..d0ba641310 100644 --- a/internal/evidence/reactor.go +++ b/internal/evidence/reactor.go @@ -101,6 +101,19 @@ func (r *Reactor) handleEvidenceMessage(ctx context.Context, envelope *p2p.Envel switch msg := envelope.Message.(type) { case *tmproto.Evidence: + + // Only accept evidence if we are an active validator. + // On other hosts, signatures in evidence (if any) cannot be verified due to lack of validator public keys, + // and it creates risk of adding invalid evidence to the pool. + // + // TODO: We need to figure out how to handle evidence from non-validator nodes, to avoid scenarios where some + // evidence is lost. + if !r.evpool.hasPublicKeys() { + // silently drop the message + logger.Debug("dropping evidence message as we don't have validator public keys", "evidence", envelope.Message) + return nil + } + // Process the evidence received from a peer // Evidence is sent and received one by one ev, err := types.EvidenceFromProto(msg) diff --git a/internal/evidence/verify.go b/internal/evidence/verify.go index bb3b4bd4dd..0010a11b7c 100644 --- a/internal/evidence/verify.go +++ b/internal/evidence/verify.go @@ -5,6 +5,7 @@ import ( "context" "fmt" + "github.com/dashpay/tenderdash/libs/log" "github.com/dashpay/tenderdash/types" ) @@ -64,7 +65,7 @@ func (evpool *Pool) verify(ctx context.Context, evidence types.Evidence) error { return err } - if err := VerifyDuplicateVote(ev, state.ChainID, valSet); err != nil { + if err := VerifyDuplicateVote(ev, state.ChainID, valSet, evpool.logger); err != nil { return types.NewErrInvalidEvidence(evidence, err) } @@ -90,17 +91,15 @@ func (evpool *Pool) verify(ctx context.Context, evidence types.Evidence) error { // - the validator is in the validator set at the height of the evidence // - the height, round, type and validator address of the votes must be the same // - the block ID's must be different -// - The signatures must both be valid -func VerifyDuplicateVote(e *types.DuplicateVoteEvidence, chainID string, valSet *types.ValidatorSet) error { +// - The signatures must both be valid (only if we have public keys of the validator set) +// +// TODO: Double-check that this will still work for quite old heights, where validator set has already been rotated. +func VerifyDuplicateVote(e *types.DuplicateVoteEvidence, chainID string, valSet *types.ValidatorSet, logger log.Logger) error { _, val := valSet.GetByProTxHash(e.VoteA.ValidatorProTxHash) if val == nil { return fmt.Errorf("protxhash %X was not a validator at height %d", e.VoteA.ValidatorProTxHash, e.Height()) } proTxHash := val.ProTxHash - pubKey := val.PubKey - if pubKey == nil { - return fmt.Errorf("we don't have a public key of validator %X at height %d", proTxHash, e.Height()) - } // H/R/S must be the same if e.VoteA.Height != e.VoteB.Height || @@ -134,14 +133,22 @@ func VerifyDuplicateVote(e *types.DuplicateVoteEvidence, chainID string, valSet voteProTxHash, proTxHash) } - va := e.VoteA.ToProto() - vb := e.VoteB.ToProto() - // Signatures must be valid - if !pubKey.VerifySignatureDigest(types.VoteBlockSignID(chainID, va, valSet.QuorumType, valSet.QuorumHash), e.VoteA.BlockSignature) { - return fmt.Errorf("verifying VoteA: %w", types.ErrVoteInvalidBlockSignature) - } - if !pubKey.VerifySignatureDigest(types.VoteBlockSignID(chainID, vb, valSet.QuorumType, valSet.QuorumHash), e.VoteB.BlockSignature) { - return fmt.Errorf("verifying VoteB: %w", types.ErrVoteInvalidBlockSignature) + pubKey := val.PubKey + + if pubKey != nil { + va := e.VoteA.ToProto() + vb := e.VoteB.ToProto() + // Signatures must be valid + if !pubKey.VerifySignatureDigest(types.VoteBlockSignID(chainID, va, valSet.QuorumType, valSet.QuorumHash), e.VoteA.BlockSignature) { + return fmt.Errorf("verifying VoteA: %w", types.ErrVoteInvalidBlockSignature) + } + if !pubKey.VerifySignatureDigest(types.VoteBlockSignID(chainID, vb, valSet.QuorumType, valSet.QuorumHash), e.VoteB.BlockSignature) { + return fmt.Errorf("verifying VoteB: %w", types.ErrVoteInvalidBlockSignature) + } + } else if valSet.HasPublicKeys { + return fmt.Errorf("we don't have a public key of validator %X at height %d", proTxHash, e.Height()) + } else { + logger.Debug("skipping verification of duplicate vote evidence signatures - no access public key", "evidence", e) } return nil diff --git a/internal/evidence/verify_test.go b/internal/evidence/verify_test.go index 39e678f51c..18db60a228 100644 --- a/internal/evidence/verify_test.go +++ b/internal/evidence/verify_test.go @@ -84,9 +84,9 @@ func TestVerifyDuplicateVoteEvidence(t *testing.T) { Timestamp: defaultEvidenceTime, } if c.valid { - assert.Nil(t, evidence.VerifyDuplicateVote(ev, chainID, valSet), "evidence should be valid") + assert.Nil(t, evidence.VerifyDuplicateVote(ev, chainID, valSet, logger), "evidence should be valid") } else { - assert.NotNil(t, evidence.VerifyDuplicateVote(ev, chainID, valSet), "evidence should be invalid") + assert.NotNil(t, evidence.VerifyDuplicateVote(ev, chainID, valSet, logger), "evidence should be invalid") } }