You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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
#66434
Open
odeke-em opened this issue
Mar 21, 2024
· 2 comments
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(vbool) {
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
#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:
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 #63855Motivation
To fix net/http security bug #63855, for multipart requests whose content is streamed and not yet ready until one invokes
req.MultipartReader()
orreq.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.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
The text was updated successfully, but these errors were encountered: