328817 - Content injection spoofing with Content-Length header overflow

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[Kohei Yoshino [:kohei]]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: N/A

Note: This bug was originally reported to IPA (Information-technology Promotion Agency, Japan) and forwarded to us at Mozilla Japan. I'm NOT original reporter. Don't mention my name in the security advisory. For more information about IPA, visit http://www.ipa.go.jp/about/english/ 

IPA#36721438

Attackers can forge a Web page on another domain by overflowing Content-Length response header. This could be used for the real phishing to steal sensitive data, such as user password or cookies. This bug was originally reported as a vulnerability discovered in Internet Explorer, but Firefox shares the same problem.

Windows XP Professional SP2
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060129 Firefox/1.6a1


Reproducible: Always

Steps to Reproduce:
1. Send and receive two pairs of HTTP requests/responses on the same TCP session with keep-alive of HTTP 1.0 or 1.1.
2. Set HTTP response of first pair to contain Content-Length header over 0x80000301 bytes.

Actual Results:  
The partial HTTP response of first pair is shown as response to the second request.


Expected Results:  
The partial HTTP response of first pair is NOT shown as response to the second request.


The malicious Web site admins can target Firefox (and IE) users who use HTTP proxy (that keep-alive anabled) to steal cookies or build web pages including fake login form for arbitrary domains.

For Firefox (and IE) users who don't use HTTP proxy, the attackers can also exploit same problem for web site with another domain name on the same IP address.

Workaround for site admins: disable keep-alive

More info coming.

Comment 1

[Kohei Yoshino [:kohei]]

Attachment: data flow chart

IPA send us the data flow chart. I just translated it.

Comment 2

[Kohei Yoshino [:kohei]]

Attachment: test suite (zipped)

I attached the testcase provided by original reporter.

==
Testcase 1: testmoz.html and largefilemoz.gif
This code is provided to learn "Steps to Reproduce" noted above.

Steps:
Put these files on the same directory of web server and set image to be executed as cgi. Open testmoz.html in your browser and click the link.

Expected Results:
Linked nonexistent is shown.

Actual Results:
Output of largefilemoz.gif (with bad Content-Length) is shown. Alert dialog popups. Click OK to load HTTP headers in your browser window.

Conclusion:
Firefox thinks the partial HTTP response including bad Content-Length header as the following response in the same connection.

==
Testcase 2: test2moz.html and largefile2moz.gif
This code is provided to reproduce the problem with HTTP proxy that keep-alive enabled.

Steps:
Put these files on the same directory of web server and set image to be executed as cgi. Set your browser to use HTTP proxy that keep-alive enabled. Open test2moz.html in your browser.

Expected Results:
Your browser is redirected to Gmail.

Actual Results:
Your browser is redirected to Gmail but output of largefile2moz.gif is shown.

Conclusion:
Attackers can exploit this bug to insert content to another domain.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

> 0x80000301

urg, that's not supposed to overflow anything :-/

Comment 4

[Kohei Yoshino [:kohei]]

Any updates?

Comment 5

[Darin Fisher]

The Content-Length header value is parsed using PR_sscanf:

  PR_sscanf(val, "%lld", &mContentLength);

I'm investigating further to understand where things go wrong.

Comment 6

[Darin Fisher]

I wrote some test code that does the following:

  PRInt64 len;
  PR_sscanf("18446744073709551618", "%lld", &len);

The resulting value of len is 2.  So, this means that we either need to teach PR_sscanf how to protect against overflow, or we need to teach the HTTP code how to protect itself against overflow.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

oh, so comment 0 was wrong when it mentioned 0x80000301

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

strtoull is probably not portable? :/

Comment 9

[Darin Fisher]

glibc's implementation of sscanf has the property that it will not overflow the result.  if the input string numerically exceeds max long long, then it will return max long long.  that behavior could probably be abused too, but it seems safer than the behavior of PR_sscanf.  i suspect that changing PR_sscanf would not be an acceptable solution because it would mean changing a frozen API.

Comment 10

[Darin Fisher]

> strtoull is probably not portable? :/

My RH9 man pages say that strtoll is defined by POSIX-2001, so yeah... probably not portable enough.

Comment 11

[Darin Fisher]

strtoll also has the property of being locale sensitive (accepting the thousands delimiter for the current locale).  that definitely makes it something we could not use in HTTP land.

Comment 12

[Darin Fisher]

Attachment: v1 patch

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 217116 [details] [diff] [review]
v1 patch

>Index: nsHttp.cpp
>+        (*r) *= 10;
>+        (*r) += delta;
>+        if (*r < delta) // overflow?

So this assumes that for nonzero *r, (*r)*10 can't overflow to zero, right?  I guess that's not an unreasonable assumption, but should we maybe guard against that eventuality?

>Index: nsHttp.h
>The |terminators|
>+    // parameter defines the set of bytes that may terminate the parse stream
>+    // ('\0' is an implicit terminator).

And bytes '0' through '9' are ignored in |terminators|, right?

With those nits picked, looks good.

Comment 14

[Darin Fisher]

Attachment: testcase: python script

Here's a very simple python script that can be used to exercise this bug.  Just run the script and load http://host:4444/ twice.  If you get a popup that says "hacked" then you're browser is vulnerable.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 217116 [details] [diff] [review]
v1 patch

I'm really not sure that HTTP is the right place for code like this. What if other code wants to parse a string into a 64-bit number? like:

/extensions/webservices/soap/src/nsDefaultSOAPEncoder.cpp, line 3345 -- PRInt32 r = PR_sscanf(NS_ConvertUTF16toUTF8(value).get(), " %lld %n", &f, &n);
/netwerk/base/src/nsIncrementalDownload.cpp, line 584 -- if (PR_sscanf(buf.get() + slash + 1, "%lld", (PRInt64 *) &mTotalSize) != 1)
/netwerk/cookie/src/nsCookieService.cpp, line 1056 -- numInts = PR_sscanf(buffer.get() + expiresIndex, "%lld", &expires);
/netwerk/cookie/src/nsCookieService.cpp, line 1200 -- dateLen = PR_snprintf(dateString, sizeof(dateString), "%lld", PRInt64(cookie->Expiry()));
/netwerk/cookie/src/nsCookieService.cpp, line 2001 -- PRInt32 numInts = PR_sscanf(aCookieAttributes.maxage.get(), "%lld", &maxage);
/netwerk/streamconv/converters/nsDirIndexParser.cpp, line 330 -- PRInt32 status = PR_sscanf(value, "%lld", &len);
and a few in history code


Maybe PR_sscanf should fail (return a lower return value, 0 in the cases here)? Or maybe this new function should live in NSPR. Or both.


I guess this will do for now, so if you want to go with this patch:

+++ nsHttp.cpp	4 Apr 2006 03:40:21 -0000
+        if (*r < delta) // overflow?

Hm... does this suffice? Won't a string consisting of a 1 followed by a lot of zeroes fail this test? I think it would be better to compare with the previous value of *r.

+++ nsHttp.h	4 Apr 2006 03:40:21 -0000
+    // This function reads a base-10 non-negative 64-bit integer value.  If the
...
+                             PRInt64 *result);

If this reads a non-negative value, why not use an unsigned integer?

Is there a particular reason for the terminators parameter? Neither of the two callers pass a nonempty value for it.

+++ nsHttpResponseHead.cpp	4 Apr 2006 03:40:21 -0000
         size = LL_MaxUint();

Might want to change this to LL_MAXUINT (probably my fault)

Comment 16

[Darin Fisher]

biesi:

Well, this function is not a good replacement for PR_sscanf("%lld") in some cases.  For example, it does not accept hexidecimal input or negative valued input.  However, I agree that it does seem like a generic routine that should be moved someplace more general, perhaps in xpcom or nspr.  Since I want a patch for the FF 1.5.0.3 release, I need to avoid changing nspr.


> Hm... does this suffice? Won't a string consisting of a 1 followed by a lot of
> zeroes fail this test? I think it would be better to compare with the previous
> value of *r.

1000000000000000000 * 10 ==> -8446744073709551616

That is less than 0, so the test works.


> If this reads a non-negative value, why not use an unsigned integer?

The consumers want to restrict the value to the interval [0, MAX_INT64], so I chose to work with signed 64-bit integers.  I could have choosen to work with unsigned 64-bit integers, but then I would have to do extra work at the callsites.  Another choice would have been to parse negative values, but I don't have a consumer that needs that.  I would probably add support for that when moving this function into a more generic location.


> Is there a particular reason for the terminators parameter? Neither of the two
> callers pass a nonempty value for it.

When I first wrote the patch, I thought that I might want to stop on a comma since that's a valid header value separator.  But, as you can see that turned out to not be necessary.  I could remove the terminators parameter easily enough, but it seemed like a nice-to-have.

I'll make the LL_MAXUINT correction, thanks.

As for the other questionable consumers of PR_sscanf, I think most are not a concern.  At least I don't see ways in which overflow could be abused.  Maybe the call in nsDefaultSOAPEncoder could be problematic -- not sure.  Interestingly enough, I can't see where nsLongEncoder is ever instantiated or used.  I suspect that it is compiled away.  That SOAP code needs to be cvs removed! ;-)

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 217116 [details] [diff] [review]
v1 patch

> That is less than 0, so the test works.

oh. I forgot that this is an unsigned integer. ok then.

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

> oh. I forgot that this is an unsigned integer. ok then.

a _signed_ integer.

Comment 19

[Darin Fisher]

Attachment: v1.1 patch - for checkin

This version of the patch is slightly simpler.  I'm asking for reviews again because it is significantly different.  This version includes the following changes:

1) Simplify overflow checking
2) Replace |terminators| with |endPtr| a'la strtoll
3) Include a comment about replacing ParseInt64 with something more generic

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 217359 [details] [diff] [review]
v1.1 patch - for checkin

+        return ParseInt64(input, &next, result) && *next == '\0';

this returns a false positive (and a value of 0) if the string was empty. does that matter?
(a proxy might conceivable ignore a Content-Length header without a value. Mozilla would treat it as zero, leading to another content injection attack. right?)

> I'll make the LL_MAXUINT correction, thanks.

doesn't look like you did :)
+    if (!nsHttp::ParseInt64(slash, &size))
         size = LL_MaxUint();

Comment 21

[Darin Fisher]

> this returns a false positive (and a value of 0) if the string was empty. does
> that matter?

Good catch.

Comment 22

[Darin Fisher]

Attachment: v1.2 patch

OK, here's the final revised patch.

Comment 23

[Darin Fisher]

fixed-on-trunk

Comment 24

[Darin Fisher]

fixed1.8.1

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 217580 [details] [diff] [review]
v1.2 patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 26

[Darin Fisher]

fixed1.8.0.3

Comment 27

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.8.0.2) Gecko/20060420 Firefox/1.5.0.2, no "hacked" popups using Darin's python script (for future testers: I had to hack the script to point to "localhost" instead of using socket.gethostname() to get it to work for me)

Comment 28

[Kohei Yoshino [:kohei]]

dveditz: IPA asked us to delay publishing the security advisory for this bug. Is it possible?

Other browsers, at least IE (see my comment 0) also have this vulnerability, so they are coordinating with another vendors too.

Comment 29

[Kohei Yoshino [:kohei]]

(In reply to comment #28)
> Other browsers, at least IE (see my comment 0) also have this vulnerability, so
> they are coordinating with another vendors too.

The another vendor = Microsoft, of course. We don't know when they'll fix this vulnerability... Maybe one year later ;)

Comment 30

[Bob Clary [:bc] (inactive)]

(In reply to comment #14)
> Created an attachment (id=217163) [edit]
> testcase: python script

no alert ff2b2 winxp, linux
verified fixed 1.8

Comment 31

[Daniel Veditz [:dveditz]]

Kohei: has MS fixed or IPA announced this yet, or should the bug stay hidden. It's almost two years after comment 28

Comment 32

[u217243]

Asking JPCERT/CC about the status of IE...

Comment 33

[Kohei Yoshino [:kohei]]

Yesterday, almost 5 years later, JPCERT reported us the status of IE on this bug: Microsoft has confirmed the issue is not reproducible with their current product line. JPCERT will publish the advisory on July 28.