236084 - Add a Content-Length header for each binary part in "multipart/form-data"

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, defect)

Description

[Ralf Hauser]

User-Agent:       
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:) Gecko/20040226

this allows a more targeted DOS prevention especially when the excessive upload
is unintentional.
see also http://nagoya.apache.org/bugzilla/show_bug.cgi?id=27327 for the
corresponding server-side mechanisms needed to benefit from this.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Actual Results:  
.

Expected Results:  
.

.

Comment 1

[Ralf Hauser]

i.e. fields transmitted prior to the field of offending size can be processed at
least for the purpose of a targeted/graceful error message by the server

Comment 2

[Gervase Markham [:gerv]]

This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/

Comment 3

[Gervase Markham [:gerv]]

This bug has been automatically resolved after a period of inactivity (see above
comment). If anyone thinks this is incorrect, they should feel free to reopen it.

Comment 5

[Wilfried Goesgens]

I can confirm this is still around.

Comment 6

[KUSHAGRA VARADE]

I want to work on this bug may i know what i the actual problem

Comment 7

[Wilfried Goesgens]

if you have a multipart form data post, simply put content-length: - header for each item into it.

Comment 8

[Giorgio Maone [:ma1]]

(In reply to KUSHAGRA VARADE from comment #6)
> I want to work on this bug may i know what i the actual problem

The actual problem is that there's no way to parse the data skipping over potentially gigabyte-long binary parts originating from file / blob uploads.

Comment 9

[Giorgio Maone [:ma1]]

CCing some Necko guys even though this is a different, unowned module, just in case they can see some side effect not apparent to my inexperienced eyes.

Comment 10

[Giorgio Maone [:ma1]]

Attachment: MozReview Request: Bug 236084 - Content-Length on binary form parts. r?kmag

Review commit: https://reviewboard.mozilla.org/r/47657/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47657/

Comment 11

[Giorgio Maone [:ma1]]

I'm gonna add tests for this and for bug 442010 in bug 1201979, rather than duplicating around.

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8743232 [details]
MozReview Request: Bug 236084 - Content-Length on binary form parts. r?kmag

https://reviewboard.mozilla.org/r/47657/#review44495

::: dom/html/nsFormSubmission.cpp:545
(Diff revision 1)
> +      mPostDataChunk.AppendLiteral(CRLF);
>        error.SuppressException();
>      } else {
> +      mPostDataChunk.AppendLiteral("Content-Length: ");
> +      mPostDataChunk.AppendInt(size);
> +      mPostDataChunk += NS_LITERAL_CSTRING(CRLF CRLF);

`.AppendLiteral(CRLF CRLF)`

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8743232 [details]
MozReview Request: Bug 236084 - Content-Length on binary form parts. r?kmag

This looks OK to me, but it needs review by a DOM or necko peer.

Comment 14

[Giorgio Maone [:ma1]]

(In reply to Kris Maglione [:kmag] from comment #13)
> Comment on attachment 8743232 [details]
> MozReview Request: Bug 236084 - Content-Length on binary form parts. r?kmag
> 
> This looks OK to me, but it needs review by a DOM or necko peer.

Thank you, in the meanwhile the try run told me about the bunch of tests which rely on the headers being 2 rather than 3, so I'm gonna fix those as well and therefore implement local tests for this bug as well.

Comment 15

[Patrick McManus [:mcmanus]]

https://reviewboard.mozilla.org/r/47657/#review44515

I'm not going to outright r- this, but it makes me nervous. As far as I can tell the definitions of the MIME type don't define what Content-Length would mean on multipart headers, they rely exclusively on boundaries for demarcation. I also know from experience that there is a lot of implementation confusion around the single content-length (that belongs to the HTTP message) and how it applies to the individual multiparts already.. I feel like adding more content-lengths in here could lead to weird interop issues. There is also a whole class of devices that scan for tokens like Content-Length to get resynchronized when sniffing or dealing with poorly formatted messages (i.e. HTTP 0.9).

What I would want to see is a consumer for the information.. over in bug 1201979 you propose a different part of gecko as the consumer which is reasonable enough, but could also be done with the inclusion of a less risky header (e.g. sec-subpart-content-length). There is a mention of apache in comment 0 but the link is dead. If we're minting something new here I would argue for doing it with a new name - if we're going for interop then its a harder question.

Comment 16

[Giorgio Maone [:ma1]]

(In reply to Patrick McManus [:mcmanus] from comment #15)

Thank you for looking at this.

> What I would want to see is a consumer for the information.. over in bug
> 1201979 you propose a different part of gecko as the consumer which is
> reasonable enough, but could also be done with the inclusion of a less risky
> header (e.g. sec-subpart-content-length). 

OK, then I'm gonna check whether the alternate strategy based on adding a getter to nsIMimeInputStream actually works (this one for sure would): if it does, I'm gonna close this bug as "WONTFIX" for now, otherwise I'm gonna rename the header into "X-Subpart-Content-Length" and fix the aforementioned tests.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8743232 [details]
MozReview Request: Bug 236084 - Content-Length on binary form parts. r?kmag

https://reviewboard.mozilla.org/r/47657/#review44559

The spec says
"  The multipart/form-data media type does not support any MIME header
   fields in parts other than Content-Type, Content-Disposition, and (in
   limited circumstances) Content-Transfer-Encoding.  Other header
   fields MUST NOT be included and MUST be ignored."

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8743232 [details]
MozReview Request: Bug 236084 - Content-Length on binary form parts. r?kmag

The specs being
https://html.spec.whatwg.org/multipage/forms.html#form-submission-algorithm:attr-fs-enctype-formdata
and
https://tools.ietf.org/html/rfc7578

If you want to change the spec, please file spec bugs.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Does any other browser use Content-Length in this case?

Comment 20

[Giorgio Maone [:ma1]]

Opened bug 1266184 with patch attached, closing this one per comment 16.