From fd7ba900ca760cdd732c0bd90eedaba6f445d036 Mon Sep 17 00:00:00 2001 From: Andre Duffeck Date: Tue, 5 Jul 2022 10:54:06 +0200 Subject: [PATCH] Prevent recursive copy/move operations on the API level (#3009) * Prevent recursive copy/move operations on the API level * Add changelog * Enable machine auth in ocdav * Fix error message * Also detect recursive operations when the SSP is involved * Fix hound issues * Set the owner on the stat response to the share jail * Implement GetPath() for the share jail root * Return a better error when the ocdav machine auth config is missing --- .../unreleased/prevent-recursive-copy-move.md | 5 + .../sharesstorageprovider.go | 16 ++- internal/http/services/owncloud/ocdav/copy.go | 33 ++++-- internal/http/services/owncloud/ocdav/move.go | 31 ++++- .../http/services/owncloud/ocdav/ocdav.go | 109 ++++++++++++++++++ pkg/micro/ocdav/option.go | 7 ++ .../drone/frontend-global.toml | 1 + .../oc-integration-tests/drone/frontend.toml | 1 + .../local-mesh/frontend-global.toml | 1 + .../local-mesh/frontend.toml | 1 + .../oc-integration-tests/local/combined.toml | 1 + .../local/frontend-global.toml | 1 + .../oc-integration-tests/local/frontend.toml | 1 + 13 files changed, 194 insertions(+), 14 deletions(-) create mode 100644 changelog/unreleased/prevent-recursive-copy-move.md diff --git a/changelog/unreleased/prevent-recursive-copy-move.md b/changelog/unreleased/prevent-recursive-copy-move.md new file mode 100644 index 0000000000..9ce85b6119 --- /dev/null +++ b/changelog/unreleased/prevent-recursive-copy-move.md @@ -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 diff --git a/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go b/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go index ab3b30d9dd..2ff40af8ef 100644 --- a/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go +++ b/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go @@ -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" @@ -280,6 +281,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 + } + return nil, gstatus.Errorf(codes.Unimplemented, "method not implemented") } @@ -670,6 +679,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 @@ -698,7 +711,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, }, }, nil } diff --git a/internal/http/services/owncloud/ocdav/copy.go b/internal/http/services/owncloud/ocdav/copy.go index d38d3f0d64..335617b5bc 100644 --- a/internal/http/services/owncloud/ocdav/copy.go +++ b/internal/http/services/owncloud/ocdav/copy.go @@ -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" @@ -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", "") + errors.HandleWebdavError(log, w, b, err) + return nil + } + oh := r.Header.Get(net.HeaderOverwrite) overwrite, err := net.ParseOverwrite(oh) if err != nil { @@ -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 { diff --git a/internal/http/services/owncloud/ocdav/move.go b/internal/http/services/owncloud/ocdav/move.go index 562df4b254..85a7ed9b5b 100644 --- a/internal/http/services/owncloud/ocdav/move.go +++ b/internal/http/services/owncloud/ocdav/move.go @@ -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" @@ -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 } diff --git a/internal/http/services/owncloud/ocdav/ocdav.go b/internal/http/services/owncloud/ocdav/ocdav.go index 1de9831c88..f107647ee8 100644 --- a/internal/http/services/owncloud/ocdav/ocdav.go +++ b/internal/http/services/owncloud/ocdav/ocdav.go @@ -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" @@ -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. @@ -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() { @@ -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) + 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 +} diff --git a/pkg/micro/ocdav/option.go b/pkg/micro/ocdav/option.go index 386542e639..2c94830440 100644 --- a/pkg/micro/ocdav/option.go +++ b/pkg/micro/ocdav/option.go @@ -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) { diff --git a/tests/oc-integration-tests/drone/frontend-global.toml b/tests/oc-integration-tests/drone/frontend-global.toml index 40ba34506d..1d29cdf233 100644 --- a/tests/oc-integration-tests/drone/frontend-global.toml +++ b/tests/oc-integration-tests/drone/frontend-global.toml @@ -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] diff --git a/tests/oc-integration-tests/drone/frontend.toml b/tests/oc-integration-tests/drone/frontend.toml index 35e5d2f92a..3f4eac85dd 100644 --- a/tests/oc-integration-tests/drone/frontend.toml +++ b/tests/oc-integration-tests/drone/frontend.toml @@ -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" diff --git a/tests/oc-integration-tests/local-mesh/frontend-global.toml b/tests/oc-integration-tests/local-mesh/frontend-global.toml index 255acfeea6..c9ef145466 100644 --- a/tests/oc-integration-tests/local-mesh/frontend-global.toml +++ b/tests/oc-integration-tests/local-mesh/frontend-global.toml @@ -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] diff --git a/tests/oc-integration-tests/local-mesh/frontend.toml b/tests/oc-integration-tests/local-mesh/frontend.toml index 2e6f5fdd9a..0e23a75375 100644 --- a/tests/oc-integration-tests/local-mesh/frontend.toml +++ b/tests/oc-integration-tests/local-mesh/frontend.toml @@ -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] diff --git a/tests/oc-integration-tests/local/combined.toml b/tests/oc-integration-tests/local/combined.toml index 1eab1b89e1..bcc158b1a1 100644 --- a/tests/oc-integration-tests/local/combined.toml +++ b/tests/oc-integration-tests/local/combined.toml @@ -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" diff --git a/tests/oc-integration-tests/local/frontend-global.toml b/tests/oc-integration-tests/local/frontend-global.toml index 7c62831589..548b6f1d4c 100644 --- a/tests/oc-integration-tests/local/frontend-global.toml +++ b/tests/oc-integration-tests/local/frontend-global.toml @@ -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] diff --git a/tests/oc-integration-tests/local/frontend.toml b/tests/oc-integration-tests/local/frontend.toml index f4b0f1ec32..8bdfa766ca 100644 --- a/tests/oc-integration-tests/local/frontend.toml +++ b/tests/oc-integration-tests/local/frontend.toml @@ -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]