diff --git a/CHANGELOG.md b/CHANGELOG.md index bbe06901e..efaf660f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,6 +65,9 @@ Ref: https://keepachangelog.com/en/1.0.0/ * `server/types.AppExporter` requires extra argument: `AppOptions`. * `server.AddCommands` requires extra argument: `addStartFlags types.ModuleInitFlags` * `x/crisis.NewAppModule` has a new attribute: `skipGenesisInvariants`. [PR](https://github.com/cosmos/cosmos-sdk/pull/7764) +* [#7918](https://github.com/cosmos/cosmos-sdk/pull/7918) Add x/capability safety checks: + * All outward facing APIs will now check that capability is not nil and name is not empty before performing any state-machine changes + * `SetIndex` has been renamed to `InitializeIndex` ### Features diff --git a/x/capability/genesis.go b/x/capability/genesis.go index 9bcca42a3..ba8e09dcd 100644 --- a/x/capability/genesis.go +++ b/x/capability/genesis.go @@ -9,7 +9,7 @@ import ( // InitGenesis initializes the capability module's state from a provided genesis // state. func InitGenesis(ctx sdk.Context, k keeper.Keeper, genState types.GenesisState) { - if err := k.SetIndex(ctx, genState.Index); err != nil { + if err := k.InitializeIndex(ctx, genState.Index); err != nil { panic(err) } diff --git a/x/capability/keeper/keeper.go b/x/capability/keeper/keeper.go index 492d56c91..61de03389 100644 --- a/x/capability/keeper/keeper.go +++ b/x/capability/keeper/keeper.go @@ -2,6 +2,7 @@ package keeper import ( "fmt" + "strings" "github.com/tendermint/tendermint/libs/log" @@ -49,6 +50,8 @@ type ( } ) +// NewKeeper constructs a new CapabilityKeeper instance and initializes maps +// for capability map and scopedModules map. func NewKeeper(cdc codec.BinaryMarshaler, storeKey, memKey sdk.StoreKey) *Keeper { return &Keeper{ cdc: cdc, @@ -67,6 +70,9 @@ func (k *Keeper) ScopeToModule(moduleName string) ScopedKeeper { if k.sealed { panic("cannot scope to module via a sealed capability keeper") } + if strings.TrimSpace(moduleName) == "" { + panic("cannot scope to an empty module name") + } if _, ok := k.scopedModules[moduleName]; ok { panic(fmt.Sprintf("cannot create multiple scoped keepers for the same module name: %s", moduleName)) @@ -117,10 +123,10 @@ func (k *Keeper) InitializeAndSeal(ctx sdk.Context) { k.sealed = true } -// SetIndex sets the index to one (or greater) in InitChain according +// InitializeIndex sets the index to one (or greater) in InitChain according // to the GenesisState. It must only be called once. // It will panic if the provided index is 0, or if the index is already set. -func (k Keeper) SetIndex(ctx sdk.Context, index uint64) error { +func (k Keeper) InitializeIndex(ctx sdk.Context, index uint64) error { if index == 0 { panic("SetIndex requires index > 0") } @@ -200,6 +206,9 @@ func (k Keeper) InitializeCapability(ctx sdk.Context, index uint64, owners types // Note, namespacing is completely local, which is safe since records are prefixed // with the module name and no two ScopedKeeper can have the same module name. func (sk ScopedKeeper) NewCapability(ctx sdk.Context, name string) (*types.Capability, error) { + if strings.TrimSpace(name) == "" { + return nil, sdkerrors.Wrap(types.ErrInvalidCapabilityName, "capability name cannot be empty") + } store := ctx.KVStore(sk.storeKey) if _, ok := sk.GetCapability(ctx, name); ok { @@ -247,6 +256,9 @@ func (sk ScopedKeeper) NewCapability(ctx sdk.Context, name string) (*types.Capab // Note, the capability's forward mapping is indexed by a string which should // contain its unique memory reference. func (sk ScopedKeeper) AuthenticateCapability(ctx sdk.Context, cap *types.Capability, name string) bool { + if strings.TrimSpace(name) == "" || cap == nil { + return false + } return sk.GetCapabilityName(ctx, cap) == name } @@ -256,6 +268,12 @@ func (sk ScopedKeeper) AuthenticateCapability(ctx sdk.Context, cap *types.Capabi // index. If the owner already exists, it will return an error. Otherwise, it will // also set a forward and reverse index for the capability and capability name. func (sk ScopedKeeper) ClaimCapability(ctx sdk.Context, cap *types.Capability, name string) error { + if cap == nil { + return sdkerrors.Wrap(types.ErrNilCapability, "cannot claim nil capability") + } + if strings.TrimSpace(name) == "" { + return sdkerrors.Wrap(types.ErrInvalidCapabilityName, "capability name cannot be empty") + } // update capability owner set if err := sk.addOwner(ctx, cap, name); err != nil { return err @@ -282,6 +300,9 @@ func (sk ScopedKeeper) ClaimCapability(ctx sdk.Context, cap *types.Capability, n // previously claimed or created. After releasing the capability, if no more // owners exist, the capability will be globally removed. func (sk ScopedKeeper) ReleaseCapability(ctx sdk.Context, cap *types.Capability) error { + if cap == nil { + return sdkerrors.Wrap(types.ErrNilCapability, "cannot release nil capability") + } name := sk.GetCapabilityName(ctx, cap) if len(name) == 0 { return sdkerrors.Wrap(types.ErrCapabilityNotOwned, sk.module) @@ -321,6 +342,9 @@ func (sk ScopedKeeper) ReleaseCapability(ctx sdk.Context, cap *types.Capability) // by name. The module is not allowed to retrieve capabilities which it does not // own. func (sk ScopedKeeper) GetCapability(ctx sdk.Context, name string) (*types.Capability, bool) { + if strings.TrimSpace(name) == "" { + return nil, false + } memStore := ctx.KVStore(sk.memKey) key := types.RevCapabilityKey(sk.module, name) @@ -332,15 +356,14 @@ func (sk ScopedKeeper) GetCapability(ctx sdk.Context, name string) (*types.Capab // to still have the capability in the go map since changes to // go map do not automatically get reverted on tx failure, // so we delete here to remove unnecessary values in map - delete(sk.capMap, index) + // TODO: Delete index correctly from capMap by storing some reverse lookup + // in-memory map. Issue: https://github.com/cosmos/cosmos-sdk/issues/7805 return nil, false } cap := sk.capMap[index] if cap == nil { - // delete key from store to remove unnecessary mapping - memStore.Delete(key) - return nil, false + panic("capability found in memstore is missing from map") } return cap, true @@ -349,14 +372,20 @@ func (sk ScopedKeeper) GetCapability(ctx sdk.Context, name string) (*types.Capab // GetCapabilityName allows a module to retrieve the name under which it stored a given // capability given the capability func (sk ScopedKeeper) GetCapabilityName(ctx sdk.Context, cap *types.Capability) string { + if cap == nil { + return "" + } memStore := ctx.KVStore(sk.memKey) return string(memStore.Get(types.FwdCapabilityKey(sk.module, cap))) } -// Get all the Owners that own the capability associated with the name this ScopedKeeper uses +// GetOwners all the Owners that own the capability associated with the name this ScopedKeeper uses // to refer to the capability func (sk ScopedKeeper) GetOwners(ctx sdk.Context, name string) (*types.CapabilityOwners, bool) { + if strings.TrimSpace(name) == "" { + return nil, false + } cap, ok := sk.GetCapability(ctx, name) if !ok { return nil, false @@ -382,6 +411,9 @@ func (sk ScopedKeeper) GetOwners(ctx sdk.Context, name string) (*types.Capabilit // The method returns an error if either the capability or the owners cannot be // retreived from the memstore. func (sk ScopedKeeper) LookupModules(ctx sdk.Context, name string) ([]string, *types.Capability, error) { + if strings.TrimSpace(name) == "" { + return nil, nil, sdkerrors.Wrap(types.ErrInvalidCapabilityName, "cannot lookup modules with empty capability name") + } cap, ok := sk.GetCapability(ctx, name) if !ok { return nil, nil, sdkerrors.Wrap(types.ErrCapabilityNotFound, name) diff --git a/x/capability/keeper/keeper_test.go b/x/capability/keeper/keeper_test.go index c6f1d174b..e62176a72 100644 --- a/x/capability/keeper/keeper_test.go +++ b/x/capability/keeper/keeper_test.go @@ -38,6 +38,9 @@ func (suite *KeeperTestSuite) SetupTest() { func (suite *KeeperTestSuite) TestInitializeAndSeal() { sk := suite.keeper.ScopeToModule(banktypes.ModuleName) + suite.Require().Panics(func() { + suite.keeper.ScopeToModule(" ") + }) caps := make([]*types.Capability, 5) // Get Latest Index before creating new ones to sychronize indices correctly @@ -105,6 +108,10 @@ func (suite *KeeperTestSuite) TestNewCapability() { suite.Require().True(ok) suite.Require().Equal(cap, got) suite.Require().True(cap == got, "expected memory addresses to be equal") + + cap, err = sk.NewCapability(suite.ctx, " ") + suite.Require().Error(err) + suite.Require().Nil(cap) } func (suite *KeeperTestSuite) TestOriginalCapabilityKeeper() { @@ -151,11 +158,15 @@ func (suite *KeeperTestSuite) TestAuthenticateCapability() { badCap := types.NewCapability(100) suite.Require().False(sk1.AuthenticateCapability(suite.ctx, badCap, "transfer")) suite.Require().False(sk2.AuthenticateCapability(suite.ctx, badCap, "bond")) + + suite.Require().False(sk1.AuthenticateCapability(suite.ctx, cap1, " ")) + suite.Require().False(sk1.AuthenticateCapability(suite.ctx, nil, "transfer")) } func (suite *KeeperTestSuite) TestClaimCapability() { sk1 := suite.keeper.ScopeToModule(banktypes.ModuleName) sk2 := suite.keeper.ScopeToModule(stakingtypes.ModuleName) + sk3 := suite.keeper.ScopeToModule("foo") cap, err := sk1.NewCapability(suite.ctx, "transfer") suite.Require().NoError(err) @@ -171,6 +182,9 @@ func (suite *KeeperTestSuite) TestClaimCapability() { got, ok = sk2.GetCapability(suite.ctx, "transfer") suite.Require().True(ok) suite.Require().Equal(cap, got) + + suite.Require().Error(sk3.ClaimCapability(suite.ctx, cap, " ")) + suite.Require().Error(sk3.ClaimCapability(suite.ctx, nil, "transfer")) } func (suite *KeeperTestSuite) TestGetOwners() { @@ -237,6 +251,8 @@ func (suite *KeeperTestSuite) TestGetOwners() { } } + _, ok := sk1.GetOwners(suite.ctx, " ") + suite.Require().False(ok, "got owners from empty capability name") } func (suite *KeeperTestSuite) TestReleaseCapability() { @@ -264,6 +280,8 @@ func (suite *KeeperTestSuite) TestReleaseCapability() { got, ok = sk1.GetCapability(suite.ctx, "transfer") suite.Require().False(ok) suite.Require().Nil(got) + + suite.Require().Error(sk1.ReleaseCapability(suite.ctx, nil)) } func (suite KeeperTestSuite) TestRevertCapability() { diff --git a/x/capability/types/errors.go b/x/capability/types/errors.go index 93f185426..7c582ccbb 100644 --- a/x/capability/types/errors.go +++ b/x/capability/types/errors.go @@ -8,9 +8,11 @@ import ( // x/capability module sentinel errors var ( - ErrCapabilityTaken = sdkerrors.Register(ModuleName, 2, "capability name already taken") - ErrOwnerClaimed = sdkerrors.Register(ModuleName, 3, "given owner already claimed capability") - ErrCapabilityNotOwned = sdkerrors.Register(ModuleName, 4, "capability not owned by module") - ErrCapabilityNotFound = sdkerrors.Register(ModuleName, 5, "capability not found") - ErrCapabilityOwnersNotFound = sdkerrors.Register(ModuleName, 6, "owners not found for capability") + ErrInvalidCapabilityName = sdkerrors.Register(ModuleName, 2, "capability name not valid") + ErrNilCapability = sdkerrors.Register(ModuleName, 3, "provided capability is nil") + ErrCapabilityTaken = sdkerrors.Register(ModuleName, 4, "capability name already taken") + ErrOwnerClaimed = sdkerrors.Register(ModuleName, 5, "given owner already claimed capability") + ErrCapabilityNotOwned = sdkerrors.Register(ModuleName, 6, "capability not owned by module") + ErrCapabilityNotFound = sdkerrors.Register(ModuleName, 7, "capability not found") + ErrCapabilityOwnersNotFound = sdkerrors.Register(ModuleName, 8, "owners not found for capability") ) diff --git a/x/ibc/light-clients/07-tendermint/types/misbehaviour_handle.go b/x/ibc/light-clients/07-tendermint/types/misbehaviour_handle.go index e3793ee86..b27cc06a4 100644 --- a/x/ibc/light-clients/07-tendermint/types/misbehaviour_handle.go +++ b/x/ibc/light-clients/07-tendermint/types/misbehaviour_handle.go @@ -108,7 +108,7 @@ func checkMisbehaviourHeader( chainID, _ = clienttypes.SetVersionNumber(chainID, header.GetHeight().GetVersionNumber()) } - // - ValidatorSet must have 2/3 similarity with trusted FromValidatorSet + // - ValidatorSet must have TrustLevel similarity with trusted FromValidatorSet // - ValidatorSets on both headers are valid given the last trusted ValidatorSet if err := tmTrustedValset.VerifyCommitLightTrusting( chainID, tmCommit, clientState.TrustLevel.ToTendermint(),