Skip to content

Commit

Permalink
Restrict code access config modifications
Browse files Browse the repository at this point in the history
  • Loading branch information
alpe committed Jul 8, 2022
1 parent 39be44b commit e5ff7c1
Show file tree
Hide file tree
Showing 8 changed files with 199 additions and 13 deletions.
9 changes: 9 additions & 0 deletions x/wasm/keeper/authz_policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ type AuthorizationPolicy interface {
CanCreateCode(c types.AccessConfig, creator sdk.AccAddress) bool
CanInstantiateContract(c types.AccessConfig, actor sdk.AccAddress) bool
CanModifyContract(admin, actor sdk.AccAddress) bool
CanModifyCodeAccessConfig(creator, actor sdk.AccAddress, isSubset bool) bool
}

type DefaultAuthorizationPolicy struct{}
Expand All @@ -26,6 +27,10 @@ func (p DefaultAuthorizationPolicy) CanModifyContract(admin, actor sdk.AccAddres
return admin != nil && admin.Equals(actor)
}

func (p DefaultAuthorizationPolicy) CanModifyCodeAccessConfig(creator, actor sdk.AccAddress, isSubset bool) bool {
return creator != nil && creator.Equals(actor) && isSubset
}

type GovAuthorizationPolicy struct{}

func (p GovAuthorizationPolicy) CanCreateCode(types.AccessConfig, sdk.AccAddress) bool {
Expand All @@ -39,3 +44,7 @@ func (p GovAuthorizationPolicy) CanInstantiateContract(types.AccessConfig, sdk.A
func (p GovAuthorizationPolicy) CanModifyContract(sdk.AccAddress, sdk.AccAddress) bool {
return true
}

func (p GovAuthorizationPolicy) CanModifyCodeAccessConfig(sdk.AccAddress, sdk.AccAddress, bool) bool {
return true
}
6 changes: 3 additions & 3 deletions x/wasm/keeper/contract_keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ type decoratedKeeper interface {
execute(ctx sdk.Context, contractAddress sdk.AccAddress, caller sdk.AccAddress, msg []byte, coins sdk.Coins) ([]byte, error)
Sudo(ctx sdk.Context, contractAddress sdk.AccAddress, msg []byte) ([]byte, error)
setContractInfoExtension(ctx sdk.Context, contract sdk.AccAddress, extra types.ContractInfoExtension) error
setAccessConfig(ctx sdk.Context, codeID uint64, config types.AccessConfig) error
setAccessConfig(ctx sdk.Context, codeID uint64, caller sdk.AccAddress, newConfig types.AccessConfig, autz AuthorizationPolicy) error
}

type PermissionedKeeper struct {
Expand Down Expand Up @@ -81,6 +81,6 @@ func (p PermissionedKeeper) SetContractInfoExtension(ctx sdk.Context, contract s
}

// SetAccessConfig updates the access config of a code id.
func (p PermissionedKeeper) SetAccessConfig(ctx sdk.Context, codeID uint64, config types.AccessConfig) error {
return p.nested.setAccessConfig(ctx, codeID, config)
func (p PermissionedKeeper) SetAccessConfig(ctx sdk.Context, codeID uint64, caller sdk.AccAddress, newConfig types.AccessConfig) error {
return p.nested.setAccessConfig(ctx, codeID, caller, newConfig, p.authZPolicy)
}
9 changes: 7 additions & 2 deletions x/wasm/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -879,12 +879,17 @@ func (k Keeper) setContractInfoExtension(ctx sdk.Context, contractAddr sdk.AccAd
}

// setAccessConfig updates the access config of a code id.
func (k Keeper) setAccessConfig(ctx sdk.Context, codeID uint64, config types.AccessConfig) error {
func (k Keeper) setAccessConfig(ctx sdk.Context, codeID uint64, caller sdk.AccAddress, newConfig types.AccessConfig, authz AuthorizationPolicy) error {
info := k.GetCodeInfo(ctx, codeID)
if info == nil {
return sdkerrors.Wrap(types.ErrNotFound, "code info")
}
info.InstantiateConfig = config
isSubset := newConfig.Permission.IsSubset(k.getInstantiateAccessConfig(ctx))
if !authz.CanModifyCodeAccessConfig(sdk.MustAccAddressFromBech32(info.Creator), caller, isSubset) {
return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "can not modify code access config")
}

info.InstantiateConfig = newConfig
k.storeCodeInfo(ctx, codeID, *info)
return nil
}
Expand Down
84 changes: 84 additions & 0 deletions x/wasm/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1823,3 +1823,87 @@ func TestBuildContractAddress(t *testing.T) {
})
}
}
func TestSetAccessConfig(t *testing.T) {
parentCtx, keepers := CreateTestInput(t, false, SupportedFeatures)
k := keepers.WasmKeeper
creatorAddr := RandomAccountAddress(t)
nonCreatorAddr := RandomAccountAddress(t)

specs := map[string]struct {
authz AuthorizationPolicy
chainPermission types.AccessType
newConfig types.AccessConfig
caller sdk.AccAddress
expErr bool
}{
"user with new permissions == chain permissions": {
authz: DefaultAuthorizationPolicy{},
chainPermission: types.AccessTypeEverybody,
newConfig: types.AllowEverybody,
caller: creatorAddr,
},
"user with new permissions < chain permissions": {
authz: DefaultAuthorizationPolicy{},
chainPermission: types.AccessTypeEverybody,
newConfig: types.AllowNobody,
caller: creatorAddr,
},
"user with new permissions > chain permissions": {
authz: DefaultAuthorizationPolicy{},
chainPermission: types.AccessTypeNobody,
newConfig: types.AllowEverybody,
caller: creatorAddr,
expErr: true,
},
"different actor": {
authz: DefaultAuthorizationPolicy{},
chainPermission: types.AccessTypeEverybody,
newConfig: types.AllowEverybody,
caller: nonCreatorAddr,
expErr: true,
},
"gov with new permissions == chain permissions": {
authz: GovAuthorizationPolicy{},
chainPermission: types.AccessTypeEverybody,
newConfig: types.AllowEverybody,
caller: creatorAddr,
},
"gov with new permissions < chain permissions": {
authz: GovAuthorizationPolicy{},
chainPermission: types.AccessTypeEverybody,
newConfig: types.AllowNobody,
caller: creatorAddr,
},
"gov with new permissions > chain permissions": {
authz: GovAuthorizationPolicy{},
chainPermission: types.AccessTypeNobody,
newConfig: types.AccessTypeOnlyAddress.With(creatorAddr),
caller: creatorAddr,
},
"gov without actor": {
authz: GovAuthorizationPolicy{},
chainPermission: types.AccessTypeEverybody,
newConfig: types.AllowEverybody,
},
}
const codeID = 1
for name, spec := range specs {
t.Run(name, func(t *testing.T) {
ctx, _ := parentCtx.CacheContext()
newParams := types.DefaultParams()
newParams.InstantiateDefaultPermission = spec.chainPermission
k.SetParams(ctx, newParams)

k.storeCodeInfo(ctx, codeID, types.NewCodeInfo(nil, creatorAddr, types.AllowNobody))
// when
gotErr := k.setAccessConfig(ctx, codeID, spec.caller, spec.newConfig, spec.authz)
if spec.expErr {
require.Error(t, gotErr)
return
}
require.NoError(t, gotErr)

})
}

}
3 changes: 2 additions & 1 deletion x/wasm/keeper/proposal_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,9 @@ func handleUpdateInstantiateConfigProposal(ctx sdk.Context, k types.ContractOpsK
return err
}

var emptyCaller sdk.AccAddress
for _, accessConfigUpdate := range p.AccessConfigUpdates {
if err := k.SetAccessConfig(ctx, accessConfigUpdate.CodeID, accessConfigUpdate.InstantiatePermission); err != nil {
if err := k.SetAccessConfig(ctx, accessConfigUpdate.CodeID, emptyCaller, accessConfigUpdate.InstantiatePermission); err != nil {
return sdkerrors.Wrapf(err, "code id: %d", accessConfigUpdate.CodeID)
}
}
Expand Down
2 changes: 1 addition & 1 deletion x/wasm/types/exported_keepers.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ type ContractOpsKeeper interface {
SetContractInfoExtension(ctx sdk.Context, contract sdk.AccAddress, extra ContractInfoExtension) error

// SetAccessConfig updates the access config of a code id.
SetAccessConfig(ctx sdk.Context, codeID uint64, config AccessConfig) error
SetAccessConfig(ctx sdk.Context, codeID uint64, caller sdk.AccAddress, newConfig AccessConfig) error
}

// IBCContractKeeper IBC lifecycle event handler
Expand Down
22 changes: 17 additions & 5 deletions x/wasm/types/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -333,18 +333,30 @@ func VerifyAddressLen() func(addr []byte) error {

// 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 {
func (a AccessType) IsSubset(superSet AccessType) bool {
switch superSet {
case AccessTypeEverybody:
// Everything is a subset of this
return a.Permission != AccessTypeUnspecified
return a != AccessTypeUnspecified
case AccessTypeNobody:
// Only an exact match is a subset of this
return a.Permission == AccessTypeNobody
return a == AccessTypeNobody
case AccessTypeOnlyAddress:
// An exact match or nobody
return a.Permission == AccessTypeNobody || (a.Permission == AccessTypeOnlyAddress && a.Address == superSet.Address)
return a == AccessTypeNobody || a == AccessTypeOnlyAddress
default:
return false
}
}

// 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 AccessTypeOnlyAddress:
// An exact match or nobody
return a.Permission == AccessTypeNobody || (a.Permission == AccessTypeOnlyAddress && a.Address == superSet.Address)
default:
return a.Permission.IsSubset(superSet.Permission)
}
}
77 changes: 76 additions & 1 deletion x/wasm/types/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ func TestVerifyAddressLen(t *testing.T) {
}
}

func TestAccesConfigSubset(t *testing.T) {
func TestAccessConfigSubset(t *testing.T) {
specs := map[string]struct {
check AccessConfig
superSet AccessConfig
Expand Down Expand Up @@ -453,3 +453,78 @@ func TestAccesConfigSubset(t *testing.T) {
})
}
}

func TestAccessTypeSubset(t *testing.T) {
specs := map[string]struct {
check AccessType
superSet AccessType
isSubSet bool
}{
"nobody <= nobody": {
superSet: AccessTypeNobody,
check: AccessTypeNobody,
isSubSet: true,
},
"only > nobody": {
superSet: AccessTypeNobody,
check: AccessTypeOnlyAddress,
isSubSet: false,
},
"everybody > nobody": {
superSet: AccessTypeNobody,
check: AccessTypeEverybody,
isSubSet: false,
},
"unspecified > nobody": {
superSet: AccessTypeNobody,
check: AccessTypeUnspecified,
isSubSet: false,
},
"nobody <= everybody": {
superSet: AccessTypeEverybody,
check: AccessTypeNobody,
isSubSet: true,
},
"only <= everybody": {
superSet: AccessTypeEverybody,
check: AccessTypeOnlyAddress,
isSubSet: true,
},
"everybody <= everybody": {
superSet: AccessTypeEverybody,
check: AccessTypeEverybody,
isSubSet: true,
},
"unspecified > everybody": {
superSet: AccessTypeEverybody,
check: AccessTypeUnspecified,
isSubSet: false,
},
"nobody <= only": {
superSet: AccessTypeOnlyAddress,
check: AccessTypeNobody,
isSubSet: true,
},
"only <= only(same)": {
superSet: AccessTypeOnlyAddress,
check: AccessTypeOnlyAddress,
isSubSet: true,
},
"everybody > only": {
superSet: AccessTypeOnlyAddress,
check: AccessTypeEverybody,
isSubSet: false,
},
"nobody > unspecified": {
superSet: AccessTypeUnspecified,
check: AccessTypeNobody,
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)
})
}
}

0 comments on commit e5ff7c1

Please sign in to comment.