From fc1eae205cb47a33df37ae73e7788e975c1078e1 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Fri, 30 Jul 2021 09:53:24 +0200 Subject: [PATCH] fix permissions for share jail --- changelog/unreleased/fix-share-jail-perms | 5 + .../utils/decomposedfs/decomposedfs.go | 11 +- pkg/storage/utils/decomposedfs/lookup.go | 5 + .../decomposedfs/mocks/PermissionsChecker.go | 8 +- pkg/storage/utils/decomposedfs/node/node.go | 23 ++-- .../utils/decomposedfs/node/node_test.go | 8 +- .../utils/decomposedfs/node/permissions.go | 104 +++++++++++------- pkg/storage/utils/decomposedfs/recycle.go | 4 +- pkg/storage/utils/decomposedfs/tree/tree.go | 1 + .../utils/decomposedfs/tree/tree_test.go | 5 +- .../expected-failures-on-OCIS-storage.md | 4 - .../expected-failures-on-S3NG-storage.md | 4 - 12 files changed, 107 insertions(+), 75 deletions(-) create mode 100644 changelog/unreleased/fix-share-jail-perms diff --git a/changelog/unreleased/fix-share-jail-perms b/changelog/unreleased/fix-share-jail-perms new file mode 100644 index 0000000000..44279fcd1d --- /dev/null +++ b/changelog/unreleased/fix-share-jail-perms @@ -0,0 +1,5 @@ +Bugfix: fix the share jail permissions in the decomposedfs + +The share jail should be not writable + +https://github.com/cs3org/reva/pull/1939 diff --git a/pkg/storage/utils/decomposedfs/decomposedfs.go b/pkg/storage/utils/decomposedfs/decomposedfs.go index ccce2b4fce..042de3052b 100644 --- a/pkg/storage/utils/decomposedfs/decomposedfs.go +++ b/pkg/storage/utils/decomposedfs/decomposedfs.go @@ -53,7 +53,7 @@ import ( // PermissionsChecker defines an interface for checking permissions on a Node type PermissionsChecker interface { - AssemblePermissions(ctx context.Context, n *node.Node) (ap *provider.ResourcePermissions, err error) + AssemblePermissions(ctx context.Context, n *node.Node) (ap provider.ResourcePermissions, err error) HasPermission(ctx context.Context, n *node.Node, check func(*provider.ResourcePermissions) bool) (can bool, err error) } @@ -148,7 +148,7 @@ func (fs *Decomposedfs) GetQuota(ctx context.Context) (total uint64, inUse uint6 return 0, 0, errtypes.PermissionDenied(n.ID) } - ri, err := n.AsResourceInfo(ctx, rp, []string{"treesize", "quota"}) + ri, err := n.AsResourceInfo(ctx, &rp, []string{"treesize", "quota"}) if err != nil { return 0, 0, err } @@ -399,7 +399,7 @@ func (fs *Decomposedfs) GetMD(ctx context.Context, ref *provider.Reference, mdKe return nil, errtypes.PermissionDenied(node.ID) } - return node.AsResourceInfo(ctx, rp, mdKeys) + return node.AsResourceInfo(ctx, &rp, mdKeys) } // ListFolder returns a list of resources in the specified folder @@ -431,8 +431,9 @@ func (fs *Decomposedfs) ListFolder(ctx context.Context, ref *provider.Reference, for i := range children { np := rp // add this childs permissions - node.AddPermissions(np, n.PermissionSet(ctx)) - if ri, err := children[i].AsResourceInfo(ctx, np, mdKeys); err == nil { + pset := n.PermissionSet(ctx) + node.AddPermissions(&np, &pset) + if ri, err := children[i].AsResourceInfo(ctx, &np, mdKeys); err == nil { finfos = append(finfos, ri) } } diff --git a/pkg/storage/utils/decomposedfs/lookup.go b/pkg/storage/utils/decomposedfs/lookup.go index fe1fc315bc..ade953423a 100644 --- a/pkg/storage/utils/decomposedfs/lookup.go +++ b/pkg/storage/utils/decomposedfs/lookup.go @@ -168,3 +168,8 @@ func (lu *Lookup) mustGetUserLayout(ctx context.Context) string { u := user.ContextMustGetUser(ctx) return templates.WithUser(u, lu.Options.UserLayout) } + +// ShareFolder returns the internal storage root directory +func (lu *Lookup) ShareFolder() string { + return lu.Options.ShareFolder +} diff --git a/pkg/storage/utils/decomposedfs/mocks/PermissionsChecker.go b/pkg/storage/utils/decomposedfs/mocks/PermissionsChecker.go index 2d05f81ec5..bff1d67cce 100644 --- a/pkg/storage/utils/decomposedfs/mocks/PermissionsChecker.go +++ b/pkg/storage/utils/decomposedfs/mocks/PermissionsChecker.go @@ -35,15 +35,15 @@ type PermissionsChecker struct { } // AssemblePermissions provides a mock function with given fields: ctx, n -func (_m *PermissionsChecker) AssemblePermissions(ctx context.Context, n *node.Node) (*providerv1beta1.ResourcePermissions, error) { +func (_m *PermissionsChecker) AssemblePermissions(ctx context.Context, n *node.Node) (providerv1beta1.ResourcePermissions, error) { ret := _m.Called(ctx, n) - var r0 *providerv1beta1.ResourcePermissions - if rf, ok := ret.Get(0).(func(context.Context, *node.Node) *providerv1beta1.ResourcePermissions); ok { + var r0 providerv1beta1.ResourcePermissions + if rf, ok := ret.Get(0).(func(context.Context, *node.Node) providerv1beta1.ResourcePermissions); ok { r0 = rf(ctx, n) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(*providerv1beta1.ResourcePermissions) + r0 = ret.Get(0).(providerv1beta1.ResourcePermissions) } } diff --git a/pkg/storage/utils/decomposedfs/node/node.go b/pkg/storage/utils/decomposedfs/node/node.go index 4b8a1f9f18..afb0bc757d 100644 --- a/pkg/storage/utils/decomposedfs/node/node.go +++ b/pkg/storage/utils/decomposedfs/node/node.go @@ -84,6 +84,7 @@ type PathLookup interface { InternalRoot() string InternalPath(ID string) string Path(ctx context.Context, n *Node) (path string, err error) + ShareFolder() string } // New returns a new instance of Node @@ -313,20 +314,20 @@ func (n *Node) Owner() (o *userpb.UserId, err error) { // PermissionSet returns the permission set for the current user // the parent nodes are not taken into account -func (n *Node) PermissionSet(ctx context.Context) *provider.ResourcePermissions { +func (n *Node) PermissionSet(ctx context.Context) provider.ResourcePermissions { u, ok := user.ContextGetUser(ctx) if !ok { appctx.GetLogger(ctx).Debug().Interface("node", n).Msg("no user in context, returning default permissions") - return NoPermissions + return NoPermissions() } if o, _ := n.Owner(); isSameUserID(u.Id, o) { - return OwnerPermissions + return OwnerPermissions() } // read the permissions for the current user from the acls of the current node if np, err := n.ReadUserPermissions(ctx, u); err == nil { return np } - return NoPermissions + return NoPermissions() } // InternalPath returns the internal path of the Node @@ -733,25 +734,25 @@ func (n *Node) UnsetTempEtag() (err error) { } // ReadUserPermissions will assemble the permissions for the current user on the given node without parent nodes -func (n *Node) ReadUserPermissions(ctx context.Context, u *userpb.User) (ap *provider.ResourcePermissions, err error) { +func (n *Node) ReadUserPermissions(ctx context.Context, u *userpb.User) (ap provider.ResourcePermissions, err error) { // check if the current user is the owner o, err := n.Owner() if err != nil { // TODO check if a parent folder has the owner set? appctx.GetLogger(ctx).Error().Err(err).Interface("node", n).Msg("could not determine owner, returning default permissions") - return NoPermissions, err + return NoPermissions(), err } if o.OpaqueId == "" { // this happens for root nodes in the storage. the extended attributes are set to emptystring to indicate: no owner // TODO what if no owner is set but grants are present? - return NoOwnerPermissions, nil + return NoOwnerPermissions(), nil } if isSameUserID(u.Id, o) { appctx.GetLogger(ctx).Debug().Interface("node", n).Msg("user is owner, returning owner permissions") - return OwnerPermissions, nil + return OwnerPermissions(), nil } - ap = &provider.ResourcePermissions{} + ap = provider.ResourcePermissions{} // for an efficient group lookup convert the list of groups to a map // groups are just strings ... groupnames ... or group ids ??? AAARGH !!! @@ -766,7 +767,7 @@ func (n *Node) ReadUserPermissions(ctx context.Context, u *userpb.User) (ap *pro var grantees []string if grantees, err = n.ListGrantees(ctx); err != nil { appctx.GetLogger(ctx).Error().Err(err).Interface("node", n).Msg("error listing grantees") - return nil, err + return NoPermissions(), err } // instead of making n getxattr syscalls we are going to list the acls and filter them here @@ -796,7 +797,7 @@ func (n *Node) ReadUserPermissions(ctx context.Context, u *userpb.User) (ap *pro switch { case err == nil: - AddPermissions(ap, g.GetPermissions()) + AddPermissions(&ap, g.GetPermissions()) case isNoData(err): err = nil appctx.GetLogger(ctx).Error().Interface("node", n).Str("grant", grantees[i]).Interface("grantees", grantees).Msg("grant vanished from node after listing") diff --git a/pkg/storage/utils/decomposedfs/node/node_test.go b/pkg/storage/utils/decomposedfs/node/node_test.go index 84796bb8a6..d7bac23011 100644 --- a/pkg/storage/utils/decomposedfs/node/node_test.go +++ b/pkg/storage/utils/decomposedfs/node/node_test.go @@ -171,20 +171,22 @@ var _ = Describe("Node", func() { Describe("the Etag field", func() { It("is set", func() { - ri, err := n.AsResourceInfo(env.Ctx, node.OwnerPermissions, []string{}) + perms := node.OwnerPermissions() + ri, err := n.AsResourceInfo(env.Ctx, &perms, []string{}) Expect(err).ToNot(HaveOccurred()) Expect(len(ri.Etag)).To(Equal(34)) }) It("changes when the tmtime is set", func() { - ri, err := n.AsResourceInfo(env.Ctx, node.OwnerPermissions, []string{}) + perms := node.OwnerPermissions() + ri, err := n.AsResourceInfo(env.Ctx, &perms, []string{}) Expect(err).ToNot(HaveOccurred()) Expect(len(ri.Etag)).To(Equal(34)) before := ri.Etag Expect(n.SetTMTime(time.Now().UTC())).To(Succeed()) - ri, err = n.AsResourceInfo(env.Ctx, node.OwnerPermissions, []string{}) + ri, err = n.AsResourceInfo(env.Ctx, &perms, []string{}) Expect(err).ToNot(HaveOccurred()) Expect(len(ri.Etag)).To(Equal(34)) Expect(ri.Etag).ToNot(Equal(before)) diff --git a/pkg/storage/utils/decomposedfs/node/permissions.go b/pkg/storage/utils/decomposedfs/node/permissions.go index ea3e5cae9d..49ebd92214 100644 --- a/pkg/storage/utils/decomposedfs/node/permissions.go +++ b/pkg/storage/utils/decomposedfs/node/permissions.go @@ -32,35 +32,54 @@ import ( "github.com/pkg/xattr" ) -// NoPermissions represents an empty set of permssions -var NoPermissions *provider.ResourcePermissions = &provider.ResourcePermissions{} +// NoPermissions represents an empty set of permissions +func NoPermissions() provider.ResourcePermissions { + return provider.ResourcePermissions{} +} // NoOwnerPermissions defines permissions for nodes that don't have an owner set, eg the root node -var NoOwnerPermissions *provider.ResourcePermissions = &provider.ResourcePermissions{ - Stat: true, +func NoOwnerPermissions() provider.ResourcePermissions { + return provider.ResourcePermissions{ + Stat: true, + } +} + +// ShareFolderPermissions defines permissions for the shared jail +func ShareFolderPermissions() provider.ResourcePermissions { + return provider.ResourcePermissions{ + // read permissions + ListContainer: true, + Stat: true, + InitiateFileDownload: true, + GetPath: true, + GetQuota: true, + ListFileVersions: true, + } } // OwnerPermissions defines permissions for nodes owned by the user -var OwnerPermissions *provider.ResourcePermissions = &provider.ResourcePermissions{ - // all permissions - AddGrant: true, - CreateContainer: true, - Delete: true, - GetPath: true, - GetQuota: true, - InitiateFileDownload: true, - InitiateFileUpload: true, - ListContainer: true, - ListFileVersions: true, - ListGrants: true, - ListRecycle: true, - Move: true, - PurgeRecycle: true, - RemoveGrant: true, - RestoreFileVersion: true, - RestoreRecycleItem: true, - Stat: true, - UpdateGrant: true, +func OwnerPermissions() provider.ResourcePermissions { + return provider.ResourcePermissions{ + // all permissions + AddGrant: true, + CreateContainer: true, + Delete: true, + GetPath: true, + GetQuota: true, + InitiateFileDownload: true, + InitiateFileUpload: true, + ListContainer: true, + ListFileVersions: true, + ListGrants: true, + ListRecycle: true, + Move: true, + PurgeRecycle: true, + RemoveGrant: true, + RestoreFileVersion: true, + RestoreRecycleItem: true, + Stat: true, + UpdateGrant: true, + } } // Permissions implements permission checks @@ -76,38 +95,41 @@ func NewPermissions(lu PathLookup) *Permissions { } // AssemblePermissions will assemble the permissions for the current user on the given node, taking into account all parent nodes -func (p *Permissions) AssemblePermissions(ctx context.Context, n *Node) (ap *provider.ResourcePermissions, err error) { +func (p *Permissions) AssemblePermissions(ctx context.Context, n *Node) (ap provider.ResourcePermissions, err error) { u, ok := user.ContextGetUser(ctx) if !ok { appctx.GetLogger(ctx).Debug().Interface("node", n).Msg("no user in context, returning default permissions") - return NoPermissions, nil + return NoPermissions(), nil } // check if the current user is the owner o, err := n.Owner() if err != nil { // TODO check if a parent folder has the owner set? appctx.GetLogger(ctx).Error().Err(err).Interface("node", n).Msg("could not determine owner, returning default permissions") - return NoPermissions, err + return NoPermissions(), err } if o.OpaqueId == "" { // this happens for root nodes in the storage. the extended attributes are set to emptystring to indicate: no owner // TODO what if no owner is set but grants are present? - return NoOwnerPermissions, nil + return NoOwnerPermissions(), nil } if isSameUserID(u.Id, o) { + lp, err := n.lu.Path(ctx, n) + if err == nil && lp == n.lu.ShareFolder() { + return ShareFolderPermissions(), nil + } appctx.GetLogger(ctx).Debug().Interface("node", n).Msg("user is owner, returning owner permissions") - return OwnerPermissions, nil + return OwnerPermissions(), nil } - // determine root var rn *Node if rn, err = p.lu.RootNode(ctx); err != nil { - return nil, err + return NoPermissions(), err } cn := n - ap = &provider.ResourcePermissions{} + ap = provider.ResourcePermissions{} // for an efficient group lookup convert the list of groups to a map // groups are just strings ... groupnames ... or group ids ??? AAARGH !!! @@ -118,14 +140,12 @@ func (p *Permissions) AssemblePermissions(ctx context.Context, n *Node) (ap *pro // for all segments, starting at the leaf for cn.ID != rn.ID { - if np, err := cn.ReadUserPermissions(ctx, u); err == nil { - AddPermissions(ap, np) + AddPermissions(&ap, &np) } else { appctx.GetLogger(ctx).Error().Err(err).Interface("node", cn).Msg("error reading permissions") // continue with next segment } - if cn, err = cn.Parent(); err != nil { return ap, errors.Wrap(err, "Decomposedfs: error getting parent "+cn.ParentID) } @@ -232,7 +252,7 @@ func (p *Permissions) HasPermission(ctx context.Context, n *Node, check func(*pr } } - appctx.GetLogger(ctx).Debug().Interface("permissions", NoPermissions).Interface("node", n).Interface("user", u).Msg("no grant found, returning default permissions") + appctx.GetLogger(ctx).Debug().Interface("permissions", NoPermissions()).Interface("node", n).Interface("user", u).Msg("no grant found, returning default permissions") return false, nil } @@ -240,22 +260,26 @@ func (p *Permissions) getUserAndPermissions(ctx context.Context, n *Node) (*user u, ok := user.ContextGetUser(ctx) if !ok { appctx.GetLogger(ctx).Debug().Interface("node", n).Msg("no user in context, returning default permissions") - return nil, NoPermissions + perms := NoPermissions() + return nil, &perms } // check if the current user is the owner o, err := n.Owner() if err != nil { appctx.GetLogger(ctx).Error().Err(err).Interface("node", n).Msg("could not determine owner, returning default permissions") - return nil, NoPermissions + perms := NoPermissions() + return nil, &perms } if o.OpaqueId == "" { // this happens for root nodes in the storage. the extended attributes are set to emptystring to indicate: no owner // TODO what if no owner is set but grants are present? - return nil, NoOwnerPermissions + perms := NoOwnerPermissions() + return nil, &perms } if isSameUserID(u.Id, o) { appctx.GetLogger(ctx).Debug().Interface("node", n).Msg("user is owner, returning owner permissions") - return u, OwnerPermissions + perms := OwnerPermissions() + return u, &perms } return u, nil } diff --git a/pkg/storage/utils/decomposedfs/recycle.go b/pkg/storage/utils/decomposedfs/recycle.go index d3dcebe24a..cd45269d14 100644 --- a/pkg/storage/utils/decomposedfs/recycle.go +++ b/pkg/storage/utils/decomposedfs/recycle.go @@ -54,12 +54,12 @@ func (fs *Decomposedfs) ListRecycle(ctx context.Context, key, path string) ([]*p // TODO how do we check if the storage allows listing the recycle for the current user? check owner of the root of the storage? // use permissions ReadUserPermissions? if fs.o.EnableHome { - if !node.OwnerPermissions.ListContainer { + if !node.OwnerPermissions().ListContainer { log.Debug().Msg("owner not allowed to list trash") return items, errtypes.PermissionDenied("owner not allowed to list trash") } } else { - if !node.NoPermissions.ListContainer { + if !node.NoPermissions().ListContainer { log.Debug().Msg("default permissions prevent listing trash") return items, errtypes.PermissionDenied("default permissions prevent listing trash") } diff --git a/pkg/storage/utils/decomposedfs/tree/tree.go b/pkg/storage/utils/decomposedfs/tree/tree.go index 934799f9e1..c7211fc882 100644 --- a/pkg/storage/utils/decomposedfs/tree/tree.go +++ b/pkg/storage/utils/decomposedfs/tree/tree.go @@ -61,6 +61,7 @@ type PathLookup interface { InternalRoot() string InternalPath(ID string) string Path(ctx context.Context, n *node.Node) (path string, err error) + ShareFolder() string } // Tree manages a hierarchical tree diff --git a/pkg/storage/utils/decomposedfs/tree/tree_test.go b/pkg/storage/utils/decomposedfs/tree/tree_test.go index 08c9362a7c..d7f920578e 100644 --- a/pkg/storage/utils/decomposedfs/tree/tree_test.go +++ b/pkg/storage/utils/decomposedfs/tree/tree_test.go @@ -237,13 +237,14 @@ var _ = Describe("Tree", func() { file, err := env.CreateTestFile("file1", "", 1, dir.ID) Expect(err).ToNot(HaveOccurred()) - riBefore, err := dir.AsResourceInfo(env.Ctx, node.OwnerPermissions, []string{}) + perms := node.OwnerPermissions() + riBefore, err := dir.AsResourceInfo(env.Ctx, &perms, []string{}) Expect(err).ToNot(HaveOccurred()) err = env.Tree.Propagate(env.Ctx, file) Expect(err).ToNot(HaveOccurred()) - riAfter, err := dir.AsResourceInfo(env.Ctx, node.OwnerPermissions, []string{}) + riAfter, err := dir.AsResourceInfo(env.Ctx, &perms, []string{}) Expect(err).ToNot(HaveOccurred()) Expect(riAfter.Etag).ToNot(Equal(riBefore.Etag)) }) diff --git a/tests/acceptance/expected-failures-on-OCIS-storage.md b/tests/acceptance/expected-failures-on-OCIS-storage.md index b2b7333f7b..cbfbf84f27 100644 --- a/tests/acceptance/expected-failures-on-OCIS-storage.md +++ b/tests/acceptance/expected-failures-on-OCIS-storage.md @@ -1031,10 +1031,6 @@ _ocs: api compatibility, return correct status code_ - [apiShareManagementBasicToShares/createShareToSharesFolder.feature:741](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareManagementBasicToShares/createShareToSharesFolder.feature#L741) - [apiShareManagementBasicToShares/createShareToSharesFolder.feature:742](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareManagementBasicToShares/createShareToSharesFolder.feature#L742) -#### [No way to set default folder for received shares](https://github.com/owncloud/ocis/issues/1327) -- [apiShareCreateSpecialToShares2/createShareDefaultFolderForReceivedShares.feature:22](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareCreateSpecialToShares2/createShareDefaultFolderForReceivedShares.feature#L22) -- [apiShareCreateSpecialToShares2/createShareDefaultFolderForReceivedShares.feature:23](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareCreateSpecialToShares2/createShareDefaultFolderForReceivedShares.feature#L23) - #### [Group shares support ](https://github.com/owncloud/ocis/issues/1289) - [apiShareOperationsToShares1/gettingShares.feature:188](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareOperationsToShares1/gettingShares.feature#L188) diff --git a/tests/acceptance/expected-failures-on-S3NG-storage.md b/tests/acceptance/expected-failures-on-S3NG-storage.md index a5adaec24b..56505406f0 100644 --- a/tests/acceptance/expected-failures-on-S3NG-storage.md +++ b/tests/acceptance/expected-failures-on-S3NG-storage.md @@ -1040,10 +1040,6 @@ _ocs: api compatibility, return correct status code_ - [apiShareManagementBasicToShares/createShareToSharesFolder.feature:741](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareManagementBasicToShares/createShareToSharesFolder.feature#L741) - [apiShareManagementBasicToShares/createShareToSharesFolder.feature:742](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareManagementBasicToShares/createShareToSharesFolder.feature#L742) -#### [No way to set default folder for received shares](https://github.com/owncloud/ocis/issues/1327) -- [apiShareCreateSpecialToShares2/createShareDefaultFolderForReceivedShares.feature:22](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareCreateSpecialToShares2/createShareDefaultFolderForReceivedShares.feature#L22) -- [apiShareCreateSpecialToShares2/createShareDefaultFolderForReceivedShares.feature:23](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareCreateSpecialToShares2/createShareDefaultFolderForReceivedShares.feature#L23) - #### [Group shares support ](https://github.com/owncloud/ocis/issues/1289) - [apiShareOperationsToShares1/gettingShares.feature:188](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareOperationsToShares1/gettingShares.feature#L188)