1184068 - Crash in mozilla::dom::CrashReporterParent::GenerateChildData due to plugin crash in NP_Shutdown

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Chris Coulson]

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

Comment 1

[Daniel Veditz [:dveditz]]

More likely plugins screwing up than crash reporter.

Comment 2

[Chris Coulson]

I guess this was introduced by http://hg.mozilla.org/releases/mozilla-release/rev/8d2d128ce671

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch

Thanks for the analysis!

Changing the setting of mShutdown from assignment to OR should take care of this.

Comment 4

[Jim Mathies [:jimm]]

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.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch (r2)

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.

Comment 6

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. We should take this on Aurora and Beta too.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8bae34af92ea

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/739a7ea4679c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8bae34af92ea

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ef641c2968c2

Comment 12

[Matt Wobensmith [:mwobensmith][:matt:]]

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.

Comment 13

[Chris Coulson]

(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!