From 3edee0a9296083ef61bfc4aa49d1a3a3b559d532 Mon Sep 17 00:00:00 2001 From: Riccardo Montagnin Date: Mon, 4 Apr 2022 12:50:30 +0200 Subject: [PATCH] fix: sanitize permissions (#801) ## Description This PR adds permissions sanitation before storing the permissions while handling a `MsgSetUserGroupPermissions` or `MsgSetUserPermissions`. Special review required by @manu0466 since he was the one to report the bug (and suggest a solution). Closes: #800 --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/desmos-labs/desmos/blob/master/CONTRIBUTING.md#pr-targeting)) - [x] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://docs.cosmos.network/v0.44/building-modules/intro.html) - [x] included the necessary unit and integration [tests](https://github.com/desmos-labs/desmos/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [x] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable) --- ...1467a646116fddc2952e79e559b1d4cc58d79.yaml | 6 ++ testutil/context.go | 32 +++++++ x/profiles/legacy/v5/store_test.go | 32 +------ x/subspaces/keeper/migrations.go | 26 ++++++ x/subspaces/keeper/msg_server.go | 10 +++ x/subspaces/keeper/msg_server_test.go | 59 +++++++++++- x/subspaces/legacy/v2/store.go | 87 ++++++++++++++++++ x/subspaces/legacy/v2/store_test.go | 90 +++++++++++++++++++ x/subspaces/module.go | 12 ++- x/subspaces/types/permissions.go | 15 ++++ x/subspaces/types/permissions_test.go | 74 +++++++++++++++ 11 files changed, 411 insertions(+), 32 deletions(-) create mode 100644 .changeset/entries/6e78e6f7f9486799935df288f3d1467a646116fddc2952e79e559b1d4cc58d79.yaml create mode 100644 testutil/context.go create mode 100644 x/subspaces/keeper/migrations.go create mode 100644 x/subspaces/legacy/v2/store.go create mode 100644 x/subspaces/legacy/v2/store_test.go diff --git a/.changeset/entries/6e78e6f7f9486799935df288f3d1467a646116fddc2952e79e559b1d4cc58d79.yaml b/.changeset/entries/6e78e6f7f9486799935df288f3d1467a646116fddc2952e79e559b1d4cc58d79.yaml new file mode 100644 index 0000000000..13ed43b160 --- /dev/null +++ b/.changeset/entries/6e78e6f7f9486799935df288f3d1467a646116fddc2952e79e559b1d4cc58d79.yaml @@ -0,0 +1,6 @@ +type: fix +module: x/subspaces +pull_request: 801 +description: Added permissions sanitization +backward_compatible: true +date: 2022-04-01T09:09:14.889652713Z diff --git a/testutil/context.go b/testutil/context.go new file mode 100644 index 0000000000..a9ac012733 --- /dev/null +++ b/testutil/context.go @@ -0,0 +1,32 @@ +package testutil + +import ( + "github.com/cosmos/cosmos-sdk/store" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/tendermint/tendermint/libs/log" + tmproto "github.com/tendermint/tendermint/proto/tendermint/types" + dbm "github.com/tendermint/tm-db" +) + +func BuildContext( + keys map[string]*sdk.KVStoreKey, tKeys map[string]*sdk.TransientStoreKey, memKeys map[string]*sdk.MemoryStoreKey, +) sdk.Context { + db := dbm.NewMemDB() + cms := store.NewCommitMultiStore(db) + for _, key := range keys { + cms.MountStoreWithDB(key, sdk.StoreTypeIAVL, db) + } + for _, tKey := range tKeys { + cms.MountStoreWithDB(tKey, sdk.StoreTypeTransient, db) + } + for _, memKey := range memKeys { + cms.MountStoreWithDB(memKey, sdk.StoreTypeMemory, nil) + } + + err := cms.LoadLatestVersion() + if err != nil { + panic(err) + } + + return sdk.NewContext(cms, tmproto.Header{}, false, log.NewNopLogger()) +} diff --git a/x/profiles/legacy/v5/store_test.go b/x/profiles/legacy/v5/store_test.go index 9e6f1b3669..5f6bf90f73 100644 --- a/x/profiles/legacy/v5/store_test.go +++ b/x/profiles/legacy/v5/store_test.go @@ -12,44 +12,16 @@ import ( "github.com/desmos-labs/desmos/v3/testutil" profilestypes "github.com/desmos-labs/desmos/v3/x/profiles/types" - "github.com/cosmos/cosmos-sdk/store" + sdk "github.com/cosmos/cosmos-sdk/types" authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" paramstypes "github.com/cosmos/cosmos-sdk/x/params/types" - "github.com/tendermint/tendermint/libs/log" - tmproto "github.com/tendermint/tendermint/proto/tendermint/types" - dbm "github.com/tendermint/tm-db" - - sdk "github.com/cosmos/cosmos-sdk/types" "github.com/stretchr/testify/require" "github.com/desmos-labs/desmos/v3/app" "github.com/desmos-labs/desmos/v3/x/relationships/types" ) -func buildContext( - keys map[string]*sdk.KVStoreKey, tKeys map[string]*sdk.TransientStoreKey, memKeys map[string]*sdk.MemoryStoreKey, -) sdk.Context { - db := dbm.NewMemDB() - cms := store.NewCommitMultiStore(db) - for _, key := range keys { - cms.MountStoreWithDB(key, sdk.StoreTypeIAVL, db) - } - for _, tKey := range tKeys { - cms.MountStoreWithDB(tKey, sdk.StoreTypeTransient, db) - } - for _, memKey := range memKeys { - cms.MountStoreWithDB(memKey, sdk.StoreTypeMemory, nil) - } - - err := cms.LoadLatestVersion() - if err != nil { - panic(err) - } - - return sdk.NewContext(cms, tmproto.Header{}, false, log.NewNopLogger()) -} - func TestMigrateStore(t *testing.T) { cdc, _ := app.MakeCodecs() @@ -154,7 +126,7 @@ func TestMigrateStore(t *testing.T) { for _, tc := range testCases { tc := tc t.Run(tc.name, func(t *testing.T) { - ctx := buildContext(keys, tKeys, memKeys) + ctx := testutil.BuildContext(keys, tKeys, memKeys) if tc.store != nil { tc.store(ctx) } diff --git a/x/subspaces/keeper/migrations.go b/x/subspaces/keeper/migrations.go new file mode 100644 index 0000000000..45b0d72d56 --- /dev/null +++ b/x/subspaces/keeper/migrations.go @@ -0,0 +1,26 @@ +package keeper + +import ( + sdk "github.com/cosmos/cosmos-sdk/types" + + v2 "github.com/desmos-labs/desmos/v3/x/subspaces/legacy/v2" +) + +// DONTCOVER + +// Migrator is a struct for handling in-place store migrations. +type Migrator struct { + keeper Keeper +} + +// NewMigrator returns a new Migrator +func NewMigrator(keeper Keeper) Migrator { + return Migrator{ + keeper: keeper, + } +} + +// Migrate1to2 migrates from version 1 to 2. +func (m Migrator) Migrate1to2(ctx sdk.Context) error { + return v2.MigrateStore(ctx, m.keeper.storeKey, m.keeper.cdc) +} diff --git a/x/subspaces/keeper/msg_server.go b/x/subspaces/keeper/msg_server.go index 4602c56475..5e728da715 100644 --- a/x/subspaces/keeper/msg_server.go +++ b/x/subspaces/keeper/msg_server.go @@ -276,6 +276,11 @@ func (k msgServer) SetUserGroupPermissions(goCtx context.Context, msg *types.Msg return nil, sdkerrors.Wrapf(types.ErrPermissionDenied, "you cannot manage permissions in this subspace") } + // Make sure the permission is valid + if !types.IsPermissionValid(msg.Permissions) { + return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "invalid permission value") + } + // Set the group permissions and store the group group.Permissions = msg.Permissions k.SaveUserGroup(ctx, group) @@ -474,6 +479,11 @@ func (k msgServer) SetUserPermissions(goCtx context.Context, msg *types.MsgSetUs return nil, sdkerrors.Wrapf(types.ErrPermissionDenied, "you cannot manage permissions in this subspace") } + // Make sure the permission is valid + if !types.IsPermissionValid(msg.Permissions) { + return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "invalid permission value") + } + // Set the permissions if msg.Permissions == types.PermissionNothing { // Remove the permission to clear the store if PermissionNothing is used diff --git a/x/subspaces/keeper/msg_server_test.go b/x/subspaces/keeper/msg_server_test.go index e344b335ed..4185ed86f4 100644 --- a/x/subspaces/keeper/msg_server_test.go +++ b/x/subspaces/keeper/msg_server_test.go @@ -914,7 +914,39 @@ func (suite *KeeperTestsuite) TestMsgServer_SetUserGroupPermissions() { shouldErr: true, }, { - name: "existing group is deleted properly", + name: "invalid permission value returns error", + store: func(ctx sdk.Context) { + suite.k.SaveSubspace(ctx, types.NewSubspace( + 1, + "Test subspace", + "This is a test subspace", + "cosmos1qzskhrcjnkdz2ln4yeafzsdwht8ch08j4wed69", + "cosmos1m0czrla04f7rp3zg7dsgc4kla54q7pc4xt00l5", + "cosmos1qzskhrcjnkdz2ln4yeafzsdwht8ch08j4wed69", + time.Date(2020, 1, 1, 12, 00, 00, 000, time.UTC), + )) + suite.k.SaveUserGroup(ctx, types.NewUserGroup( + 1, + 1, + "Test group", + "This is a test group", + types.PermissionWrite, + )) + + sdkAddr, err := sdk.AccAddressFromBech32("cosmos1x5pjlvufs4znnhhkwe8v4tw3kz30f3lxgwza53") + suite.Require().NoError(err) + suite.k.SetUserPermissions(ctx, 1, sdkAddr, types.PermissionSetPermissions) + }, + msg: types.NewMsgSetUserGroupPermissions( + 1, + 1, + 256, + "cosmos1x5pjlvufs4znnhhkwe8v4tw3kz30f3lxgwza53", + ), + shouldErr: true, + }, + { + name: "group permissions are updated correctly", store: func(ctx sdk.Context) { suite.k.SaveSubspace(ctx, types.NewSubspace( 1, @@ -1535,6 +1567,31 @@ func (suite *KeeperTestsuite) TestMsgServer_SetUserPermissions() { ), shouldErr: true, }, + { + name: "invalid permission returns error", + store: func(ctx sdk.Context) { + suite.k.SaveSubspace(ctx, types.NewSubspace( + 1, + "Test subspace", + "This is a test subspace", + "cosmos1qzskhrcjnkdz2ln4yeafzsdwht8ch08j4wed69", + "cosmos1m0czrla04f7rp3zg7dsgc4kla54q7pc4xt00l5", + "cosmos1qzskhrcjnkdz2ln4yeafzsdwht8ch08j4wed69", + time.Date(2020, 1, 1, 12, 00, 00, 000, time.UTC), + )) + + sdkAddr, err := sdk.AccAddressFromBech32("cosmos1x5pjlvufs4znnhhkwe8v4tw3kz30f3lxgwza53") + suite.Require().NoError(err) + suite.k.SetUserPermissions(ctx, 1, sdkAddr, types.PermissionSetPermissions) + }, + msg: types.NewMsgSetUserPermissions( + 1, + "cosmos1x5pjlvufs4znnhhkwe8v4tw3kz30f3lxgwza53", + 256, + "cosmos1x5pjlvufs4znnhhkwe8v4tw3kz30f3lxgwza53", + ), + shouldErr: true, + }, { name: "permissions are set correctly", store: func(ctx sdk.Context) { diff --git a/x/subspaces/legacy/v2/store.go b/x/subspaces/legacy/v2/store.go new file mode 100644 index 0000000000..ec9df98078 --- /dev/null +++ b/x/subspaces/legacy/v2/store.go @@ -0,0 +1,87 @@ +package v2 + +import ( + "github.com/cosmos/cosmos-sdk/codec" + "github.com/cosmos/cosmos-sdk/store/prefix" + sdk "github.com/cosmos/cosmos-sdk/types" + + "github.com/desmos-labs/desmos/v3/x/subspaces/types" +) + +func MigrateStore(ctx sdk.Context, storeKey sdk.StoreKey, cdc codec.BinaryCodec) error { + store := ctx.KVStore(storeKey) + + err := fixGroupsPermissions(store, cdc) + if err != nil { + return err + } + + fixUsersPermissions(store) + + return nil +} + +func fixGroupsPermissions(store sdk.KVStore, cdc codec.BinaryCodec) error { + groupsStore := prefix.NewStore(store, types.GroupsPrefix) + iterator := groupsStore.Iterator(nil, nil) + + var groups []types.UserGroup + for ; iterator.Valid(); iterator.Next() { + var group types.UserGroup + err := cdc.Unmarshal(iterator.Value(), &group) + if err != nil { + return err + } + + // Sanitize the permissions + group.Permissions = types.SanitizePermission(group.Permissions) + groups = append(groups, group) + } + + iterator.Close() + + // Store the new groups + for i, group := range groups { + bz, err := cdc.Marshal(&groups[i]) + if err != nil { + return err + } + + store.Set(types.GroupStoreKey(group.SubspaceID, group.ID), bz) + } + + return nil +} + +type userPermissionDetails struct { + subspaceID uint64 + user sdk.AccAddress + permissions types.Permission +} + +func fixUsersPermissions(store sdk.KVStore) { + permissionsStore := prefix.NewStore(store, types.UserPermissionsStorePrefix) + iterator := permissionsStore.Iterator(nil, nil) + + var permissions []userPermissionDetails + for ; iterator.Valid(); iterator.Next() { + // The first 8 bytes are the subspace id (uint64 takes up 8 bytes) + // The remaining bytes are the user address + subspaceBz, addressBz := iterator.Key()[:8], iterator.Key()[8:] + + permissions = append(permissions, userPermissionDetails{ + subspaceID: types.GetSubspaceIDFromBytes(subspaceBz), + user: types.GetAddressBytes(addressBz), + + // Sanitize the permission + permissions: types.SanitizePermission(types.UnmarshalPermission(iterator.Value())), + }) + } + + iterator.Close() + + // Store the new permissions + for _, entry := range permissions { + store.Set(types.UserPermissionStoreKey(entry.subspaceID, entry.user), types.MarshalPermission(entry.permissions)) + } +} diff --git a/x/subspaces/legacy/v2/store_test.go b/x/subspaces/legacy/v2/store_test.go new file mode 100644 index 0000000000..2e29129f60 --- /dev/null +++ b/x/subspaces/legacy/v2/store_test.go @@ -0,0 +1,90 @@ +package v2_test + +import ( + "testing" + + sdk "github.com/cosmos/cosmos-sdk/types" + capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" + paramstypes "github.com/cosmos/cosmos-sdk/x/params/types" + "github.com/stretchr/testify/require" + + "github.com/desmos-labs/desmos/v3/app" + "github.com/desmos-labs/desmos/v3/testutil" + v2 "github.com/desmos-labs/desmos/v3/x/subspaces/legacy/v2" + "github.com/desmos-labs/desmos/v3/x/subspaces/types" +) + +func TestMigrateStore(t *testing.T) { + cdc, _ := app.MakeCodecs() + + // Build all the necessary keys + keys := sdk.NewKVStoreKeys(types.StoreKey) + tKeys := sdk.NewTransientStoreKeys(paramstypes.TStoreKey) + memKeys := sdk.NewMemoryStoreKeys(capabilitytypes.MemStoreKey) + + testCases := []struct { + name string + store func(ctx sdk.Context) + shouldErr bool + check func(ctx sdk.Context) + }{ + { + name: "groups permissions are sanitized properly", + store: func(ctx sdk.Context) { + kvStore := ctx.KVStore(keys[types.StoreKey]) + + group := types.NewUserGroup(11, 11, "Test group", "", 0b11111111111111111111111111000001) + kvStore.Set(types.GroupStoreKey(group.SubspaceID, group.ID), cdc.MustMarshal(&group)) + }, + check: func(ctx sdk.Context) { + kvStore := ctx.KVStore(keys[types.StoreKey]) + + // Check the permissions + var group types.UserGroup + cdc.MustUnmarshal(kvStore.Get(types.GroupStoreKey(11, 11)), &group) + require.Equal(t, types.PermissionWrite, group.Permissions) + }, + }, + { + name: "user permissions are sanitized properly", + store: func(ctx sdk.Context) { + kvStore := ctx.KVStore(keys[types.StoreKey]) + + addr, err := sdk.AccAddressFromBech32("cosmos12e7ejq92sma437d3svemgfvl8sul8lxfs69mjv") + require.NoError(t, err) + + kvStore.Set(types.UserPermissionStoreKey(11, addr), types.MarshalPermission(0b11111111111111111111111111000001)) + }, + check: func(ctx sdk.Context) { + kvStore := ctx.KVStore(keys[types.StoreKey]) + + addr, err := sdk.AccAddressFromBech32("cosmos12e7ejq92sma437d3svemgfvl8sul8lxfs69mjv") + require.NoError(t, err) + + // Check the permissions + stored := types.UnmarshalPermission(kvStore.Get(types.UserPermissionStoreKey(11, addr))) + require.Equal(t, types.PermissionWrite, stored) + }, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + ctx := testutil.BuildContext(keys, tKeys, memKeys) + if tc.store != nil { + tc.store(ctx) + } + + err := v2.MigrateStore(ctx, keys[types.StoreKey], cdc) + if tc.shouldErr { + require.Error(t, err) + } else { + require.NoError(t, err) + if tc.check != nil { + tc.check(ctx) + } + } + }) + } +} diff --git a/x/subspaces/module.go b/x/subspaces/module.go index bf7c1730dd..c1f4ba5a24 100644 --- a/x/subspaces/module.go +++ b/x/subspaces/module.go @@ -27,6 +27,10 @@ import ( abci "github.com/tendermint/tendermint/abci/types" ) +const ( + consensusVersion = 2 +) + // type check to ensure the interface is properly implemented var ( _ module.AppModule = AppModule{} @@ -100,6 +104,12 @@ type AppModule struct { func (am AppModule) RegisterServices(cfg module.Configurator) { types.RegisterMsgServer(cfg.MsgServer(), keeper.NewMsgServerImpl(am.keeper)) types.RegisterQueryServer(cfg.QueryServer(), am.keeper) + + m := keeper.NewMigrator(am.keeper) + err := cfg.RegisterMigration(types.ModuleName, 1, m.Migrate1to2) + if err != nil { + panic(err) + } } // NewAppModule creates a new AppModule Object @@ -157,7 +167,7 @@ func (am AppModule) ExportGenesis(ctx sdk.Context, cdc codec.JSONCodec) json.Raw // ConsensusVersion implements AppModule. func (AppModule) ConsensusVersion() uint64 { - return 1 + return consensusVersion } // BeginBlock returns the begin blocker for the subspaces module. diff --git a/x/subspaces/types/permissions.go b/x/subspaces/types/permissions.go index 11ce4020af..dc70f88123 100644 --- a/x/subspaces/types/permissions.go +++ b/x/subspaces/types/permissions.go @@ -94,3 +94,18 @@ func CombinePermissions(permissions ...Permission) Permission { } return result } + +// SanitizePermission sanitizes the given permission to remove any unwanted bits set to 1 +func SanitizePermission(permission Permission) Permission { + mask := PermissionNothing + for perm := range permissionsMap { + mask = CombinePermissions(mask, perm) + } + + return permission & mask +} + +// IsPermissionValid checks whether the given value represents a valid permission or not +func IsPermissionValid(permission Permission) bool { + return SanitizePermission(permission) == permission +} diff --git a/x/subspaces/types/permissions_test.go b/x/subspaces/types/permissions_test.go index 20027c731a..e93570dff2 100644 --- a/x/subspaces/types/permissions_test.go +++ b/x/subspaces/types/permissions_test.go @@ -155,3 +155,77 @@ func TestCombinePermissions(t *testing.T) { }) } } + +func TestSanitizePermission(t *testing.T) { + testCases := []struct { + name string + permission types.Permission + expResult types.Permission + }{ + { + name: "valid permission returns the same value", + permission: types.PermissionWrite, + expResult: types.PermissionWrite, + }, + { + name: "combined permission returns the same value", + permission: types.PermissionWrite & types.PermissionChangeInfo, + expResult: types.PermissionWrite & types.PermissionChangeInfo, + }, + { + name: "invalid permission returns permission nothing", + permission: 256, + expResult: types.PermissionNothing, + }, + { + name: "extra bits are set to 0", + permission: 0b11111111111111111111111111000001, + expResult: types.PermissionWrite, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + result := types.SanitizePermission(tc.permission) + require.Equal(t, tc.expResult, result) + }) + } +} + +func TestIsPermissionValid(t *testing.T) { + testCases := []struct { + name string + permission types.Permission + expValid bool + }{ + { + name: "valid permission returns true", + permission: types.PermissionWrite, + expValid: true, + }, + { + name: "valid combined permission returns true", + permission: types.PermissionWrite & types.PermissionChangeInfo, + expValid: true, + }, + { + name: "invalid permission returns false", + permission: 256, + expValid: false, + }, + { + name: "invalid combined permission returns false", + permission: 0b11111111111111111111111111000001, + expValid: false, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + valid := types.IsPermissionValid(tc.permission) + require.Equal(t, tc.expValid, valid) + }) + } +}