Closed
Bug 597706
Opened 14 years ago
Closed 14 years ago
Response Smuggling - Dealing with multiple Content-Length Headers
Categories
(Core :: Networking: HTTP, enhancement)
Tracking
()
RESOLVED
FIXED
People
(Reporter: mcmanus, Assigned: mcmanus)
References
Details
(Whiteboard: [sg:want])
Attachments
(2 files, 6 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
jduell.mcbugs
:
review+
|
Details | Diff | Splinter Review |
The latest draft of HTTPbis <http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-11#section-3.3> adds a new requirement to error when it's apparent response smuggling is happening: > 3. If a message is received without Transfer-Encoding and with > either multiple Content-Length header fields or a single Content- > Length header field with an invalid value, then the message > framing is invalid and MUST be treated as an error to prevent > request or response smuggling. If this is a request message, the > server MUST respond with a 400 (Bad Request) status code and then > close the connection. If this is a response message received by > a proxy or gateway, the proxy or gateway MUST discard the > received response, send a 502 (Bad Gateway) status code as its > downstream response, and then close the connection. If this is a > response message received by a user-agent, the message-body > length is determined by reading the connection until it is > closed; an error SHOULD be indicated to the user. FF will use the last Content-Length in the message to determine the length, and not show any error to the user. Apparently other browsers use the first C-L. I think if the response contains more than 1 different content-length value (i.e. I don't care if there are multiple CL's with the same value) that the right behavior is indeed to throw an error and close the connection. I reason 1] This is a classic response smuggling signature. Something fishy is likely going on. 2] The existing different behavior between browsers indicates we likely aren't breaking anything innocent and significant 3] rfc 2616 implicitly disallows this kind of thing - often talking about the decimal value of the header which doesn't make any sense if you combine them together somehow. what do you folks think?
See also: http://trac.tools.ietf.org/wg/httpbis/trac/ticket/95 There's some current discussion about refining this on-list.
![]() |
||
Comment 2•14 years ago
|
||
I'd be fine with trying that on trunk once 4.0 branches. It's too late in the 4.0 cycle for changes like this that need a long time to shake out, imo.
Adam Barth pointed out that it would be nice to get some numbers on how much of the Web this will break -- would it be possible / interesting to run a Test Pilot study to see how common duplicated / multiple Content-Length headers are?
Assignee | ||
Comment 4•14 years ago
|
||
Assignee | ||
Comment 5•14 years ago
|
||
prev version of patch accidentally required 'merge' attribute of set header to be set even in i-meant-to-overwrite it cases. whoops - fix that.
Attachment #481287 -
Attachment is obsolete: true
Attachment #482252 -
Flags: review?(honzab.moz)
Attachment #481287 -
Flags: review?(jduell.mcbugs)
Assignee | ||
Comment 6•14 years ago
|
||
From: http://www.w3.org/mid/AANLkTi=8faPdrt+gVFedHSM7a26_VuVRP2E6Xx__GsYP@mail.gmail.com "FWIW, on Windows Chrome 7.0.536.2 (a recent dev channel release), .0009% of main frames (where an error would result in a user-visible Chrome network error page, which we don't show for subresources) have responses with multiple content-lengths."
FYI - Chrome currently rejects duplicate values; i.e., they don't check to see if the values are equal. It would be good if there was consistency between implementations regarding this. Any thoughts?
Assignee | ||
Comment 8•14 years ago
|
||
(In reply to comment #7) > FYI - Chrome currently rejects duplicate values; i.e., they don't check to see > if the values are equal. > > It would be good if there was consistency between implementations regarding > this. Any thoughts? what is the security argument for rejecting duplicates headers?
Comment 9•14 years ago
|
||
(In reply to comment #8) > what is the security argument for rejecting duplicates headers? I think security is risk when different recipients disagree on the actual message length. A duplicate header is invalid (just like a comma-separated list of values), so some implementations may ignore the header, reading up to the end of stream. Not sure whether this is a problem in practice.
Comment 10•14 years ago
|
||
AFAICT the implementations honour one of the headers, none currently reads to end of stream. I think that if implementers can agree that duplicates are OK, that's fine; we just need to drive this to see if we can get agreement.
Assignee | ||
Comment 11•14 years ago
|
||
update bitrot, confrom better to style guide, etc..
Attachment #482252 -
Attachment is obsolete: true
Attachment #495138 -
Flags: review?(honzab.moz)
Attachment #482252 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 12•14 years ago
|
||
Apply to response header only
Attachment #495138 -
Attachment is obsolete: true
Attachment #513668 -
Flags: review?(honzab.moz)
Attachment #495138 -
Flags: review?(honzab.moz)
![]() |
||
Comment 13•14 years ago
|
||
Comment on attachment 513668 [details] [diff] [review] reject transaction with multiple non identical or invalid content-length headers v4 >+PRBool >+nsHttpHeaderArray::CanOverwriteHeader(nsHttpAtom header) >+{ >+ if (header == nsHttp::Content_Length && >+ mType == HTTP_RESPONSE_HEADER) >+ return PR_FALSE; >+ return PR_TRUE; >+} I'd reformat this method the same was as nsHttpHeaderArray::CanAppendToHeader is: +nsHttpHeaderArray::CanOverwriteHeader(nsHttpAtom header) +{ + if (mType != HTTP_RESPONSE_HEADER) + return PR_TRUE; + + return header != nsHttp::Content_Length; +} >diff --git a/netwerk/protocol/http/nsHttpHeaderArray.h b/netwerk/protocol/http/nsHttpHeaderArray.h >+ enum nsHttpHeaderType { >+ HTTP_REQUEST_HEADER, >+ HTTP_RESPONSE_HEADER >+ }; Hmm.. As this is a property of an array (headers), shouldn't the enums be HTTP_REQUEST_HEADERS, HTTP_RESPONSE_HEADERS (plural) ? If we ever have an enum for marking just a single header, people might get confused what enum to use. >+ nsHttpHeaderType mType; You should serialize this new member in http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/PHttpChannelParams.h#142. It is not currently send separately between processes (AFAIK), so it can be done in a different bug. I'll file it and add dep. >diff --git a/netwerk/protocol/http/nsHttpRequestHead.h b/netwerk/protocol/http/nsHttpRequestHead.h > class nsHttpRequestHead > { > public: >- nsHttpRequestHead() : mMethod(nsHttp::Get), mVersion(NS_HTTP_VERSION_1_1) {} >+ nsHttpRequestHead() : mHeaders(nsHttpHeaderArray::HTTP_REQUEST_HEADER) >+ , mMethod(nsHttp::Get) >+ , mVersion(NS_HTTP_VERSION_1_1) {} > ~nsHttpRequestHead() {} Is the constructor indented wrongly or is it again some problem on my win7 screen? >diff --git a/netwerk/protocol/http/nsHttpResponseHead.h b/netwerk/protocol/http/nsHttpResponseHead.h > class nsHttpResponseHead > { > public: >- nsHttpResponseHead() : mVersion(NS_HTTP_VERSION_1_1) >+ nsHttpResponseHead() : mHeaders(nsHttpHeaderArray::HTTP_RESPONSE_HEADER) >+ , mVersion(NS_HTTP_VERSION_1_1) > , mStatus(200) As well here? Otherwise looks good. r=honzab
Attachment #513668 -
Flags: review?(honzab.moz) → review+
Assignee | ||
Comment 14•14 years ago
|
||
updates from comment 13 - carry fwd r=honzab
Attachment #513668 -
Attachment is obsolete: true
![]() |
||
Updated•14 years ago
|
Blocks: pipelining-review
![]() |
||
Comment 15•14 years ago
|
||
This is merged to current m-c. This can be landed even w/o the Content-MD5 check patch.
Attachment #524579 -
Attachment is obsolete: true
![]() |
||
Updated•14 years ago
|
Attachment #535946 -
Flags: review+
![]() |
||
Comment 16•14 years ago
|
||
This particular bug is not dependent on Content-MD5 check bug.
No longer depends on: 232030
![]() |
||
Comment 17•14 years ago
|
||
Apparently is using the new NS_ERROR_CORRUPTED_CONTENT. Its declaration added to the patch.
Attachment #535946 -
Attachment is obsolete: true
![]() |
||
Updated•14 years ago
|
Attachment #535946 -
Flags: review+
Assignee | ||
Comment 18•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/42d996c34679
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 20•14 years ago
|
||
I am reopening only because the testcase wasn't included in the patch and I couldn't find another existing test that would cover this. If there's already a testcase in the tree, please include a link to the testcase and re-close.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 22•14 years ago
|
||
Comment on attachment 536631 [details] [diff] [review] a unit test case v1 Review of attachment 536631 [details] [diff] [review]: ----------------------------------------------------------------- Good enough for now. I'm expanding this test as part of some other duplicate header bugs I'm working on, but this covers the Clen case well enough. (thanks for figuring out how to emit duplicate headers from httpd.js--that was stumping me).
Attachment #536631 -
Flags: review?(bsmith) → review+
Comment 23•14 years ago
|
||
Note that right now when we run into our new error code (NS_ERROR_CORRUPTED_CONTENT) we don't show an error page to the user--we just do nothing if a URL is typed into the location bar and it hits multiple C-lens. Honza tells me the UI part is part of the patch in bug 232030 (IIRC). That's unfortunate--if that bug will land soon, I guess we can wait. Otherwise, let's pull that part out and land it as part of this. We also don't have an e10s _wrap test, nor does the patch that landed register NS_ERROR_CORRUPTED_CONTENT with xpcshell so we can test for it in unit tests. I've got those items done in my other patch (which will probably show up in bug 655389), so don't worry about them here. Also don't worry about copying nsHttpHeaderArray::mType to the child--I'm planning to remove mType :)
Assignee | ||
Comment 24•14 years ago
|
||
test case landed as http://hg.mozilla.org/mozilla-central/rev/46faa246d6af I'll split up the CORRUPTED_CONTENT bits from 232030 over in that bug.
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Whiteboard: [sg:want]
You need to log in
before you can comment on or make changes to this bug.
Description
•