726777 - use after free in ~nsXMLHttpRequest

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Some more information, please. Is this about bug 718284?

Comment 2

[Andrew McCreight [:mccr8]]

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.

Comment 3

[Andrew McCreight [:mccr8]]

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.

Comment 4

[Andrew McCreight [:mccr8]]

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.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

I wonder how we get into this state. Does the addon change ownership of XHR somehow?

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: 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.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: 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.

Comment 8

[Andrew McCreight [:mccr8]]

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.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: 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.

Comment 10

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be845a0d6234

Comment 11

[Andrew McCreight [:mccr8]]

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

Comment 12

[Andrew McCreight [:mccr8]]

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.

Comment 13

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/mozilla-central/rev/be845a0d6234

Comment 14

[Andrew McCreight [:mccr8]]

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. ;)

Comment 15

[Andrew McCreight [:mccr8]]

Two NoteXPCOMChild crashes with the 2/16 build so far, none with the signature of this particular crash.

Comment 16

[Alex Keybl [:akeybl]]

(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.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

The patch looks very safe to me.

Comment 18

[Andrew McCreight [:mccr8]]

I don't actually know this code, so it will fall on others to really get a good assessment of that.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

nsCOMPtr<nsIStreamListener> listener should keep XHR alive as long as 
XML_HTTP_REQUEST_SYNCLOOPING is set.

Comment 21

[Andrew McCreight [:mccr8]]

Attachment: clear flag on abort

Jonas is this what you mean?

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 23

[Andrew McCreight [:mccr8]]

landed followup https://hg.mozilla.org/integration/mozilla-inbound/rev/b59f89947b54

Comment 24

[Andrew McCreight [:mccr8]]

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.

Comment 25

[Andrew McCreight [:mccr8]]

Attachment: folded together for aurora

Carrying forward sicking's r+.

Comment 26

[Andrew McCreight [:mccr8]]

Attachment: folded, rebased for beta

Comment 27

[Ed Morley [:emorley]]

(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

Comment 28

[Alex Keybl [:akeybl]]

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.

Comment 29

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c98c40344937
https://hg.mozilla.org/releases/mozilla-beta/rev/6df895a33af9

Comment 30

[u279076]

qa+ to verify using bug 718284

Comment 31

[Jason Smith [:jsmith]]

Verified FF 11 Beta 6 using reproduction steps from bug 718284.

Comment 32

[Jason Smith [:jsmith]]

Verified FF 12 Aurora using reproduction steps from bug 718284.

Comment 33

[Jason Smith [:jsmith]]

Verified on Nightly.