Guard against path traversal in AddNamespace RPCjc-guard-path-traversal

3 files changed, 110 insertions, 9 deletions

diff --git a/changelogs/unreleased/jc-add-mkdirp-to-rename-namespace.yml b/changelogs/unreleased/jc-add-mkdirp-to-rename-namespace.yml
new file mode 100644
index 000000000..8c63f0615
--- /dev/null
+++ b/changelogs/unreleased/jc-add-mkdirp-to-rename-namespace.yml
@@ -0,0 +1,5 @@
+---
+title: Create parent directory in RenameNamespace
+merge_request: 1452
+author:
+type: other
diff --git a/internal/service/namespace/namespace.go b/internal/service/namespace/namespace.go
index 005c03655..909d310f5 100644
--- a/internal/service/namespace/namespace.go
+++ b/internal/service/namespace/namespace.go
@@ -2,8 +2,11 @@ package namespace
 
 import (
 	"context"
+	"errors"
+	"fmt"
 	"os"
 	"path"
+	"path/filepath"
 
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
@@ -52,26 +55,49 @@ func (s *server) AddNamespace(ctx context.Context, in *gitalypb.AddNamespaceRequ
 	return &gitalypb.AddNamespaceResponse{}, nil
 }
 
-func (s *server) RenameNamespace(ctx context.Context, in *gitalypb.RenameNamespaceRequest) (*gitalypb.RenameNamespaceResponse, error) {
-	storagePath, err := helper.GetStorageByName(in.GetStorageName())
-	if err != nil {
-		return nil, err
+func (s *server) validateRenameNamespaceRequest(ctx context.Context, in *gitalypb.RenameNamespaceRequest) error {
+	if in.GetFrom() == "" || in.GetTo() == "" {
+		return errors.New("from and to cannot be empty")
 	}
 
-	if in.GetFrom() == "" || in.GetTo() == "" {
-		return nil, status.Errorf(codes.InvalidArgument, "from and to cannot be empty")
+	if helper.ContainsPathTraversal(in.To) {
+		return errors.New("destination cannot contain path traversal")
+	}
+
+	if helper.ContainsPathTraversal(in.From) {
+		return errors.New("source cannot contain path traversal")
 	}
 
 	// No need to check if the from path exists, if it doesn't, we'd later get an
 	// os.LinkError
 	toExistsCheck := &gitalypb.NamespaceExistsRequest{StorageName: in.StorageName, Name: in.GetTo()}
 	if exists, err := s.NamespaceExists(ctx, toExistsCheck); err != nil {
-		return nil, err
+		return err
 	} else if exists.Exists {
-		return nil, status.Errorf(codes.InvalidArgument, "to directory %s already exists", in.GetTo())
+		return fmt.Errorf("to directory %s already exists", in.GetTo())
+	}
+
+	return nil
+}
+
+func (s *server) RenameNamespace(ctx context.Context, in *gitalypb.RenameNamespaceRequest) (*gitalypb.RenameNamespaceResponse, error) {
+	if err := s.validateRenameNamespaceRequest(ctx, in); err != nil {
+		return nil, helper.ErrInvalidArgument(err)
+	}
+
+	storagePath, err := helper.GetStorageByName(in.GetStorageName())
+	if err != nil {
+		return nil, err
+	}
+
+	targetPath := namespacePath(storagePath, in.GetTo())
+
+	// Create the parent directory.
+	if err = os.MkdirAll(filepath.Dir(targetPath), 0775); err != nil {
+		return nil, helper.ErrInternalf("create directory: %v", err)
 	}
 
-	err = os.Rename(namespacePath(storagePath, in.GetFrom()), namespacePath(storagePath, in.GetTo()))
+	err = os.Rename(namespacePath(storagePath, in.GetFrom()), targetPath)
 	if _, ok := err.(*os.LinkError); ok {
 		return nil, status.Errorf(codes.InvalidArgument, "from directory %s not found", in.GetFrom())
 	} else if err != nil {
diff --git a/internal/service/namespace/namespace_test.go b/internal/service/namespace/namespace_test.go
index f21dc0174..6ae48b298 100644
--- a/internal/service/namespace/namespace_test.go
+++ b/internal/service/namespace/namespace_test.go
@@ -297,3 +297,73 @@ func TestRenameNamespace(t *testing.T) {
 		})
 	}
 }
+
+func TestRenameNamespaceWithNonexistentParentDir(t *testing.T) {
+	server, serverSocketPath := runNamespaceServer(t)
+	defer server.Stop()
+
+	client, conn := newNamespaceClient(t, serverSocketPath)
+	defer conn.Close()
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	_, err := client.AddNamespace(ctx, &gitalypb.AddNamespaceRequest{
+		StorageName: "default",
+		Name:        "existing",
+	})
+	require.NoError(t, err)
+
+	testCases := []struct {
+		desc      string
+		request   *gitalypb.RenameNamespaceRequest
+		errorCode codes.Code
+	}{
+		{
+			desc: "existing source, non existing target directory",
+			request: &gitalypb.RenameNamespaceRequest{
+				From:        "existing",
+				To:          "some/other/new-path",
+				StorageName: "default",
+			},
+			errorCode: codes.OK,
+		},
+		{
+			desc: "path traversal in source",
+			request: &gitalypb.RenameNamespaceRequest{
+				From:        "../some/traversed/path",
+				To:          "some/other/new-path",
+				StorageName: "default",
+			},
+			errorCode: codes.InvalidArgument,
+		},
+		{
+			desc: "path traversal in destination",
+			request: &gitalypb.RenameNamespaceRequest{
+				From:        "existing",
+				To:          "../some/traversed/path",
+				StorageName: "default",
+			},
+			errorCode: codes.InvalidArgument,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+
+			_, err = client.RenameNamespace(ctx, &gitalypb.RenameNamespaceRequest{
+				From:        "existing",
+				To:          "some/other/new-path",
+				StorageName: "default"})
+			require.Equal(t, tc.errorCode, helper.GrpcCode(err))
+
+			if tc.errorCode == codes.OK {
+				storagePath, err := helper.GetStorageByName(tc.request.StorageName)
+				require.NoError(t, err)
+
+				path := namespacePath(storagePath, tc.request.GetTo())
+				require.NoError(t, os.RemoveAll(path))
+			}
+		})
+	}
+}