1141081 - (CVE-2015-2706) crash in AsyncPaintWaitEvent::AsyncPaintWaitEvent(nsIContent*, bool), often with 0x5a5a5a5e address

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Robert Kaiser]

This bug was filed from the Socorro interface and is 
report bp-b508abe3-d725-42e3-927c-a650e2150306.
=============================================================

Top frames:
0 	xul.dll 	nsCOMPtr<nsILoadGroup>::nsCOMPtr<nsILoadGroup>(nsILoadGroup*) 	xpcom/glue/nsCOMPtr.h
1 	xul.dll 	AsyncPaintWaitEvent::AsyncPaintWaitEvent(nsIContent*, bool) 	dom/plugins/base/nsPluginInstanceOwner.cpp
2 	xul.dll 	nsPluginInstanceOwner::~nsPluginInstanceOwner() 	dom/plugins/base/nsPluginInstanceOwner.cpp
3 	xul.dll 	nsPluginInstanceOwner::`scalar deleting destructor'(unsigned int) 	
4 	xul.dll 	nsPluginInstanceOwner::Release() 	dom/plugins/base/nsPluginInstanceOwner.cpp
5 	xul.dll 	nsTArray_Impl<mozilla::EventListenerManager::Listener, nsTArrayInfallibleAllocator>::DestructRange(unsigned int, unsigned int) 	xpcom/glue/nsTArray.h
6 	xul.dll 	nsTArray_Impl<mozilla::EventListenerManager::Listener, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned int, unsigned int) 	xpcom/glue/nsTArray.h
7 	xul.dll 	mozilla::EventListenerManager::RemoveAllListeners() 	dom/events/EventListenerManager.cpp
8 	xul.dll 	nsWindowRoot::~nsWindowRoot() 	dom/base/nsWindowRoot.cpp
9 	xul.dll 	nsWindowRoot::`scalar deleting destructor'(unsigned int) 	
10 	xul.dll 	nsWindowRoot::DeleteCycleCollectable() 	dom/base/nsWindowRoot.cpp
11 	xul.dll 	nsWindowRoot::cycleCollection::DeleteCycleCollectable(void*) 	dom/base/nsWindowRoot.h
12 	xul.dll 	SnowWhiteKiller::~SnowWhiteKiller() 	xpcom/base/nsCycleCollector.cpp
[...]

So far, we only see this with Firefox 37.0b3, but this gets high exploitability rating and an address 0x5a5a5a5e (offset 4 from the memory poisoning) in many cases, so I'm cautiously marking it a security issue. It seems to involve plugins and CC, judging from the stack.
(Click on the Crash Signature field to get more stats and reports.)

Comment 1

[Andrew McCreight [:mccr8]]

The CC is just in the stack because nsWindowRoot is cycle collected.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

nsIContent                 *mContent; // WEAK, content owns us
in nsPluginInstanceOwner. And mContent is passed to AsyncPaintWaitEvent...
That at least looks very suspicious. It used to be nsCOMPtr before Bug 90268.

But I'm not at all familiar with this code.

Comment 3

[(Away)]

> So far, we only see this with Firefox 37.0b3
I see variants of this on all three betas so far (comdat folding is probably messing with those comptr signatures) but it definitely started with 37.

Comment 4

[(Away)]

In fact it looks like this began with nightly 20141212030201. https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0cf461e62ce5&tochange=5288b15d22de

Possibly this?
	a6db8f54f5aa	Josh Aas — Bug 1092630: Get rid of native widgets for OS X NPAPI plugins, make things work much better under e10s. Patch by Josh Aas, Markus Stange, Steven Michaud, David Parks. r=smichaud/jst/josh (more reviews pending)

Comment 5

[Daniel Veditz [:dveditz]]

Josh, is this likely from your patch? There were also a couple other plugin checkins in that range (evilpie's, for instance).

Comment 6

[Daniel Veditz [:dveditz]]

Bug 1109424 was also in that regression range but seems less likely.

Comment 7

[(Away)]

(In reply to Daniel Veditz [:dveditz] from comment #5)
> Josh, is this likely from your patch? There were also a couple other plugin
> checkins in that range (evilpie's, for instance).

This has been sitting for a while. Can anyone else confirm whether that's the right bug?

Comment 8

[Robert Kaiser]

[Tracking Requested - why for this release]:

This seems to be around in every beta for 37, but changes signatures in different builds (the first frame changes, it's always some kind of nsRefPtr or nsCOMPtr, the second frame stays the same).

See https://crash-stats.mozilla.com/search/?product=Firefox&signature=~AsyncPaintWaitEvent%3A%3AAsyncPaintWaitEvent&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature for a search of all those signatures, I'm adding some to this bug.

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

There are a few bugs right now with strange circumstances around construction/destruction of nsPluginInstanceOwner objects. Bug 1143133 and bug 1128064 both come to mind.

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

Tracking sec-high crash. Also confirmed that aklotz is working on this today.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

It is Josh et al's patch that delivered this call stack, but from what I'm seeing it is not the root cause.

That patch added event listeners to the window root. They should have been removed when nsPluginInstanceOwner::Destroy was called. I'm suspecting that somewhere we missed calling nsPluginInstanceOwner::Destroy and a bunch of stuff wasn't cleaned up properly because of it.

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Ensure nsPluginInstanceOwner::Destroy is called before returning from failed plugin instantiation

I found some places where we weren't calling nsPluginInstanceOwner::Destroy but should have been. It is unclear whether this site will fix everything or not, but it's the only place I found where there is clearly some missing calls.

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f0b849ae2b0

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8579081 [details] [diff] [review]
Ensure nsPluginInstanceOwner::Destroy is called before returning from failed plugin instantiation

Approval Request Comment
[Feature/regressing bug #]: Plugin initialization
[User impact if declined]: Potential security issue, crashes
[Describe test coverage new/current, TreeHerder]: try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e710560eb96
[Risks and why]: Low, very simple patch
[String/UUID change made/needed]: None

Comment 15

[Andrew McCreight [:mccr8]]

This should have gotten sec-approval before landing.

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8579081 [details] [diff] [review]
Ensure nsPluginInstanceOwner::Destroy is called before returning from failed plugin instantiation

Sorry, I was unaware of the sec-approval process until now. I can back out the inbound landing once the trees reopen if necessary.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It would not be obvious from looking at the patch how to get to the crashes at issue in this bug. Furthermore, it would also depend on being able to induce a failure during plugin instantiation.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I would say no. It's not obvious from the comment that adding Destroy() calls corrects a potential use-after-free issue. In fact, at initial glance that seems counter intuitive.

Which older supported branches are affected by this flaw?
All supported branches.

If not all supported branches, which bug introduced the flaw?
N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Yes. This patch will easily merge into the other branches.

How likely is this patch to cause regressions; how much testing does it need?
Extremely unlikely. This patch calls some teardown code that is already extensively tested as part of normal plugin shutdown scenarios.

Comment 17

[Andrew McCreight [:mccr8]]

(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #16)
> I can back out the inbound landing once the trees reopen if necessary.
The patch is already public, so that wouldn't help.  The goal of the sec-approval process is to minimize the time between public exposure of a patch and when we ship the fix to users, without developers having to worry about things like where we are in the beta cycle.

Comment 18

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #16)
> Which older supported branches are affected by this flaw?
> All supported branches.

Based on the fact that 36 is marked as unaffected, I think this is incorrect. Note that ESR 31 is a supported branch.

Comment 19

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #18)
> (In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #16)
> > Which older supported branches are affected by this flaw?
> > All supported branches.
> 
> Based on the fact that 36 is marked as unaffected, I think this is
> incorrect. Note that ESR 31 is a supported branch.

The same code that is at issue in this patch is also present in ESR31. The specific changes that generate this particular signature were added in a later release, however the root cause is still present.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4f0b849ae2b0

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0e5b241da92a
https://hg.mozilla.org/releases/mozilla-beta/rev/2710769c40a5

Comment 22

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

So is this "won't fix" for ESR31?

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #19)
> The same code that is at issue in this patch is also present in ESR31. The
> specific changes that generate this particular signature were added in a
> later release, however the root cause is still present.

(In reply to Al Billings [:abillings] from comment #22)
> So is this "won't fix" for ESR31?

Based on Aaron's response I think ESR31 is affected. ni aklotz to ensure I read that right.

Al - I assume that the sec team would prefer to backport this sec-high fix to ESR. Can you confirm?

Ben - Another bug that looks like it may be required for ESR.

Comment 24

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

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #23)

> Al - I assume that the sec team would prefer to backport this sec-high fix
> to ESR. Can you confirm?


Ideally we would.

Comment 25

[Lawrence Mandel [:lmandel] (use needinfo)]

I think taking this makes sense because it has already landed on 37-39 as long as we have time to rebuild, which we probably do.

aklotz - Can you please prepare a patch for ESR31?

Comment 26

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Ensure nsPluginInstanceOwner::Destroy is called before returning from failed plugin instantiation (ESR31)

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a sec:high bug.
User impact if declined: Potential security issue, crashes
Fix Landed on Version: 37, 38, 39
Risk to taking this patch (and alternatives if risky): Low, very simple, well understood patch
String or UUID changes made by this patch: None

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/180e63ae2aef

Comment 28

[Robert Kaiser]

Hrm, we still have https://crash-stats.mozilla.com/report/list?signature=nsCOMPtr%3CnsIXPCScriptable%3E%3A%3AnsCOMPtr%3CnsIXPCScriptable%3E%28nsIXPCScriptable%2A%29%20%7C%20AsyncPaintWaitEvent%3A%3AAsyncPaintWaitEvent%28nsIContent%2A%2C%20bool%29 in 37.0b7, where this bug has landed.

Aaron, any idea what's up?

I guess we probably need to reopen the bug.

Comment 29

[(No longer employed by Mozilla) Aaron Klotz]

Grrr, we must be missing more nsPluginInstanceOwner::Destroy calls somewhere. I tried locating them all, but there must still be something missing.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Perhaps add a stack class which calls Destroy by default, and one needs to explicitly call
some "success" method on it to prevent Destroy call?

Comment 31

[(No longer employed by Mozilla) Aaron Klotz]

Yeah, that's definitely a reasonable thing to do. I've got to enumerate all of the sites that would need this, though -- there's some place that needs it that doesn't have anything, and it's not obvious.

Comment 32

[Daniel Veditz [:dveditz]]

The crash rate seems to have gotten slightly worse since this fix, somewhere around 2800 AsyncPaintWaitEvent crashes a week submitted using 37.0 before 3/19, and about 3050 the week after (of course maybe a 9% fluctuation is normal noise). I only see the <nsIXPCScriptable> variant in the 3/19 builds and not at all before then; they now account for about half the crashes. Looks like the crash just moved around a bit.

Comment 33

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I don't think there is any way to verify the fix on ESR... there are no steps to reproduce, and I did not find any crashes in AsyncPaintWaitEvent::AsyncPaintWaitEvent(nsIContent*, bool) for Firefox ESR [1]. If anyone knows of any way to manually verify this on ESR, please let us know.

[1] - https://crash-stats.mozilla.com/search/?signature=~AsyncPaintWaitEvent%3A%3AAsyncPaintWaitEvent(nsIContent*%2C+bool)&product=Firefox&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

Comment 34

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: null out mContent in nsPluginInstanceOwner::Destroy

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It would be difficult given that executing an exploit would need to be perfectly timed during plugin destruction.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The check-in comment is vague. A comment in the patch points out the object lifetime issues surrounding this bug.

Which older supported branches are affected by this flaw?
All branches including ESR31, but worsened in 37+ because of changes made in bug 1092630.

If not all supported branches, which bug introduced the flaw?
bug 1092630 worsened the situation.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch should merge easily with all affected branches.

How likely is this patch to cause regressions; how much testing does it need?
Extremely unlikely. Running a full test suite on treeherder should be sufficient.

Comment 35

[(No longer employed by Mozilla) Aaron Klotz]

Also: This patch is likely to clear up bug 1128064 as well: a 2 for 1 deal!

Comment 36

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

Comment on attachment 8587737 [details] [diff] [review]
null out mContent in nsPluginInstanceOwner::Destroy

sec-approval+

Comment 37

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Add proper nsWeakPtr support

Sorry for the review/approval spam. I decided that given the way that nsPluginInstanceOwner is used, the best way to mitigate this stuff is to convert nsPluginInstanceOwner to use a proper weak pointer. Olli, can you please review the HTMLObjectElement part of this patch?

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It would be difficult given that executing an exploit would need to be perfectly timed during plugin destruction.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No, there is just a mention of a conversion to proper weak references.

Which older supported branches are affected by this flaw?
All branches including ESR31, but worsened in 37+ because of changes made in bug 1092630.

If not all supported branches, which bug introduced the flaw?
bug 1092630 worsened the situation.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Most affected branches should be a simple merge, except for ESR31. Nonetheless, an ESR31 variant would be easy to create and is actually a smaller patch than for the other versions.

How likely is this patch to cause regressions; how much testing does it need?
Extremely unlikely. Running a full test suite on treeherder should be sufficient.

Comment 38

[Robert Kaiser]

Given that the new patch is needed on all channels, I'm resetting all fixed to affected.

Comment 39

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

Comment on attachment 8588375 [details] [diff] [review]
Add proper nsWeakPtr support

sec-approval+

Comment 40

[Jim Mathies [:jimm]]

Comment on attachment 8588375 [details] [diff] [review]
Add proper nsWeakPtr support

Looks ok to me.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8588375 [details] [diff] [review]
Add proper nsWeakPtr support

All the nodes support nsISupportsWeakReference already.
See bug 360291, and the current code for Element
http://mxr.mozilla.org/mozilla-central/source/dom/base/FragmentOrElement.cpp?rev=f43843d3fb0e&mark=1951-1952#1943

So r- for the changes to HTMLObjectElement, but as far as I see, those are just useless.

Random comment, nsINode::OwnerDoc() returns always non-null value.

Comment 42

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Add proper nsWeakPtr support (r2)

Patch is slightly simpler now since smaug pointed out that the DOM changes were unnecessary. Carrying forward jimm's r+. It's basically the same patch minus 3 lines so I'm not going to bug everybody with sec-approval again unless somebody suggests otherwise.

Approval Request Comment
[Feature/regressing bug #]: bug 90268 + bug 1092630 = fail
[User impact if declined]: Crashes, possible security risk
[Describe test coverage new/current, TreeHerder]: Try and locally, verified that weak reference works correctly and plugin unit tests pass.
[Risks and why]: Low, simple conversion from raw pointer to weak reference.
[String/UUID change made/needed]: None

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee8c9c1c2bec

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5b448d48857f

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ee8c9c1c2bec

Comment 48

[u279076]

A note about bug 1152379, it's the #5 topcrash in Firefox 37 and only happens in 37.0.1. I'd consider nominating this as a 37.0.2 driver.

Comment 49

[Ryan VanderMeulen [:RyanVM]]

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

Comment 50

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8589247 [details] [diff] [review]
Add proper nsWeakPtr support (r2)

Goes without saying that this needs some serious rebasing for esr31 :)

Comment 51

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Add proper nsWeakPtr support (esr31)

Here it is!

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/67cb150788e5

Comment 53

[Robert Kaiser]

Both the signature cluster from here and the signature from bug 1128064 are gone in 38.0b3, so I can verify this as fixed on 38.

Comment 54

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8589247 [details] [diff] [review]
Add proper nsWeakPtr support (r2)

With the fix verified, we're going to take this change in 37.0.2. Release+

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/fa7d8b9db216

Comment 56

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #33)
> I don't think there is any way to verify the fix on ESR... there are no
> steps to reproduce, and I did not find any crashes in
> AsyncPaintWaitEvent::AsyncPaintWaitEvent(nsIContent*, bool) for Firefox ESR
> [1]. If anyone knows of any way to manually verify this on ESR, please let
> us know.
> 
> [1] -
> https://crash-stats.mozilla.com/search/
> ?signature=~AsyncPaintWaitEvent%3A%3AAsyncPaintWaitEvent(nsIContent*%2C+bool)
> &product=Firefox&_facets=signature&_columns=date&_columns=signature&_columns=
> product&_columns=version&_columns=build_id&_columns=platform#facet-signature

Socorro still shows no data for ESR so I think this cannot be verified for ESR.