Last Comment Bug 704227 - Corrupted Content Error in the router's web interface
: Corrupted Content Error in the router's web interface
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: 8 Branch
: x86 Windows XP
: -- normal (vote)
: mozilla13
Assigned To: Patrick McManus [:mcmanus] PTO until Sep 6
:
Mentors:
: 719259 (view as bug list)
Depends on:
Blocks: 742174
  Show dependency treegraph
 
Reported: 2011-11-21 11:22 PST by motz
Modified: 2012-04-09 16:55 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
http request/response headers (773 bytes, text/plain)
2011-11-21 11:22 PST, motz
no flags Details
patch 0 (14.04 KB, patch)
2012-02-08 10:24 PST, Patrick McManus [:mcmanus] PTO until Sep 6
jduell.mcbugs: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description motz 2011-11-21 11:22:43 PST
Created attachment 575919 [details]
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] 2011-11-22 02:40:58 PST
Do you have the "do not track" feature enabled in the Firefox options ?
Comment 2 motz 2011-11-22 02:48:18 PST
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] 2011-11-22 02:54:41 PST
Can you either attach (via the "add an attachment link") a http log or a packet trace ?
Comment 4 Matthias Versen [:Matti] 2011-11-22 02:58:33 PST
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 [:bz] 2011-11-22 05:37:50 PST
Yeah, Content-Length: -1 is invalid per spec...  Patrick, Jason, thoughts?  I hate broken router firmware.... :(
Comment 6 Patrick McManus [:mcmanus] PTO until Sep 6 2011-11-22 05:49:06 PST
(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 7 Patrick McManus [:mcmanus] PTO until Sep 6 2012-02-08 09:32:04 PST
*** Bug 719259 has been marked as a duplicate of this bug. ***
Comment 8 Patrick McManus [:mcmanus] PTO until Sep 6 2012-02-08 09:35:59 PST
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] PTO until Sep 6 2012-02-08 10:24:31 PST
Created attachment 595444 [details] [diff] [review]
patch 0
Comment 10 Jason Duell [:jduell] (needinfo me) 2012-02-08 15:28:20 PST
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] PTO until Sep 6 2012-02-09 08:47:27 PST
> 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] PTO until Sep 6 2012-02-09 11:54:37 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8d16f5bbceb
Comment 13 Ed Morley [:emorley] 2012-02-10 05:01:30 PST
https://hg.mozilla.org/mozilla-central/rev/b8d16f5bbceb
Comment 14 Jason Duell [:jduell] (needinfo me) 2012-04-06 23:10:15 PDT
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] 2012-04-09 09:47:03 PDT
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 [:jduell] (needinfo me) 2012-04-09 16:55:14 PDT
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

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