diff --git a/models/repo_editor.go b/models/repo_editor.go index 810556ec5..a302a8de1 100644 --- a/models/repo_editor.go +++ b/models/repo_editor.go @@ -328,7 +328,7 @@ func (upload *Upload) LocalPath() string { func NewUpload(name string, buf []byte, file multipart.File) (_ *Upload, err error) { upload := &Upload{ UUID: gouuid.NewV4().String(), - Name: name, + Name: tool.SanitizePath(name), } localPath := upload.LocalPath() diff --git a/pkg/tool/path.go b/pkg/tool/path.go index e478abc52..3c0d2d027 100644 --- a/pkg/tool/path.go +++ b/pkg/tool/path.go @@ -4,9 +4,18 @@ package tool +import ( + "strings" +) + // IsSameSiteURLPath returns true if the URL path belongs to the same site, false otherwise. // False: //url, http://url, /\url // True: /url 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 { + return strings.TrimLeft(path, "./") +} diff --git a/pkg/tool/path_test.go b/pkg/tool/path_test.go index 530238cef..c9e182941 100644 --- a/pkg/tool/path_test.go +++ b/pkg/tool/path_test.go @@ -30,3 +30,19 @@ func Test_IsSameSiteURLPath(t *testing.T) { } }) } + +func Test_SanitizePath(t *testing.T) { + Convey("Sanitize malicious user-defined path", t, func() { + testCases := []struct { + path string + expect string + }{ + {"../../../../../../../../../data/gogs/data/sessions/a/9/a9f0ab6c3ef63dd8", "data/gogs/data/sessions/a/9/a9f0ab6c3ef63dd8"}, + + {"data/sessions/a/9/a9f0ab6c3ef63dd8", "data/sessions/a/9/a9f0ab6c3ef63dd8"}, + } + for _, tc := range testCases { + So(SanitizePath(tc.path), ShouldEqual, tc.expect) + } + }) +} diff --git a/routes/repo/editor.go b/routes/repo/editor.go index 67c2be1ed..7afc825e2 100644 --- a/routes/repo/editor.go +++ b/routes/repo/editor.go @@ -518,7 +518,7 @@ func UploadFilePost(c *context.Context, f form.UploadRepoFile) { func UploadFileToServer(c *context.Context) { file, header, err := c.Req.FormFile("file") if err != nil { - c.Error(500, fmt.Sprintf("FormFile: %v", err)) + c.Error(http.StatusInternalServerError, fmt.Sprintf("FormFile: %v", err)) return } defer file.Close() @@ -541,19 +541,19 @@ func UploadFileToServer(c *context.Context) { } if !allowed { - c.Error(400, ErrFileTypeForbidden.Error()) + c.Error(http.StatusBadRequest, ErrFileTypeForbidden.Error()) return } } upload, err := models.NewUpload(header.Filename, buf, file) if err != nil { - c.Error(500, fmt.Sprintf("NewUpload: %v", err)) + c.Error(http.StatusInternalServerError, fmt.Sprintf("NewUpload: %v", err)) return } - log.Trace("New file uploaded: %s", upload.UUID) - c.JSON(200, map[string]string{ + log.Trace("New file uploaded by user[%d]: %s", c.UserID(), upload.UUID) + c.JSONSuccess(map[string]string{ "uuid": upload.UUID, }) }