proposal: mime/multipart: add method Reader.SetRejectContentTransferEncoding(bool) to support net/http rejecting multipart form parts with "Content-Transfer-Encoding" header which is explicitly deprecated by RFC 7578 Section 4.7

[odeke-em]

Proposal Details

Suggestion

This issue proposes adding the method Reader.SetRejectContentTransferEncoding(v bool) which if set, serves the purpose of rejecting multipart form parts that contain the "Content-Transfer-Encoding" header for binary data, including HTTP and that serves to fix security issue #63855

Motivation

To fix net/http security bug #63855, for multipart requests whose content is streamed and not yet ready until one invokes req.MultipartReader() or req.ParseMultipartForm(MAX), it is inefficient and non-elegant to trawl the bytes being streamed in. Instead it is much better to a method on the Reader which will be set by the net/http caller.

// SetRejectContentTransferEncoding toggles whether to decode parts if
// the "Content-Transfer-Encoding" header is present or not.
// If set to true, trying to decode a part will return an error if that header is present.
func (r *Reader) SetRejectContentTransferEncoding(v bool) {
        r.rejectContentTransferEncoding = v
}

Rationale

Internet Engineering Task Force (IETF) RFC 7578 circa 2015 is the current/prevailing advice for parsing multipart/form-data and it obsoletes the prior standing RFC 2388 from 1998.
RFC 7578 Section 4.7 recommends that we should reject multipart/form-data carrying the header "Content-Transfer-Encoding" and it mentions that currently no deployments supporting that legacy configuration have been discovered.

Action

I have CLs (CL 573195 and 573196) available to smoothly experiment and show how this would appear ergonomically.

@neild who was advised by the bug reporters Qi Wang and Jianjun Chen, mentions that not rejecting such rejects can serve as a content smuggling vector and the current state of affairs would allow this code to print out "Joe owes €100." per
https://go.dev/play/p/wG5hidF_Uhh or inlined below

package main

import (
	"io"
	"net/http"
	"strings"
)

func main() {
	req := &http.Request{
		Header: http.Header{"Content-Type": {`multipart/form-data; boundary=xxx`}},
		Body: io.NopCloser(strings.NewReader(`--xxx
Content-Disposition: form-data; name="file"
Content-Type: text/plain
content-transfer-encoding: quoted-printable

Joe owes =E2=82=AC100
--xxx--`)),
	}

	mr, err := req.MultipartReader()
	if err != nil {
		panic(err)
	}

	part, err := mr.NextPart()
	if err != nil {
		panic(err)
	}
	defer part.Close()
	blob, err := io.ReadAll(part)
	if err != nil {
		panic(err)
	}
	println(string(blob))
}

[gopherbot]

Change https://go.dev/cl/573195 mentions this issue: mime/multipart: add Reader.SetRejectContentTransferEncoding

[neild]

#63855 proposes that http.Request.ParseMultipartForm should reject multipart form parts with Content-Transfer-Encoding headers. (See that issue for more background.)

The net/http package uses the mime/multipart package to parse forms.

mime/multipart.Reader automatically decodes parts with a Content-Transfer-Encoding: quoted-printable header. The Reader.NextRawPart method decodes a part without applying this automatic decoding, but:

• net/http.Request.ParseMultipartForm uses Reader.ReadForm, which always decodes quoted-printable parts; and
• net/http.Request.MultipartReader returns a *mime/multipart.Reader, and cannot ensure that users call NextRawPart rather than NextPart.

Therefore, if we want to make this the default behavior in net/http we need a way to tell mime/multipart not to decode these parts. That could take the form of a new public API (as proposed here), or we could have a std-internal API of some form (which would not require a proposal). A public API is the better choice in general, since this feature is potentially useful in places other than net/http.

I support adding a feature to mime/multipart.Reader to support rejecting parts that include a Content-Transfer-Encoding.

---

mime/multipart.Reader does not currently have any user-customizable options. This would be the first one.

We could add an option either as a method (the proposal above), or as a field on mime/multipart.Reader:

type Reader struct {
  RejectContentTransferEncoding bool

  // existing unexported fields
}

I don't have a strong opinion on this either way. Either seems fine to me.

[odeke-em]

Kind ping @golang/proposal-review as this issue has been open for longer than 1 year and I already have the code ready to go plus the design fleshed out. Thank you.