crash in mozilla::plugins::PluginAsyncSurrogate::Cast on 42 beta, often with 0xffffffffe5e5e5e5 address

VERIFIED FIXED in Firefox 43

Status

()

defect
--
critical
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: kairo, Assigned: aklotz)

Tracking

(4 keywords)

Trunk
mozilla45
x86
Windows NT
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 wontfix, firefox42+ wontfix, firefox43+ fixed, firefox44+ fixed, firefox45 fixed, firefox-esr38 unaffected)

Details

(Whiteboard: [adv-main43+], crash signature)

Attachments

(1 attachment)

[Tracking Requested - why for this release]:

This bug was filed from the Socorro interface and is 
report bp-5397eb78-fe15-4051-b8c9-129862150924.
=============================================================

Top Stack Frames:
0 	xul.dll 	mozilla::plugins::PluginAsyncSurrogate::Cast(_NPP*) 	dom/plugins/ipc/PluginAsyncSurrogate.cpp
1 	xul.dll 	mozilla::plugins::PluginInstanceParent::PluginInstanceParent(mozilla::plugins::PluginModuleParent*, _NPP*, nsCString const&, _NPNetscapeFuncs const*) 	dom/plugins/ipc/PluginInstanceParent.cpp
2 	xul.dll 	mozilla::plugins::PluginModuleParent::NPP_NewInternal(char*, _NPP*, unsigned short, nsTArray<nsCString>&, nsTArray<nsCString>&, _NPSavedData*, short*) 	dom/plugins/ipc/PluginModuleParent.cpp
3 	xul.dll 	mozilla::plugins::PluginModuleParent::InitAsyncSurrogates() 	dom/plugins/ipc/PluginModuleParent.cpp
4 	xul.dll 	mozilla::plugins::PluginModuleParent::RecvNP_InitializeResult(short const&) 	dom/plugins/ipc/PluginModuleParent.cpp
5 	xul.dll 	mozilla::plugins::PluginModuleChromeParent::RecvNP_InitializeResult(short const&) 	dom/plugins/ipc/PluginModuleParent.cpp
6 	xul.dll 	mozilla::plugins::PPluginModuleParent::OnMessageReceived(IPC::Message const&) 	obj-firefox/ipc/ipdl/PPluginModuleParent.cpp
7 	xul.dll 	mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) 	ipc/glue/MessageChannel.cpp
8 	xul.dll 	mozilla::ipc::MessageChannel::DispatchMessageW(IPC::Message const&) 	ipc/glue/MessageChannel.cpp
9 	xul.dll 	mozilla::ipc::MessageChannel::OnMaybeDequeueOne() 	ipc/glue/MessageChannel.cpp
10 	xul.dll 	MessageLoop::DoWork() 	ipc/chromium/src/base/message_loop.cc
[...]

This is the #5 crash signature with 2% of all crashes in early 42.0 Beta 1 data. A lot of those crashes have a 0xffffffffe5e5e5e5 address (how do we get 64bit addresses on 32bit builds?) and as the lower 32bit of that match our (new) poison-on-free value, that smells like UAF and I'm cautiously marking it security for the moment.

Aaron, this sounds like Async Plugin Init would be involved, can you take a look?
Flags: needinfo?(aklotz)
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #0)
> 0xffffffffe5e5e5e5 address (how do we get 64bit addresses on 32bit builds?)

From IRC:
<dmajor> that's probably a socorro artifact. 'e' has its topmost bit set, so when you sign-extend you get a bunch of f's
Group: core-security → dom-core-security
Looks like pending surrogates aren't being elided when their associated NPP is destroyed. Not sure why this is happening all of a sudden. Investigating...
Flags: needinfo?(aklotz)
Aaron, have you made some progress on this? Thanks
Flags: needinfo?(aklotz)
Flags: needinfo?(aklotz)
Posted patch PatchSplinter Review
I think the most surefire way to deal with this is to convert PluginAsyncSurrogate's reference to the plugin instance from an NPP to a WeakPtr<nsNPAPIPluginInstance>.
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Attachment #8675851 - Flags: review?(jmathies)
Is the npp data invalid by the time we get here?

http://hg.mozilla.org/releases/mozilla-beta/annotate/a5dca53807c6/dom/plugins/ipc/PluginModuleParent.cpp#l2628

We try to cast it at this and crash in some internal code right?

I'm trying to understand how we get here - 

http://hg.mozilla.org/releases/mozilla-beta/annotate/a5dca53807c6/dom/plugins/ipc/PluginModuleParent.cpp#l2370

and find an invalid NPP.

You fix is an attempt to clean up the npp point before we get to this point, correct?
In async init mode, we defer NPP_New at
http://hg.mozilla.org/releases/mozilla-beta/annotate/a5dca53807c6/dom/plugins/ipc/PluginModuleParent.cpp#l2564
NPP is good at this point since this is the entry point where a sync NPP_New would occur, so it's a good time to populate the weak reference.

The deferred NPP_New calls are played back here
http://hg.mozilla.org/releases/mozilla-beta/annotate/a5dca53807c6/dom/plugins/ipc/PluginModuleParent.cpp#l2370
once the result of async NP_Initialize has been returned via RecvNP_InitializeResult

The way that this code is *supposed* to work is that if a NPP_Destroy() comes in before RecvNP_InitializeResult, we just elide that instance's entry from mSurrogateInstances. However, the existence of this bug suggests to me that the nsNPAPIPluginInstance is being destroyed somewhere without a proper call to nsNPAPIPluginInstance::Destroy to take care of that bookkeeping (this is much like the nsPluginInstanceOwner case that I fixed a while back).
Too late for 42 but tracking it for 43.
Attachment #8675851 - Flags: review?(jmathies) → review+
Comment on attachment 8675851 [details] [diff] [review]
Patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily. Highly dependent on user actions and timing.

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 just talks about converting to a WeakPtr.

Which older supported branches are affected by this flaw?
None. Feature is only enabled during beta at the moment.

If not all supported branches, which bug introduced the flaw?
Asynchronous Plugin Initialization, bug 998863.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be easy to hg graft between branches.

How likely is this patch to cause regressions; how much testing does it need?
The existing plugin test suite should suffice.
Attachment #8675851 - Flags: sec-approval?
Whiteboard: [checkin on 11/10]
Comment on attachment 8675851 [details] [diff] [review]
Patch

Sec-approval+ for checkin to trunk on November 10.
Attachment #8675851 - Flags: sec-approval? → sec-approval+
This can be checked in now.
Flags: needinfo?(aklotz)
This landed on m-c on 11/13.

https://hg.mozilla.org/mozilla-central/rev/335c2779216b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [checkin on 11/10]
Target Milestone: --- → mozilla45
Can you please nominate this for uplift to branches as appropriate, Aaron? Thanks.
Flags: needinfo?(aklotz)
Comment on attachment 8675851 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: async plugin init
[User impact if declined]: UAF crashes
[Describe test coverage new/current, TreeHerder]: Plugin test suite
[Risks and why]: Low, simple change to convert raw pointers to use weak pointers
[String/UUID change made/needed]: None
Flags: needinfo?(aklotz)
Attachment #8675851 - Flags: approval-mozilla-beta?
Attachment #8675851 - Flags: approval-mozilla-aurora?
Group: dom-core-security → core-security-release
Comment on attachment 8675851 [details] [diff] [review]
Patch

Taking it as it's a sec fix and has been in Nightly for a few days now.
Attachment #8675851 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8675851 [details] [diff] [review]
Patch

Crash fix, ok to uplift to beta.
Attachment #8675851 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
KaiRo, can you see if this patch has decreased or eliminated the crash?
Flags: needinfo?(kairo)
Kairo's away this week. This signature hasn't been seen since Nov 22.
Flags: needinfo?(kairo)
Thanks Aaron. I'm marking verified based on your info.
Status: RESOLVED → VERIFIED
Whiteboard: [adv-main43+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.