From 4571f0fbe8b583c30310e9df5fd0d3d0be79992d Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Wed, 28 Nov 2018 06:09:27 -0800 Subject: [PATCH] Enforce validators can only use the correct pubkey type (#2739) * Enforce validators can only use the correct pubkey type * adapt to variable renames * Address comments from #2636 * separate updating and validation logic * update spec * Add test case for TestStringSliceEqual, clarify slice copying code * Address @ebuchman's comments * Split up testing validator update execution, and its validation --- CHANGELOG_PENDING.md | 1 + docs/spec/blockchain/state.md | 4 ++ libs/common/string.go | 13 ++++++ libs/common/string_test.go | 21 +++++++++ state/execution.go | 35 +++++++++++++-- state/execution_test.go | 83 ++++++++++++++++++++++++++++++----- types/params.go | 27 ++++++------ types/protobuf.go | 13 +++--- 8 files changed, 161 insertions(+), 36 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index f4ade6038..aef86c927 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -20,6 +20,7 @@ program](https://hackerone.com/tendermint). - [db] [\#2913](https://github.com/tendermint/tendermint/pull/2913) ReverseIterator API change -- start < end, and end is exclusive. * Blockchain Protocol + * [state] \#2714 Validators can now only use pubkeys allowed within ConsensusParams.ValidatorParams * P2P Protocol diff --git a/docs/spec/blockchain/state.md b/docs/spec/blockchain/state.md index 0a07890f8..0b46e5035 100644 --- a/docs/spec/blockchain/state.md +++ b/docs/spec/blockchain/state.md @@ -98,6 +98,10 @@ type Evidence struct { type Validator struct { PubKeyTypes []string } + +type ValidatorParams struct { + PubKeyTypes []string +} ``` #### BlockSize diff --git a/libs/common/string.go b/libs/common/string.go index e125043d1..ddf350b10 100644 --- a/libs/common/string.go +++ b/libs/common/string.go @@ -61,3 +61,16 @@ func ASCIITrim(s string) string { } return string(r) } + +// StringSliceEqual checks if string slices a and b are equal +func StringSliceEqual(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := 0; i < len(a); i++ { + if a[i] != b[i] { + return false + } + } + return true +} diff --git a/libs/common/string_test.go b/libs/common/string_test.go index 5e7ae98cc..35b6fafc7 100644 --- a/libs/common/string_test.go +++ b/libs/common/string_test.go @@ -3,6 +3,8 @@ package common import ( "testing" + "github.com/stretchr/testify/require" + "github.com/stretchr/testify/assert" ) @@ -35,3 +37,22 @@ func TestASCIITrim(t *testing.T) { assert.Equal(t, ASCIITrim(" a "), "a") assert.Panics(t, func() { ASCIITrim("\xC2\xA2") }) } + +func TestStringSliceEqual(t *testing.T) { + tests := []struct { + a []string + b []string + want bool + }{ + {[]string{"hello", "world"}, []string{"hello", "world"}, true}, + {[]string{"test"}, []string{"test"}, true}, + {[]string{"test1"}, []string{"test2"}, false}, + {[]string{"hello", "world."}, []string{"hello", "world!"}, false}, + {[]string{"only 1 word"}, []string{"two", "words!"}, false}, + {[]string{"two", "words!"}, []string{"only 1 word"}, false}, + } + for i, tt := range tests { + require.Equal(t, tt.want, StringSliceEqual(tt.a, tt.b), + "StringSliceEqual failed on test %d", i) + } +} diff --git a/state/execution.go b/state/execution.go index 61a48d367..f1fc5e4ad 100644 --- a/state/execution.go +++ b/state/execution.go @@ -107,12 +107,16 @@ func (blockExec *BlockExecutor) ApplyBlock(state State, blockID types.BlockID, b fail.Fail() // XXX - // these are tendermint types now - validatorUpdates, err := types.PB2TM.ValidatorUpdates(abciResponses.EndBlock.ValidatorUpdates) + // validate the validator updates and convert to tendermint types + abciValUpdates := abciResponses.EndBlock.ValidatorUpdates + err = validateValidatorUpdates(abciValUpdates, state.ConsensusParams.Validator) + if err != nil { + return state, fmt.Errorf("Error in validator updates: %v", err) + } + validatorUpdates, err := types.PB2TM.ValidatorUpdates(abciValUpdates) if err != nil { return state, err } - if len(validatorUpdates) > 0 { blockExec.logger.Info("Updates to validators", "updates", makeValidatorUpdatesLogString(validatorUpdates)) } @@ -318,17 +322,40 @@ func getBeginBlockValidatorInfo(block *types.Block, lastValSet *types.ValidatorS } +func validateValidatorUpdates(abciUpdates []abci.ValidatorUpdate, + params types.ValidatorParams) error { + for _, valUpdate := range abciUpdates { + if valUpdate.GetPower() < 0 { + return fmt.Errorf("Voting power can't be negative %v", valUpdate) + } else if valUpdate.GetPower() == 0 { + // continue, since this is deleting the validator, and thus there is no + // pubkey to check + continue + } + + // Check if validator's pubkey matches an ABCI type in the consensus params + thisKeyType := valUpdate.PubKey.Type + if !params.IsValidPubkeyType(thisKeyType) { + return fmt.Errorf("Validator %v is using pubkey %s, which is unsupported for consensus", + valUpdate, thisKeyType) + } + } + return nil +} + // If more or equal than 1/3 of total voting power changed in one block, then // a light client could never prove the transition externally. See // ./lite/doc.go for details on how a light client tracks validators. func updateValidators(currentSet *types.ValidatorSet, updates []*types.Validator) error { for _, valUpdate := range updates { + // should already have been checked if valUpdate.VotingPower < 0 { return fmt.Errorf("Voting power can't be negative %v", valUpdate) } address := valUpdate.Address _, val := currentSet.GetByAddress(address) + // valUpdate.VotingPower is ensured to be non-negative in validation method if valUpdate.VotingPower == 0 { // remove val _, removed := currentSet.Remove(address) @@ -367,7 +394,7 @@ func updateState( // Update the validator set with the latest abciResponses. lastHeightValsChanged := state.LastHeightValidatorsChanged - if len(abciResponses.EndBlock.ValidatorUpdates) > 0 { + if len(validatorUpdates) > 0 { err := updateValidators(nValSet, validatorUpdates) if err != nil { return state, fmt.Errorf("Error changing validator set: %v", err) diff --git a/state/execution_test.go b/state/execution_test.go index 03f521793..21df1ee56 100644 --- a/state/execution_test.go +++ b/state/execution_test.go @@ -12,6 +12,7 @@ import ( "github.com/tendermint/tendermint/abci/example/kvstore" abci "github.com/tendermint/tendermint/abci/types" "github.com/tendermint/tendermint/crypto/ed25519" + "github.com/tendermint/tendermint/crypto/secp256k1" cmn "github.com/tendermint/tendermint/libs/common" dbm "github.com/tendermint/tendermint/libs/db" "github.com/tendermint/tendermint/libs/log" @@ -152,6 +153,76 @@ func TestBeginBlockByzantineValidators(t *testing.T) { } } +func TestValidateValidatorUpdates(t *testing.T) { + pubkey1 := ed25519.GenPrivKey().PubKey() + pubkey2 := ed25519.GenPrivKey().PubKey() + + secpKey := secp256k1.GenPrivKey().PubKey() + + defaultValidatorParams := types.ValidatorParams{[]string{types.ABCIPubKeyTypeEd25519}} + + testCases := []struct { + name string + + abciUpdates []abci.ValidatorUpdate + validatorParams types.ValidatorParams + + shouldErr bool + }{ + { + "adding a validator is OK", + + []abci.ValidatorUpdate{{PubKey: types.TM2PB.PubKey(pubkey2), Power: 20}}, + defaultValidatorParams, + + false, + }, + { + "updating a validator is OK", + + []abci.ValidatorUpdate{{PubKey: types.TM2PB.PubKey(pubkey1), Power: 20}}, + defaultValidatorParams, + + false, + }, + { + "removing a validator is OK", + + []abci.ValidatorUpdate{{PubKey: types.TM2PB.PubKey(pubkey2), Power: 0}}, + defaultValidatorParams, + + false, + }, + { + "adding a validator with negative power results in error", + + []abci.ValidatorUpdate{{PubKey: types.TM2PB.PubKey(pubkey2), Power: -100}}, + defaultValidatorParams, + + true, + }, + { + "adding a validator with pubkey thats not in validator params results in error", + + []abci.ValidatorUpdate{{PubKey: types.TM2PB.PubKey(secpKey), Power: -100}}, + defaultValidatorParams, + + true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + err := validateValidatorUpdates(tc.abciUpdates, tc.validatorParams) + if tc.shouldErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + func TestUpdateValidators(t *testing.T) { pubkey1 := ed25519.GenPrivKey().PubKey() val1 := types.NewValidator(pubkey1, 10) @@ -194,7 +265,6 @@ func TestUpdateValidators(t *testing.T) { types.NewValidatorSet([]*types.Validator{val1}), false, }, - { "removing a non-existing validator results in error", @@ -204,16 +274,6 @@ func TestUpdateValidators(t *testing.T) { types.NewValidatorSet([]*types.Validator{val1}), true, }, - - { - "adding a validator with negative power results in error", - - types.NewValidatorSet([]*types.Validator{val1}), - []abci.ValidatorUpdate{{PubKey: types.TM2PB.PubKey(pubkey2), Power: -100}}, - - types.NewValidatorSet([]*types.Validator{val1}), - true, - }, } for _, tc := range testCases { @@ -224,6 +284,7 @@ func TestUpdateValidators(t *testing.T) { if tc.shouldErr { assert.Error(t, err) } else { + assert.NoError(t, err) require.Equal(t, tc.resultingSet.Size(), tc.currentSet.Size()) assert.Equal(t, tc.resultingSet.TotalVotingPower(), tc.currentSet.TotalVotingPower()) diff --git a/types/params.go b/types/params.go index 81cf429ff..ec8a8f576 100644 --- a/types/params.go +++ b/types/params.go @@ -69,6 +69,15 @@ func DefaultValidatorParams() ValidatorParams { return ValidatorParams{[]string{ABCIPubKeyTypeEd25519}} } +func (params *ValidatorParams) IsValidPubkeyType(pubkeyType string) bool { + for i := 0; i < len(params.PubKeyTypes); i++ { + if params.PubKeyTypes[i] == pubkeyType { + return true + } + } + return false +} + // Validate validates the ConsensusParams to ensure all values are within their // allowed limits, and returns an error if they are not. func (params *ConsensusParams) Validate() error { @@ -124,19 +133,7 @@ func (params *ConsensusParams) Hash() []byte { func (params *ConsensusParams) Equals(params2 *ConsensusParams) bool { return params.BlockSize == params2.BlockSize && params.Evidence == params2.Evidence && - stringSliceEqual(params.Validator.PubKeyTypes, params2.Validator.PubKeyTypes) -} - -func stringSliceEqual(a, b []string) bool { - if len(a) != len(b) { - return false - } - for i := 0; i < len(a); i++ { - if a[i] != b[i] { - return false - } - } - return true + cmn.StringSliceEqual(params.Validator.PubKeyTypes, params2.Validator.PubKeyTypes) } // Update returns a copy of the params with updates from the non-zero fields of p2. @@ -157,7 +154,9 @@ func (params ConsensusParams) Update(params2 *abci.ConsensusParams) ConsensusPar res.Evidence.MaxAge = params2.Evidence.MaxAge } if params2.Validator != nil { - res.Validator.PubKeyTypes = params2.Validator.PubKeyTypes + // Copy params2.Validator.PubkeyTypes, and set result's value to the copy. + // This avoids having to initialize the slice to 0 values, and then write to it again. + res.Validator.PubKeyTypes = append([]string{}, params2.Validator.PubKeyTypes...) } return res } diff --git a/types/protobuf.go b/types/protobuf.go index 1535c1e37..0f0d25de3 100644 --- a/types/protobuf.go +++ b/types/protobuf.go @@ -187,20 +187,19 @@ var PB2TM = pb2tm{} type pb2tm struct{} func (pb2tm) PubKey(pubKey abci.PubKey) (crypto.PubKey, error) { - // TODO: define these in crypto and use them - sizeEd := 32 - sizeSecp := 33 switch pubKey.Type { case ABCIPubKeyTypeEd25519: - if len(pubKey.Data) != sizeEd { - return nil, fmt.Errorf("Invalid size for PubKeyEd25519. Got %d, expected %d", len(pubKey.Data), sizeEd) + if len(pubKey.Data) != ed25519.PubKeyEd25519Size { + return nil, fmt.Errorf("Invalid size for PubKeyEd25519. Got %d, expected %d", + len(pubKey.Data), ed25519.PubKeyEd25519Size) } var pk ed25519.PubKeyEd25519 copy(pk[:], pubKey.Data) return pk, nil case ABCIPubKeyTypeSecp256k1: - if len(pubKey.Data) != sizeSecp { - return nil, fmt.Errorf("Invalid size for PubKeyEd25519. Got %d, expected %d", len(pubKey.Data), sizeSecp) + if len(pubKey.Data) != secp256k1.PubKeySecp256k1Size { + return nil, fmt.Errorf("Invalid size for PubKeySecp256k1. Got %d, expected %d", + len(pubKey.Data), secp256k1.PubKeySecp256k1Size) } var pk secp256k1.PubKeySecp256k1 copy(pk[:], pubKey.Data)