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

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Robert Kaiser]

[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?

Comment 1

[Robert Kaiser]

(In reply to Robert Kaiser (:[e-mail]) 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

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

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...

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Aaron, have you made some progress on this? Thanks

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch

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>.

Comment 5

[Jim Mathies [:jimm]]

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?

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

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).

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Too late for 42 but tracking it for 43.

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 9

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

Comment on attachment 8675851 [details] [diff] [review]
Patch

Sec-approval+ for checkin to trunk on November 10.

Comment 10

[Andrew McCreight [:mccr8]]

This can be checked in now.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/335c2779216b

Comment 12

[Andrew McCreight [:mccr8]]

This landed on m-c on 11/13.

https://hg.mozilla.org/mozilla-central/rev/335c2779216b

Comment 13

[Andrew McCreight [:mccr8]]

Can you please nominate this for uplift to branches as appropriate, Aaron? Thanks.

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 16

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/ff8b0e450582

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8675851 [details] [diff] [review]
Patch

Crash fix, ok to uplift to beta.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7c7fdd813867

Comment 19

[Matt Wobensmith [:mwobensmith][:matt:]]

KaiRo, can you see if this patch has decreased or eliminated the crash?

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

Kairo's away this week. This signature hasn't been seen since Nov 22.

Comment 21

[Matt Wobensmith [:mwobensmith][:matt:]]

Thanks Aaron. I'm marking verified based on your info.