Closed
Bug 1419373
Opened 6 years ago
Closed 5 years ago
XPConnect can access freed JSObjects during shutdown
Categories
(Core :: XPConnect, enhancement, P1)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla61
People
(Reporter: jonco, Assigned: jonco)
References
Details
(Keywords: csectype-uaf, sec-moderate, Whiteboard: [sec-triage-backlog][adv-main61+][post-critsmash-triage])
Attachments
(2 files)
10.13 KB,
text/plain
|
Details | |
6.20 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•6 years ago
|
||
Updated•6 years ago
|
Keywords: csectype-uaf,
sec-high
Comment 3•5 years ago
|
||
Do I correctly understand that this is only a security issue if the attacker can cause our shutdown sequence to begin?
Flags: needinfo?(jcoppeard)
Assignee | ||
Comment 4•5 years ago
|
||
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)
Comment 5•5 years ago
|
||
(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•5 years ago
|
||
Should we downrate to sec-moderate? This seems only in xpcshells.
Flags: needinfo?(dveditz)
Comment 7•5 years ago
|
||
(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.
Updated•5 years ago
|
Flags: needinfo?(dveditz)
Keywords: sec-high → sec-moderate
Assignee | ||
Comment 8•5 years ago
|
||
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+
Assignee | ||
Updated•5 years ago
|
Attachment #8969614 -
Flags: review+ → review?(continuation)
Comment 9•5 years ago
|
||
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+
Assignee | ||
Comment 10•5 years ago
|
||
(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.
Assignee | ||
Comment 11•5 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1c67d1afe266d91994533cf9e31e3ead393efe8
Comment 12•5 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d1c67d1afe266d91994533cf9e31e3ead393efe8
status-firefox61:
--- → fixed
Target Milestone: --- → mozilla61
Updated•5 years ago
|
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Whiteboard: [sec-triage-backlog]
Assignee | ||
Comment 14•5 years ago
|
||
(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)
Updated•5 years ago
|
Updated•5 years ago
|
Group: javascript-core-security → core-security-release
Updated•5 years ago
|
Whiteboard: [sec-triage-backlog] → [sec-triage-backlog][adv-main61+]
Updated•5 years ago
|
Flags: qe-verify-
Whiteboard: [sec-triage-backlog][adv-main61+] → [sec-triage-backlog][adv-main61+][post-critsmash-triage]
Updated•4 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•