Check that user-provided paths are inside the expected dirda-path-traversal

6 files changed, 11 insertions, 16 deletions

diff --git a/internal/cache/keyer.go b/internal/cache/keyer.go
index 86957c3fe..4cf8bdbef 100644
--- a/internal/cache/keyer.go
+++ b/internal/cache/keyer.go
@@ -240,7 +240,7 @@ func getRepoStatePath(repo *gitalypb.Repository) (string, error) {
 		return "", fmt.Errorf("getRepoStatePath: relative path missing from %+v", repo)
 	}
 
-	if helper.ContainsPathTraversal(relativePath) {
+	if helper.ContainsPathTraversal(stateDir, relativePath) {
 		return "", fmt.Errorf("getRepoStatePath: relative path can't contain directory traversal")
 	}
 
diff --git a/internal/helper/repo.go b/internal/helper/repo.go
index 9274949a4..3bea9c516 100644
--- a/internal/helper/repo.go
+++ b/internal/helper/repo.go
@@ -50,7 +50,7 @@ func GetPath(repo repository.GitRepo) (string, error) {
 		return "", err
 	}
 
-	if ContainsPathTraversal(relativePath) {
+	if ContainsPathTraversal(storagePath, relativePath) {
 		return "", status.Errorf(codes.InvalidArgument, "GetRepoPath: relative path can't contain directory traversal")
 	}
 
@@ -109,7 +109,7 @@ func GetObjectDirectoryPath(repo repository.GitRepo) (string, error) {
 		return "", status.Errorf(codes.InvalidArgument, "GetObjectDirectoryPath: empty directory")
 	}
 
-	if ContainsPathTraversal(objectDirectoryPath) {
+	if ContainsPathTraversal(repoPath, objectDirectoryPath) {
 		return "", status.Errorf(codes.InvalidArgument, "GetObjectDirectoryPath: relative path can't contain directory traversal")
 	}
 
diff --git a/internal/helper/security.go b/internal/helper/security.go
index e66186580..a6df6546a 100644
--- a/internal/helper/security.go
+++ b/internal/helper/security.go
@@ -2,18 +2,16 @@ package helper
 
 import (
 	"errors"
-	"os"
+	"path"
 	"regexp"
 	"strings"
 )
 
-// ContainsPathTraversal checks if the path contains any traversal
-func ContainsPathTraversal(path string) bool {
+// ContainsPathTraversal checks if relPath contains any traversal
+func ContainsPathTraversal(baseDir string, relPath string) bool {
 	// Disallow directory traversal for security
-	separator := string(os.PathSeparator)
-	return strings.HasPrefix(path, ".."+separator) ||
-		strings.Contains(path, separator+".."+separator) ||
-		strings.HasSuffix(path, separator+"..")
+	absPath := path.Clean((path.Join(baseDir, relPath)))
+	return !strings.HasPrefix(absPath, baseDir)
 }
 
 // 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..12b909577 100644
--- a/internal/helper/security_test.go
+++ b/internal/helper/security_test.go
@@ -19,7 +19,7 @@ func TestContainsPathTraversal(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
-		assert.Equal(t, tc.containsTraversal, ContainsPathTraversal(tc.path))
+		assert.Equal(t, tc.containsTraversal, ContainsPathTraversal("/repo/storage/path", tc.path))
 	}
 }
 
diff --git a/internal/service/repository/archive.go b/internal/service/repository/archive.go
index 3b4ca3630..36d6b47d5 100644
--- a/internal/service/repository/archive.go
+++ b/internal/service/repository/archive.go
@@ -67,7 +67,8 @@ func validateGetArchiveRequest(in *gitalypb.GetArchiveRequest, format string, pa
 		return helper.ErrInvalidArgumentf("invalid format")
 	}
 
-	if helper.ContainsPathTraversal(path) {
+	repoPath, _ := helper.GetRepoPath(in.GetRepository())
+	if helper.ContainsPathTraversal(repoPath, path) {
 		return helper.ErrInvalidArgumentf("path can't contain directory traversal")
 	}
 
diff --git a/internal/service/repository/rename.go b/internal/service/repository/rename.go
index 10bbee028..d25ef872f 100644
--- a/internal/service/repository/rename.go
+++ b/internal/service/repository/rename.go
@@ -49,9 +49,5 @@ func validateRenameRepositoryRequest(in *gitalypb.RenameRepositoryRequest) error
 		return errors.New("destination relative path is empty")
 	}
 
-	if helper.ContainsPathTraversal(in.GetRelativePath()) {
-		return errors.New("relative_path contains path traversal")
-	}
-
 	return nil
 }