Merge branch '2421-alternates-file-inclusion' into 'master'

Validate content of alternates file

Closes #2421

See merge request gitlab-org/gitaly!1946

9 files changed, 211 insertions, 88 deletions

diff --git a/changelogs/unreleased/smh-validate-alternate-files.yml b/changelogs/unreleased/smh-validate-alternate-files.yml
new file mode 100644
index 000000000..02c0a5fbf
--- /dev/null
+++ b/changelogs/unreleased/smh-validate-alternate-files.yml
@@ -0,0 +1,5 @@
+---
+title: Validate content of alternates file
+merge_request: 1946
+author:
+type: security
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..e619b6ccf 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,24 +11,40 @@ 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)
+	return altObjectDirs(ctx, storageRoot+string(os.PathSeparator), objDir, 0)
+}
+
+// AlternateObjectDirectories reads the alternates file of the repository and returns absolute paths
+// to its alternate object directories, if any. The returned directories are verified to exist and that
+// they are within the storage root. The alternate directories are returned recursively, not only the
+// immediate alternates.
+func AlternateObjectDirectories(ctx context.Context, storageRoot, repoPath string) ([]string, error) {
+	dirs, err := ObjectDirectories(ctx, storageRoot, repoPath)
 	if err != nil {
 		return nil, err
 	}
 
-	return dirs, nil
+	// first directory is the repository's own object dir
+	return dirs[1:], nil
 }
 
-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
 	if depth > maxAlternatesDepth {
 		logEntry.WithField("objdir", objDir).Warn("ignoring deeply nested alternate object directory")
@@ -65,7 +82,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..ae73b912a 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,64 @@ 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)
+
+	out, err = AlternateObjectDirectories(ctx, "testdata/objdirs", repo)
+	require.NoError(t, err)
+	require.Equal(t, altObjDirs, 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)
+
+	out, err = AlternateObjectDirectories(ctx, "testdata/objdirs", repo)
 	require.NoError(t, err)
+	require.Equal(t, []string{}, out)
+}
 
-	require.Equal(t, expected, out)
+func TestObjectDirsOutsideStorage(t *testing.T) {
+	tmp, clean := testhelper.TempDir(t, "")
+	defer clean()
+
+	storageRoot := filepath.Join(tmp, "storage-root")
+	repoPath := filepath.Join(storageRoot, "repo")
+	alternatesFile := filepath.Join(repoPath, "objects", "info", "alternates")
+	altObjDir := filepath.Join(tmp, "outside-storage-sibling", "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-sibling/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/repository/snapshot.go b/internal/service/repository/snapshot.go
index d013a1fac..4cb4d2bea 100644
--- a/internal/service/repository/snapshot.go
+++ b/internal/service/repository/snapshot.go
@@ -1,16 +1,13 @@
 package repository
 
 import (
-	"bufio"
 	"context"
 	"fmt"
-	"io"
 	"os"
 	"path/filepath"
 	"regexp"
 
 	grpc_logrus "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus"
-	log "github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/internal/archive"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
@@ -81,39 +78,28 @@ func (s *server) GetSnapshot(in *gitalypb.GetSnapshotRequest, stream gitalypb.Re
 }
 
 func addAlternateFiles(ctx context.Context, repository *gitalypb.Repository, builder *archive.TarBuilder) error {
-	alternateFilePath, err := git.InfoAlternatesPath(repository)
+	storageRoot, err := helper.GetStorageByName(repository.GetStorageName())
 	if err != nil {
-		return fmt.Errorf("error when getting alternates file path: %v", err)
+		return err
 	}
 
-	if stat, err := os.Stat(alternateFilePath); err == nil && stat.Size() > 0 {
-		alternatesFile, err := os.Open(alternateFilePath)
-		if err != nil {
-			grpc_logrus.Extract(ctx).WithField("error", err).Warn("error opening alternates file")
-			return nil
-		}
-		defer alternatesFile.Close()
-
-		alternateObjDir, err := bufio.NewReader(alternatesFile).ReadString('\n')
-		if err != nil && err != io.EOF {
-			grpc_logrus.Extract(ctx).WithField("error", err).Warn("error reading alternates file")
-			return nil
-		}
-
-		if err == nil {
-			alternateObjDir = alternateObjDir[:len(alternateObjDir)-1]
-		}
+	repoPath, err := helper.GetRepoPath(repository)
+	if err != nil {
+		return err
+	}
 
-		if stat, err := os.Stat(alternateObjDir); err != nil || !stat.IsDir() {
-			grpc_logrus.Extract(ctx).WithFields(
-				log.Fields{"error": err, "object_dir": alternateObjDir}).Warn("error reading alternate objects directory")
-			return nil
-		}
+	altObjDirs, err := git.AlternateObjectDirectories(ctx, storageRoot, repoPath)
+	if err != nil {
+		grpc_logrus.Extract(ctx).WithField("error", err).Warn("error getting alternate object directories")
+		return nil
+	}
 
-		if err := walkAndAddToBuilder(alternateObjDir, builder); err != nil {
+	for _, altObjDir := range altObjDirs {
+		if err := walkAndAddToBuilder(altObjDir, builder); err != nil {
 			return fmt.Errorf("walking alternates file: %v", err)
 		}
 	}
+
 	return nil
 }
 
diff --git a/internal/service/repository/snapshot_test.go b/internal/service/repository/snapshot_test.go
index 6c0d61d1c..c36af632c 100644
--- a/internal/service/repository/snapshot_test.go
+++ b/internal/service/repository/snapshot_test.go
@@ -18,6 +18,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/archive"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/catfile"
+	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/streamio"
@@ -91,52 +92,80 @@ func TestGetSnapshotSuccess(t *testing.T) {
 }
 
 func TestGetSnapshotWithDedupe(t *testing.T) {
-	testRepo, repoPath, cleanup := testhelper.NewTestRepoWithWorktree(t)
-	defer cleanup()
-
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
-	committerName := "Scrooge McDuck"
-	committerEmail := "scrooge@mcduck.com"
-
-	cmd := exec.Command("git", "-C", repoPath,
-		"-c", fmt.Sprintf("user.name=%s", committerName),
-		"-c", fmt.Sprintf("user.email=%s", committerEmail),
-		"commit", "--allow-empty", "-m", "An empty commit")
-	alternateObjDir := "./alt-objects"
-	commitSha := testhelper.CreateCommitInAlternateObjectDirectory(t, repoPath, alternateObjDir, cmd)
-	originalAlternatesCommit := string(commitSha)
-
-	// ensure commit cannot be found in current repository
-	c, err := catfile.New(ctx, testRepo)
-	require.NoError(t, err)
-	_, err = c.Info(originalAlternatesCommit)
-	require.True(t, catfile.IsNotFound(err))
-
-	// write alternates file to point to alt objects folder
-	alternatesPath, err := git.InfoAlternatesPath(testRepo)
-	require.NoError(t, err)
-	require.NoError(t, ioutil.WriteFile(alternatesPath, []byte(path.Join(repoPath, ".git", fmt.Sprintf("%s\n", alternateObjDir))), 0644))
-
-	// write another commit and ensure we can find it
-	cmd = exec.Command("git", "-C", repoPath,
-		"-c", fmt.Sprintf("user.name=%s", committerName),
-		"-c", fmt.Sprintf("user.email=%s", committerEmail),
-		"commit", "--allow-empty", "-m", "Another empty commit")
-	commitSha = testhelper.CreateCommitInAlternateObjectDirectory(t, repoPath, alternateObjDir, cmd)
-
-	c, err = catfile.New(ctx, testRepo)
-	require.NoError(t, err)
-	_, err = c.Info(string(commitSha))
-	require.NoError(t, err)
-
-	_, repoCopyPath, cleanupCopy := copyRepoUsingSnapshot(t, testRepo)
-	defer cleanupCopy()
-
-	// ensure the sha committed to the alternates directory can be accessed
-	testhelper.MustRunCommand(t, nil, "git", "-C", repoCopyPath, "cat-file", "-p", originalAlternatesCommit)
-	testhelper.MustRunCommand(t, nil, "git", "-C", repoCopyPath, "fsck")
+	for _, tc := range []struct {
+		desc              string
+		alternatePathFunc func(t *testing.T, objDir string) string
+	}{
+		{
+			desc:              "subdirectory",
+			alternatePathFunc: func(*testing.T, string) string { return "./alt-objects" },
+		},
+		{
+			desc: "absolute path",
+			alternatePathFunc: func(*testing.T, string) string {
+				return filepath.Join(testhelper.GitlabTestStoragePath(), testhelper.NewTestObjectPoolName(t), "objects")
+			},
+		},
+		{
+			desc: "relative path",
+			alternatePathFunc: func(t *testing.T, objDir string) string {
+				altObjDir, err := filepath.Rel(objDir, filepath.Join(
+					testhelper.GitlabTestStoragePath(), testhelper.NewTestObjectPoolName(t), "objects",
+				))
+				require.NoError(t, err)
+				return altObjDir
+			},
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			testRepo, repoPath, cleanup := testhelper.NewTestRepoWithWorktree(t)
+			defer cleanup()
+
+			ctx, cancel := testhelper.Context()
+			defer cancel()
+
+			const committerName = "Scrooge McDuck"
+			const committerEmail = "scrooge@mcduck.com"
+
+			cmd := exec.Command("git", "-C", repoPath,
+				"-c", fmt.Sprintf("user.name=%s", committerName),
+				"-c", fmt.Sprintf("user.email=%s", committerEmail),
+				"commit", "--allow-empty", "-m", "An empty commit")
+			alternateObjDir := tc.alternatePathFunc(t, filepath.Join(repoPath, "objects"))
+			commitSha := testhelper.CreateCommitInAlternateObjectDirectory(t, repoPath, alternateObjDir, cmd)
+			originalAlternatesCommit := string(commitSha)
+
+			// ensure commit cannot be found in current repository
+			c, err := catfile.New(ctx, testRepo)
+			require.NoError(t, err)
+			_, err = c.Info(originalAlternatesCommit)
+			require.True(t, catfile.IsNotFound(err))
+
+			// write alternates file to point to alt objects folder
+			alternatesPath, err := git.InfoAlternatesPath(testRepo)
+			require.NoError(t, err)
+			require.NoError(t, ioutil.WriteFile(alternatesPath, []byte(path.Join(repoPath, ".git", fmt.Sprintf("%s\n", alternateObjDir))), 0644))
+
+			// write another commit and ensure we can find it
+			cmd = exec.Command("git", "-C", repoPath,
+				"-c", fmt.Sprintf("user.name=%s", committerName),
+				"-c", fmt.Sprintf("user.email=%s", committerEmail),
+				"commit", "--allow-empty", "-m", "Another empty commit")
+			commitSha = testhelper.CreateCommitInAlternateObjectDirectory(t, repoPath, alternateObjDir, cmd)
+
+			c, err = catfile.New(ctx, testRepo)
+			require.NoError(t, err)
+			_, err = c.Info(string(commitSha))
+			require.NoError(t, err)
+
+			_, repoCopyPath, cleanupCopy := copyRepoUsingSnapshot(t, testRepo)
+			defer cleanupCopy()
+
+			// ensure the sha committed to the alternates directory can be accessed
+			testhelper.MustRunCommand(t, nil, "git", "-C", repoCopyPath, "cat-file", "-p", originalAlternatesCommit)
+			testhelper.MustRunCommand(t, nil, "git", "-C", repoCopyPath, "fsck")
+		})
+	}
 }
 
 func TestGetSnapshotWithDedupeSoftFailures(t *testing.T) {
@@ -154,6 +183,16 @@ func TestGetSnapshotWithDedupeSoftFailures(t *testing.T) {
 	_, err = getSnapshot(t, req)
 	assert.NoError(t, err)
 	require.NoError(t, os.Remove(alternatesPath))
+
+	// write alternates file to point outside storage root
+	storageRoot, err := helper.GetStorageByName(testRepo.GetStorageName())
+	require.NoError(t, err)
+	require.NoError(t, ioutil.WriteFile(alternatesPath, []byte(filepath.Join(storageRoot, "..")), 0600))
+
+	_, err = getSnapshot(t, &gitalypb.GetSnapshotRequest{Repository: testRepo})
+	assert.NoError(t, err)
+	require.NoError(t, os.Remove(alternatesPath))
+
 	// write alternates file with bad permissions
 	require.NoError(t, ioutil.WriteFile(alternatesPath, []byte(fmt.Sprintf("%s\n", alternateObjPath)), 0000))
 	_, err = getSnapshot(t, req)
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) {