Merge branch 'smh-path-traversal-fix' into 'master'

Improve path traversal protection

Closes #2419 and #2299

See merge request gitlab-org/gitaly!2014

6 files changed, 27 insertions, 5 deletions

diff --git a/changelogs/unreleased/smh-path-traversal-fix.yml b/changelogs/unreleased/smh-path-traversal-fix.yml
new file mode 100644
index 000000000..09f60891e
--- /dev/null
+++ b/changelogs/unreleased/smh-path-traversal-fix.yml
@@ -0,0 +1,5 @@
+---
+title: Improve path traversal protection.
+merge_request: 2014
+author:
+type: security
diff --git a/internal/git/stats/git_test.go b/internal/git/stats/git_test.go
index 0909b8400..f167cf83b 100644
--- a/internal/git/stats/git_test.go
+++ b/internal/git/stats/git_test.go
@@ -58,10 +58,13 @@ func TestLogObjectInfo(t *testing.T) {
 		// clone existing local repo with two alternates
 		testhelper.MustRunCommand(t, nil, "git", "clone", "--shared", repoPath1, "--reference", repoPath1, "--reference", repoPath2, tmpDir)
 
+		relativePath, err := filepath.Rel(storagePath, tmpDir)
+		require.NoError(t, err)
+
 		logBuffer.Reset()
 		LogObjectsInfo(testCtx, &gitalypb.Repository{
 			StorageName:  repo1.StorageName,
-			RelativePath: filepath.Join(strings.TrimPrefix(tmpDir, storagePath), ".git"),
+			RelativePath: filepath.Join(relativePath, ".git"),
 		})
 
 		countObjects := requireLog(logBuffer.String())
diff --git a/internal/helper/security.go b/internal/helper/security.go
index e66186580..18c721ba0 100644
--- a/internal/helper/security.go
+++ b/internal/helper/security.go
@@ -3,6 +3,7 @@ package helper
 import (
 	"errors"
 	"os"
+	"path/filepath"
 	"regexp"
 	"strings"
 )
@@ -13,7 +14,9 @@ func ContainsPathTraversal(path string) bool {
 	separator := string(os.PathSeparator)
 	return strings.HasPrefix(path, ".."+separator) ||
 		strings.Contains(path, separator+".."+separator) ||
-		strings.HasSuffix(path, separator+"..")
+		strings.HasSuffix(path, separator+"..") ||
+		filepath.IsAbs(path) ||
+		path == ".."
 }
 
 // Pattern taken from Regular Expressions Cookbook, slightly modified though
diff --git a/internal/helper/security_test.go b/internal/helper/security_test.go
index 9a8125dac..e983c56e1 100644
--- a/internal/helper/security_test.go
+++ b/internal/helper/security_test.go
@@ -16,10 +16,21 @@ func TestContainsPathTraversal(t *testing.T) {
 		{"subdir/..", true},
 		{"subdir", false},
 		{"./subdir", false},
+		{"..", true},
+		{".", false},
+		{"/absolute", true},
+		{"double//slash", false},
+		{"trailing-slash/", false},
+		{"whitespace-named/ /directory", false},
+		{" whitespace/leading-name", false},
+		{"whitespace-trailing/name ", false},
+		{"whitespace in/directory name", false},
 	}
 
 	for _, tc := range testCases {
-		assert.Equal(t, tc.containsTraversal, ContainsPathTraversal(tc.path))
+		t.Run(tc.path, func(t *testing.T) {
+			assert.Equal(t, tc.containsTraversal, ContainsPathTraversal(tc.path))
+		})
 	}
 }
 
diff --git a/internal/service/ref/refs_test.go b/internal/service/ref/refs_test.go
index c9159c67e..e69df74c2 100644
--- a/internal/service/ref/refs_test.go
+++ b/internal/service/ref/refs_test.go
@@ -420,7 +420,7 @@ func TestInvalidRepoFindDefaultBranchNameRequest(t *testing.T) {
 
 	client, conn := newRefServiceClient(t, serverSocketPath)
 	defer conn.Close()
-	repo := &gitalypb.Repository{StorageName: "default", RelativePath: "/made/up/path"}
+	repo := &gitalypb.Repository{StorageName: "default", RelativePath: "made/up/path"}
 	rpcRequest := &gitalypb.FindDefaultBranchNameRequest{Repository: repo}
 
 	ctx, cancel := context.WithCancel(context.Background())
diff --git a/internal/service/repository/remove_test.go b/internal/service/repository/remove_test.go
index 24b31f1cd..6debd9e6b 100644
--- a/internal/service/repository/remove_test.go
+++ b/internal/service/repository/remove_test.go
@@ -38,6 +38,6 @@ func TestRemoveRepositoryDoesNotExist(t *testing.T) {
 	defer cancel()
 
 	_, err := client.RemoveRepository(ctx, &gitalypb.RemoveRepositoryRequest{
-		Repository: &gitalypb.Repository{StorageName: "default", RelativePath: "/does/not/exist"}})
+		Repository: &gitalypb.Repository{StorageName: "default", RelativePath: "does/not/exist"}})
 	require.NoError(t, err)
 }