diff --git a/.changelog/unreleased/bug-fixes/0016-abc-light-proposer-priorities.md b/.changelog/unreleased/bug-fixes/0016-abc-light-proposer-priorities.md new file mode 100644 index 00000000000..9b2cb2c0c7b --- /dev/null +++ b/.changelog/unreleased/bug-fixes/0016-abc-light-proposer-priorities.md @@ -0,0 +1,2 @@ +- `[light]` Cross-check proposer priorities in retrieved validator sets + ([\#ABC-0016](https://github.com/cometbft/cometbft/security/advisories/GHSA-g5xx-c4hv-9ccc)) diff --git a/.changelog/unreleased/improvements/0016-abc-types-validator-set.md b/.changelog/unreleased/improvements/0016-abc-types-validator-set.md new file mode 100644 index 00000000000..2851a4f6524 --- /dev/null +++ b/.changelog/unreleased/improvements/0016-abc-types-validator-set.md @@ -0,0 +1,2 @@ +- `[types]` Check that proposer is one of the validators in `ValidateBasic` + ([\#ABC-0016](https://github.com/cometbft/cometbft/security/advisories/GHSA-g5xx-c4hv-9ccc)) diff --git a/light/client.go b/light/client.go index 5860cb0e68d..b9153b4d5fe 100644 --- a/light/client.go +++ b/light/client.go @@ -384,7 +384,7 @@ func (c *Client) initializeWithTrustOptions(ctx context.Context, options TrustOp } // 3) Cross-verify with witnesses to ensure everybody has the same state. - if err := c.compareFirstHeaderWithWitnesses(ctx, l.SignedHeader); err != nil { + if err := c.compareFirstLightBlockWithWitnesses(ctx, l); err != nil { return err } @@ -1126,9 +1126,9 @@ func (c *Client) findNewPrimary(ctx context.Context, height int64, remove bool) return nil, lastError } -// compareFirstHeaderWithWitnesses compares h with all witnesses. If any +// compareFirstLightBlockWithWitnesses compares light block l with all witnesses. If any // witness reports a different header than h, the function returns an error. -func (c *Client) compareFirstHeaderWithWitnesses(ctx context.Context, h *types.SignedHeader) error { +func (c *Client) compareFirstLightBlockWithWitnesses(ctx context.Context, l *types.LightBlock) error { compareCtx, cancel := context.WithCancel(ctx) defer cancel() @@ -1141,7 +1141,7 @@ func (c *Client) compareFirstHeaderWithWitnesses(ctx context.Context, h *types.S errc := make(chan error, len(c.witnesses)) for i, witness := range c.witnesses { - go c.compareNewHeaderWithWitness(compareCtx, errc, h, witness, i) + go c.compareNewLightBlockWithWitness(compareCtx, errc, l, witness, i) } witnessesToRemove := make([]int, 0, len(c.witnesses)) @@ -1153,23 +1153,29 @@ func (c *Client) compareFirstHeaderWithWitnesses(ctx context.Context, h *types.S switch e := err.(type) { case nil: continue - case errConflictingHeaders: - c.logger.Error(fmt.Sprintf(`Witness #%d has a different header. Please check primary is correct -and remove witness. Otherwise, use the different primary`, e.WitnessIndex), "witness", c.witnesses[e.WitnessIndex]) + case ErrConflictingHeaders: + c.logger.Error("Witness reports a conflicting header. "+ + "Please check if the primary is correct or use a different witness.", + "witness", c.witnesses[e.WitnessIndex], "err", err) return err case errBadWitness: // If witness sent us an invalid header, then remove it - c.logger.Info("witness sent an invalid light block, removing...", + c.logger.Info("Witness sent an invalid light block, removing...", "witness", c.witnesses[e.WitnessIndex], "err", err) witnessesToRemove = append(witnessesToRemove, e.WitnessIndex) + case ErrProposerPrioritiesDiverge: + c.logger.Error("Witness reports conflicting proposer priorities. "+ + "Please check if the primary is correct or use a different witness.", + "witness", c.witnesses[e.WitnessIndex], "err", err) + return err default: // benign errors can be ignored with the exception of context errors if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { return err } // the witness either didn't respond or didn't have the block. We ignore it. - c.logger.Info("error comparing first header with witness. You may want to consider removing the witness", + c.logger.Info("Error comparing first header with witness. You may want to consider removing the witness", "err", err) } @@ -1177,7 +1183,7 @@ and remove witness. Otherwise, use the different primary`, e.WitnessIndex), "wit // remove witnesses that have misbehaved if err := c.removeWitnesses(witnessesToRemove); err != nil { - c.logger.Error("failed to remove witnesses", "err", err, "witnessesToRemove", witnessesToRemove) + c.logger.Error("Failed to remove witnesses", "err", err, "witnessesToRemove", witnessesToRemove) } return nil diff --git a/light/client_test.go b/light/client_test.go index 6e975212d62..24c0dbe8695 100644 --- a/light/client_test.go +++ b/light/client_test.go @@ -32,11 +32,13 @@ var ( bTime, _ = time.Parse(time.RFC3339, "2006-01-02T15:04:05Z") h1 = keys.GenSignedHeader(chainID, 1, bTime, nil, vals, vals, hash("app_hash"), hash("cons_hash"), hash("results_hash"), 0, len(keys)) - // 3/3 signed - h2 = keys.GenSignedHeaderLastBlockID(chainID, 2, bTime.Add(30*time.Minute), nil, vals, vals, + // 3/3 signed. + vals2 = vals.CopyIncrementProposerPriority(1) + h2 = keys.GenSignedHeaderLastBlockID(chainID, 2, bTime.Add(30*time.Minute), nil, vals2, vals2, hash("app_hash"), hash("cons_hash"), hash("results_hash"), 0, len(keys), types.BlockID{Hash: h1.Hash()}) - // 3/3 signed - h3 = keys.GenSignedHeaderLastBlockID(chainID, 3, bTime.Add(1*time.Hour), nil, vals, vals, + // 3/3 signed. + vals3 = vals2.CopyIncrementProposerPriority(1) + h3 = keys.GenSignedHeaderLastBlockID(chainID, 3, bTime.Add(1*time.Hour), nil, vals3, vals3, hash("app_hash"), hash("cons_hash"), hash("results_hash"), 0, len(keys), types.BlockID{Hash: h2.Hash()}) trustPeriod = 4 * time.Hour trustOptions = light.TrustOptions{ @@ -46,9 +48,9 @@ var ( } valSet = map[int64]*types.ValidatorSet{ 1: vals, - 2: vals, - 3: vals, - 4: vals, + 2: vals2, + 3: vals3, + 4: vals.CopyIncrementProposerPriority(1), } headerSet = map[int64]*types.SignedHeader{ 1: h1, @@ -58,7 +60,7 @@ var ( 3: h3, } l1 = &types.LightBlock{SignedHeader: h1, ValidatorSet: vals} - l2 = &types.LightBlock{SignedHeader: h2, ValidatorSet: vals} + l2 = &types.LightBlock{SignedHeader: h2, ValidatorSet: vals2} fullNode = mockp.New( chainID, headerSet, @@ -913,13 +915,13 @@ func TestClientRemovesWitnessIfItSendsUsIncorrectHeader(t *testing.T) { chainID, map[int64]*types.SignedHeader{ 1: h1, - 2: keys.GenSignedHeaderLastBlockID(chainID, 2, bTime.Add(30*time.Minute), nil, vals, vals, + 2: keys.GenSignedHeaderLastBlockID(chainID, 2, bTime.Add(30*time.Minute), nil, vals2, vals2, hash("app_hash2"), hash("cons_hash"), hash("results_hash"), len(keys), len(keys), types.BlockID{Hash: h1.Hash()}), }, map[int64]*types.ValidatorSet{ 1: vals, - 2: vals, + 2: vals2, }, ) // header is empty @@ -931,7 +933,7 @@ func TestClientRemovesWitnessIfItSendsUsIncorrectHeader(t *testing.T) { }, map[int64]*types.ValidatorSet{ 1: vals, - 2: vals, + 2: vals2, }, ) @@ -1155,3 +1157,56 @@ func TestClientHandlesContexts(t *testing.T) { require.Error(t, err) require.True(t, errors.Is(err, context.Canceled)) } + +// TestClientErrorsDifferentProposerPriorities tests the case where the witness +// sends us a light block with a validator set with different proposer priorities. +func TestClientErrorsDifferentProposerPriorities(t *testing.T) { + primary := mockp.New( + chainID, + map[int64]*types.SignedHeader{ + 1: h1, + 2: h2, + }, + map[int64]*types.ValidatorSet{ + 1: vals, + 2: vals2, + }, + ) + witness := mockp.New( + chainID, + map[int64]*types.SignedHeader{ + 1: h1, + 2: h2, + }, + map[int64]*types.ValidatorSet{ + 1: vals, + 2: vals, + }, + ) + + // Proposer priorities in vals and vals2 are different. + // This is because vals2 = vals.CopyIncrementProposerPriority(1) + require.Equal(t, vals.Hash(), vals2.Hash()) + require.NotEqual(t, vals.ProposerPriorityHash(), vals2.ProposerPriorityHash()) + + c, err := light.NewClient( + ctx, + chainID, + trustOptions, + fullNode, + []provider.Provider{primary, witness}, + dbs.New(dbm.NewMemDB(), chainID), + light.Logger(log.TestingLogger()), + light.MaxRetryAttempts(1), + ) + // witness should have behaved properly -> no error + require.NoError(t, err) + assert.EqualValues(t, 2, len(c.Witnesses())) + + // witness behaves incorrectly, but we can't prove who's guilty -> error + _, err = c.VerifyLightBlockAtHeight(ctx, 2, bTime.Add(2*time.Hour)) + require.Error(t, err) + + // witness left in the list + assert.EqualValues(t, 2, len(c.Witnesses())) +} diff --git a/light/detector.go b/light/detector.go index 228dec61a6c..5b742761eba 100644 --- a/light/detector.go +++ b/light/detector.go @@ -31,7 +31,8 @@ func (c *Client) detectDivergence(ctx context.Context, primaryTrace []*types.Lig } var ( headerMatched bool - lastVerifiedHeader = primaryTrace[len(primaryTrace)-1].SignedHeader + lastVerifiedBlock = primaryTrace[len(primaryTrace)-1] + lastVerifiedHeader = lastVerifiedBlock.SignedHeader witnessesToRemove = make([]int, 0) ) c.logger.Debug("Running detector against trace", "finalizeBlockHeight", lastVerifiedHeader.Height, @@ -48,7 +49,7 @@ func (c *Client) detectDivergence(ctx context.Context, primaryTrace []*types.Lig // and compare it with the header from the primary errc := make(chan error, len(c.witnesses)) for i, witness := range c.witnesses { - go c.compareNewHeaderWithWitness(ctx, errc, lastVerifiedHeader, witness, i) + go c.compareNewLightBlockWithWitness(ctx, errc, lastVerifiedBlock, witness, i) } // handle errors from the header comparisons as they come in @@ -58,7 +59,7 @@ func (c *Client) detectDivergence(ctx context.Context, primaryTrace []*types.Lig switch e := err.(type) { case nil: // at least one header matched headerMatched = true - case errConflictingHeaders: + case ErrConflictingHeaders: // We have conflicting headers. This could possibly imply an attack on the light client. // First we need to verify the witness's header using the same skipping verification and then we // need to find the point that the headers diverge and examine this for any evidence of an attack. @@ -79,6 +80,10 @@ func (c *Client) detectDivergence(ctx context.Context, primaryTrace []*types.Lig c.logger.Info("witness returned an error during header comparison, removing...", "witness", c.witnesses[e.WitnessIndex], "err", err) witnessesToRemove = append(witnessesToRemove, e.WitnessIndex) + case ErrProposerPrioritiesDiverge: + c.logger.Info("witness reported validator set with different proposer priorities", + "witness", c.witnesses[e.WitnessIndex], "err", err) + return e default: // Benign errors which can be ignored unless there was a context // canceled @@ -104,17 +109,19 @@ func (c *Client) detectDivergence(ctx context.Context, primaryTrace []*types.Lig return ErrFailedHeaderCrossReferencing } -// compareNewHeaderWithWitness takes the verified header from the primary and compares it with a +// compareNewLightBlockWithWitness takes the verified header from the primary and compares it with a // header from a specified witness. The function can return one of three errors: // -// 1: errConflictingHeaders -> there may have been an attack on this light client +// 1: ErrConflictingHeaders -> there may have been an attack on this light client // 2: errBadWitness -> the witness has either not responded, doesn't have the header or has given us an invalid one // // Note: In the case of an invalid header we remove the witness // // 3: nil -> the hashes of the two headers match -func (c *Client) compareNewHeaderWithWitness(ctx context.Context, errc chan error, h *types.SignedHeader, - witness provider.Provider, witnessIndex int) { +func (c *Client) compareNewLightBlockWithWitness(ctx context.Context, errc chan error, l *types.LightBlock, + witness provider.Provider, witnessIndex int, +) { + h := l.SignedHeader lightBlock, err := witness.LightBlock(ctx, h.Height) switch err { @@ -150,7 +157,7 @@ func (c *Client) compareNewHeaderWithWitness(ctx context.Context, errc chan erro // witness' last header is below the primary's header. We check the times to see if the blocks // have conflicting times if !lightBlock.Time.Before(h.Time) { - errc <- errConflictingHeaders{Block: lightBlock, WitnessIndex: witnessIndex} + errc <- ErrConflictingHeaders{Block: lightBlock, WitnessIndex: witnessIndex} return } @@ -175,7 +182,7 @@ func (c *Client) compareNewHeaderWithWitness(ctx context.Context, errc chan erro // the witness still doesn't have a block at the height of the primary. // Check if there is a conflicting time if !lightBlock.Time.Before(h.Time) { - errc <- errConflictingHeaders{Block: lightBlock, WitnessIndex: witnessIndex} + errc <- ErrConflictingHeaders{Block: lightBlock, WitnessIndex: witnessIndex} return } @@ -197,7 +204,13 @@ func (c *Client) compareNewHeaderWithWitness(ctx context.Context, errc chan erro } if !bytes.Equal(h.Hash(), lightBlock.Hash()) { - errc <- errConflictingHeaders{Block: lightBlock, WitnessIndex: witnessIndex} + errc <- ErrConflictingHeaders{Block: lightBlock, WitnessIndex: witnessIndex} + } + + // ProposerPriorityHash is not part of the header hash, so we need to check it separately. + wanted, got := l.ValidatorSet.ProposerPriorityHash(), lightBlock.ValidatorSet.ProposerPriorityHash() + if !bytes.Equal(wanted, got) { + errc <- ErrProposerPrioritiesDiverge{WitnessHash: got, WitnessIndex: witnessIndex, PrimaryHash: wanted} } c.logger.Debug("Matching header received by witness", "height", h.Height, "witness", witnessIndex) @@ -245,7 +258,7 @@ func (c *Client) handleConflictingHeaders( if primaryBlock.Commit.Round != witnessTrace[len(witnessTrace)-1].Commit.Round { c.logger.Info("The light client has detected, and prevented, an attempted amnesia attack." + " We think this attack is pretty unlikely, so if you see it, that's interesting to us." + - " Can you let us know by opening an issue through https://github.com/tendermint/tendermint/issues/new?") + " Can you let us know by opening an issue through https://github.com/cometbft/cometbft/issues/new?") } // This may not be valid because the witness itself is at fault. So now we reverse it, examining the diff --git a/light/errors.go b/light/errors.go index bc6357def66..066f709c654 100644 --- a/light/errors.go +++ b/light/errors.go @@ -75,20 +75,36 @@ var ErrLightClientAttack = errors.New(`attempted attack detected. // continue running the light client. var ErrNoWitnesses = errors.New("no witnesses connected. please reset light client") -// ----------------------------- INTERNAL ERRORS --------------------------------- - // ErrConflictingHeaders is thrown when two conflicting headers are discovered. -type errConflictingHeaders struct { +type ErrConflictingHeaders struct { Block *types.LightBlock WitnessIndex int } -func (e errConflictingHeaders) Error() string { +func (e ErrConflictingHeaders) Error() string { return fmt.Sprintf( "header hash (%X) from witness (%d) does not match primary", e.Block.Hash(), e.WitnessIndex) } +// ErrProposerPrioritiesDiverge is thrown when two conflicting headers are +// discovered, but the error is non-attributable comparing to ErrConflictingHeaders. +// The difference is in validator set proposer priorities, which may change +// with every round of consensus. +type ErrProposerPrioritiesDiverge struct { + WitnessHash []byte + WitnessIndex int + PrimaryHash []byte +} + +func (e ErrProposerPrioritiesDiverge) Error() string { + return fmt.Sprintf( + "validator set's proposer priority hashes do not match: witness[%d]=%X, primary=%X", + e.WitnessIndex, e.WitnessHash, e.PrimaryHash) +} + +// ----------------------------- INTERNAL ERRORS --------------------------------- + // errBadWitness is returned when the witness either does not respond or // responds with an invalid header. type errBadWitness struct { diff --git a/types/validator_set.go b/types/validator_set.go index 0a56dcbe77c..aeb4feb7b54 100644 --- a/types/validator_set.go +++ b/types/validator_set.go @@ -2,6 +2,7 @@ package types import ( "bytes" + "encoding/binary" "errors" "fmt" "math" @@ -10,6 +11,7 @@ import ( "strings" "github.com/cometbft/cometbft/crypto/merkle" + "github.com/cometbft/cometbft/crypto/tmhash" cmtmath "github.com/cometbft/cometbft/libs/math" cmtproto "github.com/cometbft/cometbft/proto/tendermint/types" ) @@ -35,6 +37,9 @@ const ( var ErrTotalVotingPowerOverflow = fmt.Errorf("total voting power of resulting valset exceeds max %d", MaxTotalVotingPower) +// ErrProposerNotInVals is returned if the proposer is not in the validator set. +var ErrProposerNotInVals = errors.New("proposer not in validator set") + // ValidatorSet represent a set of *Validator at a given height. // // The validators can be fetched by address or index. @@ -94,7 +99,13 @@ func (vals *ValidatorSet) ValidateBasic() error { return fmt.Errorf("proposer failed validate basic, error: %w", err) } - return nil + for _, val := range vals.Validators { + if bytes.Equal(val.Address, vals.Proposer.Address) { + return nil + } + } + + return ErrProposerNotInVals } // IsNilOrEmpty returns true if validator set is nil or empty. @@ -356,6 +367,8 @@ func (vals *ValidatorSet) findProposer() *Validator { // Hash returns the Merkle root hash build using validators (as leaves) in the // set. +// +// See merkle.HashFromByteSlices. func (vals *ValidatorSet) Hash() []byte { bzs := make([][]byte, len(vals.Validators)) for i, val := range vals.Validators { @@ -364,6 +377,23 @@ func (vals *ValidatorSet) Hash() []byte { return merkle.HashFromByteSlices(bzs) } +// ProposerPriorityHash returns the tmhash of the proposer priorities. +// Validator set must be sorted to get the same hash. +// If the validator set is empty, nil is returned. +func (vals *ValidatorSet) ProposerPriorityHash() []byte { + if len(vals.Validators) == 0 { + return nil + } + + buf := make([]byte, binary.MaxVarintLen64*len(vals.Validators)) + total := 0 + for _, val := range vals.Validators { + n := binary.PutVarint(buf, val.ProposerPriority) + total += n + } + return tmhash.Sum(buf[:total]) +} + // Iterate will run the given function over the set. func (vals *ValidatorSet) Iterate(fn func(index int, val *Validator) bool) { for i, val := range vals.Validators { diff --git a/types/validator_set_test.go b/types/validator_set_test.go index dc0270aebc9..2b8969b8be6 100644 --- a/types/validator_set_test.go +++ b/types/validator_set_test.go @@ -78,9 +78,10 @@ func TestValidatorSetBasic(t *testing.T) { assert.Equal(t, proposerPriority, val.ProposerPriority) } -func TestValidatorSetValidateBasic(t *testing.T) { +func TestValidatorSet_ValidateBasic(t *testing.T) { val, _ := RandValidator(false, 1) badVal := &Validator{} + val2, _ := RandValidator(false, 1) testCases := []struct { vals ValidatorSet @@ -121,6 +122,14 @@ func TestValidatorSetValidateBasic(t *testing.T) { err: false, msg: "", }, + { + vals: ValidatorSet{ + Validators: []*Validator{val}, + Proposer: val2, + }, + err: true, + msg: ErrProposerNotInVals.Error(), + }, } for _, tc := range testCases { @@ -150,6 +159,30 @@ func TestCopy(t *testing.T) { } } +func TestValidatorSet_ProposerPriorityHash(t *testing.T) { + vset := NewValidatorSet(nil) + assert.Equal(t, []byte(nil), vset.ProposerPriorityHash()) + + vset = randValidatorSet(3) + assert.NotNil(t, vset.ProposerPriorityHash()) + + // Marshaling and unmarshalling do not affect ProposerPriorityHash + bz, err := vset.ToProto() + assert.NoError(t, err) + vsetProto, err := ValidatorSetFromProto(bz) + assert.NoError(t, err) + assert.Equal(t, vset.ProposerPriorityHash(), vsetProto.ProposerPriorityHash()) + + // Copy does not affect ProposerPriorityHash + vsetCopy := vset.Copy() + assert.Equal(t, vset.ProposerPriorityHash(), vsetCopy.ProposerPriorityHash()) + + // Incrementing priorities changes ProposerPriorityHash() but not Hash() + vset.IncrementProposerPriority(1) + assert.Equal(t, vset.Hash(), vsetCopy.Hash()) + assert.NotEqual(t, vset.ProposerPriorityHash(), vsetCopy.ProposerPriorityHash()) +} + // Test that IncrementProposerPriority requires positive times. func TestIncrementProposerPriorityPositiveTimes(t *testing.T) { vset := NewValidatorSet([]*Validator{