Closed Bug 1171453 Opened 6 years ago Closed 6 years ago

crash in mozilla::plugins::PluginAsyncSurrogate::ScriptableInvalidate mostly with, often with 0x5a5a5a66 address


(Core :: Plug-ins, defect)

Windows NT
Not set



Tracking Status
firefox38.0.5 --- unaffected
firefox39 + fixed
firefox40 + fixed
firefox41 + 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-master --- unaffected


(Reporter: kairo, Assigned: bugzilla)



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

Crash Data


(1 file)

[Tracking Requested - why for this release]:

This bug was filed from the Socorro interface and is 
report bp-fc56b0d3-8b7e-4a35-a05a-fb6e02150602.

This is a new crash in 39.0 beta, seems to be connected to async plugin init (given the signature mentioning "PluginAsyncSurrogate"), and most of the addresses are 0x5a5a5a66, which is a small offset from the memory poison-on-free value so I'm marking it security due to possible use-after-free.
This is #5 in Top Crash Score for 39.0b2, mostly due to people hitting it repeatedly, right now it's listed there with 3.3 crashes per installation.

Stack frames:
0 	xul.dll 	mozilla::plugins::PluginAsyncSurrogate::ScriptableInvalidate(NPObject*) 	dom/plugins/ipc/PluginAsyncSurrogate.cpp
1 	xul.dll 	NPObjWrapperPluginDestroyedCallback 	dom/plugins/base/nsJSNPRuntime.cpp
2 	xul.dll 	nsJSNPRuntime::OnPluginDestroy(_NPP*) 	dom/plugins/base/nsJSNPRuntime.cpp
3 	xul.dll 	nsNPAPIPluginInstance::Stop() 	dom/plugins/base/nsNPAPIPluginInstance.cpp
4 	xul.dll 	nsNPAPIPluginInstance::Stop() 	dom/plugins/base/nsNPAPIPluginInstance.cpp
5 	xul.dll 	nsPluginHost::StopPluginInstance(nsNPAPIPluginInstance*) 	dom/plugins/base/nsPluginHost.cpp
6 	xul.dll 	nsObjectLoadingContent::DoStopPlugin(nsPluginInstanceOwner*, bool, bool) 	dom/base/nsObjectLoadingContent.cpp
7 	xul.dll 	nsObjectLoadingContent::StopPluginInstance() 	dom/base/nsObjectLoadingContent.cpp
8 	xul.dll 	CheckPluginStopEvent::Run() 	dom/base/nsObjectLoadingContent.cpp
9 	xul.dll 	nsBaseAppShell::RunSyncSectionsInternal(bool, unsigned int) 	widget/nsBaseAppShell.cpp
10 	xul.dll 	nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal*, bool, unsigned int) 	widget/nsBaseAppShell.cpp
11 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp

Top URLs:
12 	about:blank

So this is vastly dominated by but there are a few others in there.
Flags: needinfo?
Flags: needinfo? → needinfo?(aklotz)
Assignee: nobody → aklotz
Flags: needinfo?(aklotz)
Duplicate of this bug: 1171532
Good STR for this one:
1) Install the Google Earth Plugin
2) Go to
3) Click "View Live in Google Earth"
4) In the new tab that comes up, activate the plugin
5) Once everything has loaded, close the tab.
6) Kablooey
(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #2)
> 6) Kablooey

FYI that is a technical term and I won't discuss this issue unless you use the correct nomenclature.
Attached patch FixSplinter Review
ParentNPObjects need to be made aware of AsyncNPObjects. When the JSNP runtime cleans up the objects for an instance, it tries to delete the ParentNPObjects directly without checking reference counts. This is bad if there are still some AsyncNPObjects that are referencing the ParentNPObject being deleted.

This patch modifies the deallocation code for ParentNPObjects to be aware of references from AsyncNPObjects and to only delete itself if there are no more AsyncNPObjects referencing it.

Note that the AsyncNPObjects themselves will be force-deleted, which will then cause them to release their references to the ParentNPObjects in the usual way.
Attachment #8620451 - Flags: review?(jmathies)
Attachment #8620451 - Flags: review?(jmathies) → review+
Comment on attachment 8620451 [details] [diff] [review]

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Difficult. It's sensitive to prefs, timing, plugin, and content.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

There is a comment that mentions that we need to keep an object alive instead of deallocating because it is still referenced. I tried to be a bit vague here but I think that the comment is critical to long-term maintenance of this code.

Which older supported branches are affected by this flaw?

The bug is present on all branches, but is preffed off on all branches except for beta 39. 39 is going to be disabled today as I have decided that async init isn't ready to go to release. When 40 moves to beta it will be preffed on again on that channel.

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


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

It should be easy to backport; hg should be able to merge this with ease.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely; we have good STR in this bug to ensure that the fix works. If there are regressions they would be as memory leaks which would be picked up by our tests.
Attachment #8620451 - Flags: sec-approval?
Comment on attachment 8620451 [details] [diff] [review]


Let's get this fix onto branches too.
Attachment #8620451 - Flags: sec-approval? → sec-approval+
Duplicate of this bug: 1173879
Comment on attachment 8620451 [details] [diff] [review]

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

User impact if declined:
Crashes, security risk due to UAF

Fix Landed on Version:

Risk to taking this patch (and alternatives if risky): 
Low risk. Patch is simple, fix manually verified via STR. Only risk is memory leaks which would be caught by test suites.

String or UUID changes made by this patch:

See for more info.

Approval Request Comment
[Feature/regressing bug #]: async plugin init
[User impact if declined]: Crashes, security risk due to UAF
[Describe test coverage new/current, TreeHerder]: Plugin test suite
[Risks and why]: Low risk. Patch is simple, fix manually verified via STR. Only risk is memory leaks which would be caught by test suites.
[String/UUID change made/needed]: None
Attachment #8620451 - Flags: approval-mozilla-esr38?
Attachment #8620451 - Flags: approval-mozilla-beta?
Attachment #8620451 - Flags: approval-mozilla-aurora?
Comment on attachment 8620451 [details] [diff] [review]

Do we need this on ESR38 if it is preff'd off?
Attachment #8620451 - Flags: approval-mozilla-beta?
Attachment #8620451 - Flags: approval-mozilla-beta+
Attachment #8620451 - Flags: approval-mozilla-aurora?
Attachment #8620451 - Flags: approval-mozilla-aurora+
Not really, I was just being a bit aggressive in my uplift requests. :-)
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Attachment #8620451 - Flags: approval-mozilla-esr38?
Whiteboard: [adv-main39-]
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.