342333 - Safe mode should disable plug-ins

Status: RESOLVED WONTFIX

(Core Graveyard :: Plug-ins, defect)

Description

[Doron Rosenberg (IBM)]

Plug-ins should not be loaded when we are in safe mode so that safe mode is clear of any external evils :)

Comment 1

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

This works, but I would like to avoid introducing the "browser.safe_mode"
pref if possible.  I tried various other methods of propagating the value
of 'gSafeMode' to nsPluginHostImpl but failed...
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/xre/nsAppRunner.cpp&rev=1.156&root=/cvsroot&mark=493#493

Suggestions for something better than a pref would be appreciated...

Comment 2

[Benjamin Smedberg]

We should hoist the nsIXULRuntime/nsIXULAppInfo interfaces to be available to gecko and necko.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

you could add a readonly attribute boolean pluginsEnabled to nsIPluginHost.

Comment 4

[Mats Palmgren (inactive)]

(In reply to comment #2)
> We should hoist the nsIXULRuntime/nsIXULAppInfo interfaces to be
> available to gecko and necko.

I agree.  Is this just a Makefile change or do we need to copy the
CVS files to a new directory and remove branch tags or could I just
do a "cvs add/rm"?  What needs to be done?


(In reply to comment #3)
> you could add a readonly attribute boolean pluginsEnabled to nsIPluginHost.

The only consumer at this point would be browser.js, but nsIPluginHost isn't
scriptable.  Also, at the moment the patch is fairly low risk and does not
change any interfaces so it could potentially be considered for branches
since it would help us triage crashes better.  (But I see your point of
decoupling pluginsEnabled from safe mode.)

Comment 5

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Here's a patch that assumes nsIXULRuntime/nsIXULAppInfo is available
to modules/plugin/base/src/nsPluginHostImpl.cpp

Comment 6

[Benjamin Smedberg]

> I agree.  Is this just a Makefile change or do we need to copy the
> CVS files to a new directory and remove branch tags or could I just
> do a "cvs add/rm"?  What needs to be done?

We do need to have the files in another directory, and I wouldn't worry about cvs copies. Just cvs add/rm them and put them in... xpcom/system?

--BDS

Comment 7

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

Comment 8

[Mike Hommey [:glandium]]

Shouldn't a choice whether to enable/disable plugins be added to the safe-mode initialization dialog ?

Comment 9

[Mats Palmgren (inactive)]

IMO, permanently disabling all plugins is a separate feature.
Probably best implemented as a pref.

Comment 10

[Benjamin Smedberg]

Comment on attachment 259917 [details] [diff] [review]
Patch rev. 3

>Index: modules/plugin/base/src/nsPluginHostImpl.cpp

>+#include "nsIXULAppInfo.h"
>+#include "nsIXULRuntime.h"

You don't need nsIXULAppInfo at all.

>+  nsCOMPtr<nsIXULAppInfo> appInfo = 
>+    do_GetService("@mozilla.org/xre/app-info;1");
>+  nsCOMPtr<nsIXULRuntime> xulRuntime = do_QueryInterface(appInfo);

You don't need separate getservice/QI. Use xulruntime = getservice() and skip appinfo altogether.

Otherwise, this looks fine. I'd like dbaron to sr the new patch.

Comment 11

[Mats Palmgren (inactive)]

(In reply to comment #10)
> You don't need separate getservice/QI. Use xulruntime = getservice() 

I actually had it like that at first but I got warnings when testing the
patch in SeaMonkey:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'Illegal value' when calling method: [nsIFactory::createInstance]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "<unknown>"  data: no]
************************************************************

It seems they have an app-info service that lacks nsIXULRuntime.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/suite/components/xulappinfo/xulappinfo.js&rev=1.5&root=/cvsroot&mark=21-45

Comment 12

[Benjamin Smedberg]

> It seems they have an app-info service that lacks nsIXULRuntime.

Sure. In that case the getservice should fail. That's expected and ok. I'm not sure why it's reporting that particular error to the JS console, but we shouldn't be bloating this code to work around that issue.

Comment 13

[Mats Palmgren (inactive)]

Ok, I'll change it to "xulRuntime = do_GetService" before checkin...

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 259917 [details] [diff] [review]
Patch rev. 3

sr=dbaron, although I'm a little suspicious of adding InSafeMode() checks in multiple places when what you mean is !PluginsEnabled() -- if you want to later change the conditions under which plugins are enabled, you have to go hunt for multiple places to change.  That said, I'm not sure where the "are plugins enabled" thing really belongs.  Maybe the document?

At least get a bug filed on fixing that if you don't want to fix it now (although I'd prefer if you fixed it now).

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 259917 [details] [diff] [review]
Patch rev. 3

I take that back -- I think given the comment I should mark it sr-.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #14)
> change the conditions under which plugins are enabled, you have to go hunt for
> multiple places to change.  That said, I'm not sure where the "are plugins

And a second reason is that it may be hard to understand why there's an "in safe mode" check if somebody doesn't know about this particular feature.  Somebody might even think it's a mistake.

Comment 17

[Mats Palmgren (inactive)]

(In reply to comment #15)
> (From update of attachment 259917 [details] [diff] [review])
> I take that back -- I think given the comment I should mark it sr-.

As I said in comment 4:
I think the patch is low risk and that it would really help triaging
crash bugs, so the benefit/risk ratio might be high enough for branch
and that's why I didn't add a PluginsEnabled() to some interface.
I'll be happy to add it on trunk though.

Comment 18

[Mats Palmgren (inactive)]

David, see my last comment above (I forgot to CC you).

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, so the above patch is ok for the branch if you land a better patch on the trunk first.

Comment 21

[Mats Palmgren (inactive)]

Probably not worth messing with this on branch...

Plugins can be individually disabled on trunk through Tools->Add-ons
so it's less critical to implement this for triage purposes IMO.
It should still be fixed of course, but my patch is likely obsolete by now...

Comment 22

[Nochum Sossonko [:Natch]]

If the patch isn't going to land the information bar in the addons manager when in safe mode should be changed to "...all extensions are disabled" being that nothing else is disabled!

Comment 23

[Mike Hommey [:glandium]]

FWIW, plugins are still actually loaded even when disabled, and thus still have a chance to blow things up...

Comment 24

[Matthew Gertner]

I've been taking a look at this since it looks like it should be fixed in order to fix bug 462334. Based on discussions with Mossop and biesi, I'm thinking along these lines:

- The pluginsDisabled attribute could go into nsIDocument as dbaron suggested. Not sure where it should be set though (wherever the document is instantiated I guess).

- The changes to pluginGlue.js don't look right since they won't run once the component has been registered. Maybe change missingPlugin.xml instead? It should use a different label and turn off the click handlers if plugins are disabled. This should mean that the change to browser.js isn't necessary since the PluginNotFound event will never fire anyway.

- We change nsObjectLoadingContent so that it doesn't try to load the plugin. Not sure where to put the check. Biesi suggested Instantiate, which could be right since apparently it only knows it's a plugin after OnStartRequest in some cases. It doesn't seem to be a big deal to me if it does the request even if the plugin is disabled.

- If we block plugin loading in nsObjectLoadingContent, however, I am far from convinced that FindPlugins will never be called. For example, won't nsPluginHostImpl::GetPluginCount still be called in some cases? nsPluginArray calls this, for example, though I'm not clear on what that is used for (nsNavigator::GetPlugins, but what is that for?).

- We probably want to handle plugins like extensions in safe mode. This means the list should still be displayed with an info banner saying they are disabled due to safe mode. Since we don't want to risk loading plugins, how about getting the info from pluginreg.dat? Basically I would suggest changing nsPluginHostImpl to use pluginreg.dat for FindPlugins when plugins are disabled. Hopefully there is some way for it to get at the document.

- Disabling of plugins would be triggered both by safe mode and by a plugins.disabled pref.

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #24)
> - The changes to pluginGlue.js don't look right since they won't run once the
> component has been registered. Maybe change missingPlugin.xml instead? It
> should use a different label and turn off the click handlers if plugins are
> disabled. This should mean that the change to browser.js isn't necessary since
> the PluginNotFound event will never fire anyway.

missingPlugin.xml doesn't have chrome privileges I'm pretty sure, so that wouldn't quite work.

> - We change nsObjectLoadingContent so that it doesn't try to load the plugin.
> Not sure where to put the check. Biesi suggested Instantiate, which could be
> right since apparently it only knows it's a plugin after OnStartRequest in some
> cases. It doesn't seem to be a big deal to me if it does the request even if
> the plugin is disabled.

Yeah that's probably OK

> - If we block plugin loading in nsObjectLoadingContent, however, I am far from
> convinced that FindPlugins will never be called. For example, won't
> nsPluginHostImpl::GetPluginCount still be called in some cases? nsPluginArray
> calls this, for example, though I'm not clear on what that is used for
> (nsNavigator::GetPlugins, but what is that for?).

Why is it a problem if FindPlugins is called?

(nsNavigator::GetPlugins is for navigator.plugins (https://developer.mozilla.org/en/navigator.plugins))

> - We probably want to handle plugins like extensions in safe mode. This means
> the list should still be displayed with an info banner saying they are disabled
> due to safe mode. Since we don't want to risk loading plugins, how about
> getting the info from pluginreg.dat? Basically I would suggest changing
> nsPluginHostImpl to use pluginreg.dat for FindPlugins when plugins are
> disabled. Hopefully there is some way for it to get at the document.

Which "the document"?

I guess your suggestion would work (although on Windows you don't need to load a plugin to find its description etc). I would only do that when in safe mode though, not in all cases when plugins are disabled.

Comment 26

[Benjamin Smedberg]

Realistically we're not going to fix this now, and plugins are OOP so less worrisome.