models/repo_editor: sanitize user-defined file name to prevent RCE

models/repo_editor.go

@@ -11,6 +11,7 @@ import (
 	"gitote/gitote/models/errors"
 	"gitote/gitote/pkg/process"
 	"gitote/gitote/pkg/setting"
+	"gitote/gitote/pkg/tool"
 	"io"
 	"io/ioutil"
 	"mime/multipart"
@@ -27,6 +28,41 @@ import (
 	log "gopkg.in/clog.v1"
 )
 
+const (
+	ENV_AUTH_USER_ID           = "GITOTE_AUTH_USER_ID"
+	ENV_AUTH_USER_NAME         = "GITOTE_AUTH_USER_NAME"
+	ENV_AUTH_USER_EMAIL        = "GITOTE_AUTH_USER_EMAIL"
+	ENV_REPO_OWNER_NAME        = "GITOTE_REPO_OWNER_NAME"
+	ENV_REPO_OWNER_SALT_MD5    = "GITOTE_REPO_OWNER_SALT_MD5"
+	ENV_REPO_ID                = "GITOTE_REPO_ID"
+	ENV_REPO_NAME              = "GITOTE_REPO_NAME"
+	ENV_REPO_CUSTOM_HOOKS_PATH = "GITOTE_REPO_CUSTOM_HOOKS_PATH"
+)
+
+type ComposeHookEnvsOptions struct {
+	AuthUser  *User
+	OwnerName string
+	OwnerSalt string
+	RepoID    int64
+	RepoName  string
+	RepoPath  string
+}
+
+func ComposeHookEnvs(opts ComposeHookEnvsOptions) []string {
+	envs := []string{
+		"SSH_ORIGINAL_COMMAND=1",
+		ENV_AUTH_USER_ID + "=" + com.ToStr(opts.AuthUser.ID),
+		ENV_AUTH_USER_NAME + "=" + opts.AuthUser.Name,
+		ENV_AUTH_USER_EMAIL + "=" + opts.AuthUser.Email,
+		ENV_REPO_OWNER_NAME + "=" + opts.OwnerName,
+		ENV_REPO_OWNER_SALT_MD5 + "=" + tool.MD5(opts.OwnerSalt),
+		ENV_REPO_ID + "=" + com.ToStr(opts.RepoID),
+		ENV_REPO_NAME + "=" + opts.RepoName,
+		ENV_REPO_CUSTOM_HOOKS_PATH + "=" + path.Join(opts.RepoPath, "custom_hooks"),
+	}
+	return envs
+}
+
 // discardLocalRepoBranchChanges discards local commits/changes of
 // given branch to make sure it is even to remote branch.
 func discardLocalRepoBranchChanges(localPath, branch string) error {
@@ -327,7 +363,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

@@ -6,6 +6,10 @@
 
 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
@@ -13,3 +17,10 @@ package tool
 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)
+	return 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/gitote/data/sessions/a/9/a9f0ab6c3ef63dd8", "data/gitote/data/sessions/a/9/a9f0ab6c3ef63dd8"},
+			{"data/gitote/../../../../../../../../../data/sessions/a/9/a9f0ab6c3ef63dd8", "data/gitote/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)
-	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,
 	})
 }