Closed Bug 1141081 (CVE-2015-2706) Opened 9 years ago Closed 9 years ago

crash in AsyncPaintWaitEvent::AsyncPaintWaitEvent(nsIContent*, bool), often with 0x5a5a5a5e address

Categories

(Core Graveyard :: Plug-ins, defect)

x86
Windows NT
defect
Not set
critical

Tracking

(firefox36 wontfix, firefox37+ fixed, firefox38+ verified, firefox39+ fixed, firefox40 fixed, firefox-esr3137+ fixed, firefox-esr38 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
mozilla40
Tracking Status
firefox36 --- wontfix
firefox37 + fixed
firefox38 + verified
firefox39 + fixed
firefox40 --- fixed
firefox-esr31 37+ fixed
firefox-esr38 --- 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: kairo, Assigned: bugzilla)

References

Details

(Keywords: crash, csectype-uaf, sec-high, Whiteboard: [adv-main37+])

Crash Data

Attachments

(4 files, 2 obsolete files)

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.)
The CC is just in the stack because nsWindowRoot is cycle collected.
Assignee: nobody → aklotz
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.
> 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.
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)
Josh, is this likely from your patch? There were also a couple other plugin checkins in that range (evilpie's, for instance).
Flags: needinfo?(joshmoz)
Bug 1109424 was also in that regression range but seems less likely.
(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?
[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.
Crash Signature: [@ nsCOMPtr<nsILoadGroup>::nsCOMPtr<nsILoadGroup>(nsILoadGroup*) | AsyncPaintWaitEvent::AsyncPaintWaitEvent(nsIContent*, bool)] → [@ nsCOMPtr<nsILoadGroup>::nsCOMPtr<nsILoadGroup>(nsILoadGroup*) | AsyncPaintWaitEvent::AsyncPaintWaitEvent(nsIContent*, bool)] [@ nsRefPtr<mozilla::EventStateManager>::nsRefPtr<mozilla::EventStateManager>(mozilla::EventStateManager*) | AsyncPaintWaitEve…
Summary: crash in syncPaintWaitEvent::AsyncPaintWaitEvent(nsIContent*, bool), often with 0x5a5a5a5e address → crash in AsyncPaintWaitEvent::AsyncPaintWaitEvent(nsIContent*, bool), often with 0x5a5a5a5e address
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.
Tracking sec-high crash. Also confirmed that aklotz is working on this today.
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.
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.
Attachment #8579081 - Flags: review?(jmathies)
Attachment #8579081 - Flags: review?(jmathies) → review+
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
Attachment #8579081 - Flags: approval-mozilla-beta?
Attachment #8579081 - Flags: approval-mozilla-aurora?
This should have gotten sec-approval before landing.
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.
Attachment #8579081 - Flags: sec-approval?
(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.
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/4f0b849ae2b0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Attachment #8579081 - Flags: sec-approval?
Attachment #8579081 - Flags: sec-approval+
Attachment #8579081 - Flags: approval-mozilla-beta?
Attachment #8579081 - Flags: approval-mozilla-beta+
Attachment #8579081 - Flags: approval-mozilla-aurora?
Attachment #8579081 - Flags: approval-mozilla-aurora+
Crash Signature: , bool)] [@ nsCOMPtr<imgIContainer>::nsCOMPtr<imgIContainer>(imgIContainer*) | AsyncPaintWaitEvent::AsyncPaintWaitEvent(nsIContent*, bool)] → , bool)] [@ nsCOMPtr<imgIContainer>::nsCOMPtr<imgIContainer>(imgIContainer*) | AsyncPaintWaitEvent::AsyncPaintWaitEvent(nsIContent*, bool)] [@ nsCOMPtr<nsIScreen>::nsCOMPtr<nsIScreen>(nsIScreen*) | AsyncPaintWaitEvent::AsyncPaintWaitEvent(nsIContent*, b…
So is this "won't fix" for ESR31?
Flags: needinfo?(lmandel)
Whiteboard: [adv-main37+]
(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.
Flags: needinfo?(lmandel)
Flags: needinfo?(bkerensa)
Flags: needinfo?(aklotz)
Flags: needinfo?(abillings)
(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.
Flags: needinfo?(abillings)
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?
[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.
Flags: needinfo?(aklotz)
Attachment #8582844 - Flags: review+
Attachment #8582844 - Flags: approval-mozilla-esr31?
Attachment #8582844 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
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.
Crash Signature: , bool)] → , bool)] [@ nsCOMPtr<nsIXPCScriptable>::nsCOMPtr<nsIXPCScriptable>(nsIXPCScriptable*) | AsyncPaintWaitEvent::AsyncPaintWaitEvent(nsIContent*, bool)]
Flags: needinfo?(aklotz)
Grrr, we must be missing more nsPluginInstanceOwner::Destroy calls somewhere. I tried locating them all, but there must still be something missing.
Status: RESOLVED → REOPENED
Flags: needinfo?(aklotz)
Resolution: FIXED → ---
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?
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.
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.
Crash Signature: , bool)] [@ nsCOMPtr<nsIXPCScriptable>::nsCOMPtr<nsIXPCScriptable>(nsIXPCScriptable*) | AsyncPaintWaitEvent::AsyncPaintWaitEvent(nsIContent*, bool)] → , bool)] [@ nsCOMPtr<nsIXPCScriptable>::nsCOMPtr<nsIXPCScriptable>(nsIXPCScriptable*) | AsyncPaintWaitEvent::AsyncPaintWaitEvent(nsIContent*, bool)] [@ nsCOMPtr<nsIInterfaceInfo>::nsCOMPtr<nsIInterfaceInfo>(nsIInterfaceInfo*) | AsyncPaintWaitEvent::Asyn…
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
[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.
Attachment #8587737 - Flags: sec-approval?
Attachment #8587737 - Flags: review?(jmathies)
Also: This patch is likely to clear up bug 1128064 as well: a 2 for 1 deal!
Comment on attachment 8587737 [details] [diff] [review]
null out mContent in nsPluginInstanceOwner::Destroy

sec-approval+
Attachment #8587737 - Flags: sec-approval? → sec-approval+
Attached patch Add proper nsWeakPtr support (obsolete) — Splinter Review
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.
Attachment #8587737 - Attachment is obsolete: true
Attachment #8587737 - Flags: review?(jmathies)
Attachment #8588375 - Flags: sec-approval?
Attachment #8588375 - Flags: review?(jmathies)
Attachment #8588375 - Flags: review?(bugs)
Given that the new patch is needed on all channels, I'm resetting all fixed to affected.
Crash Signature: , bool)] [@ nsCOMPtr<nsIInterfaceInfo>::nsCOMPtr<nsIInterfaceInfo>(nsIInterfaceInfo*) | AsyncPaintWaitEvent::AsyncPaintWaitEvent(nsIContent*, bool)] → , bool)] [@ nsCOMPtr<nsIInterfaceInfo>::nsCOMPtr<nsIInterfaceInfo>(nsIInterfaceInfo*) | AsyncPaintWaitEvent::AsyncPaintWaitEvent(nsIContent*, bool)] [@ nsRefPtr<mozilla::dom::DocumentFragment>::nsRefPtr<mozilla::dom::DocumentFragment>(mozilla::dom::Docu…
Comment on attachment 8588375 [details] [diff] [review]
Add proper nsWeakPtr support

sec-approval+
Attachment #8588375 - Flags: sec-approval? → sec-approval+
Comment on attachment 8588375 [details] [diff] [review]
Add proper nsWeakPtr support

Looks ok to me.
Attachment #8588375 - Flags: review?(jmathies) → review+
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.
Attachment #8588375 - Flags: review?(bugs) → review-
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
Attachment #8589247 - Flags: review+
Attachment #8589247 - Flags: approval-mozilla-release?
Attachment #8589247 - Flags: approval-mozilla-beta?
Attachment #8589247 - Flags: approval-mozilla-aurora?
Attachment #8589247 - Flags: approval-mozilla-beta?
Attachment #8589247 - Flags: approval-mozilla-beta+
Attachment #8589247 - Flags: approval-mozilla-aurora?
Attachment #8589247 - Flags: approval-mozilla-aurora+
Attachment #8588375 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ee8c9c1c2bec
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: mozilla39 → mozilla40
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 on attachment 8589247 [details] [diff] [review]
Add proper nsWeakPtr support (r2)

Goes without saying that this needs some serious rebasing for esr31 :)
Flags: needinfo?(aklotz)
Here it is!
Flags: needinfo?(aklotz)
Attachment #8590374 - Flags: review+
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 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+
Attachment #8589247 - Flags: approval-mozilla-release? → approval-mozilla-release+
Alias: CVE-2015-2706
(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.
Group: core-security
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.