Add validation to prevent empty store keys (#6754)

Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

CHANGELOG.md

@@ -313,6 +313,7 @@ pagination.
 * (types) [\#6195](https://github.com/cosmos/cosmos-sdk/pull/6195) Add codespace to broadcast(sync/async) response.
 * (baseapp) [\#6053](https://github.com/cosmos/cosmos-sdk/pull/6053) Customizable panic recovery handling added for `app.runTx()` method (as proposed in the [ADR 22](https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/adr-022-custom-panic-handling.md)). Adds ability for developers to register custom panic handlers extending standard ones.
 * (store) [\#6481](https://github.com/cosmos/cosmos-sdk/pull/6481) Move `SimpleProofsFromMap` from Tendermint into the SDK.
+* (store) [\#6719](https://github.com/cosmos/cosmos-sdk/6754) Add validity checks to stores for nil and empty keys.
 
 ## [v0.38.5] - 2020-07-02
 

client/testutil/suite.go

@@ -3,11 +3,12 @@ package testutil
 import (
 	"bytes"
 
+	"github.com/stretchr/testify/suite"
+	"github.com/tendermint/tendermint/crypto"
+
 	"github.com/cosmos/cosmos-sdk/crypto/types/multisig"
 	"github.com/cosmos/cosmos-sdk/testutil/testdata"
 	"github.com/cosmos/cosmos-sdk/x/auth/signing"
-	"github.com/stretchr/testify/suite"
-	"github.com/tendermint/tendermint/crypto"
 
 	"github.com/cosmos/cosmos-sdk/client"
 

client/utils.go

@@ -1,9 +1,10 @@
 package client
 
 import (
+	"github.com/spf13/pflag"
+
 	"github.com/cosmos/cosmos-sdk/client/flags"
 	"github.com/cosmos/cosmos-sdk/types/query"
-	"github.com/spf13/pflag"
 )
 
 // Paginate returns the correct starting and ending index for a paginated query,

server/mock/store.go

@@ -131,6 +131,7 @@ func (kv kvStore) Has(key []byte) bool {
 }
 
 func (kv kvStore) Set(key, value []byte) {
+	store.AssertValidKey(key)
 	kv.store[string(key)] = value
 }
 

server/mock/store_test.go

@@ -30,4 +30,6 @@ func TestStore(t *testing.T) {
 	require.Equal(t, v, store.Get(k))
 	store.Delete(k)
 	require.False(t, store.Has(k))
+	require.Panics(t, func() { store.Set([]byte(""), v) }, "setting an empty key should panic")
+	require.Panics(t, func() { store.Set(nil, v) }, "setting a nil key should panic")
 }

store/cachekv/store_test.go

@@ -68,6 +68,8 @@ func TestCacheKVStoreNoNilSet(t *testing.T) {
 	mem := dbadapter.Store{DB: dbm.NewMemDB()}
 	st := cachekv.NewStore(mem)
 	require.Panics(t, func() { st.Set([]byte("key"), nil) }, "setting a nil value should panic")
+	require.Panics(t, func() { st.Set(nil, []byte("value")) }, "setting a nil key should panic")
+	require.Panics(t, func() { st.Set([]byte(""), []byte("value")) }, "setting an empty key should panic")
 }
 
 func TestCacheKVStoreNested(t *testing.T) {

store/dbadapter/store.go

@@ -37,6 +37,7 @@ func (dsa Store) Has(key []byte) bool {
 
 // Set wraps the underlying DB's Set method panicing on error.
 func (dsa Store) Set(key, value []byte) {
+	types.AssertValidKey(key)
 	if err := dsa.DB.Set(key, value); err != nil {
 		panic(err)
 	}

store/dbadapter/store_test.go

@@ -24,6 +24,9 @@ func TestAccessors(t *testing.T) {
 	key := []byte("test")
 	value := []byte("testvalue")
 
+	require.Panics(t, func() { store.Set(nil, []byte("value")) }, "setting a nil key should panic")
+	require.Panics(t, func() { store.Set([]byte(""), []byte("value")) }, "setting an empty key should panic")
+
 	require.Equal(t, types.StoreTypeDB, store.GetStoreType())
 	store.GetStoreType()
 

store/gaskv/store.go

@@ -51,6 +51,7 @@ func (gs *Store) Get(key []byte) (value []byte) {
 func (gs *Store) Set(key []byte, value []byte) {
 	defer telemetry.MeasureSince(time.Now(), "store", "gaskv", "set")
 
+	types.AssertValidKey(key)
 	types.AssertValidValue(value)
 	gs.gasMeter.ConsumeGas(gs.gasConfig.WriteCostFlat, types.GasWriteCostFlatDesc)
 	// TODO overflow-safe math?

store/gaskv/store_test.go

@@ -27,6 +27,9 @@ func TestGasKVStoreBasic(t *testing.T) {
 	require.Panics(t, func() { st.CacheWrap() })
 	require.Panics(t, func() { st.CacheWrapWithTrace(nil, nil) })
 
+	require.Panics(t, func() { st.Set(nil, []byte("value")) }, "setting a nil key should panic")
+	require.Panics(t, func() { st.Set([]byte(""), []byte("value")) }, "setting an empty key should panic")
+
 	require.Empty(t, st.Get(keyFmt(1)), "Expected `key1` to be empty")
 	st.Set(keyFmt(1), valFmt(1))
 	require.Equal(t, valFmt(1), st.Get(keyFmt(1)))

store/iavl/store.go

@@ -147,6 +147,7 @@ func (st *Store) CacheWrapWithTrace(w io.Writer, tc types.TraceContext) types.Ca
 // Implements types.KVStore.
 func (st *Store) Set(key, value []byte) {
 	defer telemetry.MeasureSince(time.Now(), "store", "iavl", "set")
+	types.AssertValidKey(key)
 	types.AssertValidValue(value)
 	st.tree.Set(key, value)
 }

store/iavl/store_test.go

@@ -131,6 +131,10 @@ func TestIAVLStoreNoNilSet(t *testing.T) {
 	db := dbm.NewMemDB()
 	tree, _ := newAlohaTree(t, db)
 	iavlStore := UnsafeNewStore(tree)
+
+	require.Panics(t, func() { iavlStore.Set(nil, []byte("value")) }, "setting a nil key should panic")
+	require.Panics(t, func() { iavlStore.Set([]byte(""), []byte("value")) }, "setting an empty key should panic")
+
 	require.Panics(t, func() { iavlStore.Set([]byte("key"), nil) }, "setting a nil value should panic")
 }
 

store/prefix/store_test.go

@@ -246,7 +246,6 @@ func mockStoreWithStuff() types.KVStore {
 	store.Set(bz("key2"), bz("value2"))
 	store.Set(bz("key3"), bz("value3"))
 	store.Set(bz("something"), bz("else"))
-	store.Set(bz(""), bz(""))
 	store.Set(bz("k"), bz(sdk.PrefixValidator))
 	store.Set(bz("ke"), bz("valu"))
 	store.Set(bz("kee"), bz("valuu"))

store/rootmulti/internal/maps/maps.go

@@ -6,6 +6,8 @@ import (
 	"github.com/tendermint/tendermint/crypto/merkle"
 	"github.com/tendermint/tendermint/crypto/tmhash"
 	"github.com/tendermint/tendermint/libs/kv"
+
+	"github.com/cosmos/cosmos-sdk/store/types"
 )
 
 // merkleMap defines a merkle-ized tree from a map. Leave values are treated as
@@ -26,6 +28,9 @@ func newMerkleMap() *merkleMap {
 // to creating a kv.Pair. The created kv.Pair is appended to the MerkleMap's slice
 // of kv.Pairs. Whenever called, the MerkleMap must be resorted.
 func (sm *merkleMap) set(key string, value []byte) {
+	byteKey := []byte(key)
+	types.AssertValidKey(byteKey)
+
 	sm.sorted = false
 
 	// The value is hashed, so you can check for equality with a cached value (say)
@@ -33,7 +38,7 @@ func (sm *merkleMap) set(key string, value []byte) {
 	vhash := tmhash.Sum(value)
 
 	sm.kvs = append(sm.kvs, kv.Pair{
-		Key:   []byte(key),
+		Key:   byteKey,
 		Value: vhash,
 	})
 }
@@ -84,6 +89,8 @@ func newSimpleMap() *simpleMap {
 // Set creates a kv pair of the key and the hash of the value,
 // and then appends it to SimpleMap's kv pairs.
 func (sm *simpleMap) Set(key string, value []byte) {
+	byteKey := []byte(key)
+	types.AssertValidKey(byteKey)
 	sm.sorted = false
 
 	// The value is hashed, so you can
@@ -92,7 +99,7 @@ func (sm *simpleMap) Set(key string, value []byte) {
 	vhash := tmhash.Sum(value)
 
 	sm.Kvs = append(sm.Kvs, kv.Pair{
-		Key:   []byte(key),
+		Key:   byteKey,
 		Value: vhash,
 	})
 }

store/rootmulti/internal/maps/maps_test.go

@@ -5,8 +5,13 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
+func TestEmptyKeyMerkleMap(t *testing.T) {
+	db := newMerkleMap()
+	require.Panics(t, func() { db.set("", []byte("value")) }, "setting an empty key should panic")
+}
 func TestMerkleMap(t *testing.T) {
 	tests := []struct {
 		keys   []string
@@ -49,6 +54,10 @@ func TestMerkleMap(t *testing.T) {
 	}
 }
 
+func TestEmptyKeySimpleMap(t *testing.T) {
+	db := newSimpleMap()
+	require.Panics(t, func() { db.Set("", []byte("value")) }, "setting an empty key should panic")
+}
 func TestSimpleMap(t *testing.T) {
 	tests := []struct {
 		keys   []string

store/tracekv/store.go

@@ -60,6 +60,7 @@ func (tkv *Store) Get(key []byte) []byte {
 // Set implements the KVStore interface. It traces a write operation and
 // delegates the Set call to the parent KVStore.
 func (tkv *Store) Set(key []byte, value []byte) {
+	types.AssertValidKey(key)
 	writeOperation(tkv.writer, writeOp, tkv.context, key, value)
 	tkv.parent.Set(key, value)
 }

store/tracekv/store_test.go

@@ -85,16 +85,21 @@ func TestTraceKVStoreSet(t *testing.T) {
 		value       []byte
 		expectedOut string
 	}{
-		{
-			key:         []byte{},
-			value:       nil,
-			expectedOut: "{\"operation\":\"write\",\"key\":\"\",\"value\":\"\",\"metadata\":{\"blockHeight\":64}}\n",
-		},
 		{
 			key:         kvPairs[0].Key,
 			value:       kvPairs[0].Value,
 			expectedOut: "{\"operation\":\"write\",\"key\":\"a2V5MDAwMDAwMDE=\",\"value\":\"dmFsdWUwMDAwMDAwMQ==\",\"metadata\":{\"blockHeight\":64}}\n",
 		},
+		{
+			key:         kvPairs[1].Key,
+			value:       kvPairs[1].Value,
+			expectedOut: "{\"operation\":\"write\",\"key\":\"a2V5MDAwMDAwMDI=\",\"value\":\"dmFsdWUwMDAwMDAwMg==\",\"metadata\":{\"blockHeight\":64}}\n",
+		},
+		{
+			key:         kvPairs[2].Key,
+			value:       kvPairs[2].Value,
+			expectedOut: "{\"operation\":\"write\",\"key\":\"a2V5MDAwMDAwMDM=\",\"value\":\"dmFsdWUwMDAwMDAwMw==\",\"metadata\":{\"blockHeight\":64}}\n",
+		},
 	}
 
 	for _, tc := range testCases {
@@ -106,6 +111,12 @@ func TestTraceKVStoreSet(t *testing.T) {
 
 		require.Equal(t, tc.expectedOut, buf.String())
 	}
+
+	var buf bytes.Buffer
+	store := newEmptyTraceKVStore(&buf)
+	require.Panics(t, func() { store.Set([]byte(""), []byte("value")) }, "setting an empty key should panic")
+	require.Panics(t, func() { store.Set(nil, []byte("value")) }, "setting a nil key should panic")
+
 }
 
 func TestTraceKVStoreDelete(t *testing.T) {

store/types/store.go

@@ -320,6 +320,9 @@ type KVStoreKey struct {
 // NewKVStoreKey returns a new pointer to a KVStoreKey.
 // Use a pointer so keys don't collide.
 func NewKVStoreKey(name string) *KVStoreKey {
+	if name == "" {
+		panic("empty key name not allowed")
+	}
 	return &KVStoreKey{
 		name: name,
 	}

store/types/store_test.go

@@ -75,6 +75,14 @@ func TestKVStoreKey(t *testing.T) {
 	require.Equal(t, fmt.Sprintf("KVStoreKey{%p, test}", key), key.String())
 }
 
+func TestNilKVStoreKey(t *testing.T) {
+	t.Parallel()
+
+	require.Panics(t, func() {
+		_ = NewKVStoreKey("")
+	}, "setting an empty key should panic")
+}
+
 func TestTransientStoreKey(t *testing.T) {
 	t.Parallel()
 	key := NewTransientStoreKey("test")

store/types/validity.go

@@ -2,7 +2,7 @@ package types
 
 // Check if the key is valid(key is not nil)
 func AssertValidKey(key []byte) {
-	if key == nil {
+	if len(key) == 0 {
 		panic("key is nil")
 	}
 }

store/types/validity_test.go

@@ -10,8 +10,8 @@ import (
 
 func TestAssertValidKey(t *testing.T) {
 	t.Parallel()
-	require.NotPanics(t, func() { types.AssertValidKey([]byte{}) })
 	require.NotPanics(t, func() { types.AssertValidKey([]byte{0x01}) })
+	require.Panics(t, func() { types.AssertValidKey([]byte{}) })
 	require.Panics(t, func() { types.AssertValidKey(nil) })
 }
 

testutil/testdata/test_tx.go

@@ -3,9 +3,10 @@ package testdata
 import (
 	"encoding/json"
 
-	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/tendermint/tendermint/crypto"
 	"github.com/tendermint/tendermint/crypto/secp256k1"
+
+	sdk "github.com/cosmos/cosmos-sdk/types"
 )
 
 // KeyTestPubAddr generates a new secp256k1 keypair.

x/auth/ante/basic_test.go

@@ -5,9 +5,10 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/cosmos/cosmos-sdk/testutil/testdata"
 	"github.com/stretchr/testify/suite"
 
+	"github.com/cosmos/cosmos-sdk/testutil/testdata"
+
 	"github.com/tendermint/tendermint/crypto"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"

x/auth/ante/fee_test.go

@@ -3,9 +3,10 @@ package ante_test
 import (
 	"testing"
 
-	"github.com/cosmos/cosmos-sdk/testutil/testdata"
 	"github.com/stretchr/testify/suite"
 
+	"github.com/cosmos/cosmos-sdk/testutil/testdata"
+
 	"github.com/tendermint/tendermint/crypto"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"

x/auth/ante/setup_test.go

@@ -3,9 +3,10 @@ package ante_test
 import (
 	"testing"
 
-	"github.com/cosmos/cosmos-sdk/testutil/testdata"
 	"github.com/stretchr/testify/suite"
 
+	"github.com/cosmos/cosmos-sdk/testutil/testdata"
+
 	"github.com/tendermint/tendermint/crypto"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"

x/auth/ante/sigverify_test.go

@@ -4,9 +4,10 @@ import (
 	"fmt"
 	"testing"
 
-	"github.com/cosmos/cosmos-sdk/testutil/testdata"
 	"github.com/stretchr/testify/suite"
 
+	"github.com/cosmos/cosmos-sdk/testutil/testdata"
+
 	"github.com/cosmos/cosmos-sdk/types/tx/signing"
 
 	"github.com/cosmos/cosmos-sdk/simapp"

x/auth/signing/sig_verifiable_tx.go

@@ -1,9 +1,10 @@
 package signing
 
 import (
+	"github.com/tendermint/tendermint/crypto"
+
 	"github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/types/tx/signing"
-	"github.com/tendermint/tendermint/crypto"
 )
 
 // SigVerifiableTx defines a Tx interface for all signature verification decorators

x/distribution/keeper/grpc_query.go

@@ -3,14 +3,15 @@ package keeper
 import (
 	"context"
 
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+
 	"github.com/cosmos/cosmos-sdk/store/prefix"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 	"github.com/cosmos/cosmos-sdk/types/query"
 	"github.com/cosmos/cosmos-sdk/x/distribution/types"
 	"github.com/cosmos/cosmos-sdk/x/staking/exported"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
 var _ types.QueryServer = Keeper{}

x/distribution/keeper/grpc_query_test.go

@@ -5,6 +5,9 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/stretchr/testify/suite"
+	abci "github.com/tendermint/tendermint/abci/types"
+
 	"github.com/cosmos/cosmos-sdk/baseapp"
 	"github.com/cosmos/cosmos-sdk/simapp"
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -12,8 +15,6 @@ import (
 	"github.com/cosmos/cosmos-sdk/x/distribution/types"
 	"github.com/cosmos/cosmos-sdk/x/staking"
 	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
-	"github.com/stretchr/testify/suite"
-	abci "github.com/tendermint/tendermint/abci/types"
 )
 
 type KeeperTestSuite struct {

x/gov/keeper/grpc_query.go

@@ -3,12 +3,13 @@ package keeper
 import (
 	"context"
 
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+
 	"github.com/cosmos/cosmos-sdk/store/prefix"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/types/query"
 	"github.com/cosmos/cosmos-sdk/x/gov/types"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
 var _ types.QueryServer = Keeper{}

x/ibc/types/query.go

@@ -1,11 +1,12 @@
 package types
 
 import (
+	"github.com/gogo/protobuf/grpc"
+
 	connection "github.com/cosmos/cosmos-sdk/x/ibc/03-connection"
 	connectiontypes "github.com/cosmos/cosmos-sdk/x/ibc/03-connection/types"
 	channel "github.com/cosmos/cosmos-sdk/x/ibc/04-channel"
 	channeltypes "github.com/cosmos/cosmos-sdk/x/ibc/04-channel/types"
-	"github.com/gogo/protobuf/grpc"
 )
 
 // QueryServer defines the IBC interfaces that the gRPC query server must implement

x/mint/keeper/grpc_query_test.go

@@ -4,12 +4,13 @@ import (
 	gocontext "context"
 	"testing"
 
+	"github.com/stretchr/testify/suite"
+	abci "github.com/tendermint/tendermint/abci/types"
+
 	"github.com/cosmos/cosmos-sdk/baseapp"
 	"github.com/cosmos/cosmos-sdk/simapp"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/x/mint/types"
-	"github.com/stretchr/testify/suite"
-	abci "github.com/tendermint/tendermint/abci/types"
 )
 
 type MintTestSuite struct {

x/staking/keeper/grpc_query_test.go

@@ -583,7 +583,7 @@ func (suite *KeeperTestSuite) TestGRPCQueryRedelegation() {
 		expPass  bool
 		expErr   bool
 	}{
-		{"request redelegations for non existant addr",
+		{"request redelegations for non existent addr",
 			func() {
 				req = &types.QueryRedelegationsRequest{DelegatorAddr: addrAcc}
 			},