597706 - Response Smuggling - Dealing with multiple Content-Length Headers

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Patrick McManus [:mcmanus]]

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

[u7736]

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 [:bzbarsky]]

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

[u7736]

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]]

Attachment: reject transaction with multiple non identical or invalid content-length headers

Comment 5

[Patrick McManus [:mcmanus]]

Attachment: 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]]

From: http://www.w3.org/mid/AANLkTi=[e-mail]

"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

[u7736]

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]]

(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]

(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

[u7736]

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]]

Attachment: reject transaction with multiple non identical or invalid content-length headers

update bitrot, confrom better to style guide, etc..

Comment 12

[Patrick McManus [:mcmanus]]

Attachment: reject transaction with multiple non identical or invalid content-length headers v4

Apply to response header only

Comment 13

[Honza Bambas (:mayhemer)]

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]]

Attachment: 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)]

Attachment: 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)]

This particular bug is not dependent on Content-MD5 check bug.

Comment 17

[Honza Bambas (:mayhemer)]

Attachment: 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]]

http://hg.mozilla.org/mozilla-central/rev/42d996c34679

Comment 19

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Where is the test?

Comment 20

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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]]

Attachment: a unit test case v1

Comment 22

[Jason Duell]

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]

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]]

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.