use after free in ~nsXMLHttpRequest

VERIFIED FIXED in Firefox 11

Status

()

Core
DOM
--
critical
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

11 Branch
mozilla11
Points:
---

Firefox Tracking Flags

(firefox10 unaffected, firefox11+ verified, firefox12+ verified, firefox13+ verified, firefox-esr10 unaffected, status1.9.2 unaffected)

Details

(Whiteboard: [sg:critical][qa!])

Attachments

(4 attachments, 2 obsolete attachments)

Comment hidden (empty)

Comment 1

6 years ago
Some more information, please. Is this about bug 718284?
(Assignee)

Comment 2

6 years ago
This seems to be the cause of Bug 718284.

Here's the unfortunate sequence of events when you follow the STR from that bug:

enter the destructor for nsXMLHttpRequest 0x12888dc00
create an event 0x100309900 with type 1.
[...time passes...]
cycle collector runs
0x100309900 has a pointer to 0x12888dc00, which was destroyed before it was created, probably in the destructor itself.  Causes a crash.

It isn't clear to me if this is something weird the addon is doing or what.  It does do some kind of thread magic it probably shouldn't be doing.
(Assignee)

Updated

6 years ago
Blocks: 718284
(Assignee)

Comment 3

6 years ago
After we enter the destructor for the nsXMLHttpRequest and create the event, we get ref count traffic for the XHR that looks like this:

<nsXMLHttpRequest> 0x12888dc00 1 AddRef 2
nsDOMEventTargetHelper::QueryInterface(nsID const&, void**)+0x000001AD
nsXHREventTarget::QueryInterface(nsID const&, void**)+0x0000004F
nsXMLHttpRequest::QueryInterface(nsID const&, void**)+0x000000D4
nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*, nsCOMArray<nsIDOMEventTarget>*)+0x000000B2
nsEventDispatcher::DispatchDOMEvent(nsISupports*, nsEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*)+0x00000350
nsXMLHttpRequest::ChangeState(unsigned int, bool)+0x000001CA
nsXMLHttpRequest::CloseRequestWithError(nsAString_internal const&, unsigned int)+0x000000E2
nsXMLHttpRequest::~nsXMLHttpRequest()+0x00000735

<nsXMLHttpRequest> 0x12888dc00 1 AddRef 2
nsCOMPtr<nsIDOMEventTarget>::operator=(nsIDOMEventTarget*)+0x00000025
nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*, nsCOMArray<nsIDOMEventTarget>*)+0x00000301
nsEventDispatcher::DispatchDOMEvent(nsISupports*, nsEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*)+0x00000350
nsXMLHttpRequest::ChangeState(unsigned int, bool)+0x000001CA
nsXMLHttpRequest::CloseRequestWithError(nsAString_internal const&, unsigned int)+0x000000E2
nsXMLHttpRequest::~nsXMLHttpRequest()+0x00000735

I assume that this is a QI and a store to the newly created event.  The refcount doesn't go up, I assume because we've started destroying the object?

Then there's a bunch more ref count traffic with similar stacks.

jst suggested that we could add a flag to the XHR to indicate that the object is being destroyed, and then not do an Abort() if we're being shut down.  Or something like that.
(Assignee)

Comment 4

6 years ago
I've been testing on Firefox 13, but I've been able to reproduce the original crash on 11 and 12, too, but not 10.
status-firefox10: --- → unaffected
status-firefox11: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → affected
tracking-firefox11: --- → ?
tracking-firefox12: --- → ?
tracking-firefox13: --- → ?

Comment 5

6 years ago
I wonder how we get into this state. Does the addon change ownership of XHR somehow?
(Assignee)

Comment 6

6 years ago
Created attachment 596851 [details] [diff] [review]
don't dispatch from CloseRequestWithError if we've deleted the object

jst's suggested work around.  With this patch applied, in some light testing the browser did not crash when searching from the Wikipedia bar with https everywhere.
(Assignee)

Updated

6 years ago
Attachment #596851 - Flags: review?(jonas)
Attachment #596851 - Flags: review?(jonas) → review+
(Assignee)

Comment 7

6 years ago
Created attachment 597018 [details] [diff] [review]
rebased for beta

Basically the same thing, except that there's no 1<<18 flag, and we check the flag in Abort(), not in whatever Abort() calls in Aurora/Nightly.  I don't think skipping a flag matters, so I left the one I added as 1<<19 for consistency.
Assignee: nobody → continuation
(Assignee)

Comment 8

6 years ago
It isn't clear how exploitable this really is.  It may just be this addon that does something weird, but I have seen a similar set of crashes on html5test.com (that can be hard to reproduce), so it may be triggerable without it.  With the addon, it is 100% reproducable.  This doesn't always cause a crash in the cycle collector.  Occasionally it causes a crash in a finalizer in the GC.
Whiteboard: [sg:critical]
(Assignee)

Comment 9

6 years ago
Created attachment 597069 [details] [diff] [review]
rebased for beta

I forgot to return a value in Abort().

This looks good on try for m-c, but for some reason every single Android test is failing on Aurora.  I'm hoping it is just some weird infrastructure issue, and retrying it.
Attachment #597018 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/be845a0d6234
Target Milestone: --- → mozilla13
(Assignee)

Comment 11

6 years ago
Comment on attachment 596851 [details] [diff] [review]
don't dispatch from CloseRequestWithError if we've deleted the object

[Approval Request Comment]
Regression caused by (bug #): unknown, something in 11
User impact if declined: common crash with HTTPS everywhere addon, possible security problems 
Testing completed (on m-c, etc.): try runs, landed on mozilla-inbound just now
Risk to taking this patch (and alternatives if risky): low, I guess?  Should only affect nsXMLHttpRequests that are being destroyed.
String changes made by this patch: none
Attachment #596851 - Flags: approval-mozilla-beta?
Attachment #596851 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 12

6 years ago
I guess an alternative would be blocklisting HTTPS Everywhere, and/or helping them figure out why they trigger this problem.  That would deal with the top crash, but not fix the underlying issue.
(Assignee)

Comment 13

6 years ago
https://hg.mozilla.org/mozilla-central/rev/be845a0d6234
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
tracking-firefox11: ? → +
tracking-firefox12: ? → +
tracking-firefox13: ? → +
(Assignee)

Comment 14

6 years ago
The first nightly this is in is 2/16.  I'll keep an eye on crash stats.  No crashes in the build from the 16th yet. ;)
(Assignee)

Comment 15

6 years ago
Two NoteXPCOMChild crashes with the 2/16 build so far, none with the signature of this particular crash.
(In reply to Andrew McCreight [:mccr8] from comment #11)
> Risk to taking this patch (and alternatives if risky): low, I guess?  Should
> only affect nsXMLHttpRequests that are being destroyed.

How can we gain confidence in this risk evaluation? This patch is highly desirable from both the security and crash angles (suspected cause of bug 469267), so we'd really like to take the fix for Beta 4 if we can get consensus on the low-risk nature of the bug.
The patch looks very safe to me.
(Assignee)

Comment 18

6 years ago
I don't actually know this code, so it will fall on others to really get a good assessment of that.
The one thing I could think of doing to make it safer would be to also clear the XML_HTTP_REQUEST_SYNCLOOPING flag when bailing. That shouldn't be needed but is the only source of risk that I can see.

So maybe do that and then I think it'd be safe for beta/aurora.
nsCOMPtr<nsIStreamListener> listener should keep XHR alive as long as 
XML_HTTP_REQUEST_SYNCLOOPING is set.
(Assignee)

Comment 21

6 years ago
Created attachment 598575 [details] [diff] [review]
clear flag on abort

Jonas is this what you mean?
Attachment #598575 - Flags: review?(jonas)
Comment on attachment 598575 [details] [diff] [review]
clear flag on abort

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

Yes. As Olli points out, this really shouldn't be needed, but is safer than not. So for landing on beta/aurora we might as well take this too.
Attachment #598575 - Flags: review?(jonas) → review+
(Assignee)

Comment 23

6 years ago
landed followup https://hg.mozilla.org/integration/mozilla-inbound/rev/b59f89947b54
(Assignee)

Comment 24

6 years ago
There have been 5 crashes in NoteXPCOMChild in the 3 builds since this patch landed, and none were in nsDOMEvent.  This is compared to 19 crashes just in the build the day before this patch landed.
(Assignee)

Updated

6 years ago
Attachment #596851 - Flags: checkin+
(Assignee)

Updated

6 years ago
Attachment #598575 - Flags: checkin+
(Assignee)

Comment 25

6 years ago
Created attachment 598942 [details] [diff] [review]
folded together for aurora

Carrying forward sicking's r+.
Attachment #598942 - Flags: review+
(Assignee)

Comment 26

6 years ago
Created attachment 598943 [details] [diff] [review]
folded, rebased for beta
Attachment #597069 - Attachment is obsolete: true
(In reply to Andrew McCreight [:mccr8] from comment #23)
> landed followup
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b59f89947b54

https://hg.mozilla.org/mozilla-central/rev/b59f89947b54
status-firefox13: affected → fixed
Comment on attachment 596851 [details] [diff] [review]
don't dispatch from CloseRequestWithError if we've deleted the object

[Triage Comment]
Considered very safe and resolves top crasher bug 718284. Approved for Aurora 12 and Beta 11. Please land today to make beta 4 of Firefox 11.
Attachment #596851 - Flags: approval-mozilla-beta?
Attachment #596851 - Flags: approval-mozilla-beta+
Attachment #596851 - Flags: approval-mozilla-aurora?
Attachment #596851 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 29

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/c98c40344937
https://hg.mozilla.org/releases/mozilla-beta/rev/6df895a33af9
status-firefox11: affected → fixed
status-firefox12: affected → fixed
Target Milestone: mozilla13 → mozilla11
qa+ to verify using bug 718284
Whiteboard: [sg:critical] → [sg:critical][qa+]
status-firefox-esr10: --- → unaffected
status1.9.2: --- → unaffected
Verified FF 11 Beta 6 using reproduction steps from bug 718284.
status-firefox11: fixed → verified
Verified FF 12 Aurora using reproduction steps from bug 718284.
status-firefox12: fixed → verified
Verified on Nightly.
Status: RESOLVED → VERIFIED
status-firefox13: fixed → verified
Whiteboard: [sg:critical][qa+] → [sg:critical][qa!]
Group: core-security
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.