Merge branch 'security-193100-ignore-duplicate-multipart-params' into 'master'

Reject parameters that override upload fields

See merge request gitlab-org/security/gitlab-workhorse!3

CHANGELOG

@@ -2,6 +2,10 @@
 
 Formerly known as 'gitlab-git-http-server'.
 
+v 8.28.0
+
+- Reject parameters that override upload fields
+
 v 8.27.0
 
 - Remove Set-Cookie header from archive and raw blob responses !475

VERSION

-8.27.0
+8.28.0

internal/filestore/file_handler.go

@@ -60,18 +60,10 @@ func (fh *FileHandler) GitLabFinalizeFields(prefix string) map[string]string {
 		return fmt.Sprintf("%s.%s", prefix, field)
 	}
 
-	if fh.Name != "" {
-		data[key("name")] = fh.Name
-	}
-	if fh.LocalPath != "" {
-		data[key("path")] = fh.LocalPath
-	}
-	if fh.RemoteURL != "" {
-		data[key("remote_url")] = fh.RemoteURL
-	}
-	if fh.RemoteID != "" {
-		data[key("remote_id")] = fh.RemoteID
-	}
+	data[key("name")] = fh.Name
+	data[key("path")] = fh.LocalPath
+	data[key("remote_url")] = fh.RemoteURL
+	data[key("remote_id")] = fh.RemoteID
 	data[key("size")] = strconv.FormatInt(fh.Size, 10)
 	for hashName, hash := range fh.hashes {
 		data[key(hashName)] = hash

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
 }
@@ -135,6 +134,14 @@ func TestUploadHandlerRewritingMultiPartData(t *testing.T) {
 			t.Error("Expected to the file to be in tempPath")
 		}
 
+		if r.FormValue("file.remote_url") != "" {
+			t.Error("Expected to receive empty remote_url")
+		}
+
+		if r.FormValue("file.remote_id") != "" {
+			t.Error("Expected to receive empty remote_id")
+		}
+
 		if r.FormValue("file.size") != "4" {
 			t.Error("Expected to receive the file size")
 		}
@@ -152,8 +159,8 @@ func TestUploadHandlerRewritingMultiPartData(t *testing.T) {
 			}
 		}
 
-		if valueCnt := len(r.MultipartForm.Value); valueCnt != 8 {
-			t.Fatal("Expected to receive exactly 8 values but got", valueCnt)
+		if valueCnt := len(r.MultipartForm.Value); valueCnt != 10 {
+			t.Fatal("Expected to receive exactly 10 values but got", valueCnt)
 		}
 
 		w.WriteHeader(202)
@@ -188,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)
+		})
 	}
 }
 
@@ -417,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")
+	}
+}

upload_test.go

@@ -83,7 +83,7 @@ func uploadTestServer(t *testing.T, extraTests func(r *http.Request)) *httptest.
 		if err != nil {
 			t.Fatal(err)
 		}
-		nValues := 7 // file name, path, size, md5, sha1, sha256, sha512 for just the upload (no metadata because we are not POSTing a valid zip file)
+		nValues := 9 // file name, path, remote_url, remote_id, size, md5, sha1, sha256, sha512 for just the upload (no metadata because we are not POSTing a valid zip file)
 		if len(r.MultipartForm.Value) != nValues {
 			t.Errorf("Expected to receive exactly %d values", nValues)
 		}