Disallow directory traversal in repository paths for security

3 files changed, 48 insertions, 1 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3431b0705..d23e3fe79 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,6 +8,8 @@ UNRELEASED
   https://gitlab.com/gitlab-org/gitaly/merge_requests/149
 - Count accepted gRPC connections
   https://gitlab.com/gitlab-org/gitaly/merge_requests/151
+- Disallow directory traversal in repository paths for security
+  https://gitlab.com/gitlab-org/gitaly/merge_requests/152
 
 v0.10.0
 
diff --git a/internal/helper/repo.go b/internal/helper/repo.go
index c4c000f70..b8928a429 100644
--- a/internal/helper/repo.go
+++ b/internal/helper/repo.go
@@ -3,6 +3,7 @@ package helper
 import (
 	"os"
 	"path"
+	"strings"
 
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 
@@ -20,7 +21,15 @@ func GetRepoPath(repo *pb.Repository) (string, error) {
 	var repoPath string
 
 	if storagePath, ok := config.StoragePath(repo.GetStorageName()); ok {
-		repoPath = path.Join(storagePath, repo.GetRelativePath())
+		relativePath := repo.GetRelativePath()
+		// Disallow directory traversal for security
+		separator := string(os.PathSeparator)
+		if strings.HasPrefix(relativePath, ".."+separator) ||
+			strings.Contains(relativePath, separator+".."+separator) ||
+			strings.HasSuffix(relativePath, separator+"..") {
+			return "", grpc.Errorf(codes.InvalidArgument, "GetRepoPath: relative path can't contain directory traversal")
+		}
+		repoPath = path.Join(storagePath, relativePath)
 	} else {
 		repoPath = repo.GetPath()
 	}
diff --git a/internal/helper/repo_test.go b/internal/helper/repo_test.go
index 07ea8b409..29e3daa7b 100644
--- a/internal/helper/repo_test.go
+++ b/internal/helper/repo_test.go
@@ -96,6 +96,42 @@ func TestGetRepoPath(t *testing.T) {
 			repo: &pb.Repository{Path: "/made/up/path.git"},
 			err:  codes.NotFound,
 		},
+		{
+			desc:     "relative path with directory traversal",
+			storages: exampleStorages,
+			repo:     &pb.Repository{StorageName: "default", RelativePath: "../bazqux.git"},
+			err:      codes.InvalidArgument,
+		},
+		{
+			desc:     "valid path with ..",
+			storages: exampleStorages,
+			repo:     &pb.Repository{StorageName: "default", RelativePath: "foo../bazqux.git"},
+			err:      codes.NotFound, // Because the directory doesn't exist
+		},
+		{
+			desc:     "relative path with sneaky directory traversal",
+			storages: exampleStorages,
+			repo:     &pb.Repository{StorageName: "default", RelativePath: "/../bazqux.git"},
+			err:      codes.InvalidArgument,
+		},
+		{
+			desc:     "relative path with one level traversal at the end",
+			storages: exampleStorages,
+			repo:     &pb.Repository{StorageName: "default", RelativePath: testhelper.TestRelativePath + "/.."},
+			err:      codes.InvalidArgument,
+		},
+		{
+			desc:     "relative path with one level dashed traversal at the end",
+			storages: exampleStorages,
+			repo:     &pb.Repository{StorageName: "default", RelativePath: testhelper.TestRelativePath + "/../"},
+			err:      codes.InvalidArgument,
+		},
+		{
+			desc:     "relative path with deep traversal at the end",
+			storages: exampleStorages,
+			repo:     &pb.Repository{StorageName: "default", RelativePath: "bazqux.git/../.."},
+			err:      codes.InvalidArgument,
+		},
 	}
 
 	for _, tc := range testCases {