From c2ec9092d086b5ac6dd367f33ce8b5cce8e4c5f5 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Mon, 9 May 2022 21:59:18 +0200 Subject: [PATCH 1/3] Add compare logic for stricter AccessConfig --- x/wasm/types/types.go | 18 +++++++++ x/wasm/types/types_test.go | 76 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+) diff --git a/x/wasm/types/types.go b/x/wasm/types/types.go index e91879927b..7c9e14e64e 100644 --- a/x/wasm/types/types.go +++ b/x/wasm/types/types.go @@ -330,3 +330,21 @@ func VerifyAddressLen() func(addr []byte) error { return nil } } + +// IsSubset will return true if the caller is the same as the superset, +// or if the caller is more restrictive than the superset. +func (a AccessConfig) IsSubset(superSet AccessConfig) bool { + switch superSet.Permission { + case AccessTypeEverybody: + // Everything is a subset of this + return a.Permission != AccessTypeUnspecified + case AccessTypeNobody: + // Only an exact match is a subset of this + return a.Permission == AccessTypeNobody + case AccessTypeOnlyAddress: + // An exact match or nobody + return a.Permission == AccessTypeNobody || (a.Permission == AccessTypeOnlyAddress && a.Address == superSet.Address) + default: + return false + } +} diff --git a/x/wasm/types/types_test.go b/x/wasm/types/types_test.go index 72a984e1ac..9f6503b855 100644 --- a/x/wasm/types/types_test.go +++ b/x/wasm/types/types_test.go @@ -372,3 +372,79 @@ func TestVerifyAddressLen(t *testing.T) { }) } } + +func TestAccesConfigSubset(t *testing.T) { + specs := map[string]struct { + check AccessConfig + superSet AccessConfig + isSubSet bool + }{ + "nobody <= nobody": { + superSet: AccessConfig{Permission: AccessTypeNobody}, + check: AccessConfig{Permission: AccessTypeNobody}, + isSubSet: true, + }, + "only > nobody": { + superSet: AccessConfig{Permission: AccessTypeNobody}, + check: AccessConfig{Permission: AccessTypeOnlyAddress, Address: "foobar"}, + isSubSet: false, + }, + "everybody > nobody": { + superSet: AccessConfig{Permission: AccessTypeNobody}, + check: AccessConfig{Permission: AccessTypeEverybody}, + isSubSet: false, + }, + "nobody <= everybody": { + superSet: AccessConfig{Permission: AccessTypeEverybody}, + check: AccessConfig{Permission: AccessTypeNobody}, + isSubSet: true, + }, + "only <= everybody": { + superSet: AccessConfig{Permission: AccessTypeEverybody}, + check: AccessConfig{Permission: AccessTypeOnlyAddress, Address: "foobar"}, + isSubSet: true, + }, + "everybody <= everybody": { + superSet: AccessConfig{Permission: AccessTypeEverybody}, + check: AccessConfig{Permission: AccessTypeEverybody}, + isSubSet: true, + }, + "nobody <= only": { + superSet: AccessConfig{Permission: AccessTypeOnlyAddress, Address: "owner"}, + check: AccessConfig{Permission: AccessTypeNobody}, + isSubSet: true, + }, + "only <= only(same)": { + superSet: AccessConfig{Permission: AccessTypeOnlyAddress, Address: "owner"}, + check: AccessConfig{Permission: AccessTypeOnlyAddress, Address: "owner"}, + isSubSet: true, + }, + "only > only(other)": { + superSet: AccessConfig{Permission: AccessTypeOnlyAddress, Address: "owner"}, + check: AccessConfig{Permission: AccessTypeOnlyAddress, Address: "other"}, + isSubSet: false, + }, + "everybody > only": { + superSet: AccessConfig{Permission: AccessTypeOnlyAddress, Address: "owner"}, + check: AccessConfig{Permission: AccessTypeEverybody}, + isSubSet: false, + }, + "nobody > unspecified": { + superSet: AccessConfig{Permission: AccessTypeUnspecified}, + check: AccessConfig{Permission: AccessTypeNobody}, + isSubSet: false, + }, + "unspecified > everybody": { + superSet: AccessConfig{Permission: AccessTypeEverybody}, + check: AccessConfig{Permission: AccessTypeUnspecified}, + isSubSet: false, + }, + } + + for name, spec := range specs { + t.Run(name, func(t *testing.T) { + subset := spec.check.IsSubset(spec.superSet) + require.Equal(t, spec.isSubSet, subset) + }) + } +} From a67dae70b775700192fe91a91160b3bc51a39805 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Mon, 9 May 2022 22:17:58 +0200 Subject: [PATCH 2/3] Enforce permission less permissive than default --- x/wasm/keeper/keeper.go | 13 ++++++--- x/wasm/keeper/keeper_test.go | 56 ++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 4 deletions(-) diff --git a/x/wasm/keeper/keeper.go b/x/wasm/keeper/keeper.go index becc80105c..e7b4c37ee6 100644 --- a/x/wasm/keeper/keeper.go +++ b/x/wasm/keeper/keeper.go @@ -159,6 +159,15 @@ func (k Keeper) create(ctx sdk.Context, creator sdk.AccAddress, wasmCode []byte, if !authZ.CanCreateCode(k.getUploadAccessConfig(ctx), creator) { return 0, sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "can not create code") } + // figure out proper instantiate access + defaultAccessConfig := k.getInstantiateAccessConfig(ctx).With(creator) + if instantiateAccess == nil { + instantiateAccess = &defaultAccessConfig + } else if !instantiateAccess.IsSubset(defaultAccessConfig) { + // we enforce this must be subset of default upload access + return 0, sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "instantiate access must be subset of default upload access") + } + wasmCode, err = ioutils.Uncompress(wasmCode, uint64(types.MaxWasmSize)) if err != nil { return 0, sdkerrors.Wrap(types.ErrCreateFailed, err.Error()) @@ -175,10 +184,6 @@ func (k Keeper) create(ctx sdk.Context, creator sdk.AccAddress, wasmCode []byte, } codeID = k.autoIncrementID(ctx, types.KeyLastCodeID) k.Logger(ctx).Debug("storing new contract", "features", report.RequiredFeatures, "code_id", codeID) - if instantiateAccess == nil { - defaultAccessConfig := k.getInstantiateAccessConfig(ctx).With(creator) - instantiateAccess = &defaultAccessConfig - } codeInfo := types.NewCodeInfo(checksum, creator, *instantiateAccess) k.storeCodeInfo(ctx, codeID, codeInfo) diff --git a/x/wasm/keeper/keeper_test.go b/x/wasm/keeper/keeper_test.go index 749b5f59f2..d962b63fc9 100644 --- a/x/wasm/keeper/keeper_test.go +++ b/x/wasm/keeper/keeper_test.go @@ -183,6 +183,62 @@ func TestCreateWithParamPermissions(t *testing.T) { } } +// ensure that the user cannot set the code instantiate permission to something more permissive +// than the default +func TestEnforceValidPermissionsOnCreate(t *testing.T) { + ctx, keepers := CreateTestInput(t, false, SupportedFeatures) + keeper := keepers.WasmKeeper + contractKeeper := keepers.ContractKeeper + + deposit := sdk.NewCoins(sdk.NewInt64Coin("denom", 100000)) + creator := keepers.Faucet.NewFundedAccount(ctx, deposit...) + + onlyCreator := types.AccessTypeOnlyAddress.With(creator) + + specs := map[string]struct { + defaultPermssion types.AccessType + requestedPermission *types.AccessConfig + // grantedPermission is set iff no error + grantedPermission types.AccessConfig + // expError is nil iff the request is allowed + expError *sdkerrors.Error + }{ + "override everybody": { + defaultPermssion: types.AccessTypeEverybody, + requestedPermission: &onlyCreator, + grantedPermission: onlyCreator, + }, + "default to everybody": { + defaultPermssion: types.AccessTypeEverybody, + requestedPermission: nil, + grantedPermission: types.AccessConfig{Permission: types.AccessTypeEverybody}, + }, + "cannot override nobody": { + defaultPermssion: types.AccessTypeNobody, + requestedPermission: &onlyCreator, + expError: sdkerrors.ErrUnauthorized, + }, + "default to nobody": { + defaultPermssion: types.AccessTypeNobody, + requestedPermission: nil, + grantedPermission: types.AccessConfig{Permission: types.AccessTypeNobody}, + }, + } + for msg, spec := range specs { + t.Run(msg, func(t *testing.T) { + params := types.DefaultParams() + params.InstantiateDefaultPermission = spec.defaultPermssion + keeper.SetParams(ctx, params) + codeID, err := contractKeeper.Create(ctx, creator, hackatomWasm, spec.requestedPermission) + require.True(t, spec.expError.Is(err), err) + if spec.expError == nil { + codeInfo := keeper.GetCodeInfo(ctx, codeID) + require.Equal(t, codeInfo.InstantiateConfig, spec.grantedPermission) + } + }) + } +} + func TestCreateDuplicate(t *testing.T) { ctx, keepers := CreateTestInput(t, false, SupportedFeatures) keeper := keepers.ContractKeeper From 27d30513b248bb24762b7c6a5319b5c7adbdfb3b Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Tue, 10 May 2022 16:25:11 +0200 Subject: [PATCH 3/3] Add a few more tests as requested in review --- x/wasm/keeper/keeper_test.go | 22 ++++++++++++++++++++++ x/wasm/types/types_test.go | 15 ++++++++++----- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/x/wasm/keeper/keeper_test.go b/x/wasm/keeper/keeper_test.go index d962b63fc9..0886f65a10 100644 --- a/x/wasm/keeper/keeper_test.go +++ b/x/wasm/keeper/keeper_test.go @@ -192,8 +192,10 @@ func TestEnforceValidPermissionsOnCreate(t *testing.T) { deposit := sdk.NewCoins(sdk.NewInt64Coin("denom", 100000)) creator := keepers.Faucet.NewFundedAccount(ctx, deposit...) + other := keepers.Faucet.NewFundedAccount(ctx, deposit...) onlyCreator := types.AccessTypeOnlyAddress.With(creator) + onlyOther := types.AccessTypeOnlyAddress.With(other) specs := map[string]struct { defaultPermssion types.AccessType @@ -213,6 +215,11 @@ func TestEnforceValidPermissionsOnCreate(t *testing.T) { requestedPermission: nil, grantedPermission: types.AccessConfig{Permission: types.AccessTypeEverybody}, }, + "explicitly set everybody": { + defaultPermssion: types.AccessTypeEverybody, + requestedPermission: &types.AccessConfig{Permission: types.AccessTypeEverybody}, + grantedPermission: types.AccessConfig{Permission: types.AccessTypeEverybody}, + }, "cannot override nobody": { defaultPermssion: types.AccessTypeNobody, requestedPermission: &onlyCreator, @@ -223,6 +230,21 @@ func TestEnforceValidPermissionsOnCreate(t *testing.T) { requestedPermission: nil, grantedPermission: types.AccessConfig{Permission: types.AccessTypeNobody}, }, + "only defaults to code creator": { + defaultPermssion: types.AccessTypeOnlyAddress, + requestedPermission: nil, + grantedPermission: onlyCreator, + }, + "can explicitly set to code creator": { + defaultPermssion: types.AccessTypeOnlyAddress, + requestedPermission: &onlyCreator, + grantedPermission: onlyCreator, + }, + "cannot override which address in only": { + defaultPermssion: types.AccessTypeOnlyAddress, + requestedPermission: &onlyOther, + expError: sdkerrors.ErrUnauthorized, + }, } for msg, spec := range specs { t.Run(msg, func(t *testing.T) { diff --git a/x/wasm/types/types_test.go b/x/wasm/types/types_test.go index 9f6503b855..117415930d 100644 --- a/x/wasm/types/types_test.go +++ b/x/wasm/types/types_test.go @@ -394,6 +394,11 @@ func TestAccesConfigSubset(t *testing.T) { check: AccessConfig{Permission: AccessTypeEverybody}, isSubSet: false, }, + "unspecified > nobody": { + superSet: AccessConfig{Permission: AccessTypeNobody}, + check: AccessConfig{Permission: AccessTypeUnspecified}, + isSubSet: false, + }, "nobody <= everybody": { superSet: AccessConfig{Permission: AccessTypeEverybody}, check: AccessConfig{Permission: AccessTypeNobody}, @@ -409,6 +414,11 @@ func TestAccesConfigSubset(t *testing.T) { check: AccessConfig{Permission: AccessTypeEverybody}, isSubSet: true, }, + "unspecified > everybody": { + superSet: AccessConfig{Permission: AccessTypeEverybody}, + check: AccessConfig{Permission: AccessTypeUnspecified}, + isSubSet: false, + }, "nobody <= only": { superSet: AccessConfig{Permission: AccessTypeOnlyAddress, Address: "owner"}, check: AccessConfig{Permission: AccessTypeNobody}, @@ -434,11 +444,6 @@ func TestAccesConfigSubset(t *testing.T) { check: AccessConfig{Permission: AccessTypeNobody}, isSubSet: false, }, - "unspecified > everybody": { - superSet: AccessConfig{Permission: AccessTypeEverybody}, - check: AccessConfig{Permission: AccessTypeUnspecified}, - isSubSet: false, - }, } for name, spec := range specs {