Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Description • 6 years ago

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

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ 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.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 2 • 6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ca458791c2739f9c6bc7abe2b0672ed3518e9a3
https://hg.mozilla.org/mozilla-central/rev/1ca458791c27

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ 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.

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ 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

	Ryan VanderMeulen [:RyanVM]
	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.

	Ryan VanderMeulen [:RyanVM]
	Comment 6 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-esr60/rev/a08c8493ba19