From e6bf35264d2d9ca911dde0425300564ac607e683 Mon Sep 17 00:00:00 2001 From: Ishank Arora Date: Fri, 31 Jul 2020 16:21:50 +0200 Subject: [PATCH 1/4] Disallow sharing the shares directory --- changelog/unreleased/shares-fix.md | 7 +++++++ .../services/gateway/publicshareprovider.go | 4 ++++ .../grpc/services/gateway/storageprovider.go | 20 ++++++++++++++++++- .../services/gateway/usershareprovider.go | 5 +++++ 4 files changed, 35 insertions(+), 1 deletion(-) 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{ From 18de513e5143c12f7eb593faabf112b2cce05330 Mon Sep 17 00:00:00 2001 From: Ishank Arora Date: Tue, 4 Aug 2020 17:28:01 +0200 Subject: [PATCH 2/4] Use owner from ResourceInfo as the owner of the share --- pkg/share/manager/json/json.go | 9 ++++++++- pkg/share/manager/memory/memory.go | 9 ++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/pkg/share/manager/json/json.go b/pkg/share/manager/json/json.go index 0a5d20ea03..0c01dbeaed 100644 --- a/pkg/share/manager/json/json.go +++ b/pkg/share/manager/json/json.go @@ -170,6 +170,13 @@ func (m *mgr) Share(ctx context.Context, md *provider.ResourceInfo, g *collabora return nil, errors.New("json: user and grantee are the same") } + // don't share with the owner of the resource. This won't be caught by the + // next check + if g.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER && + g.Grantee.Id.Idp == md.Owner.Idp && g.Grantee.Id.OpaqueId == md.Owner.OpaqueId { + return nil, errors.New("json: grantee is the owner of the resource") + } + // check if share already exists. key := &collaboration.ShareKey{ Owner: user.Id, @@ -190,7 +197,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, diff --git a/pkg/share/manager/memory/memory.go b/pkg/share/manager/memory/memory.go index 38760ebcc4..0a4b78aa65 100644 --- a/pkg/share/manager/memory/memory.go +++ b/pkg/share/manager/memory/memory.go @@ -81,6 +81,13 @@ func (m *manager) Share(ctx context.Context, md *provider.ResourceInfo, g *colla return nil, errors.New("memory: user and grantee are the same") } + // don't share with the owner of the resource. This won't be caught by the + // next check + if g.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER && + g.Grantee.Id.Idp == md.Owner.Idp && g.Grantee.Id.OpaqueId == md.Owner.OpaqueId { + return nil, errors.New("json: grantee is the owner of the resource") + } + // check if share already exists. key := &collaboration.ShareKey{ Owner: user.Id, @@ -100,7 +107,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, From f66154624e50f647ea8983c29a1fb53746d6b33e Mon Sep 17 00:00:00 2001 From: Ishank Arora Date: Wed, 5 Aug 2020 13:37:37 +0200 Subject: [PATCH 3/4] Remove check to share with owner --- pkg/share/manager/json/json.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/share/manager/json/json.go b/pkg/share/manager/json/json.go index 0c01dbeaed..0608975dd6 100644 --- a/pkg/share/manager/json/json.go +++ b/pkg/share/manager/json/json.go @@ -172,10 +172,10 @@ func (m *mgr) Share(ctx context.Context, md *provider.ResourceInfo, g *collabora // don't share with the owner of the resource. This won't be caught by the // next check - if g.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER && - g.Grantee.Id.Idp == md.Owner.Idp && g.Grantee.Id.OpaqueId == md.Owner.OpaqueId { - return nil, errors.New("json: grantee is the owner of the resource") - } + // if g.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER && + // g.Grantee.Id.Idp == md.Owner.Idp && g.Grantee.Id.OpaqueId == md.Owner.OpaqueId { + // return nil, errors.New("json: grantee is the owner of the resource") + // } // check if share already exists. key := &collaboration.ShareKey{ From c6b982b1a8c508120048ecf5a1d90524eb397ce8 Mon Sep 17 00:00:00 2001 From: Ishank Arora Date: Wed, 5 Aug 2020 15:40:10 +0200 Subject: [PATCH 4/4] Add filters for share creator --- pkg/share/manager/json/json.go | 39 ++++++++++++++--------------- pkg/share/manager/memory/memory.go | 40 ++++++++++++++---------------- 2 files changed, 38 insertions(+), 41 deletions(-) diff --git a/pkg/share/manager/json/json.go b/pkg/share/manager/json/json.go index 0608975dd6..af390fe811 100644 --- a/pkg/share/manager/json/json.go +++ b/pkg/share/manager/json/json.go @@ -163,23 +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") } - // don't share with the owner of the resource. This won't be caught by the - // next check - // if g.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER && - // g.Grantee.Id.Idp == md.Owner.Idp && g.Grantee.Id.OpaqueId == md.Owner.OpaqueId { - // return nil, errors.New("json: grantee is the owner of the resource") - // } - // check if share already exists. key := &collaboration.ShareKey{ - Owner: user.Id, + Owner: md.Owner, ResourceId: md.Id, Grantee: g.Grantee, } @@ -230,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 @@ -254,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 } @@ -279,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 { @@ -300,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 } } @@ -313,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{ @@ -338,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) @@ -365,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 0a4b78aa65..8c70940066 100644 --- a/pkg/share/manager/memory/memory.go +++ b/pkg/share/manager/memory/memory.go @@ -75,22 +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") - } - - // don't share with the owner of the resource. This won't be caught by the - // next check - if g.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER && - g.Grantee.Id.Idp == md.Owner.Idp && g.Grantee.Id.OpaqueId == md.Owner.OpaqueId { - return nil, errors.New("json: grantee is the owner of the resource") + ((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, } @@ -132,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 @@ -156,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 } @@ -181,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 @@ -198,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 } } @@ -211,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{ @@ -231,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) @@ -259,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 {