[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: 39 https://email19.asia.secureserver.net/pcompose.php 23 https://email18.secureserver.net/pcompose.php 23 http://airlink.ubnt.com/ 15 https://email16.secureserver.net/window/compose 15 https://email07.europe.secureserver.net/pcompose.php 15 https://email15.secureserver.net/pcompose.php 13 https://email19.asia.secureserver.net/window/compose 12 about:blank 10 https://email14.secureserver.net/pcompose.php 10 https://email19.asia.secureserver.net/webmail.php?folder=INBOX&firstMessage=1 10 https://email18.secureserver.net/webmail.php?login=1 9 https://email14.secureserver.net/webmail.php?login=1 8 https://email24.secureserver.net/pcompose.php 7 https://email06.secureserver.net/webmail.php?login=1 7 https://email13.secureserver.net/pcompose.php 7 https://email07.europe.secureserver.net/webmail.php?login=1 7 https://email11.secureserver.net/pcompose.php 6 https://email19.asia.secureserver.net/webmail.php?login=1 6 http://my.xfinity.com/?cid=customer 6 https://email06.secureserver.net/pcompose.php 6 http://cwsdemo.intergraphgovsolutions.com/cwsfrontend/Main.aspx?product=MDG&s... 6 https://email10.secureserver.net/pcompose.php 6 https://email15.secureserver.net/webmail.php?login=1 6 http://internationalstarregistrant.com/component/preview/ [...] So this is vastly dominated by secureserver.net but there are a few others in there.

Good STR for this one: 1) Install the Google Earth Plugin 2) Go to http://internationalstarregistrant.com/component/preview/ 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.

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.

Comment on attachment 8620451 [details] [diff] [review] Fix [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? N/A 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.

Comment on attachment 8620451 [details] [diff] [review] Fix sec-approval+. Let's get this fix onto branches too.

ttps://hg.mozilla.org/integration/fx-team/rev/ab13fee627df

Comment on attachment 8620451 [details] [diff] [review] Fix [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: 41 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: None See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process 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

Comment on attachment 8620451 [details] [diff] [review] Fix Do we need this on ESR38 if it is preff'd off?

Not really, I was just being a bit aggressive in my uplift requests. :-)

https://hg.mozilla.org/releases/mozilla-aurora/rev/93978bbbf421 https://hg.mozilla.org/releases/mozilla-beta/rev/7898db26f3f8