Merge branch '340366_fix_workhorse_content_disposition_2' into 'master'

Reject MIME parts with unsupported encoding

See merge request gitlab-org/gitlab!78699

workhorse/internal/upload/rewrite.go

@@ -6,8 +6,10 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
+	"mime"
 	"mime/multipart"
 	"net/http"
+	"net/textproto"
 	"os"
 	"path/filepath"
 	"strings"
@@ -95,7 +97,8 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, pr
 			return err
 		}
 
-		name := p.FormName()
+		name, filename := parseAndNormalizeContentDisposition(p.Header)
+
 		if name == "" {
 			continue
 		}
@@ -104,7 +107,7 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, pr
 			return ErrInjectedClientParam
 		}
 
-		if p.FileName() != "" {
+		if filename != "" {
 			err = rew.handleFilePart(r.Context(), name, p, opts)
 		} else {
 			err = rew.copyPart(r.Context(), name, p)
@@ -118,6 +121,13 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, pr
 	return nil
 }
 
+func parseAndNormalizeContentDisposition(header textproto.MIMEHeader) (string, string) {
+	const key = "Content-Disposition"
+	mediaType, params, _ := mime.ParseMediaType(header.Get(key))
+	header.Set(key, mime.FormatMediaType(mediaType, params))
+	return params["name"], params["filename"]
+}
+
 func (rew *rewriter) handleFilePart(ctx context.Context, name string, p *multipart.Part, opts *filestore.SaveFileOpts) error {
 	if rew.filter.Count() >= maxFilesAllowed {
 		return ErrTooManyFilesUploaded

workhorse/internal/upload/rewrite_test.go

 package upload
 
 import (
+	"net/textproto"
 	"os"
 	"runtime"
 	"testing"
@@ -54,3 +55,83 @@ func TestImageTypeRecongition(t *testing.T) {
 		})
 	}
 }
+
+func TestParseAndNormalizeContentDisposition(t *testing.T) {
+	tests := []struct {
+		desc            string
+		header          string
+		name            string
+		filename        string
+		sanitizedHeader string
+	}{
+		{
+			desc:            "without content disposition",
+			header:          "",
+			name:            "",
+			filename:        "",
+			sanitizedHeader: "",
+		}, {
+			desc:            "content disposition without filename",
+			header:          `form-data; name="filename"`,
+			name:            "filename",
+			filename:        "",
+			sanitizedHeader: `form-data; name=filename`,
+		}, {
+			desc:            "with filename",
+			header:          `form-data; name="file"; filename=foobar`,
+			name:            "file",
+			filename:        "foobar",
+			sanitizedHeader: `form-data; filename=foobar; name=file`,
+		}, {
+			desc:            "with filename*",
+			header:          `form-data; name="file"; filename*=UTF-8''bar`,
+			name:            "file",
+			filename:        "bar",
+			sanitizedHeader: `form-data; filename=bar; name=file`,
+		}, {
+			desc:            "filename and filename*",
+			header:          `form-data; name="file"; filename=foobar; filename*=UTF-8''bar`,
+			name:            "file",
+			filename:        "bar",
+			sanitizedHeader: `form-data; filename=bar; name=file`,
+		}, {
+			desc:            "with empty filename",
+			header:          `form-data; name="file"; filename=""`,
+			name:            "file",
+			filename:        "",
+			sanitizedHeader: `form-data; filename=""; name=file`,
+		}, {
+			desc:            "with complex filename*",
+			header:          `form-data; name="file"; filename*=UTF-8''viel%20Spa%C3%9F`,
+			name:            "file",
+			filename:        "viel Spaß",
+			sanitizedHeader: `form-data; filename*=utf-8''viel%20Spa%C3%9F; name=file`,
+		}, {
+			desc:            "with unsupported charset",
+			header:          `form-data; name="file"; filename*=UTF-16''bar`,
+			name:            "file",
+			filename:        "",
+			sanitizedHeader: `form-data; name=file`,
+		}, {
+			desc:            "with filename and filename* with unsupported charset",
+			header:          `form-data; name="file"; filename=foobar; filename*=UTF-16''bar`,
+			name:            "file",
+			filename:        "foobar",
+			sanitizedHeader: `form-data; filename=foobar; name=file`,
+		},
+	}
+
+	for _, testCase := range tests {
+		t.Run(testCase.desc, func(t *testing.T) {
+			h := make(textproto.MIMEHeader)
+			h.Set("Content-Disposition", testCase.header)
+			h.Set("Content-Type", "application/octet-stream")
+
+			name, filename := parseAndNormalizeContentDisposition(h)
+
+			require.Equal(t, testCase.name, name)
+			require.Equal(t, testCase.filename, filename)
+			require.Equal(t, testCase.sanitizedHeader, h.Get("Content-Disposition"))
+		})
+	}
+}

workhorse/internal/upload/uploads_test.go

 package upload
 
 import (
+	"bufio"
 	"bytes"
 	"context"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"mime/multipart"
 	"net/http"
 	"net/http/httptest"
+	"net/textproto"
 	"os"
 	"regexp"
 	"strconv"
@@ -384,6 +387,89 @@ func TestInvalidFileNames(t *testing.T) {
 	}
 }
 
+func TestContentDispositionRewrite(t *testing.T) {
+	testhelper.ConfigureSecret()
+
+	tempPath, err := ioutil.TempDir("", "uploads")
+	require.NoError(t, err)
+	defer os.RemoveAll(tempPath)
+
+	tests := []struct {
+		desc            string
+		header          string
+		code            int
+		sanitizedHeader string
+	}{
+		{
+			desc:            "with name",
+			header:          `form-data; name="foo"`,
+			code:            200,
+			sanitizedHeader: `form-data; name=foo`,
+		},
+		{
+			desc:            "with name and name*",
+			header:          `form-data; name="foo"; name*=UTF-8''bar`,
+			code:            200,
+			sanitizedHeader: `form-data; name=bar`,
+		},
+		{
+			desc:            "with name and invalid name*",
+			header:          `form-data; name="foo"; name*=UTF-16''bar`,
+			code:            200,
+			sanitizedHeader: `form-data; name=foo`,
+		},
+	}
+
+	for _, testCase := range tests {
+		t.Run(testCase.desc, func(t *testing.T) {
+			h := make(textproto.MIMEHeader)
+			h.Set("Content-Disposition", testCase.header)
+			h.Set("Content-Type", "application/octet-stream")
+
+			buffer := &bytes.Buffer{}
+			writer := multipart.NewWriter(buffer)
+			file, err := writer.CreatePart(h)
+			require.NoError(t, err)
+			fmt.Fprint(file, "test")
+			writer.Close()
+
+			httpRequest := httptest.NewRequest("POST", "/example", buffer)
+			httpRequest.Header.Set("Content-Type", writer.FormDataContentType())
+
+			var upstreamRequestBuffer bytes.Buffer
+			customHandler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
+				r.Write(&upstreamRequestBuffer)
+			})
+
+			response := httptest.NewRecorder()
+			apiResponse := &api.Response{TempPath: tempPath}
+			preparer := &DefaultPreparer{}
+			opts, _, err := preparer.Prepare(apiResponse)
+			require.NoError(t, err)
+
+			HandleFileUploads(response, httpRequest, customHandler, apiResponse, &SavedFileTracker{Request: httpRequest}, opts)
+
+			upstreamRequest, err := http.ReadRequest(bufio.NewReader(&upstreamRequestBuffer))
+			require.NoError(t, err)
+
+			reader, err := upstreamRequest.MultipartReader()
+			require.NoError(t, err)
+
+			for i := 0; ; i++ {
+				p, err := reader.NextPart()
+				if err == io.EOF {
+					require.Equal(t, i, 1)
+					break
+				}
+				require.NoError(t, err)
+				require.Equal(t, testCase.sanitizedHeader, p.Header.Get("Content-Disposition"))
+			}
+
+			require.Equal(t, testCase.code, response.Code)
+		})
+	}
+}
+
 func TestUploadHandlerRemovingExif(t *testing.T) {
 	content, err := ioutil.ReadFile("exif/testdata/sample_exif.jpg")
 	require.NoError(t, err)