Closed Bug 1419373 Opened 3 years ago Closed 2 years ago

XPConnect can access freed JSObjects during shutdown

Categories

(Core :: XPConnect, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

(Keywords: csectype-uaf, sec-moderate, Whiteboard: [sec-triage-backlog][adv-main61+][post-critsmash-triage])

Attachments

(2 files)

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.
Attached file stack_trace.txt
P1 because sec-high
Priority: -- → P1
Do I correctly understand that this is only a security issue if the attacker can cause our shutdown sequence to begin?
Flags: needinfo?(jcoppeard)
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.
Flags: needinfo?(jcoppeard)
(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.
Should we downrate to sec-moderate? This seems only in xpcshells.
Flags: needinfo?(dveditz)
(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.
Flags: needinfo?(dveditz)
Keywords: sec-highsec-moderate
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?
Attachment #8969614 - Flags: review+
Attachment #8969614 - Flags: review+ → review?(continuation)
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.
Attachment #8969614 - Flags: review?(continuation) → review+
(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.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Whiteboard: [sec-triage-backlog]
Can this ride the trains?
Flags: needinfo?(jcoppeard)
(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.
Flags: needinfo?(jcoppeard)
Group: javascript-core-security → core-security-release
Whiteboard: [sec-triage-backlog] → [sec-triage-backlog][adv-main61+]
Flags: qe-verify-
Whiteboard: [sec-triage-backlog][adv-main61+] → [sec-triage-backlog][adv-main61+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.