704227 - Corrupted Content Error in the router's web interface

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[motz]

Attachment: http request/response headers

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:8.0) Gecko/20100101 Firefox/8.0
Build ID: 20111104165243

Steps to reproduce:

Accessed my D-Link router's web-admin interface. 


Actual results:

Got a "Corrupted Content Error" 


Expected results:

a basic authorization window request and a normal web-interface.
It was working on all previous Firefox versions.

Comment 1

[Matthias Versen [:Matti]]

Do you have the "do not track" feature enabled in the Firefox options ?

Comment 2

[motz]

I tried to select/unselect this option, firefox gives the same error.
I tried to delete profile and create a new one too - hopeless.

Comment 3

[Matthias Versen [:Matti]]

Can you either attach (via the "add an attachment link") a http log or a packet trace ?

Comment 4

[Matthias Versen [:Matti]]

I somehow missed your attachment....

"Content-Length: -1" could be the culprit. A negative content-length doesn't look right and we added AFAIK some checks in this area due to security problems.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yeah, Content-Length: -1 is invalid per spec...  Patrick, Jason, thoughts?  I hate broken router firmware.... :(

Comment 6

[Patrick McManus [:mcmanus]]

(In reply to Boris Zbarsky (:bz) from comment #5)
> Yeah, Content-Length: -1 is invalid per spec...  Patrick, Jason, thoughts? 
> I hate broken router firmware.... :(

Honestly, as long as it is the only content-length header in the response then I'm ok with letting it go. That's not smuggling. If there are 2 headers with anything except identical values I would want to throw the error.

same with stuff ending in semis or pretty much any other syntactical brokenness as long as it is the only header.

I know there were some implementation issues around accepting some of those things that ended up being mapped to "" and smuggling though - jason would be the go to guy on that.

Comment 8

[Patrick McManus [:mcmanus]]

I think some portions of the content-length changes were a bit of an overreach. Where we see something that looks like a smuggling attack the changes are justified, but where there are just framing problems that we've always muddled along with we should continue to do so (or in this case, resume doing so) barring a more targeted strategy.

I'll add a patch that
 * treats negative CL as rely-on-eof which is what we did previously
 * enforces that negative CL or blank CL cannot be mixed with explicit valid CL as that does look like smuggling
 * leaves the reject-non-identical cl headers logic intact as that can be smuggling
 * adds 7 test cases around this

Comment 9

[Patrick McManus [:mcmanus]]

Attachment: patch 0

Comment 10

[Jason Duell]

Comment on attachment 595444 [details] [diff] [review]
patch 0

Review of attachment 595444 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpHeaderArray.cpp
@@ +92,5 @@
>              if (HeaderMustHaveValue(header)) {
>                  return NS_ERROR_CORRUPTED_CONTENT;
>              }
> +            if (!TrackEmptyHeader(header)) {
> +                return NS_OK; // ignore empty headers by default

worth a LOG that we're ignoring empty header?

@@ +98,1 @@
>          }

So this will allow GetResponseHeader to return empty headers for the first time.  I don't *think* that will cause any trouble (I took out one assert that checked for this from nsHttpResponseHead::UpdateHeaders a while ago).   They will probably be cached, too--try a cached load to make sure that doesn't break anything.

@@ +101,5 @@
>              return NS_ERROR_OUT_OF_MEMORY;
>          entry->header = header;
>          entry->value = value;
>      } else if (!IsSingletonHeader(header)) {
>          MergeHeader(header, entry, value);

Maybe throw in a LOG a little below this when we silently ignore 2nd and following values of some headers (i.e when we don't barf from IsSuspectDuplicateHeader())?

::: netwerk/protocol/http/nsHttpResponseHead.cpp
@@ +213,5 @@
>      // handle some special case headers...
>      if (hdr == nsHttp::Content_Length) {
>          PRInt64 len;
>          // permit only a single value here.
>          if (nsHttp::ParseInt64(val, &len)) {

My understanding is that this will return 0 for "Content-Length: 64;", right?  I suggest we instead return whatever leading numeric value is present (see my one-line patch for this in bug 683699), ie 64.

Comment 11

[Patrick McManus [:mcmanus]]

> worth a LOG that we're ignoring empty header?

ok

> They will probably be cached, too--try a cached load to make sure that
> doesn't break anything.
> 

this worked ok - but I added a test to cover it.

> Maybe throw in a LOG a little below this when we silently ignore 2nd and

ok

> My understanding is that this will return 0 for "Content-Length: 64;",
> right?  I suggest we instead return whatever leading numeric value is
> present (see my one-line patch for this in bug 683699), ie 64.

good point. I integrated that patch.

Comment 12

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8d16f5bbceb

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b8d16f5bbceb

Comment 14

[Jason Duell]

Comment on attachment 595444 [details] [diff] [review]
patch 0

[Approval Request Comment]

See bug 742174 comment 17.  We need this to get a proper fix for re-allowing empty Location: headers

Regression caused by (bug #): 716801
User impact if declined: Break fair number of websites (inc Oracle PeopleSoft)
Testing completed (on m-c, etc.):  yes, tests in bug 742174 and this bug
Risk to taking this patch (and alternatives if risky): low: small code footprint.
String changes made by this patch: none

Comment 15

[Alex Keybl [:akeybl]]

Comment on attachment 595444 [details] [diff] [review]
patch 0

[Triage Comment]
Approved for Aurora 13 and Beta 12 to prevent web regressions.

Comment 16

[Jason Duell]

Comment on attachment 595444 [details] [diff] [review]
patch 0

This turned out to be on aurora already, so just landed on beta:

  https://hg.mozilla.org/releases/mozilla-beta/rev/9db3e4016c02