validate alternate object directory is within storage

Validates that the alternate object directory is within the storage
root of the repository.

6 files changed, 89 insertions, 17 deletions

diff --git a/internal/git/bitmap.go b/internal/git/bitmap.go
index 726f6f500..01f55b059 100644
--- a/internal/git/bitmap.go
+++ b/internal/git/bitmap.go
@@ -10,6 +10,8 @@ import (
 	grpc_ctxtags "github.com/grpc-ecosystem/go-grpc-middleware/tags"
 	"github.com/prometheus/client_golang/prometheus"
 	"gitlab.com/gitlab-org/gitaly/internal/git/packfile"
+	"gitlab.com/gitlab-org/gitaly/internal/git/repository"
+	"gitlab.com/gitlab-org/gitaly/internal/helper"
 )
 
 var badBitmapRequestCount = prometheus.NewCounterVec(
@@ -25,10 +27,22 @@ func init() { prometheus.MustRegister(badBitmapRequestCount) }
 // WarnIfTooManyBitmaps checks for too many (more than one) bitmaps in
 // repoPath, and if it finds any, it logs a warning. This is to help us
 // investigate https://gitlab.com/gitlab-org/gitaly/issues/1728.
-func WarnIfTooManyBitmaps(ctx context.Context, repoPath string) {
+func WarnIfTooManyBitmaps(ctx context.Context, repo repository.GitRepo) {
 	logEntry := grpc_logrus.Extract(ctx)
 
-	objdirs, err := ObjectDirectories(ctx, repoPath)
+	storageRoot, err := helper.GetStorageByName(repo.GetStorageName())
+	if err != nil {
+		logEntry.WithError(err).Info("bitmap check failed")
+		return
+	}
+
+	repoPath, err := helper.GetRepoPath(repo)
+	if err != nil {
+		logEntry.WithError(err).Info("bitmap check failed")
+		return
+	}
+
+	objdirs, err := ObjectDirectories(ctx, storageRoot, repoPath)
 	if err != nil {
 		logEntry.WithError(err).Info("bitmap check failed")
 		return
diff --git a/internal/git/dirs.go b/internal/git/dirs.go
index 102022dcf..9174fffcd 100644
--- a/internal/git/dirs.go
+++ b/internal/git/dirs.go
@@ -2,6 +2,7 @@ package git
 
 import (
 	"context"
+	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -10,22 +11,25 @@ import (
 	grpc_logrus "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus"
 )
 
+// alternateOutsideStorageError is returned when an alternates file contains an
+// alternate which is not within the storage's root.
+type alternateOutsideStorageError string
+
+func (path alternateOutsideStorageError) Error() string {
+	return fmt.Sprintf("alternate %q is outside the storage root", string(path))
+}
+
 // ObjectDirectories looks for Git object directories, including
 // alternates specified in objects/info/alternates.
 //
 // CAVEAT Git supports quoted strings in here, but we do not. We should
 // never need those on a Gitaly server.
-func ObjectDirectories(ctx context.Context, repoPath string) ([]string, error) {
+func ObjectDirectories(ctx context.Context, storageRoot, repoPath string) ([]string, error) {
 	objDir := filepath.Join(repoPath, "objects")
-	dirs, err := altObjectDirs(ctx, objDir, 0)
-	if err != nil {
-		return nil, err
-	}
-
-	return dirs, nil
+	return altObjectDirs(ctx, storageRoot+string(os.PathSeparator), objDir, 0)
 }
 
-func altObjectDirs(ctx context.Context, objDir string, depth int) ([]string, error) {
+func altObjectDirs(ctx context.Context, storagePrefix, objDir string, depth int) ([]string, error) {
 	logEntry := grpc_logrus.Extract(ctx)
 
 	const maxAlternatesDepth = 5 // Taken from https://github.com/git/git/blob/v2.23.0/sha1-file.c#L575
@@ -65,7 +69,11 @@ func altObjectDirs(ctx context.Context, objDir string, depth int) ([]string, err
 			newDir = filepath.Join(objDir, newDir)
 		}
 
-		nestedDirs, err := altObjectDirs(ctx, newDir, depth+1)
+		if !strings.HasPrefix(newDir, storagePrefix) {
+			return nil, alternateOutsideStorageError(newDir)
+		}
+
+		nestedDirs, err := altObjectDirs(ctx, storagePrefix, newDir, depth+1)
 		if err != nil {
 			return nil, err
 		}
diff --git a/internal/git/dirs_test.go b/internal/git/dirs_test.go
index 398e2db2f..8d0a32e4a 100644
--- a/internal/git/dirs_test.go
+++ b/internal/git/dirs_test.go
@@ -1,6 +1,9 @@
 package git
 
 import (
+	"io/ioutil"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -11,8 +14,7 @@ func TestObjectDirs(t *testing.T) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
-	expected := []string{
-		"testdata/objdirs/repo0/objects",
+	altObjDirs := []string{
 		"testdata/objdirs/repo1/objects",
 		"testdata/objdirs/repo2/objects",
 		"testdata/objdirs/repo3/objects",
@@ -21,8 +23,56 @@ func TestObjectDirs(t *testing.T) {
 		"testdata/objdirs/repoB/objects",
 	}
 
-	out, err := ObjectDirectories(ctx, "testdata/objdirs/repo0")
+	repo := "testdata/objdirs/repo0"
+	objDirs := append([]string{filepath.Join(repo, "objects")}, altObjDirs...)
+
+	out, err := ObjectDirectories(ctx, "testdata/objdirs", repo)
+	require.NoError(t, err)
+	require.Equal(t, objDirs, out)
+}
+
+func TestObjectDirsNoAlternates(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	repo := "testdata/objdirs/no-alternates"
+	out, err := ObjectDirectories(ctx, "testdata/objdirs", repo)
 	require.NoError(t, err)
+	require.Equal(t, []string{filepath.Join(repo, "objects")}, out)
+}
+
+func TestObjectDirsOutsideStorage(t *testing.T) {
+	tmp, clean := testhelper.TempDir(t, "")
+	defer clean()
 
-	require.Equal(t, expected, out)
+	storageRoot := filepath.Join(tmp, "storage-root")
+	repoPath := filepath.Join(storageRoot, "repo")
+	alternatesFile := filepath.Join(repoPath, "objects", "info", "alternates")
+	altObjDir := filepath.Join(tmp, "outside-storage", "objects")
+	require.NoError(t, os.MkdirAll(filepath.Dir(alternatesFile), 0700))
+	expectedErr := alternateOutsideStorageError(altObjDir)
+
+	for _, tc := range []struct {
+		desc       string
+		alternates string
+	}{
+		{
+			desc:       "relative path",
+			alternates: "../../../outside-storage/objects",
+		},
+		{
+			desc:       "absolute path",
+			alternates: altObjDir,
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			ctx, cancel := testhelper.Context()
+			defer cancel()
+
+			require.NoError(t, ioutil.WriteFile(alternatesFile, []byte(tc.alternates), 0600))
+			out, err := ObjectDirectories(ctx, storageRoot, repoPath)
+			require.Equal(t, expectedErr, err)
+			require.Nil(t, out)
+		})
+	}
 }
diff --git a/internal/git/testdata/objdirs/no-alternates/objects/info/alternates b/internal/git/testdata/objdirs/no-alternates/objects/info/alternates
new file mode 100644
index 000000000..e69de29bb
--- /dev/null
+++ b/internal/git/testdata/objdirs/no-alternates/objects/info/alternates
diff --git a/internal/service/smarthttp/upload_pack.go b/internal/service/smarthttp/upload_pack.go
index db18176e4..174bcae0b 100644
--- a/internal/service/smarthttp/upload_pack.go
+++ b/internal/service/smarthttp/upload_pack.go
@@ -83,7 +83,7 @@ func (s *server) PostUploadPack(stream gitalypb.SmartHTTPService_PostUploadPackS
 		return err
 	}
 
-	git.WarnIfTooManyBitmaps(ctx, repoPath)
+	git.WarnIfTooManyBitmaps(ctx, req.GetRepository())
 
 	var globalOpts []git.Option
 	if featureflag.IsEnabled(ctx, featureflag.UploadPackFilter) {
diff --git a/internal/service/ssh/upload_pack.go b/internal/service/ssh/upload_pack.go
index 854eff492..462158cd2 100644
--- a/internal/service/ssh/upload_pack.go
+++ b/internal/service/ssh/upload_pack.go
@@ -75,7 +75,7 @@ func (s *server) sshUploadPack(stream gitalypb.SSHService_SSHUploadPackServer, r
 		return err
 	}
 
-	git.WarnIfTooManyBitmaps(ctx, repoPath)
+	git.WarnIfTooManyBitmaps(ctx, req.GetRepository())
 
 	var globalOpts []git.Option
 	if featureflag.IsEnabled(ctx, featureflag.UploadPackFilter) {