[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?
(In reply to Robert Kaiser (:firstname.lastname@example.org) 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
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...
Aaron, have you made some progress on this? Thanks
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.
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?
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.
This landed on m-c on 11/13. https://hg.mozilla.org/mozilla-central/rev/335c2779216b
Can you please nominate this for uplift to branches as appropriate, Aaron? Thanks.
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 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?
Kairo's away this week. This signature hasn't been seen since Nov 22.
Thanks Aaron. I'm marking verified based on your info.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.