1333887 - Crash in nsNPAPIPluginInstance::GetIsOOP

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

This bug was filed from the Socorro interface and is 
report bp-0b838817-22cb-4b10-8b40-96c872170125.
=============================================================

This is a UAF crash going back to at least Firefox 31ish.

Interestingly, there are no crashes in 52 or later, and the code is still there.  Affects release 51 and ESR 45 I'm sure.

Comment 1

[Frederik Braun [:freddy] (OOO until August 18th)]

Karl, this looks like code you have been involved with. Can you take a look?
If you can not, please redirect as you deem fit, but as a a security problem, we want this fixed ASAP.

Comment 2

[Karl Tomlinson (:karlt)]

https://crash-stats.mozilla.com/report/index/5e0d015f-db82-4268-a777-5f2212170207
is 52.0b3.  Different stack to that in comment 0, but similar to other 51 crashes.

https://crash-stats.mozilla.com/report/index/455bfd22-8bd4-4099-bcd5-836422170206
is one of a minority of reports without a truncated stack.
I don't know whether the high frequency of truncated stack reports is
significant, or typical because of poor stack walking code or cfi generation.

nsBlockFrame::BuildDisplayList() is not a function I'd expect a foreign object
to be able to call.

Comment 3

[Karl Tomlinson (:karlt)]

nsPluginInstanceOwner::UseAsyncRendering() has just read
nsPluginInstanceOwner::mInstance, and so I expect the nsNPAPIPluginInstance is
still alive.

The virtual PluginLibrary::IsOOP() implementations just return true or false,
and so I doubt the crash is within the IsOOP() implementation.

If the object referenced by |library| were freed, and |library| still
contained the object's address then I'd expect the vtbl pointer to contain
0xe5e5e5e5.  IsOOP() is not the first virtual function on PluginLibrary, and
so the virtual function address would be stored at an offset to the vtbl
pointer value.  A read of this function address would usually crash at a
different address to the one reported here.

That leaves the possibility that |library| is already 0xe5e5e5e5, and so the
read of the vtbl pointer crashes on read from 0xe5e5e5e5, consistent with the
address reported here.

The use of the bad read for a function address may be sec-critical.

sec-high may be more appropriate if the bug is difficult to trigger, but I
don't know that.

nsNPAPIPluginInstance uses a bare pointer mPlugin to call
nsNPAPIPlugin::GetLibrary().  My suspicion is that this nsNPAPIPlugin has been
freed and so the read of its mLibrary returns 0xe5e5e5e5.  There is no
documentation on mPlugin to indicate the ownership model for the
nsNPAPIPlugin.

The next step I suggest is getting someone familiar with nsNPAPIPlugin ownership to look.

Comment 4

[Benjamin Smedberg]

You are almost certainly correct that the nsNPAPIPlugin is dead.

It's suspicious and interesting that there is another plugin instance lower down the stack (I verified that it's not the same nsPluginInstanceOwner object).

I will have to figure out on Monday the ownership cycle between nsPluginHost/nsNPAPIPlugin/nsNPAPIPluginInstance. I think the plugins are owned through the plugin tag array on the pluginhost. But if the plugin crashes that will go down, and perhaps the instances aren't notified. And I guess this is all non-e10s?

Comment 5

[Benjamin Smedberg]

Attachment: Hold nsNPAPIPlugin alive for active nsNPAPIPluginInstance objects. This should happen through the plugin tag, but apparently that isn't always working

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8837134 [details] [diff] [review]
Hold nsNPAPIPlugin alive for active nsNPAPIPluginInstance objects. This should happen through the plugin tag, but apparently that isn't always working

Review of attachment 8837134 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/base/nsNPAPIPlugin.h
@@ +62,5 @@
>  
>    static nsresult RetainStream(NPStream *pstream, nsISupports **aRetainedPeer);
>  
> +public:
> +  // RefCounted requires a public destructor :-(

I think you could do NS_INLINE_DECL_REFCOUNTING instead of inheriting RefCounted if you wanted to keep that protected? Since the destructor was virtual in the first place it's not like this is avoiding the vtable like RefCounted<> normally gets you.

Comment 7

[Benjamin Smedberg]

Attachment: Hold nsNPAPIPlugin alive for active nsNPAPIPluginInstance objects. This should happen through the plugin tag, but apparently that isn't always working

Comment 8

[Benjamin Smedberg]

Comment on attachment 8837265 [details] [diff] [review]
Hold nsNPAPIPlugin alive for active nsNPAPIPluginInstance objects. This should happen through the plugin tag, but apparently that isn't always working

Approval Request Comment
[Feature/Bug causing the regression]: unknown
[User impact if declined]: more crashes, possible security issue
[Is this code covered by automated tests?]: not effectively (we can't reproduce the problem)
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no STR available. Verification will be via crash-stats.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not very; main risk is that this will introduce a cycle/memory leak, but that seems unlikely
[String changes made/needed]: none

[Security approval request comment]
How easily could an exploit be constructed based on the patch? It would be difficult: I can't even figure out how to make the crash happen.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? They point to an ownership problem but nothing specific.

Which older supported branches are affected by this flaw? All

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I don't know about ESR54; everything else should be trivial backports.

How likely is this patch to cause regressions; how much testing does it need? If there were a regression, it would likely come in the form of a memory leak. But that's very unlikely if this passes automated tests.

Comment 9

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

sec-approval+ and branch approvals. 
We should investigate ESR45 patching.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b83dd97c786

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/04578bce528a
https://hg.mozilla.org/releases/mozilla-beta/rev/f961305ce9f0

I checked and this patch will in fact graft cleanly to esr45 as well.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8837265 [details] [diff] [review]
Hold nsNPAPIPlugin alive for active nsNPAPIPluginInstance objects. This should happen through the plugin tag, but apparently that isn't always working

See comment 8.

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8837265 [details] [diff] [review]
Hold nsNPAPIPlugin alive for active nsNPAPIPluginInstance objects. This should happen through the plugin tag, but apparently that isn't always working

Fix a sec-critical. ESR45+.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0b83dd97c786

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/f961305ce9f0
https://hg.mozilla.org/releases/mozilla-esr45/rev/d3fede027d06