Support SafeJoinPath and SafeJoinFilepath

models/git/lfs_lock.go

@@ -34,7 +34,7 @@ func init() {
 
 // BeforeInsert is invoked from XORM before inserting an object of this type.
 func (l *LFSLock) BeforeInsert() {
-	l.Path = util.CleanPath(l.Path)
+	l.Path = util.SafeJoinPath(l.Path)
 }
 
 // CreateLFSLock creates a new lock.
@@ -49,7 +49,7 @@ func CreateLFSLock(ctx context.Context, repo *repo_model.Repository, lock *LFSLo
 		return nil, err
 	}
 
-	lock.Path = util.CleanPath(lock.Path)
+	lock.Path = util.SafeJoinPath(lock.Path)
 	lock.RepoID = repo.ID
 
 	l, err := GetLFSLock(dbCtx, repo, lock.Path)
@@ -69,7 +69,7 @@ func CreateLFSLock(ctx context.Context, repo *repo_model.Repository, lock *LFSLo
 
 // GetLFSLock returns release by given path.
 func GetLFSLock(ctx context.Context, repo *repo_model.Repository, path string) (*LFSLock, error) {
-	path = util.CleanPath(path)
+	path = util.SafeJoinPath(path)
 	rel := &LFSLock{RepoID: repo.ID}
 	has, err := db.GetEngine(ctx).Where("lower(path) = ?", strings.ToLower(path)).Get(rel)
 	if err != nil {

modules/options/base.go

@@ -7,7 +7,6 @@ import (
 	"fmt"
 	"io/fs"
 	"os"
-	"path"
 	"path/filepath"
 
 	"code.gitea.io/gitea/modules/setting"
@@ -16,27 +15,27 @@ import (
 
 // Locale reads the content of a specific locale from static/bindata or custom path.
 func Locale(name string) ([]byte, error) {
-	return fileFromDir(path.Join("locale", util.CleanPath(name)))
+	return fileFromDir(util.SafeJoinPath("locale", name))
 }
 
 // Readme reads the content of a specific readme from static/bindata or custom path.
 func Readme(name string) ([]byte, error) {
-	return fileFromDir(path.Join("readme", util.CleanPath(name)))
+	return fileFromDir(util.SafeJoinPath("readme", name))
 }
 
 // Gitignore reads the content of a gitignore locale from static/bindata or custom path.
 func Gitignore(name string) ([]byte, error) {
-	return fileFromDir(path.Join("gitignore", util.CleanPath(name)))
+	return fileFromDir(util.SafeJoinPath("gitignore", name))
 }
 
 // License reads the content of a specific license from static/bindata or custom path.
 func License(name string) ([]byte, error) {
-	return fileFromDir(path.Join("license", util.CleanPath(name)))
+	return fileFromDir(util.SafeJoinPath("license", name))
 }
 
 // Labels reads the content of a specific labels from static/bindata or custom path.
 func Labels(name string) ([]byte, error) {
-	return fileFromDir(path.Join("label", util.CleanPath(name)))
+	return fileFromDir(util.SafeJoinPath("label", name))
 }
 
 // WalkLocales reads the content of a specific locale

modules/public/public.go

@@ -103,7 +103,7 @@ func setWellKnownContentType(w http.ResponseWriter, file string) {
 
 func (opts *Options) handle(w http.ResponseWriter, req *http.Request, fs http.FileSystem, file string) bool {
 	// use clean to keep the file is a valid path with no . or ..
-	f, err := fs.Open(util.CleanPath(file))
+	f, err := fs.Open(util.SafeJoinFilepath(file))
 	if err != nil {
 		if os.IsNotExist(err) {
 			return false

modules/storage/local.go

@@ -58,7 +58,7 @@ func NewLocalStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error
 }
 
 func (l *LocalStorage) buildLocalPath(p string) string {
-	return filepath.Join(l.dir, util.CleanPath(strings.ReplaceAll(p, "\\", "/")))
+	return util.SafeJoinFilepath(l.dir, strings.ReplaceAll(p, "\\", "/"))
 }
 
 // Open a file

modules/storage/minio.go

@@ -121,7 +121,7 @@ func NewMinioStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error
 }
 
 func (m *MinioStorage) buildMinioPath(p string) string {
-	return strings.TrimPrefix(path.Join(m.basePath, util.CleanPath(strings.ReplaceAll(p, "\\", "/"))), "/")
+	return strings.TrimPrefix(util.SafeJoinPath(m.basePath, strings.ReplaceAll(p, "\\", "/")), "/")
 }
 
 // Open open a file

modules/util/path.go

@@ -14,14 +14,6 @@ import (
 	"strings"
 )
 
-// CleanPath ensure to clean the path
-func CleanPath(p string) string {
-	if strings.HasPrefix(p, "/") {
-		return path.Clean(p)
-	}
-	return path.Clean("/" + p)[1:]
-}
-
 // EnsureAbsolutePath ensure that a path is absolute, making it
 // relative to absoluteBase if necessary
 func EnsureAbsolutePath(path, absoluteBase string) string {
@@ -248,3 +240,28 @@ func IsReadmeFileExtension(name string, ext ...string) (int, bool) {
 
 	return 0, false
 }
+
+// SafeJoinPath is like path.Join, but it will prevent directory escaping.
+func SafeJoinPath(elem ...string) string {
+	elems := make([]string, len(elem))
+	for i, v := range elem {
+		elems[i] = path.Clean("/" + v)
+	}
+	if len(elem) > 0 && !strings.HasPrefix(elem[0], "/") {
+		return path.Join(elems...)[1:]
+	}
+	return path.Join(elems...)
+}
+
+// SafeJoinFilepath is like filepath.Join, but it will prevent directory escaping.
+func SafeJoinFilepath(elem ...string) string {
+	separator := string(filepath.Separator)
+	elems := make([]string, len(elem))
+	for i, v := range elem {
+		elems[i] = filepath.Clean(separator + v)
+	}
+	if len(elem) > 0 && !strings.HasPrefix(elem[0], separator) {
+		return filepath.Join(elems...)[1:]
+	}
+	return filepath.Join(elems...)
+}

modules/util/path_test.go

@@ -137,14 +137,46 @@ func TestMisc_IsReadmeFileName(t *testing.T) {
 	}
 }
 
-func TestCleanPath(t *testing.T) {
-	cases := map[string]string{
-		"../../test": "test",
-		"/test":      "/test",
-		"/../test":   "/test",
+func TestSafeJoinPath(t *testing.T) {
+	tests := []struct {
+		name string
+		args []string
+		want string
+	}{
+		{
+			name: "empty elems",
+			args: []string{},
+			want: "",
+		},
+		{
+			name: "empty string",
+			args: []string{"", ""},
+			want: "",
+		},
+		{
+			name: "escape root",
+			args: []string{"/tmp", "../etc/passwd", "../../../../etc/passwd"},
+			want: "/tmp/etc/passwd/etc/passwd",
+		},
+		{
+			name: "normal upward",
+			args: []string{"/tmp", "/test1/../b", "test2/./test3/../../c"},
+			want: "/tmp/b/c",
+		},
+		{
+			name: "relative path",
+			args: []string{"./tmp", "/test1/../b", "test2/./test3/../../c"},
+			want: "tmp/b/c",
+		},
+		{
+			name: "as CleanPath",
+			args: []string{"../../../tmp"},
+			want: "tmp",
+		},
 	}
-
-	for k, v := range cases {
-		assert.Equal(t, v, CleanPath(k))
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equalf(t, tt.want, SafeJoinPath(tt.args...), "SafeJoinPath(%v)", tt.args)
+		})
 	}
 }

modules/util/path_unix_test.go

@@ -0,0 +1,56 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+//go:build !windows
+
+package util
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestSafeJoinFilepath(t *testing.T) {
+	tests := []struct {
+		name string
+		args []string
+		want string
+	}{
+		{
+			name: "empty elems",
+			args: []string{},
+			want: "",
+		},
+		{
+			name: "empty string",
+			args: []string{"", ""},
+			want: "",
+		},
+		{
+			name: "escape root",
+			args: []string{"/tmp", "../etc/passwd", "../../../../etc/passwd"},
+			want: "/tmp/etc/passwd/etc/passwd",
+		},
+		{
+			name: "normal upward",
+			args: []string{"/tmp", "/test1/../b", "test2/./test3/../../c"},
+			want: "/tmp/b/c",
+		},
+		{
+			name: "relative path",
+			args: []string{"./tmp", "/test1/../b", "test2/./test3/../../c"},
+			want: "tmp/b/c",
+		},
+		{
+			name: "as CleanPath",
+			args: []string{"../../../tmp"},
+			want: "tmp",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equalf(t, tt.want, SafeJoinFilepath(tt.args...), "SafeJoinPath(%v)", tt.args)
+		})
+	}
+}

modules/util/path_windows_test.go

@@ -0,0 +1,61 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+//go:build windows
+
+package util
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestSafeJoinFilepath(t *testing.T) {
+	tests := []struct {
+		name string
+		args []string
+		want string
+	}{
+		{
+			name: "empty elems",
+			args: []string{},
+			want: "",
+		},
+		{
+			name: "empty string",
+			args: []string{"", ""},
+			want: "",
+		},
+		{
+			name: "escape root",
+			args: []string{`\tmp`, `..\etc\passwd`, `..\..\..\..\etc\passwd`},
+			want: `\tmp\etc\passwd\etc\passwd`,
+		},
+		{
+			name: "normal upward",
+			args: []string{`\tmp`, `\test1\..\b`, `test2\.\test3\..\..\c`},
+			want: `\tmp\b\c`,
+		},
+		{
+			name: "relative path",
+			args: []string{`.\tmp`, `\test1\..\b`, `test2\.\test3\..\..\c`},
+			want: `tmp\b\c`,
+		},
+		{
+			name: "as CleanPath",
+			args: []string{`..\..\..\tmp`},
+			want: `tmp`,
+		},
+		{
+			name: "with drive",
+			args: []string{`C:\tmp`, `..\etc\passwd`, `\test1\..\b`, `test2\.\test3\..\..\c`},
+			want: `C:\tmp\etc\passwd\b\c`,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equalf(t, tt.want, SafeJoinFilepath(tt.args...), "SafeJoinPath(%v)", tt.args)
+		})
+	}
+}

routers/web/base.go

@@ -45,7 +45,7 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
 				routing.UpdateFuncInfo(req.Context(), funcInfo)
 
 				rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/")
-				rPath = util.CleanPath(strings.ReplaceAll(rPath, "\\", "/"))
+				rPath = util.SafeJoinPath(strings.ReplaceAll(rPath, "\\", "/"))
 
 				u, err := objStore.URL(rPath, path.Base(rPath))
 				if err != nil {
@@ -81,7 +81,7 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
 			routing.UpdateFuncInfo(req.Context(), funcInfo)
 
 			rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/")
-			rPath = util.CleanPath(strings.ReplaceAll(rPath, "\\", "/"))
+			rPath = util.SafeJoinPath(strings.ReplaceAll(rPath, "\\", "/"))
 			if rPath == "" {
 				http.Error(w, "file not found", http.StatusNotFound)
 				return

routers/web/repo/editor.go

@@ -726,7 +726,7 @@ func UploadFilePost(ctx *context.Context) {
 
 func cleanUploadFileName(name string) string {
 	// Rebase the filename
-	name = strings.Trim(util.CleanPath(name), "/")
+	name = strings.Trim(util.SafeJoinPath(name), "/")
 	// Git disallows any filenames to have a .git directory in them.
 	for _, part := range strings.Split(name, "/") {
 		if strings.ToLower(part) == ".git" {

routers/web/repo/lfs.go

@@ -207,7 +207,7 @@ func LFSLockFile(ctx *context.Context) {
 		ctx.Redirect(ctx.Repo.RepoLink + "/settings/lfs/locks")
 		return
 	}
-	lockPath = util.CleanPath(lockPath)
+	lockPath = util.SafeJoinPath(lockPath)
 	if len(lockPath) == 0 {
 		ctx.Flash.Error(ctx.Tr("repo.settings.lfs_invalid_locking_path", originalPath))
 		ctx.Redirect(ctx.Repo.RepoLink + "/settings/lfs/locks")

services/migrations/gitea_uploader.go

@@ -866,7 +866,7 @@ func (g *GiteaLocalUploader) CreateReviews(reviews ...*base.Review) error {
 			}
 
 			// SECURITY: The TreePath must be cleaned!
-			comment.TreePath = util.CleanPath(comment.TreePath)
+			comment.TreePath = util.SafeJoinPath(comment.TreePath)
 
 			var patch string
 			reader, writer := io.Pipe()

services/packages/container/blob_uploader.go

@@ -8,7 +8,6 @@ import (
 	"errors"
 	"io"
 	"os"
-	"path/filepath"
 	"strings"
 
 	packages_model "code.gitea.io/gitea/models/packages"
@@ -33,7 +32,7 @@ type BlobUploader struct {
 }
 
 func buildFilePath(id string) string {
-	return filepath.Join(setting.Packages.ChunkedUploadPath, util.CleanPath(strings.ReplaceAll(id, "\\", "/")))
+	return util.SafeJoinFilepath(setting.Packages.ChunkedUploadPath, strings.ReplaceAll(id, "\\", "/"))
 }
 
 // NewBlobUploader creates a new blob uploader for the given id

services/repository/files/file.go

@@ -129,7 +129,7 @@ func GetAuthorAndCommitterUsers(author, committer *IdentityOptions, doer *user_m
 // CleanUploadFileName Trims a filename and returns empty string if it is a .git directory
 func CleanUploadFileName(name string) string {
 	// Rebase the filename
-	name = strings.Trim(util.CleanPath(name), "/")
+	name = strings.Trim(util.SafeJoinPath(name), "/")
 	// Git disallows any filenames to have a .git directory in them.
 	for _, part := range strings.Split(name, "/") {
 		if strings.ToLower(part) == ".git" {