Merge branch '2148-object-pool-path-validation' into 'master'

Validate object pool relative paths

Closes #2418

See merge request gitlab-org/gitaly!1900

8 files changed, 178 insertions, 59 deletions

diff --git a/changelogs/unreleased/smh-prevent-path-deletion.yml b/changelogs/unreleased/smh-prevent-path-deletion.yml
new file mode 100644
index 000000000..7ae727057
--- /dev/null
+++ b/changelogs/unreleased/smh-prevent-path-deletion.yml
@@ -0,0 +1,5 @@
+---
+title: Validate object pool relative paths to prevent path traversal attacks.
+merge_request: 1900
+author:
+type: security
diff --git a/internal/git/objectpool/clone.go b/internal/git/objectpool/clone.go
index 45ab2af81..ba0febd58 100644
--- a/internal/git/objectpool/clone.go
+++ b/internal/git/objectpool/clone.go
@@ -18,17 +18,11 @@ func (o *ObjectPool) clone(ctx context.Context, repo *gitalypb.Repository) error
 		return err
 	}
 
-	targetDir := o.FullPath()
-	targetName := path.Base(targetDir)
-	if err != nil {
-		return err
-	}
-
 	cmd, err := git.SafeCmdWithoutRepo(ctx,
 		[]git.Option{
 			git.ValueFlag{
 				Name:  "-C",
-				Value: path.Dir(targetDir),
+				Value: o.storagePath,
 			},
 		},
 		git.SubCmd{
@@ -38,7 +32,7 @@ func (o *ObjectPool) clone(ctx context.Context, repo *gitalypb.Repository) error
 				git.Flag{Name: "--bare"},
 				git.Flag{Name: "--local"},
 			},
-			Args: []string{repoPath, targetName},
+			Args: []string{repoPath, o.relativePath},
 		},
 	)
 	if err != nil {
diff --git a/internal/git/objectpool/pool.go b/internal/git/objectpool/pool.go
index ca4628d1b..1278b02c2 100644
--- a/internal/git/objectpool/pool.go
+++ b/internal/git/objectpool/pool.go
@@ -8,14 +8,25 @@ import (
 	"fmt"
 	"io"
 	"os"
-	"path"
 	"path/filepath"
+	"regexp"
+	"strings"
 
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 )
 
+// poolDirRegexp is used to validate object pool directory structure and name.
+var poolDirRegexp = regexp.MustCompile(`^@pools/([0-9a-f]{2})/([0-9a-f]{2})/([0-9a-f]{64})\.git$`)
+
+type errString string
+
+func (err errString) Error() string { return string(err) }
+
+// ErrInvalidPoolDir is returned when the object pool relative path is malformed.
+const ErrInvalidPoolDir errString = "invalid object pool directory"
+
 // ObjectPool are a way to dedup objects between repositories, where the objects
 // live in a pool in a distinct repository which is used as an alternate object
 // store for other repositories.
@@ -27,13 +38,19 @@ type ObjectPool struct {
 }
 
 // NewObjectPool will initialize the object with the required data on the storage
-// shard. If the shard cannot be found, this function returns an error
+// shard. Relative path is validated to match the expected naming and directory
+// structure. If the shard cannot be found, this function returns an error.
 func NewObjectPool(storageName, relativePath string) (pool *ObjectPool, err error) {
 	storagePath, err := helper.GetStorageByName(storageName)
 	if err != nil {
 		return nil, err
 	}
 
+	matches := poolDirRegexp.FindStringSubmatch(relativePath)
+	if matches == nil || !strings.HasPrefix(matches[3], matches[1]+matches[2]) {
+		return nil, ErrInvalidPoolDir
+	}
+
 	return &ObjectPool{storageName, storagePath, relativePath}, nil
 }
 
@@ -72,10 +89,6 @@ func (o *ObjectPool) IsValid() bool {
 // Create will create a pool for a repository and pull the required data to this
 // pool. `repo` that is passed also joins the repository.
 func (o *ObjectPool) Create(ctx context.Context, repo *gitalypb.Repository) (err error) {
-	if err := os.MkdirAll(path.Dir(o.FullPath()), 0755); err != nil {
-		return err
-	}
-
 	if err := o.clone(ctx, repo); err != nil {
 		return fmt.Errorf("clone: %v", err)
 	}
diff --git a/internal/service/objectpool/create.go b/internal/service/objectpool/create.go
index 4affaa1c5..e0c377403 100644
--- a/internal/service/objectpool/create.go
+++ b/internal/service/objectpool/create.go
@@ -4,11 +4,14 @@ import (
 	"context"
 
 	"gitlab.com/gitlab-org/gitaly/internal/git/objectpool"
+	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 )
 
+var errInvalidPoolDir = helper.ErrInvalidArgument(objectpool.ErrInvalidPoolDir)
+
 func (s *server) CreateObjectPool(ctx context.Context, in *gitalypb.CreateObjectPoolRequest) (*gitalypb.CreateObjectPoolResponse, error) {
 	if in.GetOrigin() == nil {
 		return nil, status.Errorf(codes.InvalidArgument, "no origin repository")
@@ -55,5 +58,14 @@ func poolForRequest(req poolRequest) (*objectpool.ObjectPool, error) {
 		return nil, status.Errorf(codes.InvalidArgument, "no object pool repository")
 	}
 
-	return objectpool.NewObjectPool(poolRepo.GetStorageName(), poolRepo.GetRelativePath())
+	pool, err := objectpool.NewObjectPool(poolRepo.GetStorageName(), poolRepo.GetRelativePath())
+	if err != nil {
+		if err == objectpool.ErrInvalidPoolDir {
+			return nil, errInvalidPoolDir
+		}
+
+		return nil, helper.ErrInternal(err)
+	}
+
+	return pool, nil
 }
diff --git a/internal/service/objectpool/create_test.go b/internal/service/objectpool/create_test.go
index 8b6946fc9..f7a0e971c 100644
--- a/internal/service/objectpool/create_test.go
+++ b/internal/service/objectpool/create_test.go
@@ -3,6 +3,8 @@ package objectpool
 import (
 	"os"
 	"path"
+	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -73,7 +75,8 @@ func TestUnsuccessfulCreate(t *testing.T) {
 	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
-	pool, err := objectpool.NewObjectPool("default", testhelper.NewTestObjectPoolName(t))
+	validPoolPath := testhelper.NewTestObjectPoolName(t)
+	pool, err := objectpool.NewObjectPool("default", validPoolPath)
 	require.NoError(t, err)
 	defer pool.Remove(ctx)
 
@@ -96,6 +99,45 @@ func TestUnsuccessfulCreate(t *testing.T) {
 			},
 			code: codes.InvalidArgument,
 		},
+		{
+			desc: "outside pools directory",
+			request: &gitalypb.CreateObjectPoolRequest{
+				Origin: testRepo,
+				ObjectPool: &gitalypb.ObjectPool{
+					Repository: &gitalypb.Repository{
+						StorageName:  "default",
+						RelativePath: "outside-pools",
+					},
+				},
+			},
+			code: codes.InvalidArgument,
+		},
+		{
+			desc: "path must be lowercase",
+			request: &gitalypb.CreateObjectPoolRequest{
+				Origin: testRepo,
+				ObjectPool: &gitalypb.ObjectPool{
+					Repository: &gitalypb.Repository{
+						StorageName:  "default",
+						RelativePath: strings.ToUpper(validPoolPath),
+					},
+				},
+			},
+			code: codes.InvalidArgument,
+		},
+		{
+			desc: "subdirectories must match first four pool digits",
+			request: &gitalypb.CreateObjectPoolRequest{
+				Origin: testRepo,
+				ObjectPool: &gitalypb.ObjectPool{
+					Repository: &gitalypb.Repository{
+						StorageName:  "default",
+						RelativePath: "@pools/aa/bb/ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff.git",
+					},
+				},
+			},
+			code: codes.InvalidArgument,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -106,7 +148,7 @@ func TestUnsuccessfulCreate(t *testing.T) {
 	}
 }
 
-func TestRemove(t *testing.T) {
+func TestDelete(t *testing.T) {
 	server, serverSocketPath := runObjectPoolServer(t)
 	defer server.Stop()
 
@@ -119,18 +161,63 @@ func TestRemove(t *testing.T) {
 	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
-	pool, err := objectpool.NewObjectPool("default", testhelper.NewTestObjectPoolName(t))
+	validPoolPath := testhelper.NewTestObjectPoolName(t)
+	pool, err := objectpool.NewObjectPool("default", validPoolPath)
 	require.NoError(t, err)
 	require.NoError(t, pool.Create(ctx, testRepo))
 
-	req := &gitalypb.DeleteObjectPoolRequest{
-		ObjectPool: pool.ToProto(),
+	for _, tc := range []struct {
+		desc         string
+		relativePath string
+		error        error
+	}{
+		{
+			desc:         "deleting outside pools directory fails",
+			relativePath: ".",
+			error:        errInvalidPoolDir,
+		},
+		{
+			desc:         "deleting pools directory fails",
+			relativePath: "@pools",
+			error:        errInvalidPoolDir,
+		},
+		{
+			desc:         "deleting first level subdirectory fails",
+			relativePath: "@pools/ab",
+			error:        errInvalidPoolDir,
+		},
+		{
+			desc:         "deleting second level subdirectory fails",
+			relativePath: "@pools/ab/cd",
+			error:        errInvalidPoolDir,
+		},
+		{
+			desc:         "deleting pool subdirectory fails",
+			relativePath: filepath.Join(validPoolPath, "objects"),
+			error:        errInvalidPoolDir,
+		},
+		{
+			desc:         "path traversing fails",
+			relativePath: validPoolPath + "/../../../../..",
+			error:        errInvalidPoolDir,
+		},
+		{
+			desc:         "deleting pool succeeds",
+			relativePath: validPoolPath,
+		},
+		{
+			desc:         "deleting non-existent pool succeeds",
+			relativePath: validPoolPath,
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			_, err := client.DeleteObjectPool(ctx, &gitalypb.DeleteObjectPoolRequest{ObjectPool: &gitalypb.ObjectPool{
+				Repository: &gitalypb.Repository{
+					StorageName:  "default",
+					RelativePath: tc.relativePath,
+				},
+			}})
+			require.Equal(t, tc.error, err)
+		})
 	}
-
-	_, err = client.DeleteObjectPool(ctx, req)
-	require.NoError(t, err)
-
-	// Removing again should be possible
-	_, err = client.DeleteObjectPool(ctx, req)
-	require.NoError(t, err)
 }
diff --git a/internal/service/objectpool/link_test.go b/internal/service/objectpool/link_test.go
index 9265eaf08..3eafd62dd 100644
--- a/internal/service/objectpool/link_test.go
+++ b/internal/service/objectpool/link_test.go
@@ -276,7 +276,7 @@ func TestUnlink(t *testing.T) {
 				ObjectPool: &gitalypb.ObjectPool{
 					Repository: &gitalypb.Repository{
 						StorageName:  testRepo.GetStorageName(),
-						RelativePath: "pool/does/not/exist",
+						RelativePath: testhelper.NewTestObjectPoolName(t), // does not exist
 					},
 				},
 			},
diff --git a/internal/service/repository/clone_from_pool_internal_test.go b/internal/service/repository/clone_from_pool_internal_test.go
index b31b8c428..c32453ee9 100644
--- a/internal/service/repository/clone_from_pool_internal_test.go
+++ b/internal/service/repository/clone_from_pool_internal_test.go
@@ -17,7 +17,9 @@ import (
 )
 
 func NewTestObjectPool(t *testing.T) (*objectpool.ObjectPool, *gitalypb.Repository) {
-	repo, _, relativePath := testhelper.CreateRepo(t, testhelper.GitlabTestStoragePath())
+	storagePath := testhelper.GitlabTestStoragePath()
+	relativePath := testhelper.NewTestObjectPoolName(t)
+	repo := testhelper.CreateRepo(t, storagePath, relativePath)
 
 	pool, err := objectpool.NewObjectPool(repo.GetStorageName(), relativePath)
 	require.NoError(t, err)
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index 5d7d0647a..72fff1ddb 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -3,9 +3,9 @@ package testhelper
 import (
 	"bytes"
 	"context"
+	"crypto/sha256"
 	"encoding/base64"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -386,30 +386,14 @@ func Context() (context.Context, func()) {
 	return context.WithCancel(context.Background())
 }
 
-func hashedRepoPath(dirName string) (string, error) {
-	if len(dirName) < 4 {
-		return "", errors.New("dirname is too short")
-	}
-
-	return filepath.Join(dirName[0:2], dirName[2:4], dirName[4:]), nil
-}
-
 // CreateRepo creates a temporary directory for a repo, without initializing it
-func CreateRepo(t testing.TB, storagePath string) (repo *gitalypb.Repository, repoPath, relativePath string) {
-	random, err := text.RandomHex(20)
-	require.NoError(t, err)
-
-	hashedPath, err := hashedRepoPath(random)
-	require.NoError(t, err)
-
-	repoPath = filepath.Join(storagePath, hashedPath)
-	require.NoError(t, os.MkdirAll(filepath.Dir(repoPath), 0755), "making repo parent dir")
-
-	relativePath, err = filepath.Rel(storagePath, repoPath)
-	require.NoError(t, err)
-	repo = &gitalypb.Repository{StorageName: "default", RelativePath: relativePath, GlRepository: "project-1"}
-
-	return repo, repoPath, relativePath
+func CreateRepo(t testing.TB, storagePath, relativePath string) *gitalypb.Repository {
+	require.NoError(t, os.MkdirAll(filepath.Dir(storagePath), 0755), "making repo parent dir")
+	return &gitalypb.Repository{
+		StorageName:  "default",
+		RelativePath: relativePath,
+		GlRepository: "project-1",
+	}
 }
 
 // InitBareRepo creates a new bare repository
@@ -423,7 +407,10 @@ func InitRepoWithWorktree(t *testing.T) (*gitalypb.Repository, string, func()) {
 }
 
 func initRepo(t *testing.T, bare bool) (*gitalypb.Repository, string, func()) {
-	repo, repoPath, _ := CreateRepo(t, GitlabTestStoragePath())
+	storagePath := GitlabTestStoragePath()
+	relativePath := NewRepositoryName(t)
+	repoPath := filepath.Join(storagePath, relativePath)
+
 	args := []string{"init"}
 	if bare {
 		args = append(args, "--bare")
@@ -431,6 +418,7 @@ func initRepo(t *testing.T, bare bool) (*gitalypb.Repository, string, func()) {
 
 	MustRunCommand(t, nil, "git", append(args, repoPath)...)
 
+	repo := CreateRepo(t, storagePath, relativePath)
 	if !bare {
 		repo.RelativePath = path.Join(repo.RelativePath, ".git")
 	}
@@ -451,7 +439,10 @@ func NewTestRepoWithWorktree(t testing.TB) (repo *gitalypb.Repository, repoPath
 
 func cloneTestRepo(t testing.TB, bare bool) (repo *gitalypb.Repository, repoPath string, cleanup func()) {
 	storagePath := GitlabTestStoragePath()
-	repo, repoPath, relativePath := CreateRepo(t, storagePath)
+	relativePath := NewRepositoryName(t)
+	repoPath = filepath.Join(storagePath, relativePath)
+
+	repo = CreateRepo(t, storagePath, relativePath)
 	testRepo := TestRepository()
 	testRepoPath := path.Join(storagePath, testRepo.RelativePath)
 	args := []string{"clone", "--no-hardlinks", "--dissociate"}
@@ -510,12 +501,27 @@ func AssertPathNotExists(t *testing.T, path string) {
 	assert.True(t, os.IsNotExist(err), "file should not exist: %s", path)
 }
 
-// NewTestObjectPoolName returns a random pool repository name.
-func NewTestObjectPoolName(t *testing.T) string {
-	b, err := text.RandomHex(5)
+// newDiskHash generates a random directory path following the Rails app's
+// approach in the hashed storage module, formatted as '[0-9a-f]{2}/[0-9a-f]{2}/[0-9a-f]{64}'.
+// https://gitlab.com/gitlab-org/gitlab/-/blob/f5c7d8eb1dd4eee5106123e04dec26d277ff6a83/app/models/storage/hashed.rb#L38-43
+func newDiskHash(t testing.TB) string {
+	// rails app calculates a sha256 and uses its hex representation
+	// as the directory path
+	b, err := text.RandomHex(sha256.Size)
 	require.NoError(t, err)
+	return filepath.Join(b[0:2], b[2:4], fmt.Sprintf("%s.git", b))
+}
+
+// NewRepositoryName returns a random repository hash
+// in format '@hashed/[0-9a-f]{2}/[0-9a-f]{2}/[0-9a-f]{64}.git'.
+func NewRepositoryName(t testing.TB) string {
+	return filepath.Join("@hashed", newDiskHash(t))
+}
 
-	return fmt.Sprintf("pool-%s.git", b)
+// NewTestObjectPoolName returns a random pool repository name
+// in format '@pools/[0-9a-z]{2}/[0-9a-z]{2}/[0-9a-z]{64}.git'.
+func NewTestObjectPoolName(t testing.TB) string {
+	return filepath.Join("@pools", newDiskHash(t))
 }
 
 // CreateLooseRef creates a ref that points to master