Closed Bug 1184068 Opened 5 years ago Closed 5 years ago
Crash in mozilla::dom::Crash
Reporter Parent::Generate Child Data due to plugin crash in NP _Shutdown
Reporting this separately to bug 1183095 because I'm not sure whether this is security sensitive. Whilst investigating bug 1183095, I think I've discovered that there are 2 separate issues: 1) The plugin process crashes during the call to NP_Shutdown (whilst the browser is in mozilla::plugins::PPluginModuleParent::CallNP_Shutdown). This appears to only be happening in Ubuntu builds. 2) The plugin process crashing inside NP_Shutdown tickles another bug which crashes the browser process, and this is not unique to Ubuntu builds. This bug is for the second part. What happens is this: - PluginModuleParent::NP_Shutdown() gets called from nsNPAPIPlugin::Shutdown() - The plugin process crashes whilst the browser is inside PPluginModuleParent::CallNP_Shutdown(), which returns false. - PluginModuleParent::DoShutdown() calls Close(), which results in PluginModuleChromeParent::ActorDestroy() being called from inside PPluginModuleParent::OnChannelError(). - ActorDestroy() sets mShutdown to true. - CrashReporterParent is destroyed inside PPluginModuleParent::OnChannelError() - The stack unwinds, and then PluginModuleParent::DoShutdown() sets mShutdown to false (the return value of CallNP_Shutdown). - PluginModuleParent::NP_Shutdown() is called again from nsNPAPIPlugin's destructor. - Because mShutdown was reset to false, it calls DoShutdown() again, which results in ActorDestroy() being called for a second time. - Because CrashReporterParent has already been destroyed, it results in an out-of-bounds read in PluginModuleChromeParent::CrashReporter().
More likely plugins screwing up than crash reporter.
I guess this was introduced by http://hg.mozilla.org/releases/mozilla-release/rev/8d2d128ce671
Thanks for the analysis! Changing the setting of mShutdown from assignment to OR should take care of this.
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Attachment #8637541 - Flags: review?(jmathies)
Comment on attachment 8637541 [details] [diff] [review] Patch Review of attachment 8637541 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/plugins/ipc/PluginModuleParent.cpp @@ +2421,5 @@ > // plugin dso will have been unloaded on the other side by the > // CallNP_Shutdown() message > Close(); > > + mShutdown |= ok; please add a comment explaining why we assign this way.
Attachment #8637541 - Flags: review?(jmathies) → review+
Added comments as requested. Carrying forward r+. [Security approval request comment] How easily could an exploit be constructed based on the patch? It's not obvious based on the patch. Plugin destruction is such a complicated, convoluted sequence of code that it would be hard to distill an exploit from it. Also it's very timing sensitive and requires the plugin itself to crash during shutdown. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. They explain the code but it isn't obvious from that explanation that it fixes a security problem. Which older supported branches are affected by this flaw? 39+ If not all supported branches, which bug introduced the flaw? bug 1152395 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Trivial to merge with hg. How likely is this patch to cause regressions; how much testing does it need? Unlikely. Regular plugin mochitests will suffice.
sec-approval+ for trunk. We should take this on Aurora and Beta too.
Attachment #8638101 - Flags: sec-approval? → sec-approval+
Comment on attachment 8638101 [details] [diff] [review] Patch (r2) Approval Request Comment [Feature/regressing bug #]: bug 1152395 [User impact if declined]: Browser crashes if plugin crashes during its shutdown [Describe test coverage new/current, TreeHerder]: Plugin test suite [Risks and why]: None. Trivial patch. [String/UUID change made/needed]: None.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Whiteboard: [adv-main40+] → [adv-main40+][post-critsmash-triage]
Hi Chris, since you were able to observe the issue(s), can you take a look and see if this resolves the problem for you? Thanks.
(In reply to Matt Wobensmith from comment #12) > Hi Chris, since you were able to observe the issue(s), can you take a look > and see if this resolves the problem for you? Thanks. I've tested it now and it fixes the problem. Thanks!
You need to log in before you can comment on or make changes to this bug.