Last Comment Bug 644069 - (CVE-2011-0069) Crash if javascript code makes more than 100 unique cross domain requests
(CVE-2011-0069)
: Crash if javascript code makes more than 100 unique cross domain requests
Status: RESOLVED FIXED
[sg:critical?]
: regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86_64 Windows 7
: P1 critical (vote)
: mozilla5
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: xxx
  Show dependency treegraph
 
Reported: 2011-03-23 00:02 PDT by db
Modified: 2016-01-19 17:09 PST (History)
5 users (show)
rforbes: sec‑bounty+
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
Macaw+
.1-fixed
.17+
.17-fixed
.19+
.19-fixed


Attachments
HTML file including javascript to show the issue (755 bytes, text/html)
2011-03-23 00:04 PDT, db
no flags Details
Proposed fix (1.9 branches) (2.97 KB, patch)
2011-03-23 20:09 PDT, Boris Zbarsky [:bz] (still a bit busy)
jonas: review+
dveditz: approval1.9.2.17+
dveditz: approval1.9.1.19+
Details | Diff | Splinter Review
Updated to tip + add test (3.59 KB, patch)
2011-03-28 15:54 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
bzbarsky: review+
dveditz: approval2.0+
Details | Diff | Splinter Review

Description db 2011-03-23 00:02:23 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2.16) Gecko/20110319 Firefox/3.6.16
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2.16) Gecko/20110319 Firefox/3.6.16

We have a web page using JQuery 1.5.1 that makes a POST ajax request to a REST web service on another domain.
Firefox sends a preflighting OPTIONS request, then the POST.

If the server responds with a header of Access-Control-Max-Age set to a number of seconds longer than it takes to make 100 unique uri requests, firefox crashes.

On the 101st unique URL post request, Firefox makes the 101st OPTIONS request to the server, which replies, then Firefox crashes with the attached crash dump.

Reproducible: Always

Steps to Reproduce:
1. Edit CrossDomainTest.html line 22 to point to a REST webservice
2. Ensure the webservice response includes a header of Access-Control-Max-Age set to something like "1728000" which is 20 days.
3. Open the page in Firefox
4. On the 101 request Firefox crashes.
Actual Results:  
Firefox crashed and showed the submit crash dump dialog.

Expected Results:  
Firefox should continue to make requests.

This is related to the submitted crash dump 
ID: 7636c991-6b98-4abf-b2c4-7ea2c2110322
Module: xul.dll

When no "Access-Control-Max-Age" header is defined in the web service response Firefox continues to make more than 101 requests. 

We are unsure if this bug could be exploited so we are setting the "Many users could be harmed" option until further investigation confirms the extent of this issue.
Comment 1 db 2011-03-23 00:04:35 PDT
Created attachment 521120 [details]
HTML file including javascript to show the issue

Replace line 22 with a rest url for a cross domain resource.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2011-03-23 19:59:01 PDT
Oy.  So the code landed for bug 389508 is buggy.

Most pertinently, nsAccessControlLRUCache::GetEntry with aCreate = true will create an entry, put it in the hashtable, then maybe evict some entries from the hashtable if there are too many and then return the newly created entry.

Which means that it can evict the newly-created entry (and in fact will preferentially do that!).

As a separate note, RemoveExpiredEntries has a buggy check for the entry needing removal...
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2011-03-23 20:09:10 PDT
Created attachment 521387 [details] [diff] [review]
Proposed fix (1.9 branches)

Jonas, want to write a test for this?  I bet you can do that way faster than I can...
Comment 5 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-03-24 15:42:57 PDT
Comment on attachment 521387 [details] [diff] [review]
Proposed fix (1.9 branches)

Looks good. Would you mind waiting to land this until after bug 644476 has been checked in? Those patches will be a pain to merge otherwise. I'm happy to do the merging/landing of this one if you want.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2011-03-24 18:16:46 PDT
Waiting is fine, if that other bug is landing soonish.  I'd like to get this fixed on branches too...
Comment 7 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-03-28 14:05:38 PDT
Bug 644476 has been landed. I'll take a stab at writing a test for this bug.
Comment 8 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-03-28 15:54:23 PDT
Created attachment 522502 [details] [diff] [review]
Updated to tip + add test

bz, could you have a quick look over the test?
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2011-03-28 17:45:20 PDT
Comment on attachment 522502 [details] [diff] [review]
Updated to tip + add test

Looks fine to me; I assme this does fail without the patch, right?
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2011-03-28 17:45:55 PDT
Oh, and when I'm pushing this to m-c, should I include the test?  :(
Comment 11 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-03-28 17:48:22 PDT
Yup, it did
Comment 12 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-03-28 17:49:35 PDT
Oh, hmm.. yeah, you probably should not include the test :(

We need a keyword for tracking tests that are pending landing.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2011-03-28 23:02:13 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/79487686ac29 with the fix, but not the test.
Comment 14 Daniel Veditz [:dveditz] 2011-03-30 10:25:11 PDT
(In reply to comment #12)
> We need a keyword for tracking tests that are pending landing.

Jonas: we're using the in-testsuite: ? flag for that

bz: as a security bug what's the severity here? We're stuffing one too many items into a list (overrun)? Don't realize the write failed and read too far?
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2011-03-30 10:53:06 PDT
Daniel, the severity is that nsPreflightCache::GetEntry returns a pointer to a deleted entry in the case this bug is concerned about.  Then we access members of that entry; looks like most common is the entry->mMethods.Length() access or the entry->mHeaders.Length() access, which reads from memory we don't own and crashes.

Jonas, are there other GetEntry callers than AddResultToCache?
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2011-03-30 10:54:13 PDT
> Jonas: we're using the in-testsuite: ? flag for that

We're also using it to track bugs that need tests written.  I have several hundred of the latter with this flag set assigned to me, so I'm hoping that I'm not the one who's supposed to notice when we open this and land the test....
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2011-04-05 18:11:30 PDT
Comment on attachment 522502 [details] [diff] [review]
Updated to tip + add test

Requesting whatever branch approvals are relevant....
Comment 18 christian 2011-04-11 17:35:15 PDT
If we were to take this for Macaw we'd need to fix it on the branches too. Is the patch the same?
Comment 19 christian 2011-04-11 17:39:57 PDT
(I'd guess from the approval requests it is, just want to make it explicit)
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2011-04-11 17:49:31 PDT
It's not.  The correct patch for the braches is the original "Proposed fix" patch on this bug.  Would you prefer that I move the approval requests there?
Comment 21 Daniel Veditz [:dveditz] 2011-04-13 10:40:33 PDT
Comment on attachment 521387 [details] [diff] [review]
Proposed fix (1.9 branches)

Approved for 1.9.2.17 and 1.9.1.19, a=dveditz for release-drivers
Comment 22 Daniel Veditz [:dveditz] 2011-04-13 10:40:57 PDT
Comment on attachment 522502 [details] [diff] [review]
Updated to tip + add test

Approved for the mozilla2.0 repository, a=dveditz for release-drivers
Comment 23 christian 2011-04-13 17:29:23 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2abcb340f405
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/15c9a0503705
http://hg.mozilla.org/releases/mozilla-2.0/rev/647a0e97f960

Note that the "Updated to tip" patch didn't work for mozilla-2.0.

This is what I ended up doing:
1. Landed the "Proposed fix (1.9 branches)" patch on 1.9.2
2. hg transplanted to 1.9.1
3. hg transplanted from 1.9.2 onto 2.0
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2011-04-13 18:07:28 PDT
Yeah, I should have been clearer: the "proposed fix" patch was the right one for _all_ branches including 2.0.  The "updated to tip" was updated to a file move that happened after Firefox 4 ship.

Thank you for checking this in!
Comment 26 Daniel Veditz [:dveditz] 2011-05-08 17:12:20 PDT
It's OK to check in the tests now.
Comment 27 Daniel Veditz [:dveditz] 2011-05-20 12:41:16 PDT
(In reply to comment #15)
> Daniel, the severity is that nsPreflightCache::GetEntry returns a pointer to
> a deleted entry in the case this bug is concerned about.  Then we access
> members of that entry; looks like most common is the
> entry->mMethods.Length() access or the entry->mHeaders.Length() access,
> which reads from memory we don't own and crashes.

But is the deleting and using relatively atomic (with respect to the page's code)? The web page makes a call that creates, deletes, and then dereferences an entry -- how in that time can evil content insert an allocation that reuses the deleted object's memory? If you can't do that then it's going to die safely on the read, if you can then it's exploitable.

I guess you could have a web worker and have it start churning away allocating appropriately sized chunks right as you make the fatal 101st XHR?
Comment 28 Boris Zbarsky [:bz] (still a bit busy) 2011-05-20 12:48:53 PDT
> But is the deleting and using relatively atomic (with respect to the page's
> code)? 

I think so, yes...

> I guess you could have a web worker and have it start churning away allocating > appropriately sized chunks right as you make the fatal 101st XHR?

That's a possibility, indeed.  ;)
Comment 29 Boris Zbarsky [:bz] (still a bit busy) 2011-05-20 19:44:41 PDT
Pushed the test: http://hg.mozilla.org/projects/cedar/rev/a29229640bf8
Comment 30 Daniel Holbert [:dholbert] 2011-05-21 17:35:00 PDT
test is now on m-c after merging cedar:
http://hg.mozilla.org/mozilla-central/rev/a29229640bf8
Comment 31 Raymond Forbes[:rforbes] 2013-07-19 18:47:52 PDT
rforbes-bugspam-for-setting-that-bounty-flag-20130719

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