Closed
Bug 1498765
Opened 6 years ago
Closed 6 years ago
Potential use-after-free if ContentParent is destroyed with channel open after KillHard
Categories
(Core :: DOM: Content Processes, defect, P2)
Core
DOM: Content Processes
Tracking
()
RESOLVED
FIXED
mozilla64
People
(Reporter: jld, Assigned: jld)
Details
(Keywords: csectype-uaf, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main64+][adv-esr60.4+])
Attachments
(2 files)
9.04 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
9.09 KB,
patch
|
RyanVM
:
approval-mozilla-esr60+
|
Details | Diff | Splinter Review |
I believe that the fix for bug 1423261 wound up hiding a potentially exploitable bug: if the channel is still open and receives a message, it can try to access the MessageChannel and IToplevelProtocol (in this case the ContentParent) after they're freed. It should only be possible for this to happen during shutdown: if the ContentParent isn't ActorDestroy()ed, it will have strong references from observers/prefs until shutdown. That might make it harder to exploit, but I assume it could still be exploited. I have a patch that changes ContentParent to hold a strong reference to itself that's freed from ActorDestroy (via a runnable, as currently[1]), so it will be leaked if it isn't properly shut down. (For all practical purposes such a ContentParent is already leaked, because of the strong observers mentioned above, but it probably wouldn't be picked up by our leak checking.) I don't have STR for bug 1423261, but I also intend to remove the earlier workaround, so if it's not fixed then the previous crash will come back. But this approach is straightforward enough that it should work, and is unlikely to cause any other problems that wouldn't be caught in regular testing. (This isn't really the right fix, I don't think: we should try to make sure the channel is closed properly even during abnormal termination of the child process. But that's a larger problem and less simple to deal with.) [1] https://searchfox.org/mozilla-central/rev/28fe656de6a022d1fc01bbd3811023fca99cf223/dom/ipc/ContentParent.cpp#1873
Updated•6 years ago
|
Priority: -- → P2
Assignee | ||
Comment 1•6 years ago
|
||
Replacing the custom runnable class with a closure isn't strictly necessary, but if I'm going to pass this off as cleanup I might as well do some cleanup while I'm in here.
Attachment #9018312 -
Flags: review?(continuation)
Updated•6 years ago
|
Keywords: csectype-uaf,
sec-moderate
Updated•6 years ago
|
Attachment #9018312 -
Flags: review?(continuation) → review+
Comment 2•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ca458791c2739f9c6bc7abe2b0672ed3518e9a3 https://hg.mozilla.org/mozilla-central/rev/1ca458791c27
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Updated•6 years ago
|
status-firefox62:
--- → wontfix
status-firefox63:
--- → wontfix
status-firefox-esr60:
--- → affected
tracking-firefox64:
--- → +
tracking-firefox-esr60:
--- → 64+
Assignee | ||
Comment 3•6 years ago
|
||
No increase in crashes from the assertion yet; I'll give it a few more days and then consider ESR uplift.
Flags: needinfo?(jld)
Updated•6 years ago
|
Whiteboard: [post-critsmash-triage]
Assignee | ||
Comment 4•6 years ago
|
||
[ESR Uplift Approval Request] If this is not a sec:{high,crit} bug, please state case for ESR consideration: It's a sec-moderate, but it is potentially exploitable, and the risk should be low. User impact if declined: Exposure to a security bug. Fix Landed on Version: 64 Risk to taking this patch: Low Why is the change risky/not risky? (and alternatives if risky): The main risk is that we crash when we don't need to, because this effectively backs out bug 1423261, but (1) from bug 1423261 comment #52 it seems we'd crash in that case anyway, and (2) this patch has been on Nightly and Beta for 3 weeks with no such crashes reported. The possibility of leaking a ContentParent should also be considered, but in the case where this patch could cause that (calling Init but not ActorDestroy), we'd already be leaking it. And this patch did pass our automated leak-checking in CI. String or UUID changes made by this patch: none
Flags: needinfo?(jld)
Attachment #9024607 -
Flags: approval-mozilla-esr60?
Comment 5•6 years ago
|
||
Comment on attachment 9024607 [details] [diff] [review] Patch for ESR 60 Fixes an exploitable IPC issues and has baked on Beta without issue for a few weeks now. Approved for 60.4.0esr.
Attachment #9024607 -
Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Comment 6•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr60/rev/a08c8493ba19
Updated•6 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main64+][adv-esr60.4+]
Updated•6 years ago
|
Flags: qe-verify-
Updated•5 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•