pkg/tool/path: use IsMaliciousPath to replace SanitizePath (#5558)

models/repo_editor.go

@@ -327,9 +327,13 @@ func (upload *Upload) LocalPath() string {
 
 // NewUpload creates a new upload object.
 func NewUpload(name string, buf []byte, file multipart.File) (_ *Upload, err error) {
+	if tool.IsMaliciousPath(name) {
+		return nil, fmt.Errorf("malicious path detected: %s", name)
+	}
+
 	upload := &Upload{
 		UUID: gouuid.NewV4().String(),
-		Name: tool.SanitizePath(name),
+		Name: name,
 	}
 
 	localPath := upload.LocalPath()

pkg/tool/path.go

@@ -5,6 +5,7 @@
 package tool
 
 import (
+	"path/filepath"
 	"strings"
 )
 
@@ -15,10 +16,8 @@ func IsSameSiteURLPath(url string) bool {
 	return len(url) >= 2 && url[0] == '/' && url[1] != '/' && url[1] != '\\'
 }
 
-// SanitizePath sanitizes user-defined file paths to prevent remote code execution.
-func SanitizePath(path string) string {
-	path = strings.TrimLeft(path, "/")
-	path = strings.Replace(path, "../", "", -1)
-	path = strings.Replace(path, "..\\", "", -1)
-	return path
+// IsMaliciousPath returns true if given path is an absolute path or contains malicious content
+// which has potential to traverse upper level directories.
+func IsMaliciousPath(path string) bool {
+	return filepath.IsAbs(path) || strings.Contains(path, "..")
 }

pkg/tool/path_test.go

@@ -31,22 +31,23 @@ func Test_IsSameSiteURLPath(t *testing.T) {
 	})
 }
 
-func Test_SanitizePath(t *testing.T) {
-	Convey("Sanitize malicious user-defined path", t, func() {
+func Test_IsMaliciousPath(t *testing.T) {
+	Convey("Detects malicious path", t, func() {
 		testCases := []struct {
 			path   string
-			expect string
+			expect bool
 		}{
-			{"../../../../../../../../../data/gogs/data/sessions/a/9/a9f0ab6c3ef63dd8", "data/gogs/data/sessions/a/9/a9f0ab6c3ef63dd8"},
-			{"data/gogs/../../../../../../../../../data/sessions/a/9/a9f0ab6c3ef63dd8", "data/gogs/data/sessions/a/9/a9f0ab6c3ef63dd8"},
-			{"..\\..\\..\\..\\..\\..\\..\\..\\..\\data\\gogs\\data\\sessions\\a\\9\\a9f0ab6c3ef63dd8", "data\\gogs\\data\\sessions\\a\\9\\a9f0ab6c3ef63dd8"},
-			{"data\\gogs\\..\\..\\..\\..\\..\\..\\..\\..\\..\\data\\sessions\\a\\9\\a9f0ab6c3ef63dd8", "data\\gogs\\data\\sessions\\a\\9\\a9f0ab6c3ef63dd8"},
-
-			{"data/sessions/a/9/a9f0ab6c3ef63dd8", "data/sessions/a/9/a9f0ab6c3ef63dd8"},
-			{"data\\sessions\\a\\9\\a9f0ab6c3ef63dd8", "data\\sessions\\a\\9\\a9f0ab6c3ef63dd8"},
+			{"../../../../../../../../../data/gogs/data/sessions/a/9/a9f0ab6c3ef63dd8", true},
+			{"..\\/..\\/../data/gogs/data/sessions/a/9/a9f0ab6c3ef63dd8", true},
+			{"data/gogs/../../../../../../../../../data/sessions/a/9/a9f0ab6c3ef63dd8", true},
+			{"..\\..\\..\\..\\..\\..\\..\\..\\..\\data\\gogs\\data\\sessions\\a\\9\\a9f0ab6c3ef63dd8", true},
+			{"data\\gogs\\..\\..\\..\\..\\..\\..\\..\\..\\..\\data\\sessions\\a\\9\\a9f0ab6c3ef63dd8", true},
+
+			{"data/sessions/a/9/a9f0ab6c3ef63dd8", false},
+			{"data\\sessions\\a\\9\\a9f0ab6c3ef63dd8", false},
 		}
 		for _, tc := range testCases {
-			So(SanitizePath(tc.path), ShouldEqual, tc.expect)
+			So(IsMaliciousPath(tc.path), ShouldEqual, tc.expect)
 		}
 	})
 }