Response Smuggling - Dealing with multiple Content-Length Headers

RESOLVED FIXED

Status

()

Core
Networking: HTTP
--
enhancement
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:want])

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

7 years ago
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

7 years ago
See also:
  http://trac.tools.ietf.org/wg/httpbis/trac/ticket/95

There's some current discussion about refining this on-list.
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

7 years ago
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

7 years ago
Created attachment 481287 [details] [diff] [review]
reject transaction with multiple non identical or invalid content-length headers
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Attachment #481287 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

7 years ago
Depends on: 232030
(Assignee)

Updated

7 years ago
Blocks: 599164
(Assignee)

Comment 5

7 years ago
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.
Attachment #481287 - Attachment is obsolete: true
Attachment #482252 - Flags: review?(honzab.moz)
Attachment #481287 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

7 years ago
Blocks: 603503
(Assignee)

Comment 6

7 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."

Comment 7

7 years ago
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

7 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

7 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

7 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

7 years ago
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..
Attachment #482252 - Attachment is obsolete: true
Attachment #495138 - Flags: review?(honzab.moz)
Attachment #482252 - Flags: review?(honzab.moz)
(Assignee)

Comment 12

6 years ago
Created attachment 513668 [details] [diff] [review]
reject transaction with multiple non identical or invalid content-length headers v4

Apply to response header only
Attachment #495138 - Attachment is obsolete: true
Attachment #513668 - Flags: review?(honzab.moz)
Attachment #495138 - Flags: review?(honzab.moz)
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+
Blocks: 646507
(Assignee)

Comment 14

6 years ago
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
Attachment #513668 - Attachment is obsolete: true
Blocks: 659760
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.
Attachment #524579 - Attachment is obsolete: true
Attachment #535946 - Flags: review+
This particular bug is not dependent on Content-MD5 check bug.
No longer depends on: 232030
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.
Attachment #535946 - Attachment is obsolete: true
Attachment #535946 - Flags: review+
(Assignee)

Comment 18

6 years ago
http://hg.mozilla.org/mozilla-central/rev/42d996c34679
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Where is the test?
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 → ---
(Assignee)

Comment 21

6 years ago
Created attachment 536631 [details] [diff] [review]
a unit test case v1
Attachment #536631 - Flags: review?(bsmith)
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+
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

6 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
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Whiteboard: [sg:want]
Blocks: 655389
Depends on: 719259
You need to log in before you can comment on or make changes to this bug.