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

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: chrisccoulson, Assigned: aklotz)

Tracking

({csectype-uaf, sec-high})

39 Branch
mozilla42
All
Unspecified
csectype-uaf, sec-high
Points:
---

Firefox Tracking Flags

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

Details

(Whiteboard: [adv-main40+][post-critsmash-triage])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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().
(Reporter)

Updated

3 years ago
Hardware: Unspecified → All
More likely plugins screwing up than crash reporter.
Flags: needinfo?(aklotz)
Keywords: csectype-uaf, sec-high
(Reporter)

Comment 2

3 years ago
I guess this was introduced by http://hg.mozilla.org/releases/mozilla-release/rev/8d2d128ce671
(Assignee)

Updated

3 years ago
Flags: needinfo?(aklotz)
(Assignee)

Comment 3

3 years ago
Created attachment 8637541 [details] [diff] [review]
Patch

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 4

3 years ago
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+
(Assignee)

Comment 5

3 years ago
Created attachment 8638101 [details] [diff] [review]
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.
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.
status-firefox39: --- → wontfix
status-firefox40: --- → affected
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox-esr31: --- → unaffected
status-firefox-esr38: --- → unaffected
tracking-firefox40: --- → +
tracking-firefox41: --- → +
tracking-firefox42: --- → +
Attachment #8638101 - Flags: sec-approval? → sec-approval+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 7

3 years ago
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+
https://hg.mozilla.org/mozilla-central/rev/8bae34af92ea
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: --- → unaffected
status-b2g-v2.2r: --- → unaffected
status-b2g-master: --- → unaffected
status-firefox42: affected → fixed
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)
(Reporter)

Comment 13

3 years ago
(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)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1183095

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.