diff --git a/models/git/lfs_lock.go b/models/git/lfs_lock.go index 178fa72f09bf6..8d2997f4fb88c 100644 --- a/models/git/lfs_lock.go +++ b/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 { diff --git a/modules/options/base.go b/modules/options/base.go index e83e8df5d0945..b15da51732e12 100644 --- a/modules/options/base.go +++ b/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 diff --git a/modules/public/public.go b/modules/public/public.go index e1d60d89eb9f9..90d16f3852b3f 100644 --- a/modules/public/public.go +++ b/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 diff --git a/modules/storage/local.go b/modules/storage/local.go index 15f5761e8f055..3b6c2650e46c5 100644 --- a/modules/storage/local.go +++ b/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 diff --git a/modules/storage/minio.go b/modules/storage/minio.go index 8cc06bcdd3df2..f200e25153d00 100644 --- a/modules/storage/minio.go +++ b/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 diff --git a/modules/util/path.go b/modules/util/path.go index 5aa9e15f5c3e9..222820356c993 100644 --- a/modules/util/path.go +++ b/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,36 @@ 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 { + if strings.HasPrefix(v, "/") { + elems[i] = path.Clean(v) + } else { + elems[i] = path.Clean("/" + v) + } + } + if len(elem) > 0 && !strings.HasPrefix(elem[0], "/") { + return strings.TrimPrefix(path.Join(elems...), "/") + } + 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 { + if strings.HasPrefix(v, separator) { + elems[i] = filepath.Clean(v) + } else { + elems[i] = filepath.Clean(separator + v) + } + } + if len(elem) > 0 && !strings.HasPrefix(elem[0], separator) { + return strings.TrimPrefix(filepath.Join(elems...), separator) + } + return filepath.Join(elems...) +} diff --git a/modules/util/path_test.go b/modules/util/path_test.go index 2f020f924dd2a..0059fa2785ac7 100644 --- a/modules/util/path_test.go +++ b/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) + }) } } diff --git a/modules/util/path_unix_test.go b/modules/util/path_unix_test.go new file mode 100644 index 0000000000000..287f5fb5d7e2a --- /dev/null +++ b/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) + }) + } +} diff --git a/modules/util/path_windows_test.go b/modules/util/path_windows_test.go new file mode 100644 index 0000000000000..42905edc731eb --- /dev/null +++ b/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) + }) + } +} diff --git a/routers/web/base.go b/routers/web/base.go index 2eb0b6f39118a..4b67e96881239 100644 --- a/routers/web/base.go +++ b/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 diff --git a/routers/web/repo/editor.go b/routers/web/repo/editor.go index 4f208098e4766..f0416c2736d9c 100644 --- a/routers/web/repo/editor.go +++ b/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" { diff --git a/routers/web/repo/lfs.go b/routers/web/repo/lfs.go index 43f5527986b9b..cebdb639c2a7c 100644 --- a/routers/web/repo/lfs.go +++ b/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") diff --git a/services/migrations/gitea_uploader.go b/services/migrations/gitea_uploader.go index ca961524d12c2..c9e8288d3f9e1 100644 --- a/services/migrations/gitea_uploader.go +++ b/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() diff --git a/services/packages/container/blob_uploader.go b/services/packages/container/blob_uploader.go index 860672587d2b4..d9f2bc6304550 100644 --- a/services/packages/container/blob_uploader.go +++ b/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 diff --git a/services/repository/files/file.go b/services/repository/files/file.go index 7939491aec624..329fca4e05e85 100644 --- a/services/repository/files/file.go +++ b/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" {