Last Comment Bug 597706 - Response Smuggling - Dealing with multiple Content-Length Headers
: Response Smuggling - Dealing with multiple Content-Length Headers
Status: RESOLVED FIXED
[sg:want]
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: unspecified
: x86_64 Linux
: -- enhancement with 2 votes (vote)
: ---
Assigned To: Patrick McManus [:mcmanus]
:
:
Mentors:
Depends on: 719259
Blocks: 599164 603503 646507 655389 pipelining-review
  Show dependency treegraph
 
Reported: 2010-09-18 09:51 PDT by Patrick McManus [:mcmanus]
Modified: 2012-01-18 15:47 PST (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
reject transaction with multiple non identical or invalid content-length headers (10.88 KB, patch)
2010-10-06 11:28 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
reject transaction with multiple non identical or invalid content-length headers (10.87 KB, patch)
2010-10-11 09:20 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
reject transaction with multiple non identical or invalid content-length headers (10.91 KB, patch)
2010-12-03 15:22 PST, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
reject transaction with multiple non identical or invalid content-length headers v4 (14.54 KB, patch)
2011-02-18 18:32 PST, Patrick McManus [:mcmanus]
honzab.moz: review+
Details | Diff | Splinter Review
reject transaction with multiple non identical or invalid content-length headers v5 (14.96 KB, patch)
2011-04-07 22:47 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
reject transaction with multiple non identical or invalid content-length headers v5 (merged) (14.89 KB, patch)
2011-05-29 10:28 PDT, Honza Bambas (:mayhemer)
no flags Details | Diff | Splinter Review
reject transaction with multiple non identical or invalid content-length headers v5 (merged2) (15.81 KB, patch)
2011-05-30 05:27 PDT, Honza Bambas (:mayhemer)
no flags Details | Diff | Splinter Review
a unit test case v1 (2.26 KB, patch)
2011-06-01 08:40 PDT, Patrick McManus [:mcmanus]
jduell.mcbugs: review+
Details | Diff | Splinter Review

Description Patrick McManus [:mcmanus] 2010-09-18 09:51:57 PDT
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?
Comment 1 Mark Nottingham 2010-09-19 22:59:14 PDT
See also:
  http://trac.tools.ietf.org/wg/httpbis/trac/ticket/95

There's some current discussion about refining this on-list.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2010-09-19 23:02:41 PDT
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.
Comment 3 Mark Nottingham 2010-09-20 02:28:47 PDT
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?
Comment 4 Patrick McManus [:mcmanus] 2010-10-06 11:28:06 PDT
Created attachment 481287 [details] [diff] [review]
reject transaction with multiple non identical or invalid content-length headers
Comment 5 Patrick McManus [:mcmanus] 2010-10-11 09:20:38 PDT
Created attachment 482252 [details] [diff] [review]
reject transaction with multiple non identical or invalid content-length headers

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.
Comment 6 Patrick McManus [:mcmanus] 2010-10-13 09:34:52 PDT
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."
Comment 7 Mark Nottingham 2010-10-19 17:24:31 PDT
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?
Comment 8 Patrick McManus [:mcmanus] 2010-10-19 19:09:22 PDT
(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 Julian Reschke 2010-10-21 06:17:47 PDT
(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 Mark Nottingham 2010-10-21 15:43:47 PDT
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.
Comment 11 Patrick McManus [:mcmanus] 2010-12-03 15:22:20 PST
Created attachment 495138 [details] [diff] [review]
reject transaction with multiple non identical or invalid content-length headers

update bitrot, confrom better to style guide, etc..
Comment 12 Patrick McManus [:mcmanus] 2011-02-18 18:32:05 PST
Created attachment 513668 [details] [diff] [review]
reject transaction with multiple non identical or invalid content-length headers v4

Apply to response header only
Comment 13 Honza Bambas (:mayhemer) 2011-03-30 09:28:57 PDT
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
Comment 14 Patrick McManus [:mcmanus] 2011-04-07 22:47:12 PDT
Created attachment 524579 [details] [diff] [review]
reject transaction with multiple non identical or invalid content-length headers v5

updates from comment 13 - carry fwd r=honzab
Comment 15 Honza Bambas (:mayhemer) 2011-05-29 10:28:19 PDT
Created attachment 535946 [details] [diff] [review]
reject transaction with multiple non identical or invalid content-length headers v5 (merged)

This is merged to current m-c.  This can be landed even w/o the Content-MD5 check patch.
Comment 16 Honza Bambas (:mayhemer) 2011-05-29 10:29:27 PDT
This particular bug is not dependent on Content-MD5 check bug.
Comment 17 Honza Bambas (:mayhemer) 2011-05-30 05:27:51 PDT
Created attachment 536058 [details] [diff] [review]
reject transaction with multiple non identical or invalid content-length headers v5 (merged2)

Apparently is using the new NS_ERROR_CORRUPTED_CONTENT.  Its declaration added to the patch.
Comment 18 Patrick McManus [:mcmanus] 2011-05-31 17:33:34 PDT
http://hg.mozilla.org/mozilla-central/rev/42d996c34679
Comment 19 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-05-31 18:01:19 PDT
Where is the test?
Comment 20 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-05-31 18:13:19 PDT
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.
Comment 21 Patrick McManus [:mcmanus] 2011-06-01 08:40:10 PDT
Created attachment 536631 [details] [diff] [review]
a unit test case v1
Comment 22 Jason Duell [:jduell] (needinfo me) 2011-06-02 15:51:27 PDT
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).
Comment 23 Jason Duell [:jduell] (needinfo me) 2011-06-02 16:02:34 PDT
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 :)
Comment 24 Patrick McManus [:mcmanus] 2011-06-03 07:48:31 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.