Merge branch '340366_fix_workhorse_content_disposition' into 'master'

Reject MIME parts with unsupported encoding

See merge request gitlab-org/gitlab!77688

workhorse/internal/upload/rewrite.go

@@ -10,6 +10,7 @@ import (
 	"net/http"
 	"os"
 	"path/filepath"
+	"regexp"
 	"strings"
 
 	"github.com/prometheus/client_golang/prometheus"
@@ -30,8 +31,11 @@ const maxFilesAllowed = 10
 var (
 	ErrInjectedClientParam  = errors.New("injected client parameter")
 	ErrTooManyFilesUploaded = fmt.Errorf("upload request contains more than %v files", maxFilesAllowed)
+	ErrUnexpectedFilePart   = errors.New("Content-Disposition contains unexpected filepart")
 )
 
+var filePartRegex = regexp.MustCompile(`;\s*filename\b`)
+
 var (
 	multipartUploadRequests = promauto.NewCounterVec(
 		prometheus.CounterOpts{
@@ -107,6 +111,11 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, pr
 		if p.FileName() != "" {
 			err = rew.handleFilePart(r.Context(), name, p, opts)
 		} else {
+			err = verifyContentDisposition(p)
+			if err != nil {
+				return err
+			}
+
 			err = rew.copyPart(r.Context(), name, p)
 		}
 
@@ -118,6 +127,14 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, pr
 	return nil
 }
 
+func verifyContentDisposition(p *multipart.Part) error {
+	if filePartRegex.MatchString(p.Header.Get("Content-Disposition")) {
+		return ErrUnexpectedFilePart
+	}
+
+	return nil
+}
+
 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 (
+	"mime/multipart"
+	"net/textproto"
 	"os"
 	"runtime"
 	"testing"
@@ -54,3 +56,47 @@ func TestImageTypeRecongition(t *testing.T) {
 		})
 	}
 }
+
+func TestVerifyContentDisposition(t *testing.T) {
+	tests := []struct {
+		desc               string
+		contentDisposition string
+		error              error
+	}{
+		{
+			desc:               "without content disposition",
+			contentDisposition: "",
+			error:              nil,
+		}, {
+			desc:               "content disposition without filename",
+			contentDisposition: `form-data; name="filename"`,
+			error:              nil,
+		}, {
+			desc:               "with filename",
+			contentDisposition: `form-data; name="file"; filename=foobar`,
+			error:              ErrUnexpectedFilePart,
+		}, {
+			desc:               "with filename*",
+			contentDisposition: `form-data; name="file"; filename*=UTF-8''foobar`,
+			error:              ErrUnexpectedFilePart,
+		}, {
+			desc:               "filename and filename*",
+			contentDisposition: `form-data; name="file"; filename=foobar; filename*=UTF-8''foobar`,
+			error:              ErrUnexpectedFilePart,
+		},
+	}
+
+	for _, testCase := range tests {
+		t.Run(testCase.desc, func(t *testing.T) {
+			h := make(textproto.MIMEHeader)
+
+			if testCase.contentDisposition != "" {
+				h.Set("Content-Disposition", testCase.contentDisposition)
+				h.Set("Content-Type", "application/octet-stream")
+			}
+
+			require.Equal(t, testCase.error, verifyContentDisposition(&multipart.Part{Header: h}))
+		})
+	}
+
+}

workhorse/internal/upload/uploads.go

@@ -35,7 +35,7 @@ func HandleFileUploads(w http.ResponseWriter, r *http.Request, h http.Handler, p
 		switch err {
 		case ErrInjectedClientParam:
 			helper.CaptureAndFail(w, r, err, "Bad Request", http.StatusBadRequest)
-		case ErrTooManyFilesUploaded:
+		case ErrTooManyFilesUploaded, ErrUnexpectedFilePart:
 			helper.CaptureAndFail(w, r, err, err.Error(), http.StatusBadRequest)
 		case http.ErrNotMultipart:
 			h.ServeHTTP(w, r)

workhorse/internal/upload/uploads_test.go

@@ -4,10 +4,12 @@ import (
 	"bytes"
 	"context"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"mime/multipart"
 	"net/http"
 	"net/http/httptest"
+	"net/textproto"
 	"os"
 	"regexp"
 	"strconv"
@@ -384,6 +386,99 @@ func TestInvalidFileNames(t *testing.T) {
 	}
 }
 
+func TestContentDisposition(t *testing.T) {
+	testhelper.ConfigureSecret()
+
+	tempPath, err := ioutil.TempDir("", "uploads")
+	require.NoError(t, err)
+	defer os.RemoveAll(tempPath)
+
+	tests := []struct {
+		desc               string
+		contentDisposition string
+		code               int
+		body               string
+	}{
+		{
+			desc:               "empty header",
+			contentDisposition: "",
+			code:               200,
+			body:               "",
+		},
+		{
+			desc:               "without filename",
+			contentDisposition: `form-data; name="filename"`,
+			code:               200,
+			body:               "",
+		},
+		{
+			desc:               "with filename",
+			contentDisposition: `form-data; name="file"; filename=foobar`,
+			code:               200,
+			body:               "",
+		},
+		{
+			desc:               "with filename* supported charset",
+			contentDisposition: `form-data; name="file"; filename*=UTF-8''foobar`,
+			code:               200,
+			body:               "",
+		},
+		{
+			desc:               "with both filename and filename* supported charset",
+			contentDisposition: `form-data; name="file"; filename=foobar; filename*=UTF-8''foobar`,
+			code:               200,
+			body:               "",
+		},
+		{
+			desc:               "with filename and filename* unsupported charset",
+			contentDisposition: `form-data; name="file"; filename=foobar; filename*=UTF-16''foobar`,
+			code:               200,
+			body:               "",
+		},
+		{
+			desc:               "unsupported charset",
+			contentDisposition: `form-data; name="file"; filename*=UTF-16''foobar`,
+			code:               400,
+			body:               ErrUnexpectedFilePart.Error() + "\n",
+		},
+	}
+
+	for _, testCase := range tests {
+		t.Run(testCase.desc, func(t *testing.T) {
+			// Create custom Content-Disposition with filename* and charset
+			// Example: filename*=UTF-8''application.log
+			h := make(textproto.MIMEHeader)
+
+			h.Set("Content-Disposition", testCase.contentDisposition)
+			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())
+
+			response := httptest.NewRecorder()
+			apiResponse := &api.Response{TempPath: tempPath}
+			preparer := &DefaultPreparer{}
+			opts, _, err := preparer.Prepare(apiResponse)
+			require.NoError(t, err)
+
+			HandleFileUploads(response, httpRequest, nilHandler, apiResponse, &SavedFileTracker{Request: httpRequest}, opts)
+
+			body, err := io.ReadAll(response.Body)
+			require.NoError(t, err)
+
+			require.Equal(t, testCase.code, response.Code)
+			require.Equal(t, testCase.body, string(body))
+		})
+	}
+}
+
 func TestUploadHandlerRemovingExif(t *testing.T) {
 	content, err := ioutil.ReadFile("exif/testdata/sample_exif.jpg")
 	require.NoError(t, err)