Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent recursive copy/move operations on the API level #3009

Merged
merged 9 commits into from
Jul 5, 2022
5 changes: 5 additions & 0 deletions changelog/unreleased/prevent-recursive-copy-move.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Enhancement: Prevent recursive copy/move operations

We changed the ocs API to prevent copying or moving a folder into one of its children.

https://github.com/cs3org/reva/pull/3009
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import (
provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
typesv1beta1 "github.com/cs3org/go-cs3apis/cs3/types/v1beta1"
"github.com/cs3org/reva/v2/pkg/appctx"
ctxpkg "github.com/cs3org/reva/v2/pkg/ctx"
"github.com/cs3org/reva/v2/pkg/errtypes"
"github.com/cs3org/reva/v2/pkg/rgrpc"
"github.com/cs3org/reva/v2/pkg/rgrpc/status"
Expand Down Expand Up @@ -272,6 +273,14 @@ func (s *service) GetPath(ctx context.Context, req *provider.GetPathRequest) (*p
// - getPath of every received share on the same space - needs also owner permissions -> needs machine auth
// - find the shortest root path that is a prefix of the resource path
// alternatively implement this on storageprovider - it needs to know about grants to do so

if isShareJailRoot(req.ResourceId) {
return &provider.GetPathResponse{
Status: status.NewOK(ctx),
Path: "/",
}, nil
}

Comment on lines +277 to +283
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 minimal implementation so we can get the path to the share jail root

return nil, gstatus.Errorf(codes.Unimplemented, "method not implemented")
}

Expand Down Expand Up @@ -662,6 +671,10 @@ func (s *service) Stat(ctx context.Context, req *provider.StatRequest) (*provide
}

if isVirtualRoot(req.Ref) {
owner, ok := ctxpkg.ContextGetUser(ctx)
if !ok {
return nil, fmt.Errorf("missing user in context")
}
receivedShares, shareMd, err := s.fetchShares(ctx)
if err != nil {
return nil, err
Expand Down Expand Up @@ -690,7 +703,8 @@ func (s *service) Stat(ctx context.Context, req *provider.StatRequest) (*provide
PermissionSet: &provider.ResourcePermissions{
// TODO
},
Etag: shareMd[earliestShare.Id.OpaqueId].ETag,
Etag: shareMd[earliestShare.Id.OpaqueId].ETag,
Owner: owner.Id,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 yes, the share jail is always owned by the current user

},
}, nil
}
Expand Down
33 changes: 26 additions & 7 deletions internal/http/services/owncloud/ocdav/copy.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (
"github.com/cs3org/reva/v2/internal/http/services/owncloud/ocdav/net"
"github.com/cs3org/reva/v2/internal/http/services/owncloud/ocdav/spacelookup"
"github.com/cs3org/reva/v2/pkg/appctx"
"github.com/cs3org/reva/v2/pkg/errtypes"
"github.com/cs3org/reva/v2/pkg/rhttp"
"github.com/cs3org/reva/v2/pkg/rhttp/router"
"github.com/cs3org/reva/v2/pkg/utils"
Expand Down Expand Up @@ -486,6 +487,31 @@ func (s *svc) executeSpacesCopy(ctx context.Context, w http.ResponseWriter, clie
}

func (s *svc) prepareCopy(ctx context.Context, w http.ResponseWriter, r *http.Request, srcRef, dstRef *provider.Reference, log *zerolog.Logger) *copy {
client, err := s.getClient()
if err != nil {
log.Error().Err(err).Msg("error getting grpc client")
w.WriteHeader(http.StatusInternalServerError)
return nil
}

isChild, err := s.referenceIsChildOf(ctx, client, dstRef, srcRef)
if err != nil {
switch err.(type) {
case errtypes.IsNotSupported:
log.Error().Err(err).Msg("can not detect recursive copy operation. missing machine auth configuration?")
w.WriteHeader(http.StatusForbidden)
default:
log.Error().Err(err).Msg("error while trying to detect recursive copy operation")
w.WriteHeader(http.StatusInternalServerError)
}
}
if isChild {
w.WriteHeader(http.StatusBadRequest)
b, err := errors.Marshal(http.StatusBadRequest, "can not copy a folder into one of its children", "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

errors.HandleWebdavError(log, w, b, err)
return nil
}

oh := r.Header.Get(net.HeaderOverwrite)
overwrite, err := net.ParseOverwrite(oh)
if err != nil {
Expand Down Expand Up @@ -513,13 +539,6 @@ func (s *svc) prepareCopy(ctx context.Context, w http.ResponseWriter, r *http.Re

log.Debug().Bool("overwrite", overwrite).Str("depth", depth.String()).Msg("copy")

client, err := s.getClient()
if err != nil {
log.Error().Err(err).Msg("error getting grpc client")
w.WriteHeader(http.StatusInternalServerError)
return nil
}

srcStatReq := &provider.StatRequest{Ref: srcRef}
srcStatRes, err := client.Stat(ctx, srcStatReq)
if err != nil {
Expand Down
31 changes: 25 additions & 6 deletions internal/http/services/owncloud/ocdav/move.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"github.com/cs3org/reva/v2/internal/http/services/owncloud/ocdav/net"
"github.com/cs3org/reva/v2/internal/http/services/owncloud/ocdav/spacelookup"
"github.com/cs3org/reva/v2/pkg/appctx"
"github.com/cs3org/reva/v2/pkg/errtypes"
"github.com/cs3org/reva/v2/pkg/rhttp/router"
"github.com/cs3org/reva/v2/pkg/storagespace"
"github.com/cs3org/reva/v2/pkg/utils"
Expand Down Expand Up @@ -132,19 +133,37 @@ func (s *svc) handleSpacesMove(w http.ResponseWriter, r *http.Request, srcSpaceI
}

func (s *svc) handleMove(ctx context.Context, w http.ResponseWriter, r *http.Request, src, dst *provider.Reference, log zerolog.Logger) {
oh := r.Header.Get(net.HeaderOverwrite)
log.Debug().Str("overwrite", oh).Msg("move")
client, err := s.getClient()
if err != nil {
log.Error().Err(err).Msg("error getting grpc client")
w.WriteHeader(http.StatusInternalServerError)
return
}

overwrite, err := net.ParseOverwrite(oh)
isChild, err := s.referenceIsChildOf(ctx, client, dst, src)
if err != nil {
switch err.(type) {
case errtypes.IsNotSupported:
log.Error().Err(err).Msg("can not detect recursive move operation. missing machine auth configuration?")
w.WriteHeader(http.StatusForbidden)
default:
log.Error().Err(err).Msg("error while trying to detect recursive move operation")
w.WriteHeader(http.StatusInternalServerError)
}
}
if isChild {
w.WriteHeader(http.StatusBadRequest)
b, err := errors.Marshal(http.StatusBadRequest, "can not move a folder into one of its children", "")
errors.HandleWebdavError(&log, w, b, err)
return
}

client, err := s.getClient()
oh := r.Header.Get(net.HeaderOverwrite)
log.Debug().Str("overwrite", oh).Msg("move")

overwrite, err := net.ParseOverwrite(oh)
if err != nil {
log.Error().Err(err).Msg("error getting grpc client")
w.WriteHeader(http.StatusInternalServerError)
w.WriteHeader(http.StatusBadRequest)
return
}

Expand Down
109 changes: 109 additions & 0 deletions internal/http/services/owncloud/ocdav/ocdav.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,10 @@ import (

"github.com/ReneKroon/ttlcache/v2"
gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1"
gatewayv1beta1 "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1"
userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1"
rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
"github.com/cs3org/reva/v2/internal/http/services/owncloud/ocdav/net"
"github.com/cs3org/reva/v2/pkg/appctx"
ctxpkg "github.com/cs3org/reva/v2/pkg/ctx"
Expand All @@ -41,10 +43,15 @@ import (
"github.com/cs3org/reva/v2/pkg/storage/favorite"
"github.com/cs3org/reva/v2/pkg/storage/favorite/registry"
"github.com/cs3org/reva/v2/pkg/storage/utils/templates"
"github.com/cs3org/reva/v2/pkg/storagespace"
rtrace "github.com/cs3org/reva/v2/pkg/trace"
"github.com/cs3org/reva/v2/pkg/utils"
"github.com/mitchellh/mapstructure"
"github.com/rs/zerolog"
"go.opentelemetry.io/otel/trace"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/metadata"
"google.golang.org/grpc/status"
)

// name is the Tracer name used to identify this instrumentation library.
Expand Down Expand Up @@ -107,6 +114,8 @@ type Config struct {
Product string `mapstructure:"product"`
ProductName string `mapstructure:"product_name"`
ProductVersion string `mapstructure:"product_version"`

MachineAuthAPIKey string `mapstructure:"machine_auth_apikey"`
}

func (c *Config) init() {
Expand Down Expand Up @@ -384,3 +393,103 @@ func addAccessHeaders(w http.ResponseWriter, r *http.Request) {
headers.Set("Strict-Transport-Security", "max-age=63072000")
}
}

func authContextForUser(client gatewayv1beta1.GatewayAPIClient, userID *userpb.UserId, machineAuthAPIKey string) (context.Context, error) {
if machineAuthAPIKey == "" {
return nil, errtypes.NotSupported("machine auth not configured")
}
// Get auth
granteeCtx := ctxpkg.ContextSetUser(context.Background(), &userpb.User{Id: userID})

authRes, err := client.Authenticate(granteeCtx, &gateway.AuthenticateRequest{
Type: "machine",
ClientId: "userid:" + userID.OpaqueId,
ClientSecret: machineAuthAPIKey,
})
if err != nil {
return nil, err
}
if authRes.GetStatus().GetCode() != rpc.Code_CODE_OK {
return nil, errtypes.NewErrtypeFromStatus(authRes.Status)
}
granteeCtx = metadata.AppendToOutgoingContext(granteeCtx, ctxpkg.TokenHeader, authRes.Token)
return granteeCtx, nil
}

func (s *svc) sspReferenceIsChildOf(ctx context.Context, client gatewayv1beta1.GatewayAPIClient, child, parent *provider.Reference) (bool, error) {
parentStatRes, err := client.Stat(ctx, &provider.StatRequest{Ref: parent})
if err != nil {
return false, err
}
parentAuthCtx, err := authContextForUser(client, parentStatRes.Info.Owner, s.c.MachineAuthAPIKey)
aduffeck marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return false, err
}
parentPathRes, err := client.GetPath(parentAuthCtx, &provider.GetPathRequest{ResourceId: parentStatRes.Info.Id})
if err != nil {
return false, err
}

childStatRes, err := client.Stat(ctx, &provider.StatRequest{Ref: child})
if err != nil {
return false, err
}
if childStatRes.Status.Code == rpc.Code_CODE_NOT_FOUND && utils.IsRelativeReference(child) && child.Path != "." {
childParentRef := &provider.Reference{
ResourceId: child.ResourceId,
Path: utils.MakeRelativePath(path.Dir(child.Path)),
}
childStatRes, err = client.Stat(ctx, &provider.StatRequest{Ref: childParentRef})
if err != nil {
return false, err
}
}
childAuthCtx, err := authContextForUser(client, childStatRes.Info.Owner, s.c.MachineAuthAPIKey)
if err != nil {
return false, err
}
childPathRes, err := client.GetPath(childAuthCtx, &provider.GetPathRequest{ResourceId: childStatRes.Info.Id})
if err != nil {
return false, err
}

cp := childPathRes.Path + "/"
pp := parentPathRes.Path + "/"
return strings.HasPrefix(cp, pp), nil
}

func (s *svc) referenceIsChildOf(ctx context.Context, client gatewayv1beta1.GatewayAPIClient, child, parent *provider.Reference) (bool, error) {
_, csid := storagespace.SplitStorageID(child.ResourceId.StorageId)
_, psid := storagespace.SplitStorageID(parent.ResourceId.StorageId)

if child.ResourceId.StorageId != parent.ResourceId.StorageId {
return false, nil // Not on the same storage -> not a child
}

if utils.ResourceIDEqual(child.ResourceId, parent.ResourceId) {
return strings.HasPrefix(child.Path, parent.Path+"/"), nil // Relative to the same resource -> compare paths
}

if csid == utils.ShareStorageProviderID || psid == utils.ShareStorageProviderID {
// the sharesstorageprovider needs some special handling
return s.sspReferenceIsChildOf(ctx, client, child, parent)
}

// the references are on the same storage but relative to different resources
// -> we need to get the path for both resources
childPathRes, err := client.GetPath(ctx, &provider.GetPathRequest{ResourceId: child.ResourceId})
if err != nil {
if st, ok := status.FromError(err); ok && st.Code() == codes.Unimplemented {
return false, nil // the storage provider doesn't support GetPath() -> rely on it taking care of recursion issues
}
return false, err
}
parentPathRes, err := client.GetPath(ctx, &provider.GetPathRequest{ResourceId: parent.ResourceId})
if err != nil {
return false, err
}

cp := path.Join(childPathRes.Path, child.Path) + "/"
pp := path.Join(parentPathRes.Path, parent.Path) + "/"
return strings.HasPrefix(cp, pp), nil
}
7 changes: 7 additions & 0 deletions pkg/micro/ocdav/option.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,13 @@ func JWTSecret(s string) Option {
}
}

// MachineAuthAPIKey provides a function to set the machine auth api key option.
func MachineAuthAPIKey(s string) Option {
return func(o *Options) {
o.config.MachineAuthAPIKey = s
}
}

// Context provides a function to set the context option.
func Context(val context.Context) Option {
return func(o *Options) {
Expand Down
1 change: 1 addition & 0 deletions tests/oc-integration-tests/drone/frontend-global.toml
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ files_namespace = "/"
# - the oc js sdk is hardcoded to the remote.php/webdav so it will see the new tree
# - TODO android? no sync ... but will see different tree
webdav_namespace = "/"
machine_auth_apikey = "change-me-please"

[http.services.ocs]

Expand Down
1 change: 1 addition & 0 deletions tests/oc-integration-tests/drone/frontend.toml
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ files_namespace = "/users/{{.Id.OpaqueId}}"
# - the oc js sdk is hardcoded to the remote.php/webdav so it will see the new tree
# - TODO android? no sync ... but will see different tree
webdav_namespace = "/users/{{.Id.OpaqueId}}"
machine_auth_apikey = "change-me-please"

[http.services.ocs]
machine_auth_apikey = "change-me-please"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ files_namespace = "/"
# - the oc js sdk is hardcoded to the remote.php/webdav so it will see the new tree
# - TODO android? no sync ... but will see different tree
webdav_namespace = "/"
machine_auth_apikey = "change-me-please"

[http.services.ocs]

Expand Down
1 change: 1 addition & 0 deletions tests/oc-integration-tests/local-mesh/frontend.toml
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ files_namespace = "/users"
# - the oc js sdk is hardcoded to the remote.php/webdav so it will see the new tree
# - TODO android? no sync ... but will see different tree
webdav_namespace = "/home"
machine_auth_apikey = "change-me-please"

# serve /ocs which contains the sharing and user provisioning api of owncloud classic
[http.services.ocs]
Expand Down
1 change: 1 addition & 0 deletions tests/oc-integration-tests/local/combined.toml
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ files_namespace = "/personal/{{.Id.OpaqueId}}"
# - the oc js sdk is hardcoded to the remote.php/webdav so it will see the new tree
# - TODO android? no sync ... but will see different tree
webdav_namespace = "/home"
machine_auth_apikey = "change-me-please"

[http.services.ocs]
machine_auth_apikey = "change-me-please"
Expand Down
1 change: 1 addition & 0 deletions tests/oc-integration-tests/local/frontend-global.toml
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ files_namespace = "/"
# - the oc js sdk is hardcoded to the remote.php/webdav so it will see the new tree
# - TODO android? no sync ... but will see different tree
webdav_namespace = "/"
machine_auth_apikey = "change-me-please"

[http.services.ocs]

Expand Down
1 change: 1 addition & 0 deletions tests/oc-integration-tests/local/frontend.toml
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ files_namespace = "/users/{{.Id.OpaqueId}}"
# - the oc js sdk is hardcoded to the remote.php/webdav so it will see the new tree
# - TODO android? no sync ... but will see different tree
webdav_namespace = "/users/{{.Id.OpaqueId}}"
machine_auth_apikey = "change-me-please"

# serve /ocs which contains the sharing and user provisioning api of owncloud classic
[http.services.ocs]
Expand Down