From ceea2c53b913be11a1ce02f56fe2ab6f3ca558e0 Mon Sep 17 00:00:00 2001 From: Ishank Arora Date: Wed, 5 Aug 2020 17:47:38 +0200 Subject: [PATCH] Disallow sharing the shares directory (#1051) --- changelog/unreleased/shares-fix.md | 7 ++++ .../services/gateway/publicshareprovider.go | 4 +++ .../grpc/services/gateway/storageprovider.go | 20 ++++++++++- .../services/gateway/usershareprovider.go | 5 +++ pkg/share/manager/json/json.go | 34 ++++++++++-------- pkg/share/manager/memory/memory.go | 35 +++++++++++-------- 6 files changed, 75 insertions(+), 30 deletions(-) create mode 100644 changelog/unreleased/shares-fix.md diff --git a/changelog/unreleased/shares-fix.md b/changelog/unreleased/shares-fix.md new file mode 100644 index 0000000000..9be67b1c07 --- /dev/null +++ b/changelog/unreleased/shares-fix.md @@ -0,0 +1,7 @@ +Bugfix: Disallow sharing the shares directory + +Previously, it was possible to create public links for and share the shares +directory itself. However, when the recipient tried to accept the share, it +failed. This PR prevents the creation of such shares in the first place. + +https://github.com/cs3org/reva/pull/1051 diff --git a/internal/grpc/services/gateway/publicshareprovider.go b/internal/grpc/services/gateway/publicshareprovider.go index f9517ac0f3..b7894c7b99 100644 --- a/internal/grpc/services/gateway/publicshareprovider.go +++ b/internal/grpc/services/gateway/publicshareprovider.go @@ -29,6 +29,10 @@ import ( ) func (s *svc) CreatePublicShare(ctx context.Context, req *link.CreatePublicShareRequest) (*link.CreatePublicShareResponse, error) { + if s.isSharedFolder(ctx, req.ResourceInfo.GetPath()) { + return nil, errors.New("gateway: can't create a public share of the share folder itself") + } + log := appctx.GetLogger(ctx) log.Info().Msg("create public share") diff --git a/internal/grpc/services/gateway/storageprovider.go b/internal/grpc/services/gateway/storageprovider.go index 932aa79f35..656019cfaf 100644 --- a/internal/grpc/services/gateway/storageprovider.go +++ b/internal/grpc/services/gateway/storageprovider.go @@ -941,8 +941,26 @@ func (s *svc) Stat(ctx context.Context, req *provider.StatRequest) (*provider.St Path: target, }, } + req.Ref = ref - return s.stat(ctx, req) + res, err := s.stat(ctx, req) + if err != nil { + return &provider.StatResponse{ + Status: status.NewInternal(ctx, err, "gateway: error stating"), + }, nil + } + if res.Status.Code != rpc.Code_CODE_OK { + err := status.NewErrorFromCode(res.Status.Code, "gateway") + log.Err(err).Msg("gateway: error stating") + return &provider.StatResponse{ + Status: status.NewInternal(ctx, err, "gateway: error stating"), + }, nil + } + + // we need to make sure we don't expose the reference target in the resource + // information. + res.Info.Path = p + return res, nil } panic("gateway: stating an unknown path:" + p) diff --git a/internal/grpc/services/gateway/usershareprovider.go b/internal/grpc/services/gateway/usershareprovider.go index 7ef63548bb..30086aedd2 100644 --- a/internal/grpc/services/gateway/usershareprovider.go +++ b/internal/grpc/services/gateway/usershareprovider.go @@ -35,6 +35,11 @@ import ( // 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) { + + if s.isSharedFolder(ctx, req.ResourceInfo.GetPath()) { + return nil, errors.New("gateway: can't share the share folder itself") + } + c, err := pool.GetUserShareProviderClient(s.c.UserShareProviderEndpoint) if err != nil { return &collaboration.CreateShareResponse{ diff --git a/pkg/share/manager/json/json.go b/pkg/share/manager/json/json.go index 0a5d20ea03..af390fe811 100644 --- a/pkg/share/manager/json/json.go +++ b/pkg/share/manager/json/json.go @@ -163,16 +163,17 @@ func (m *mgr) Share(ctx context.Context, md *provider.ResourceInfo, g *collabora Nanos: uint32(now % 1000000000), } - // do not allow share to myself if share is for a user + // do not allow share to myself or the owner if share is for a user // TODO(labkode): should not this be caught already at the gw level? if g.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER && - g.Grantee.Id.Idp == user.Id.Idp && g.Grantee.Id.OpaqueId == user.Id.OpaqueId { - return nil, errors.New("json: user and grantee are the same") + ((g.Grantee.Id.Idp == user.Id.Idp && g.Grantee.Id.OpaqueId == user.Id.OpaqueId) || + (g.Grantee.Id.Idp == md.Owner.Idp && g.Grantee.Id.OpaqueId == md.Owner.OpaqueId)) { + return nil, errors.New("json: owner/creator and grantee are the same") } // check if share already exists. key := &collaboration.ShareKey{ - Owner: user.Id, + Owner: md.Owner, ResourceId: md.Id, Grantee: g.Grantee, } @@ -190,7 +191,7 @@ func (m *mgr) Share(ctx context.Context, md *provider.ResourceInfo, g *collabora ResourceId: md.Id, Permissions: g.Permissions, Grantee: g.Grantee, - Owner: user.Id, + Owner: md.Owner, Creator: user.Id, Ctime: ts, Mtime: ts, @@ -223,7 +224,8 @@ func (m *mgr) getByKey(ctx context.Context, key *collaboration.ShareKey) (*colla m.Lock() defer m.Unlock() for _, s := range m.model.Shares { - if key.Owner.Idp == s.Owner.Idp && key.Owner.OpaqueId == s.Owner.OpaqueId && + if ((key.Owner.Idp == s.Owner.Idp && key.Owner.OpaqueId == s.Owner.OpaqueId) || + (key.Owner.Idp == s.Creator.Idp && key.Owner.OpaqueId == s.Creator.OpaqueId)) && key.ResourceId.StorageId == s.ResourceId.StorageId && key.ResourceId.OpaqueId == s.ResourceId.OpaqueId && key.Grantee.Type == s.Grantee.Type && key.Grantee.Id.Idp == s.Grantee.Id.Idp && key.Grantee.Id.OpaqueId == s.Grantee.Id.OpaqueId { return s, nil @@ -247,9 +249,9 @@ func (m *mgr) get(ctx context.Context, ref *collaboration.ShareReference) (s *co } // check if we are the owner - // TODO(labkode): check for creator also. user := user.ContextMustGetUser(ctx) - if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId { + if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) || + (user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) { return s, nil } @@ -272,7 +274,8 @@ func (m *mgr) Unshare(ctx context.Context, ref *collaboration.ShareReference) er user := user.ContextMustGetUser(ctx) for i, s := range m.model.Shares { if equal(ref, s) { - if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId { + if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) || + (user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) { m.model.Shares[len(m.model.Shares)-1], m.model.Shares[i] = m.model.Shares[i], m.model.Shares[len(m.model.Shares)-1] m.model.Shares = m.model.Shares[:len(m.model.Shares)-1] if err := m.model.Save(); err != nil { @@ -293,7 +296,8 @@ func equal(ref *collaboration.ShareReference, s *collaboration.Share) bool { return true } } else if ref.GetKey() != nil { - if reflect.DeepEqual(*ref.GetKey().Owner, *s.Owner) && reflect.DeepEqual(*ref.GetKey().ResourceId, *s.ResourceId) && reflect.DeepEqual(*ref.GetKey().Grantee, *s.Grantee) { + if (reflect.DeepEqual(*ref.GetKey().Owner, *s.Owner) || reflect.DeepEqual(*ref.GetKey().Owner, *s.Creator)) && + reflect.DeepEqual(*ref.GetKey().ResourceId, *s.ResourceId) && reflect.DeepEqual(*ref.GetKey().Grantee, *s.Grantee) { return true } } @@ -306,7 +310,8 @@ func (m *mgr) UpdateShare(ctx context.Context, ref *collaboration.ShareReference user := user.ContextMustGetUser(ctx) for i, s := range m.model.Shares { if equal(ref, s) { - if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId { + if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) || + (user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) { now := time.Now().UnixNano() m.model.Shares[i].Permissions = p m.model.Shares[i].Mtime = &typespb.Timestamp{ @@ -331,7 +336,8 @@ func (m *mgr) ListShares(ctx context.Context, filters []*collaboration.ListShare user := user.ContextMustGetUser(ctx) for _, s := range m.model.Shares { // TODO(labkode): add check for creator. - if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId { + if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) || + (user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) { // no filter we return earlier if len(filters) == 0 { ss = append(ss, s) @@ -358,9 +364,9 @@ func (m *mgr) ListReceivedShares(ctx context.Context) ([]*collaboration.Received defer m.Unlock() user := user.ContextMustGetUser(ctx) for _, s := range m.model.Shares { - if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId { + if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) || + (user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) { // omit shares created by me - // TODO(labkode): apply check for s.Creator also. continue } if s.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER { diff --git a/pkg/share/manager/memory/memory.go b/pkg/share/manager/memory/memory.go index 38760ebcc4..8c70940066 100644 --- a/pkg/share/manager/memory/memory.go +++ b/pkg/share/manager/memory/memory.go @@ -75,15 +75,16 @@ func (m *manager) Share(ctx context.Context, md *provider.ResourceInfo, g *colla Nanos: uint32(now % 1000000000), } - // do not allow share to myself if share is for a user + // do not allow share to myself or the owner if share is for a user if g.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER && - g.Grantee.Id.Idp == user.Id.Idp && g.Grantee.Id.OpaqueId == user.Id.OpaqueId { - return nil, errors.New("memory: user and grantee are the same") + ((g.Grantee.Id.Idp == user.Id.Idp && g.Grantee.Id.OpaqueId == user.Id.OpaqueId) || + (g.Grantee.Id.Idp == md.Owner.Idp && g.Grantee.Id.OpaqueId == md.Owner.OpaqueId)) { + return nil, errors.New("json: owner/creator and grantee are the same") } // check if share already exists. key := &collaboration.ShareKey{ - Owner: user.Id, + Owner: md.Owner, ResourceId: md.Id, Grantee: g.Grantee, } @@ -100,7 +101,7 @@ func (m *manager) Share(ctx context.Context, md *provider.ResourceInfo, g *colla ResourceId: md.Id, Permissions: g.Permissions, Grantee: g.Grantee, - Owner: user.Id, + Owner: md.Owner, Creator: user.Id, Ctime: ts, Mtime: ts, @@ -125,7 +126,8 @@ func (m *manager) getByKey(ctx context.Context, key *collaboration.ShareKey) (*c m.lock.Lock() defer m.lock.Unlock() for _, s := range m.shares { - if key.Owner.Idp == s.Owner.Idp && key.Owner.OpaqueId == s.Owner.OpaqueId && + if ((key.Owner.Idp == s.Owner.Idp && key.Owner.OpaqueId == s.Owner.OpaqueId) || + (key.Owner.Idp == s.Creator.Idp && key.Owner.OpaqueId == s.Creator.OpaqueId)) && key.ResourceId.StorageId == s.ResourceId.StorageId && key.ResourceId.OpaqueId == s.ResourceId.OpaqueId && key.Grantee.Type == s.Grantee.Type && key.Grantee.Id.Idp == s.Grantee.Id.Idp && key.Grantee.Id.OpaqueId == s.Grantee.Id.OpaqueId { return s, nil @@ -149,9 +151,9 @@ func (m *manager) get(ctx context.Context, ref *collaboration.ShareReference) (s } // check if we are the owner - // TODO(labkode): check for creator also. user := user.ContextMustGetUser(ctx) - if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId { + if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) || + (user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) { return s, nil } @@ -174,7 +176,8 @@ func (m *manager) Unshare(ctx context.Context, ref *collaboration.ShareReference user := user.ContextMustGetUser(ctx) for i, s := range m.shares { if equal(ref, s) { - if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId { + if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) || + (user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) { m.shares[len(m.shares)-1], m.shares[i] = m.shares[i], m.shares[len(m.shares)-1] m.shares = m.shares[:len(m.shares)-1] return nil @@ -191,7 +194,8 @@ func equal(ref *collaboration.ShareReference, s *collaboration.Share) bool { return true } } else if ref.GetKey() != nil { - if reflect.DeepEqual(*ref.GetKey().Owner, *s.Owner) && reflect.DeepEqual(*ref.GetKey().ResourceId, *s.ResourceId) && reflect.DeepEqual(*ref.GetKey().Grantee, *s.Grantee) { + if (reflect.DeepEqual(*ref.GetKey().Owner, *s.Owner) || reflect.DeepEqual(*ref.GetKey().Owner, *s.Creator)) && + reflect.DeepEqual(*ref.GetKey().ResourceId, *s.ResourceId) && reflect.DeepEqual(*ref.GetKey().Grantee, *s.Grantee) { return true } } @@ -204,7 +208,8 @@ func (m *manager) UpdateShare(ctx context.Context, ref *collaboration.ShareRefer user := user.ContextMustGetUser(ctx) for i, s := range m.shares { if equal(ref, s) { - if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId { + if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) || + (user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) { now := time.Now().UnixNano() m.shares[i].Permissions = p m.shares[i].Mtime = &typespb.Timestamp{ @@ -224,8 +229,8 @@ func (m *manager) ListShares(ctx context.Context, filters []*collaboration.ListS defer m.lock.Unlock() user := user.ContextMustGetUser(ctx) for _, s := range m.shares { - // TODO(labkode): add check for creator. - if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId { + if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) || + (user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) { // no filter we return earlier if len(filters) == 0 { ss = append(ss, s) @@ -252,9 +257,9 @@ func (m *manager) ListReceivedShares(ctx context.Context) ([]*collaboration.Rece defer m.lock.Unlock() user := user.ContextMustGetUser(ctx) for _, s := range m.shares { - if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId { + if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) || + (user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) { // omit shares created by me - // TODO(labkode): apply check for s.Creator also. continue } if s.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER {