Closed Bug 1156861 Opened 9 years ago Closed 9 years ago

crash in mozilla::plugins::PluginProcessParent::RunLaunchCompleteTask()

Categories

(Core Graveyard :: Plug-ins, defect)

39 Branch
x86
Windows NT
defect
Not set
critical

Tracking

(firefox38 wontfix, firefox38.0.5 wontfix, firefox39+ fixed, firefox40+ fixed, firefox41+ fixed, firefox-esr31 unaffected, firefox-esr3839+ fixed, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-master unaffected)

RESOLVED FIXED
mozilla41
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 + fixed
firefox40 + fixed
firefox41 + fixed
firefox-esr31 --- unaffected
firefox-esr38 39+ fixed
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-master --- unaffected

People

(Reporter: MatsPalmgren_bugz, Assigned: bugzilla)

References

Details

(4 keywords, Whiteboard: [adv-main39+][adv-esr38.1+])

Crash Data

Attachments

(1 file)

[Tracking Requested - why for this release]: topcrash

This bug was filed from the Socorro interface and is 
report bp-3c210213-ad9d-4412-b227-c98062150412.
=============================================================

Looks like a recent regression - first reported crash is in build 2015041100.

Currently #40 topcrash in 39.0a2.  Most crashes have:
EXCEPTION_ACCESS_VIOLATION_READ  0x5a5a5a5a
so filing it security-sensitive just in case.
Looks like the signature is 39 specific (although 40 may just be too low volume). Tracking 39 in case this turns into a sec issue.
Aaron - Flagging you because this is plug-in related. Ideas?
Flags: needinfo?(aklotz)
I think what we're seeing here is a runnable that is executing after its class has been destroyed.
Flags: needinfo?(aklotz)
This is a regression but not a high volume crash (40 crashes reported total for the last 7 days). 
The signature isn't showing up at all on Firefox 40.
Untracking for 39 since it's not a topcrash (roughly defined as being in the top 10 for a channel at some point) and this doesn't have a sec rating.   Please do renominate this if it becomes a bigger issue or is sec-high or sec-critical.
Keywords: topcrash
[Tracking Requested - why for this release]:

0x5a5a5a5a is a clear indication of use-after-free, so I think sec-high
is a reasonable rating until we know more.

Also, I disagree with "top 10 for a channel" as the definition of topcrash.
I would say all 50 of the signatures on the "Top Crashers" list for each
channel at crash-stats.com should be considered.  There are always
a handful of signatures that we never track, like OOM|small, EnterBaseline
and such, and there are usually also plenty of dupes that are the same
underlying bug like igd10umd32.dll@ etc.  Usually there are also a few
signatures in binary extensions / plugins that can be ignored.
That leaves about ~30 that I would say should count as topcrash.
(a few are usually on multiple channels, so it's less than 3*30 total).

We don't need to track all of those of course.  But signatures that are
relatively new, like this bug, should *definitely* be tracked.  Engineers
that introduced this signature and/or changed related code around that
date should be asked to investigate.  We really need to address crash
regressions like this promptly, and try hard to fix them before they
reach the release channel.
Keywords: sec-high
Mats, I would love to be able to spend more time helping out with crash bugs. Here I was trying to put a priority on higher volume crashes. I agree with you that we would benefit from more resources to help fix recent regressions no matter what the crash volume is.   I'll track this again for now and see if I can help out. Thanks for re-nomming and explaining your take on it. 

Since this signature first appeared on build 2015041100. April 11th was a Saturday, maybe looking at the changeset from the day before would help. http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?startdate=34+days+ago&enddate=31days+ago   So from that, bug 1152395 looks like the most likely culprit. Aaron, maybe you can take this bug when you get back next week, or maybe someone else will work on it in the meantime.
Flags: needinfo?(aklotz)
Flags: needinfo?(aklotz)
This patch makes it possible to revoke the task in question so that it can't run after the PluginProcessParent object is deleted.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily. It is heavily sensitive to timing where a plugin instantiation and teardown happen in quick succession.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No, it's very vague. It's not even obvious by looking at the code.

Which older supported branches are affected by this flaw?
39+

If not all supported branches, which bug introduced the flaw?
bug 998863

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be a trivial merge.

How likely is this patch to cause regressions; how much testing does it need?
No regressions possible.
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Attachment #8610188 - Flags: sec-approval?
Attachment #8610188 - Flags: review?(jmathies)
Based on the regressing bug then Fx37 and 38 should be affected, too; I also don't see what would have fixed this in 40. Maybe the crashes are more or less likely based on timing affected by other code changes.
Comment on attachment 8610188 [details] [diff] [review]
Add TaskFactory to PluginProcessParent

Normally please wait for a reviewed patch before requesting sec-approval, but in this case it looks straightforward enough. sec-approval=dveditz
Attachment #8610188 - Flags: sec-approval? → sec-approval+
Attachment #8610188 - Flags: review?(jmathies) → review+
(In reply to Daniel Veditz [:dveditz] from comment #9)
> Based on the regressing bug then Fx37 and 38 should be affected, too; I also
> don't see what would have fixed this in 40. Maybe the crashes are more or
> less likely based on timing affected by other code changes.

Good point. Usually for async plugin init the affected code is disabled in 38 and above, but not in this case.
Comment on attachment 8610188 [details] [diff] [review]
Add TaskFactory to PluginProcessParent

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
This is sec-high

User impact if declined:
Possible crashes and UAF under specific timing conditions.

Fix Landed on Version:
41, uplift pending for 39 and 40

Risk to taking this patch (and alternatives if risky): 
None. Simple patch adding revocable tasks which is well tested code.

String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/regressing bug #]: bug 998863
[User impact if declined]: Possible crashes and UAF under specific timing conditions.
[Describe test coverage new/current, TreeHerder]: Plugin mochitests
[Risks and why]: None. Simple patch adding revocable tasks which is well tested code.
[String/UUID change made/needed]: None
Attachment #8610188 - Flags: approval-mozilla-esr38?
Attachment #8610188 - Flags: approval-mozilla-beta?
Attachment #8610188 - Flags: approval-mozilla-aurora?
aaron, you mention this landed in 41. Was that in a different bug? I don't see it landing here. Happy to uplift the fix though.
Flags: needinfo?(aklotz)
Attachment #8610188 - Flags: approval-mozilla-esr38?
Attachment #8610188 - Flags: approval-mozilla-esr38+
Attachment #8610188 - Flags: approval-mozilla-beta?
Attachment #8610188 - Flags: approval-mozilla-beta+
Attachment #8610188 - Flags: approval-mozilla-aurora?
Attachment #8610188 - Flags: approval-mozilla-aurora+
Approving for uplift to esr38, Beta and Aurora.
Just wanted to land them all at once, is all.
Flags: needinfo?(aklotz)
https://hg.mozilla.org/mozilla-central/rev/29a9acde728e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Whiteboard: [adv-main39+][adv-esr38.1+]
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.