Reject parameters that override upload fields

When Workhorse intercepts file uploads, we store the files and send the
information about the temporary file in new multipart form values called
`file.path`, `file.size` etc.

Since we're also copying all other multipart form values from the
original client request, it was possible to override the values we
set in Workhorse, causing Rails to e.g. load the uploaded file from
an injected `file.path` parameter.

To avoid this, we check if client parameters have the same name as any
of our own added fields and reject the request.

internal/upload/rewrite.go

@@ -2,6 +2,7 @@ package upload
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"mime/multipart"
@@ -16,6 +17,9 @@ import (
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/upload/exif"
 )
 
+// ErrInjectedClientParam means that the client sent a parameter that overrides one of our own fields
+var ErrInjectedClientParam = errors.New("injected client parameter")
+
 var (
 	multipartUploadRequests = prometheus.NewCounterVec(
 		prometheus.CounterOpts{
@@ -44,9 +48,10 @@ var (
 )
 
 type rewriter struct {
-	writer  *multipart.Writer
-	preauth *api.Response
-	filter  MultipartFormProcessor
+	writer          *multipart.Writer
+	preauth         *api.Response
+	filter          MultipartFormProcessor
+	finalizedFields map[string]bool
 }
 
 func init() {
@@ -69,9 +74,10 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, pr
 	multipartUploadRequests.WithLabelValues(filter.Name()).Inc()
 
 	rew := &rewriter{
-		writer:  writer,
-		preauth: preauth,
-		filter:  filter,
+		writer:          writer,
+		preauth:         preauth,
+		filter:          filter,
+		finalizedFields: make(map[string]bool),
 	}
 
 	for {
@@ -88,7 +94,10 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, pr
 			continue
 		}
 
-		// Copy form field
+		if rew.finalizedFields[name] {
+			return ErrInjectedClientParam
+		}
+
 		if p.FileName() != "" {
 			err = rew.handleFilePart(r.Context(), name, p)
 		} else {
@@ -143,6 +152,7 @@ func (rew *rewriter) handleFilePart(ctx context.Context, name string, p *multipa
 
 	for key, value := range fh.GitLabFinalizeFields(name) {
 		rew.writer.WriteField(key, value)
+		rew.finalizedFields[key] = true
 	}
 
 	multipartFileUploadBytes.WithLabelValues(rew.filter.Name()).Add(float64(fh.Size))

internal/upload/uploads.go

@@ -37,6 +37,8 @@ func HandleFileUploads(w http.ResponseWriter, r *http.Request, h http.Handler, p
 	err := rewriteFormFilesFromMultipart(r, writer, preauth, filter)
 	if err != nil {
 		switch err {
+		case ErrInjectedClientParam:
+			helper.CaptureAndFail(w, r, err, "Bad Request", http.StatusBadRequest)
 		case http.ErrNotMultipart:
 			h.ServeHTTP(w, r)
 		case filestore.ErrEntityTooLarge:

internal/upload/uploads_test.go

@@ -3,7 +3,6 @@ package upload
 import (
 	"bytes"
 	"context"
-	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -37,8 +36,8 @@ func (a *testFormProcessor) ProcessFile(ctx context.Context, formName string, fi
 }
 
 func (a *testFormProcessor) ProcessField(ctx context.Context, formName string, writer *multipart.Writer) error {
-	if formName != "token" {
-		return errors.New("illegal field")
+	if formName != "token" && !strings.HasPrefix(formName, "file.") && !strings.HasPrefix(formName, "other.") {
+		return fmt.Errorf("illegal field: %v", formName)
 	}
 	return nil
 }
@@ -196,18 +195,80 @@ func TestUploadHandlerRewritingMultiPartData(t *testing.T) {
 	testhelper.AssertResponseCode(t, response, 202)
 
 	cancel() // this will trigger an async cleanup
+	waitUntilDeleted(t, filePath)
+}
 
-	// Poll because the file removal is async
-	for i := 0; i < 100; i++ {
-		_, err = os.Stat(filePath)
-		if err != nil {
-			break
-		}
-		time.Sleep(100 * time.Millisecond)
+func TestUploadHandlerDetectingInjectedMultiPartData(t *testing.T) {
+	var filePath string
+
+	tempPath, err := ioutil.TempDir("", "uploads")
+	if err != nil {
+		t.Fatal(err)
 	}
+	defer os.RemoveAll(tempPath)
 
-	if !os.IsNotExist(err) {
-		t.Fatal("expected the file to be deleted")
+	tests := []struct {
+		name     string
+		field    string
+		response int
+	}{
+		{
+			name:     "injected file.path",
+			field:    "file.path",
+			response: 400,
+		},
+		{
+			name:     "injected file.remote_id",
+			field:    "file.remote_id",
+			response: 400,
+		},
+		{
+			name:     "field with other prefix",
+			field:    "other.path",
+			response: 202,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			ts := testhelper.TestServerWithHandler(regexp.MustCompile(`/url/path\z`), func(w http.ResponseWriter, r *http.Request) {
+				if r.Method != "PUT" {
+					t.Fatal("Expected PUT request")
+				}
+
+				w.WriteHeader(202)
+				fmt.Fprint(w, "RESPONSE")
+			})
+
+			var buffer bytes.Buffer
+
+			writer := multipart.NewWriter(&buffer)
+			file, err := writer.CreateFormFile("file", "my.file")
+			if err != nil {
+				t.Fatal(err)
+			}
+			fmt.Fprint(file, "test")
+
+			writer.WriteField(test.field, "value")
+			writer.Close()
+
+			httpRequest, err := http.NewRequest("PUT", ts.URL+"/url/path", &buffer)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			ctx, cancel := context.WithCancel(context.Background())
+			httpRequest = httpRequest.WithContext(ctx)
+			httpRequest.Header.Set("Content-Type", writer.FormDataContentType())
+			response := httptest.NewRecorder()
+
+			handler := newProxy(ts.URL)
+			HandleFileUploads(response, httpRequest, handler, &api.Response{TempPath: tempPath}, &testFormProcessor{})
+			testhelper.AssertResponseCode(t, response, test.response)
+
+			cancel() // this will trigger an async cleanup
+			waitUntilDeleted(t, filePath)
+		})
 	}
 }
 
@@ -425,3 +486,20 @@ func newProxy(url string) *proxy.Proxy {
 	parsedURL := helper.URLMustParse(url)
 	return proxy.NewProxy(parsedURL, "123", roundtripper.NewTestBackendRoundTripper(parsedURL))
 }
+
+func waitUntilDeleted(t *testing.T, path string) {
+	var err error
+
+	// Poll because the file removal is async
+	for i := 0; i < 100; i++ {
+		_, err = os.Stat(path)
+		if err != nil {
+			break
+		}
+		time.Sleep(100 * time.Millisecond)
+	}
+
+	if !os.IsNotExist(err) {
+		t.Fatal("expected the file to be deleted")
+	}
+}