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

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

docs/spec/blockchain/state.md

@@ -98,6 +98,10 @@ type Evidence struct {
 type Validator struct {
 	PubKeyTypes []string
 }
+
+type ValidatorParams struct {
+	PubKeyTypes []string
+}
 ```
 
 #### BlockSize

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

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

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)

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

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
 }

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)