Closed Bug 1184068 Opened 9 years ago Closed 9 years ago

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


(Core Graveyard :: Plug-ins, defect)

39 Branch
Not set


(firefox39 wontfix, firefox40+ fixed, firefox41+ fixed, firefox42+ fixed, firefox-esr31 unaffected, firefox-esr38 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-v2.2r unaffected, b2g-master unaffected)

Tracking Status
firefox39 --- wontfix
firefox40 + fixed
firefox41 + fixed
firefox42 + fixed
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.2r --- unaffected
b2g-master --- unaffected


(Reporter: chrisccoulson, Assigned: bugzilla)



(Keywords: csectype-uaf, sec-high, Whiteboard: [adv-main40+][post-critsmash-triage])


(1 file, 1 obsolete file)

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().
Hardware: Unspecified → All
More likely plugins screwing up than crash reporter.
Flags: needinfo?(aklotz)
I guess this was introduced by
Flags: needinfo?(aklotz)
Attached patch Patch (obsolete) — Splinter Review
Thanks for the analysis!

Changing the setting of mShutdown from assignment to OR should take care of this.
Assignee: nobody → aklotz
Attachment #8637541 - Flags: review?(jmathies)
Comment on attachment 8637541 [details] [diff] [review]

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+
Attached patch Patch (r2)Splinter 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?

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.
Attachment #8637541 - Attachment is obsolete: true
Attachment #8638101 - Flags: sec-approval?
Attachment #8638101 - Flags: review+
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.
Attachment #8638101 - Flags: approval-mozilla-beta?
Attachment #8638101 - Flags: approval-mozilla-aurora?
Attachment #8638101 - Flags: approval-mozilla-beta?
Attachment #8638101 - Flags: approval-mozilla-beta+
Attachment #8638101 - Flags: approval-mozilla-aurora?
Attachment #8638101 - Flags: approval-mozilla-aurora+
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Whiteboard: [adv-main40+]
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.
Flags: needinfo?(chrisccoulson)
(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!
Flags: needinfo?(chrisccoulson)
Group: core-security → core-security-release
Group: core-security-release
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.