Prevent recursive copy/move operations on the API level (cs3org#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
kobergj · Jul 11, 2022 · f2783f2 · f2783f2
1 parent b4fd8cd
commit f2783f2
Show file tree

Hide file tree

Showing 13 changed files with 194 additions and 14 deletions.
diff --git a/changelog/unreleased/prevent-recursive-copy-move.md 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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]