1419373 - XPConnect can access freed JSObjects during shutdown

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement, P1)

Description

[Jon Coppeard (:jonco)]

While working on bug 1415852 I found that XPConnect can access previously freed JSObjects during shutdown.

This happens only in xpcshell tests.  I don't know if this is a real issue in the browser.

The problem is that a JSObject2WrappedJSMap is not cleared before JS runtime shutdown and can hold Heap<JSObject*> keys that reference dead objects.  When this is destroyed the post barrier can access freed memory.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: stack_trace.txt

Comment 2

[Andrew Overholt [:overholt]]

P1 because sec-high

Comment 3

[Frederik Braun [:freddy]]

Do I correctly understand that this is only a security issue if the attacker can cause our shutdown sequence to begin?

Comment 4

[Jon Coppeard (:jonco)]

I'm not sure if this affects the browser or not.  In my testing it only affected xpcshell and only during shutdown.  That said, we should still work out what's going wrong here.

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to Frederik Braun [:freddyb] from comment #3)
> Do I correctly understand that this is only a security issue if the attacker
> can cause our shutdown sequence to begin?

This is something that will only happen when we're shutting down a chrome or content process. By definition the JS engine is shut down by that point so I'm not sure how easily you could exploit this. We only have evidence that it happens in XPCShell, but I suspect it could also happen in a regular browser if there's a leak.

Comment 6

[Frederik Braun [:freddy]]

Should we downrate to sec-moderate? This seems only in xpcshells.

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Frederik Braun [:freddyb] from comment #6)
> Should we downrate to sec-moderate? This seems only in xpcshells.

I think it will also affect the browser if there's a leak. Still, this is so late in shutdown it will likely be hard or impossible to exploit.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: bug1419373-xpconnect-changes

This patch makes sure we call ShutdownMarker on all JSObject2WrappedJSMaps at the same time.  Previously this happened later for CompartmentPrivate because it only did that in its destructor.  Also, we stop tracing wrapped JS roots at this time, because nsXPCWrappedJS::SystemIsBeingShutDown() makes it invalid to trace that object any more.

This fixes the issue.  I'm not 100% sure it's the right fix, but it seems sensible (to me).  I tried several other approaches first that didn't work out.

What do you think?

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 8969614 [details] [diff] [review]
bug1419373-xpconnect-changes

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

I'm not too familiar with the nuance of XPConnect shutdown, but this looks reasonable to me.

::: js/xpconnect/src/XPCWrappedNativeScope.cpp
@@ +550,5 @@
>              i.Remove();
>          }
> +
> +        CompartmentPrivate* priv = CompartmentPrivate::Get(cur->Compartment());
> +        priv->GetWrappedJSMap()->ShutdownMarker();

Maybe define a shutdown method on the private that does the shutdown marker, deletes the map, and nulls mWrappedJSMap? That way you'll only get null derefs if somebody tries to use it. Assuming that works. And then assert in the dtor that it is already null.

Comment 10

[Jon Coppeard (:jonco)]

(In reply to Andrew McCreight [:mccr8] from comment #9)
> Maybe define a shutdown method on the private that does the shutdown marker,
> deletes the map, and nulls mWrappedJSMap? That way you'll only get null
> derefs if somebody tries to use it. Assuming that works. And then assert in
> the dtor that it is already null.

That doesn't work in the case where zones are destroyed outside shutdown, but I'll do the refactoring into a separate method.

Comment 11

[Jon Coppeard (:jonco)]

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

Comment 12

[Phil Ringnalda (:philor)]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Can this ride the trains?

Comment 14

[Jon Coppeard (:jonco)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)
> Can this ride the trains?

Yes.  I don't have any evidence of this causing problems, and it's something I'd like to bake for a while before thinking about backporting anyway.