From ae83ae14129fb2683bbc83587df056c79e182679 Mon Sep 17 00:00:00 2001
From: dappelt <dappelt@gitlab.com>
Date: Tue, 7 Jan 2020 17:49:27 +0100
Subject: [PATCH] Check that user-provided paths are inside the expected dir

---
 internal/cache/keyer.go                |  2 +-
 internal/helper/repo.go                |  4 ++--
 internal/helper/security.go            | 12 +++++-------
 internal/helper/security_test.go       |  2 +-
 internal/service/repository/archive.go |  3 ++-
 internal/service/repository/rename.go  |  4 ----
 6 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/internal/cache/keyer.go b/internal/cache/keyer.go
index 86957c3fe8b..4cf8bdbef1e 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 9274949a4aa..3bea9c51642 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 e661865801a..a6df6546a03 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 9a8125dac61..12b909577f3 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 3b4ca36305f..36d6b47d57b 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 10bbee02819..d25ef872fd1 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
 }
-- 
GitLab