Closed
Bug 644069
(CVE-2011-0069)
Opened 14 years ago
Closed 14 years ago
Crash if javascript code makes more than 100 unique cross domain requests
Categories
(Core :: DOM: Core & HTML, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla5
People
(Reporter: db, Assigned: bzbarsky)
References
Details
(Keywords: regression, reporter-external, Whiteboard: [sg:critical?])
Attachments
(3 files)
755 bytes,
text/html
|
Details | |
2.97 KB,
patch
|
sicking
:
review+
dveditz
:
approval1.9.2.17+
dveditz
:
approval1.9.1.19+
|
Details | Diff | Splinter Review |
3.59 KB,
patch
|
bzbarsky
:
review+
dveditz
:
approval2.0+
|
Details | Diff | Splinter Review |
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 2•14 years ago
|
||
Component: General → DOM: Mozilla Extensions
Product: Firefox → Core
QA Contact: general → general
![]() |
Assignee | |
Comment 3•14 years ago
|
||
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...
Status: UNCONFIRMED → NEW
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
Ever confirmed: true
![]() |
Assignee | |
Comment 4•14 years ago
|
||
Jonas, want to write a test for this? I bet you can do that way faster than I can...
Attachment #521387 -
Flags: review?(jonas)
![]() |
Assignee | |
Updated•14 years ago
|
Assignee: nobody → bzbarsky
Priority: -- → P1
Whiteboard: [need review]
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.
Attachment #521387 -
Flags: review?(jonas) → review+
![]() |
Assignee | |
Comment 6•14 years ago
|
||
Waiting is fine, if that other bug is landing soonish. I'd like to get this fixed on branches too...
Whiteboard: [need review] → [need fix for bug 644476]
Bug 644476 has been landed. I'll take a stab at writing a test for this bug.
Whiteboard: [need fix for bug 644476]
bz, could you have a quick look over the test?
Attachment #521387 -
Attachment is obsolete: true
Attachment #522502 -
Flags: review?(bzbarsky)
![]() |
Assignee | |
Comment 9•14 years ago
|
||
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?
Attachment #522502 -
Flags: review?(bzbarsky) → review+
![]() |
Assignee | |
Comment 10•14 years ago
|
||
Oh, and when I'm pushing this to m-c, should I include the test? :(
Whiteboard: [need landing]
Yup, it did
Oh, hmm.. yeah, you probably should not include the test :(
We need a keyword for tracking tests that are pending landing.
![]() |
Assignee | |
Comment 13•14 years ago
|
||
Pushed http://hg.mozilla.org/mozilla-central/rev/79487686ac29 with the fix, but not the test.
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [need landing] → [need test landed when the bug is opened up]
Target Milestone: --- → mozilla2.2
Comment 14•14 years ago
|
||
(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?
Blocks: xxx
Keywords: regression
![]() |
Assignee | |
Comment 15•14 years ago
|
||
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?
![]() |
Assignee | |
Comment 16•14 years ago
|
||
> 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....
![]() |
Assignee | |
Comment 17•14 years ago
|
||
Comment on attachment 522502 [details] [diff] [review]
Updated to tip + add test
Requesting whatever branch approvals are relevant....
Attachment #522502 -
Flags: approval2.0?
Attachment #522502 -
Flags: approval1.9.2.18?
Attachment #522502 -
Flags: approval1.9.2.17?
Attachment #522502 -
Flags: approval1.9.1.20?
Attachment #522502 -
Flags: approval1.9.1.19?
Comment 18•14 years ago
|
||
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•14 years ago
|
||
(I'd guess from the approval requests it is, just want to make it explicit)
![]() |
Assignee | |
Comment 20•14 years ago
|
||
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?
Updated•14 years ago
|
blocking1.9.1: ? → .19+
blocking1.9.2: ? → .17+
blocking2.0: ? → Macaw+
status1.9.1:
--- → wanted
status1.9.2:
--- → wanted
Whiteboard: [need test landed when the bug is opened up] → [sg:critical?][need test landed when the bug is opened up]
Comment 21•14 years ago
|
||
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
Attachment #521387 -
Attachment description: Proposed fix → Proposed fix (1.9 branches)
Attachment #521387 -
Attachment is obsolete: false
Attachment #521387 -
Flags: approval1.9.2.17+
Attachment #521387 -
Flags: approval1.9.1.19+
Comment 22•14 years ago
|
||
Comment on attachment 522502 [details] [diff] [review]
Updated to tip + add test
Approved for the mozilla2.0 repository, a=dveditz for release-drivers
Attachment #522502 -
Flags: approval2.0?
Attachment #522502 -
Flags: approval2.0+
Attachment #522502 -
Flags: approval1.9.2.18?
Attachment #522502 -
Flags: approval1.9.2.17?
Attachment #522502 -
Flags: approval1.9.1.20?
Attachment #522502 -
Flags: approval1.9.1.19?
Comment 23•14 years ago
|
||
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
![]() |
Assignee | |
Comment 24•14 years ago
|
||
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!
Updated•14 years ago
|
Alias: CVE-2011-0069
Updated•14 years ago
|
Whiteboard: [sg:critical?][need test landed when the bug is opened up] → [sg:critical?][bug is open, need test landed]
Comment 27•14 years ago
|
||
(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?
![]() |
Assignee | |
Comment 28•14 years ago
|
||
> 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. ;)
![]() |
Assignee | |
Comment 29•14 years ago
|
||
Pushed the test: http://hg.mozilla.org/projects/cedar/rev/a29229640bf8
Flags: in-testsuite? → in-testsuite+
Whiteboard: [sg:critical?][bug is open, need test landed] → [sg:critical?]
Comment 30•14 years ago
|
||
test is now on m-c after merging cedar:
http://hg.mozilla.org/mozilla-central/rev/a29229640bf8
Updated•12 years ago
|
Component: DOM: Mozilla Extensions → DOM
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
Updated•8 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•