diff --git a/models/repo_editor.go b/models/repo_editor.go index 33887f934..62914c6d1 100644 --- a/models/repo_editor.go +++ b/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() diff --git a/pkg/tool/path.go b/pkg/tool/path.go index e8f7bcbea..e95bba8be 100644 --- a/pkg/tool/path.go +++ b/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, "..") } diff --git a/pkg/tool/path_test.go b/pkg/tool/path_test.go index d9b9fb219..44ee975f2 100644 --- a/pkg/tool/path_test.go +++ b/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/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", "data/sessions/a/9/a9f0ab6c3ef63dd8"}, - {"data\\sessions\\a\\9\\a9f0ab6c3ef63dd8", "data\\sessions\\a\\9\\a9f0ab6c3ef63dd8"}, + {"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) } }) }