models/repo_editor: sanitize user-defined file name to prevent RCE (#5558)

Reported by PentesterLab (https://pentesterlab.com).

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()

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, "./")
+}

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)
+		}
+	})
+}

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)
+	log.Trace("New file uploaded by user[%d]: %s", c.UserID(), upload.UUID)
-	c.JSON(200, map[string]string{
+	c.JSONSuccess(map[string]string{
 		"uuid": upload.UUID,
 	})
 }