diff --git a/changelog/unreleased/bugfix-space-role-bitmap-check.md b/changelog/unreleased/bugfix-space-role-bitmap-check.md new file mode 100644 index 00000000000..93919d1d97c --- /dev/null +++ b/changelog/unreleased/bugfix-space-role-bitmap-check.md @@ -0,0 +1,9 @@ +Bugfix: Harden space role classification and grant checks + +The `IsManager` space predicate treated any single grant-management bit as a full +manager, allowing a partial grant to act as a stealth manager. It now requires the +full add/update/remove grant triad. `IsEditor`/`IsViewer` keep classifying by their +defining capability bit so space-role variants still work. Partial grant-management +grants are rejected on space roots on both the create and update paths. + +https://github.com/owncloud/reva/pull/645 diff --git a/internal/grpc/services/gateway/usershareprovider.go b/internal/grpc/services/gateway/usershareprovider.go index ca731d408cf..7d0980f71ca 100644 --- a/internal/grpc/services/gateway/usershareprovider.go +++ b/internal/grpc/services/gateway/usershareprovider.go @@ -28,6 +28,7 @@ import ( provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" typesv1beta1 "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" "github.com/owncloud/reva/v2/pkg/appctx" + "github.com/owncloud/reva/v2/pkg/conversions" ctxpkg "github.com/owncloud/reva/v2/pkg/ctx" "github.com/owncloud/reva/v2/pkg/errtypes" "github.com/owncloud/reva/v2/pkg/rgrpc/status" @@ -37,6 +38,14 @@ import ( "github.com/pkg/errors" ) +// managerPerms is the canonical Manager bitmap used to reject partial grant-management grants on space roots +var managerPerms = conversions.NewManagerRole().CS3ResourcePermissions() + +// hasGrantManagementBits reports whether p carries any grant-management permission. +func hasGrantManagementBits(p *provider.ResourcePermissions) bool { + return p.GetAddGrant() || p.GetUpdateGrant() || p.GetRemoveGrant() || p.GetDenyGrant() +} + // TODO(labkode): add multi-phase commit logic when commit share or commit ref is enabled. func (s *svc) CreateShare(ctx context.Context, req *collaboration.CreateShareRequest) (*collaboration.CreateShareResponse, error) { // Don't use the share manager when sharing a space root @@ -171,6 +180,10 @@ func (s *svc) updateSpaceShare(ctx context.Context, req *collaboration.UpdateSha if req.GetShare().GetGrantee() == nil { return &collaboration.UpdateShareResponse{Status: status.NewInvalid(ctx, "updating requires a received grantee object")}, nil } + // reject partial grant-management grants that would mint a stealth manager + if perms := req.GetShare().GetPermissions().GetPermissions(); hasGrantManagementBits(perms) && !conversions.SufficientCS3Permissions(perms, managerPerms) { + return &collaboration.UpdateShareResponse{Status: status.NewInvalid(ctx, "partial grant-management permissions are not allowed on space roots")}, nil + } // If the share is a denial we call denyGrant instead. var st *rpc.Status var err error @@ -649,6 +662,10 @@ func (s *svc) addSpaceShare(ctx context.Context, req *collaboration.CreateShareR (req.GetResourceInfo().GetSpace().GetSpaceType() == _spaceTypePersonal || req.GetResourceInfo().GetSpace().GetSpaceType() == _spaceTypeVirtual) { return &collaboration.CreateShareResponse{Status: status.NewInvalid(ctx, "space type is not eligible for sharing")}, nil } + // reject partial grant-management grants that would mint a stealth manager + if perms := req.GetGrant().GetPermissions().GetPermissions(); hasGrantManagementBits(perms) && !conversions.SufficientCS3Permissions(perms, managerPerms) { + return &collaboration.CreateShareResponse{Status: status.NewInvalid(ctx, "partial grant-management permissions are not allowed on space roots")}, nil + } // If the share is a denial we call denyGrant instead. var st *rpc.Status var err error diff --git a/internal/grpc/services/gateway/usershareprovider_test.go b/internal/grpc/services/gateway/usershareprovider_test.go index 3721ac5a32d..3735b97a9e7 100644 --- a/internal/grpc/services/gateway/usershareprovider_test.go +++ b/internal/grpc/services/gateway/usershareprovider_test.go @@ -24,6 +24,7 @@ import ( userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" typesv1beta1 "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" + "github.com/owncloud/reva/v2/pkg/conversions" ) func userGrantee(id string) *provider.Grantee { @@ -104,3 +105,30 @@ func TestIsSpaceManagerRemaining(t *testing.T) { }) } } + +// spaceRootGrantRejected mirrors the guard used in both addSpaceShare and updateSpaceShare. +func spaceRootGrantRejected(perms *provider.ResourcePermissions) bool { + return hasGrantManagementBits(perms) && !conversions.SufficientCS3Permissions(perms, managerPerms) +} + +func TestSpaceShareRejectsPartialGrantManagement(t *testing.T) { + // a single grant-management bit must be rejected + if !spaceRootGrantRejected(&provider.ResourcePermissions{RemoveGrant: true}) { + t.Error("single-bit RemoveGrant grant should be rejected on a space root") + } + // read bits plus a grant-management bit (the update-path stealth-manager payload) must be rejected + if !spaceRootGrantRejected(&provider.ResourcePermissions{Stat: true, GetPath: true, ListGrants: true, RemoveGrant: true}) { + t.Error("read+RemoveGrant grant should be rejected on a space root") + } + // a full manager grant must be accepted + if spaceRootGrantRejected(conversions.NewManagerRole().CS3ResourcePermissions()) { + t.Error("full manager grant should be accepted on a space root") + } + // viewer/editor grants carry no grant-management bits and must be accepted + if spaceRootGrantRejected(conversions.NewSpaceViewerRole().CS3ResourcePermissions()) { + t.Error("space viewer grant should be accepted on a space root") + } + if spaceRootGrantRejected(conversions.NewSpaceEditorRole().CS3ResourcePermissions()) { + t.Error("space editor grant should be accepted on a space root") + } +} diff --git a/pkg/storage/utils/decomposedfs/permissions/spacepermissions.go b/pkg/storage/utils/decomposedfs/permissions/spacepermissions.go index bb8fec0f318..e7db9990658 100644 --- a/pkg/storage/utils/decomposedfs/permissions/spacepermissions.go +++ b/pkg/storage/utils/decomposedfs/permissions/spacepermissions.go @@ -7,6 +7,7 @@ import ( cs3permissions "github.com/cs3org/go-cs3apis/cs3/permissions/v1beta1" v1beta11 "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + "github.com/owncloud/reva/v2/pkg/conversions" ctxpkg "github.com/owncloud/reva/v2/pkg/ctx" "github.com/owncloud/reva/v2/pkg/rgrpc/todo/pool" "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/node" @@ -144,19 +145,26 @@ func (p Permissions) checkPermission(ctx context.Context, perm string, ref *prov return checkRes.Status.Code == v1beta11.Code_CODE_OK } -// IsManager returns true if the given resource permissions evaluate the user as "manager" -func IsManager(rp *provider.ResourcePermissions) bool { - return rp.RemoveGrant +// editorPerms is the smallest space-editor bitmap; every richer space-editor variant and +// the manager role are supersets of it, while viewers are not. +var editorPerms = conversions.NewSpaceEditorWithoutVersionsWithoutTrashbinRole().CS3ResourcePermissions() + +// IsSpaceManager returns true if the given resource permissions evaluate the user as "Space manager". +// A manager holds the full grant-management triad; a single grant-management bit is not enough. +// DenyGrant is not required, so legitimate manager/coowner grants that predate it still classify correctly. +func IsSpaceManager(rp *provider.ResourcePermissions) bool { + return rp.GetAddGrant() && rp.GetUpdateGrant() && rp.GetRemoveGrant() } -// IsEditor returns true if the given resource permissions evaluate the user as "editor" -func IsEditor(rp *provider.ResourcePermissions) bool { - return rp.InitiateFileUpload +// IsSpaceEditor returns true if the given resource permissions evaluate the user as "Space editor" +func IsSpaceEditor(rp *provider.ResourcePermissions) bool { + return conversions.SufficientCS3Permissions(rp, editorPerms) } -// IsViewer returns true if the given resource permissions evaluate the user as "viewer" -func IsViewer(rp *provider.ResourcePermissions) bool { - return rp.Stat +// IsSpaceViewer returns true if the given resource permissions evaluate the user as "Space viewer" +// We couldn't use NewSpaceViewerRole because it carries ListRecycle, which the WithoutTrashbin space editors lack. +func IsSpaceViewer(rp *provider.ResourcePermissions) bool { + return rp.GetStat() && rp.GetGetPath() && rp.GetListGrants() } func spaceRef(spaceid string) *provider.Reference { diff --git a/pkg/storage/utils/decomposedfs/permissions/spacepermissions_test.go b/pkg/storage/utils/decomposedfs/permissions/spacepermissions_test.go new file mode 100644 index 00000000000..518b0a9f6db --- /dev/null +++ b/pkg/storage/utils/decomposedfs/permissions/spacepermissions_test.go @@ -0,0 +1,106 @@ +// Copyright 2018-2021 CERN +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// In applying this license, CERN does not waive the privileges and immunities +// granted to it by virtue of its status as an Intergovernmental Organization +// or submit itself to any jurisdiction. + +package permissions + +import ( + "testing" + + provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + "github.com/owncloud/reva/v2/pkg/conversions" +) + +// TestIsManagerRejectsPartialGrantManagement ensures a single grant-management bit +// must not classify as manager, but the full triad must, without requiring DenyGrant +// (so coowner/legacy manager grants still pass). +func TestIsManagerRejectsPartialGrantManagement(t *testing.T) { + cases := []struct { + name string + rp *provider.ResourcePermissions + want bool + }{ + {"RemoveGrant only", &provider.ResourcePermissions{RemoveGrant: true}, false}, + {"AddGrant only", &provider.ResourcePermissions{AddGrant: true}, false}, + {"UpdateGrant only", &provider.ResourcePermissions{UpdateGrant: true}, false}, + {"Add+Remove without Update", &provider.ResourcePermissions{AddGrant: true, RemoveGrant: true}, false}, + {"full triad", &provider.ResourcePermissions{AddGrant: true, UpdateGrant: true, RemoveGrant: true}, true}, + {"manager role", conversions.NewManagerRole().CS3ResourcePermissions(), true}, + {"coowner role (no DenyGrant)", conversions.NewCoownerRole().CS3ResourcePermissions(), true}, + } + for _, tc := range cases { + if got := IsSpaceManager(tc.rp); got != tc.want { + t.Errorf("IsSpaceManager(%s) = %v, want %v", tc.name, got, tc.want) + } + } +} + +// TestIsEditorAcceptsSpaceEditorVariants ensures every space-editor variant (and manager, +// a superset) classifies as editor, while viewers do not. +func TestIsEditorAcceptsSpaceEditorVariants(t *testing.T) { + editors := map[string]*provider.ResourcePermissions{ + "space editor": conversions.NewSpaceEditorRole().CS3ResourcePermissions(), + "space editor without versions": conversions.NewSpaceEditorWithoutVersionsRole().CS3ResourcePermissions(), + "space editor without trashbin": conversions.NewSpaceEditorWithoutTrashbinRole().CS3ResourcePermissions(), + "space editor without versions/trash": conversions.NewSpaceEditorWithoutVersionsWithoutTrashbinRole().CS3ResourcePermissions(), + "manager": conversions.NewManagerRole().CS3ResourcePermissions(), + } + for name, rp := range editors { + if !IsSpaceEditor(rp) { + t.Errorf("IsSpaceEditor(%s) = false, want true", name) + } + } + + notEditors := map[string]*provider.ResourcePermissions{ + "space viewer": conversions.NewSpaceViewerRole().CS3ResourcePermissions(), + "secure viewer": conversions.NewSecureViewerRole().CS3ResourcePermissions(), + } + for name, rp := range notEditors { + if IsSpaceEditor(rp) { + t.Errorf("IsSpaceEditor(%s) = true, want false", name) + } + } +} + +// TestIsSpaceViewerAcceptsSpaceRoleVariants ensures the space-viewer predicate accepts every +// space role that can read (all space roles carry ListGrants), including subset editors and +// the manager, while non-space and grant-only grants do not. +func TestIsSpaceViewerAcceptsSpaceRoleVariants(t *testing.T) { + viewers := map[string]*provider.ResourcePermissions{ + "space viewer": conversions.NewSpaceViewerRole().CS3ResourcePermissions(), + "space editor without versions/trash": conversions.NewSpaceEditorWithoutVersionsWithoutTrashbinRole().CS3ResourcePermissions(), + "space editor": conversions.NewSpaceEditorRole().CS3ResourcePermissions(), + "manager": conversions.NewManagerRole().CS3ResourcePermissions(), + } + for name, rp := range viewers { + if !IsSpaceViewer(rp) { + t.Errorf("IsSpaceViewer(%s) = false, want true", name) + } + } + + // secure viewer is a file/folder role (no ListGrants) and is not a space role + notViewers := map[string]*provider.ResourcePermissions{ + "secure viewer": conversions.NewSecureViewerRole().CS3ResourcePermissions(), + "empty/denied": {}, + "remove grant only": {RemoveGrant: true}, + } + for name, rp := range notViewers { + if IsSpaceViewer(rp) { + t.Errorf("IsSpaceViewer(%s) = true, want false", name) + } + } +} diff --git a/pkg/storage/utils/decomposedfs/spaces.go b/pkg/storage/utils/decomposedfs/spaces.go index ced3254ba90..6d582719a64 100644 --- a/pkg/storage/utils/decomposedfs/spaces.go +++ b/pkg/storage/utils/decomposedfs/spaces.go @@ -632,7 +632,7 @@ func (fs *Decomposedfs) UpdateStorageSpace(ctx context.Context, req *provider.Up } - if !restore && len(metadata) == 0 && !permissions.IsViewer(sp) { + if !restore && len(metadata) == 0 && !permissions.IsSpaceViewer(sp) { // you may land here when making an update request without changes // check if user has access to the drive before continuing return &provider.UpdateStorageSpaceResponse{ @@ -640,10 +640,10 @@ func (fs *Decomposedfs) UpdateStorageSpace(ctx context.Context, req *provider.Up }, nil } - if !permissions.IsManager(sp) { + if !permissions.IsSpaceManager(sp) { // We are not a space manager. We need to check for additional permissions. k := []string{prefixes.NameAttr, prefixes.SpaceDescriptionAttr} - if !permissions.IsEditor(sp) { + if !permissions.IsSpaceEditor(sp) { k = append(k, prefixes.SpaceReadmeAttr, prefixes.SpaceAliasAttr, prefixes.SpaceImageAttr) } @@ -968,7 +968,7 @@ func (fs *Decomposedfs) StorageSpaceFromNode(ctx context.Context, n *node.Node, user := ctxpkg.ContextMustGetUser(ctx) if checkPermissions && n.SpaceRoot.IsDisabled(ctx) { rp, err := fs.p.AssemblePermissions(ctx, n) - if err != nil || !permissions.IsManager(rp) { + if err != nil || !permissions.IsSpaceManager(rp) { return nil, errtypes.PermissionDenied(fmt.Sprintf("user %s is not allowed to list deleted spaces %s", user.Username, n.ID)) } } @@ -1270,7 +1270,7 @@ func canDeleteSpace(ctx context.Context, spaceID string, typ string, purge bool, // space managers are allowed to disable and delete their project spaces rp, err := p.AssemblePermissions(ctx, n) - if err == nil && permissions.IsManager(rp) { + if err == nil && permissions.IsSpaceManager(rp) { return nil } diff --git a/pkg/storage/utils/decomposedfs/spaces_test.go b/pkg/storage/utils/decomposedfs/spaces_test.go index 57397940ef6..dca0b523aa9 100644 --- a/pkg/storage/utils/decomposedfs/spaces_test.go +++ b/pkg/storage/utils/decomposedfs/spaces_test.go @@ -28,6 +28,7 @@ import ( typesv1beta1 "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/owncloud/reva/v2/pkg/conversions" ctxpkg "github.com/owncloud/reva/v2/pkg/ctx" "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/node" helpers "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/testhelpers" @@ -292,9 +293,9 @@ var _ = Describe("Spaces", func() { case manager.GetId().GetOpaqueId(): return node.OwnerPermissions() // id of owner/admin case editor.GetId().GetOpaqueId(): - return &provider.ResourcePermissions{InitiateFileUpload: true} // mock editor + return conversions.NewSpaceEditorRole().CS3ResourcePermissions() // mock editor case viewer.GetId().GetOpaqueId(): - return &provider.ResourcePermissions{Stat: true} // mock viewer + return conversions.NewSpaceViewerRole().CS3ResourcePermissions() // mock viewer default: return node.NoPermissions() }