From 5b0fd6516113b7bb2f41f73da3824dba8f967797 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Mon, 13 Nov 2023 17:50:31 +0100 Subject: [PATCH] fix!: verify the signatures of byzantine validators in misbehaviour handling (#1422) * update byzantine validators extraction * nits * Update x/ccv/provider/keeper/misbehaviour.go Co-authored-by: insumity * last changes * udpdate changelog --------- Co-authored-by: insumity --- CHANGELOG.md | 4 +- tests/integration/misbehaviour.go | 171 +++++++++++++++++++------- testutil/crypto/evidence.go | 48 ++++---- x/ccv/provider/keeper/misbehaviour.go | 46 +++++-- 4 files changed, 191 insertions(+), 78 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fe14e982a2..501c4d0128 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,9 +4,7 @@ Add an entry to the unreleased section whenever merging a PR to main that is not targeted at a specific release. These entries will eventually be included in a release. -## v2.3.0-provider-lsm -* (fix!) [#1404](https://github.com/cosmos/interchain-security/pull/1404) Add conditions to the misbehaviour handling ensuring that validators who vote nil -are not getting punished. +* (fix!) [#1422](https://github.com/cosmos/interchain-security/pull/1422) Fix the misbehaviour handling by verifying the signatures of byzantine validators. ## v2.2.0-provider-lsm diff --git a/tests/integration/misbehaviour.go b/tests/integration/misbehaviour.go index 306fb06ec4..63b0eafb6d 100644 --- a/tests/integration/misbehaviour.go +++ b/tests/integration/misbehaviour.go @@ -135,6 +135,119 @@ func (s *CCVTestSuite) TestGetByzantineValidators() { nil, false, }, + { + "incorrect valset - shouldn't pass", + func() *ibctmtypes.Misbehaviour { + clientHeader := s.consumerChain.CreateTMClientHeader( + s.consumerChain.ChainID, + int64(clientHeight.RevisionHeight+1), + clientHeight, + altTime.Add(time.Minute), + clientTMValset, + clientTMValset, + clientTMValset, + clientSigners, + ) + + clientHeaderWithCorruptedValset := s.consumerChain.CreateTMClientHeader( + s.consumerChain.ChainID, + int64(clientHeight.RevisionHeight+1), + clientHeight, + altTime.Add(time.Hour), + clientTMValset, + clientTMValset, + clientTMValset, + clientSigners, + ) + + // change a validator public key in one the second header + testutil.CorruptValidatorPubkeyInHeader(clientHeaderWithCorruptedValset, clientTMValset.Validators[0].Address) + + return &ibctmtypes.Misbehaviour{ + ClientId: s.path.EndpointA.ClientID, + Header1: clientHeader, + Header2: clientHeaderWithCorruptedValset, + } + }, + []*tmtypes.Validator{}, + false, + }, + { + "incorrect valset 2 - shouldn't pass", + func() *ibctmtypes.Misbehaviour { + clientHeader := s.consumerChain.CreateTMClientHeader( + s.consumerChain.ChainID, + int64(clientHeight.RevisionHeight+1), + clientHeight, + altTime.Add(time.Minute), + clientTMValset, + clientTMValset, + clientTMValset, + clientSigners, + ) + + clientHeaderWithCorruptedSigs := s.consumerChain.CreateTMClientHeader( + s.consumerChain.ChainID, + int64(clientHeight.RevisionHeight+1), + clientHeight, + altTime.Add(time.Hour), + clientTMValset, + clientTMValset, + clientTMValset, + clientSigners, + ) + + // change the valset in the header + vs, _ := altValset.ToProto() + clientHeader.ValidatorSet.Validators = vs.Validators[:3] + clientHeaderWithCorruptedSigs.ValidatorSet.Validators = vs.Validators[:3] + + return &ibctmtypes.Misbehaviour{ + ClientId: s.path.EndpointA.ClientID, + Header1: clientHeader, + Header2: clientHeaderWithCorruptedSigs, + } + }, + []*tmtypes.Validator{}, + false, + }, + { + "incorrect signatures - shouldn't pass", + func() *ibctmtypes.Misbehaviour { + clientHeader := s.consumerChain.CreateTMClientHeader( + s.consumerChain.ChainID, + int64(clientHeight.RevisionHeight+1), + clientHeight, + altTime.Add(time.Minute), + clientTMValset, + clientTMValset, + clientTMValset, + clientSigners, + ) + + clientHeaderWithCorruptedSigs := s.consumerChain.CreateTMClientHeader( + s.consumerChain.ChainID, + int64(clientHeight.RevisionHeight+1), + clientHeight, + altTime.Add(time.Hour), + clientTMValset, + clientTMValset, + clientTMValset, + clientSigners, + ) + + // change the signature of one of the validator in the header + testutil.CorruptCommitSigsInHeader(clientHeaderWithCorruptedSigs, clientTMValset.Validators[0].Address) + + return &ibctmtypes.Misbehaviour{ + ClientId: s.path.EndpointA.ClientID, + Header1: clientHeader, + Header2: clientHeaderWithCorruptedSigs, + } + }, + []*tmtypes.Validator{}, + false, + }, { "light client attack - lunatic attack", func() *ibctmtypes.Misbehaviour { @@ -212,43 +325,6 @@ func (s *CCVTestSuite) TestGetByzantineValidators() { []*tmtypes.Validator{}, true, }, - { - "validators who did vote nil should not be returned", - func() *ibctmtypes.Misbehaviour { - clientHeader := s.consumerChain.CreateTMClientHeader( - s.consumerChain.ChainID, - int64(clientHeight.RevisionHeight+1), - clientHeight, - altTime.Add(time.Minute), - clientTMValset, - clientTMValset, - clientTMValset, - clientSigners, - ) - - // create conflicting header with 2/4 validators voting nil - clientHeaderWithNilVotes := s.consumerChain.CreateTMClientHeader( - s.consumerChain.ChainID, - int64(clientHeight.RevisionHeight+1), - clientHeight, - altTime.Add(time.Hour), - clientTMValset, - clientTMValset, - clientTMValset, - clientSigners, - ) - testutil.UpdateHeaderCommitWithNilVotes(clientHeaderWithNilVotes, clientTMValset.Validators[:2]) - - return &ibctmtypes.Misbehaviour{ - ClientId: s.path.EndpointA.ClientID, - Header1: clientHeader, - Header2: clientHeaderWithNilVotes, - } - }, - // Expect validators who did NOT vote nil - clientTMValset.Validators[2:], - true, - }, } for _, tc := range testCases { @@ -262,7 +338,7 @@ func (s *CCVTestSuite) TestGetByzantineValidators() { s.Equal(len(tc.expByzantineValidators), len(byzantineValidators)) // For both lunatic and equivocation attacks, all the validators - // who signed both headers and didn't vote nil should be returned + // who signed both headers if len(tc.expByzantineValidators) > 0 { expValset := tmtypes.NewValidatorSet(tc.expByzantineValidators) s.NoError(err) @@ -299,10 +375,15 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() { // create an alternative validator set using more than 1/3 of the trusted validator set altValset := tmtypes.NewValidatorSet(s.consumerChain.Vals.Validators[0:2]) - altSigners := make(map[string]tmtypes.PrivValidator, 1) + altSigners := make(map[string]tmtypes.PrivValidator, 2) altSigners[clientTMValset.Validators[0].Address.String()] = clientSigners[clientTMValset.Validators[0].Address.String()] altSigners[clientTMValset.Validators[1].Address.String()] = clientSigners[clientTMValset.Validators[1].Address.String()] + // create an alternative validator set using less than 1/3 of the trusted validator set + altValset2 := tmtypes.NewValidatorSet(s.consumerChain.Vals.Validators[0:1]) + altSigners2 := make(map[string]tmtypes.PrivValidator, 1) + altSigners2[clientTMValset.Validators[0].Address.String()] = clientSigners[clientTMValset.Validators[0].Address.String()] + clientHeader := s.consumerChain.CreateTMClientHeader( s.consumerChain.ChainID, int64(clientHeight.RevisionHeight+1), @@ -315,19 +396,17 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() { ) // create a conflicting client header with insufficient voting power - // by changing 3/4 of its validators votes to nil - clientHeaderWithNilVotes := s.consumerChain.CreateTMClientHeader( + clientHeader2 := s.consumerChain.CreateTMClientHeader( s.consumerChain.ChainID, int64(clientHeight.RevisionHeight+1), clientHeight, // use a different block time to change the header BlockID headerTs.Add(time.Hour), + altValset2, + altValset2, clientTMValset, - clientTMValset, - clientTMValset, - clientSigners, + altSigners2, ) - testutil.UpdateHeaderCommitWithNilVotes(clientHeaderWithNilVotes, clientTMValset.Validators[:4]) testCases := []struct { name string @@ -402,7 +481,7 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() { &ibctmtypes.Misbehaviour{ ClientId: s.path.EndpointA.ClientID, Header1: clientHeader, - Header2: clientHeaderWithNilVotes, + Header2: clientHeader2, }, false, }, diff --git a/testutil/crypto/evidence.go b/testutil/crypto/evidence.go index 1a2c2cbc7e..fef4ed768b 100644 --- a/testutil/crypto/evidence.go +++ b/testutil/crypto/evidence.go @@ -1,11 +1,11 @@ package crypto import ( - "fmt" "time" ibctmtypes "github.com/cosmos/ibc-go/v4/modules/light-clients/07-tendermint/types" "github.com/tendermint/tendermint/crypto/tmhash" + "github.com/tendermint/tendermint/libs/bytes" tmtypes "github.com/tendermint/tendermint/types" ) @@ -92,39 +92,43 @@ func MakeAndSignVoteWithForgedValAddress( return vote } -// UpdateHeaderCommitWithNilVotes updates the given light client header -// by changing the commit BlockIDFlag of the given validators to nil -// +// CorruptCommitSigsInHeader corrupts the header by changing the value +// of the commit signature for given validator address. // Note that this method is solely used for testing purposes -func UpdateHeaderCommitWithNilVotes(header *ibctmtypes.Header, validators []*tmtypes.Validator) { - if len(validators) > len(header.ValidatorSet.Validators) { - panic(fmt.Sprintf("cannot change more than %d validators votes: got %d", - len(header.ValidatorSet.Validators), len(header.ValidatorSet.Validators))) - } - +func CorruptCommitSigsInHeader(header *ibctmtypes.Header, valAddress bytes.HexBytes) { commit, err := tmtypes.CommitFromProto(header.Commit) if err != nil { panic(err) } + for idx, sig := range commit.Signatures { + if sig.ValidatorAddress.String() == valAddress.String() { + sig.Signature = []byte("randomsig") + commit.Signatures[idx] = sig + } + } + // update the commit in client the header + header.SignedHeader.Commit = commit.ToProto() +} + +// CorruptValidatorPubkeyInHeader corrupts the header by changing the validator pubkey +// of the given validator address in the validator set. +// Note that this method is solely used for testing purposes +func CorruptValidatorPubkeyInHeader(header *ibctmtypes.Header, valAddress bytes.HexBytes) { valset, err := tmtypes.ValidatorSetFromProto(header.ValidatorSet) if err != nil { panic(err) } - for _, v := range validators { - // get validator index in valset - idx, _ := valset.GetByAddress(v.Address) - if idx != -1 { - // get validator commit sig - s := commit.Signatures[idx] - // change BlockIDFlag to nil - s.BlockIDFlag = tmtypes.BlockIDFlagNil - // update the signatures - commit.Signatures[idx] = s + for _, v := range valset.Validators { + if v.Address.String() == valAddress.String() { + v.PubKey = tmtypes.NewMockPV().PrivKey.PubKey() } } - // update the commit in client the header - header.SignedHeader.Commit = commit.ToProto() + vs, err := valset.ToProto() + if err != nil { + panic(err) + } + header.ValidatorSet = vs } diff --git a/x/ccv/provider/keeper/misbehaviour.go b/x/ccv/provider/keeper/misbehaviour.go index 7307284655..f5b63533a0 100644 --- a/x/ccv/provider/keeper/misbehaviour.go +++ b/x/ccv/provider/keeper/misbehaviour.go @@ -101,20 +101,28 @@ func (k Keeper) GetByzantineValidators(ctx sdk.Context, misbehaviour ibctmtypes. // and return the intersection of validators who signed both // create a map with the validators' address that signed header1 - header1Signers := map[string]struct{}{} - for _, sign := range lightBlock1.Commit.Signatures { - if !sign.ForBlock() { + header1Signers := map[string]int{} + for idx, sign := range lightBlock1.Commit.Signatures { + if sign.Absent() { continue } - header1Signers[sign.ValidatorAddress.String()] = struct{}{} + header1Signers[sign.ValidatorAddress.String()] = idx } // iterate over the header2 signers and check if they signed header1 - for _, sign := range lightBlock2.Commit.Signatures { - if !sign.ForBlock() { + for sigIdxHeader2, sign := range lightBlock2.Commit.Signatures { + if sign.Absent() { continue } - if _, ok := header1Signers[sign.ValidatorAddress.String()]; ok { + if sigIdxHeader1, ok := header1Signers[sign.ValidatorAddress.String()]; ok { + if err := verifyLightBlockCommitSig(*lightBlock1, sigIdxHeader1); err != nil { + return nil, err + } + + if err := verifyLightBlockCommitSig(*lightBlock2, sigIdxHeader2); err != nil { + return nil, err + } + _, val := lightBlock1.ValidatorSet.GetByAddress(sign.ValidatorAddress) validators = append(validators, val) } @@ -192,3 +200,27 @@ func headersStateTransitionsAreConflicting(h1, h2 tmtypes.Header) bool { !bytes.Equal(h1.AppHash, h2.AppHash) || !bytes.Equal(h1.LastResultsHash, h2.LastResultsHash) } + +func verifyLightBlockCommitSig(lightBlock tmtypes.LightBlock, sigIdx int) error { + // get signature + sig := lightBlock.Commit.Signatures[sigIdx] + + // get validator + idx, val := lightBlock.ValidatorSet.GetByAddress(sig.ValidatorAddress) + if idx == -1 { + return fmt.Errorf("incorrect signature: validator address %s isn't part of the validator set", sig.ValidatorAddress.String()) + } + + // verify validator pubkey corresponds to signature validator address + if !bytes.Equal(val.PubKey.Address(), sig.ValidatorAddress) { + return fmt.Errorf("validator public key doesn't correspond to signature validator address: %s!= %s", val.PubKey.Address(), sig.ValidatorAddress) + } + + // validate signature + voteSignBytes := lightBlock.Commit.VoteSignBytes(lightBlock.ChainID, int32(sigIdx)) + if !val.PubKey.VerifySignature(voteSignBytes, sig.Signature) { + return fmt.Errorf("wrong signature (#%d): %X", sigIdx, sig.Signature) + } + + return nil +}