Closed Bug 1498765 Opened 3 years ago Closed 3 years ago

Potential use-after-free if ContentParent is destroyed with channel open after KillHard


(Core :: DOM: Content Processes, defect, P2)




Tracking Status
firefox-esr60 64+ fixed
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 + fixed


(Reporter: jld, Assigned: jld)


(Keywords: csectype-uaf, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main64+][adv-esr60.4+])


(2 files)

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

Priority: -- → P2
Attached patch PatchSplinter Review
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)
Attachment #9018312 - Flags: review?(continuation) → review+
Group: dom-core-security → core-security-release
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
No increase in crashes from the assertion yet; I'll give it a few more days and then consider ESR uplift.
Flags: needinfo?(jld)
Whiteboard: [post-critsmash-triage]
Attached patch Patch for ESR 60Splinter Review
[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 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+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main64+][adv-esr60.4+]
Flags: qe-verify-
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.