817255 - nsPluginHost::UnloadPlugins should send plugins-list-updated if there were any plugins to begin with (Tor 3547)

Status: RESOLVED INACTIVE

(Toolkit :: Add-ons Manager, enhancement, P3)

Description

[wiki131wiki]

User Agent: Mozilla/5.0 (iPad; CPU OS 6_0_1 like Mac OS X) AppleWebKit/536.26 (KHTML, like Gecko) Version/6.0 Mobile/10A523 Safari/8536.25

Steps to reproduce:

Firefox should have an option in preferences to block all plugins. This would greatly increase security and privacy. 

All a user can do now is disable plugins individually, but an option to block plugins program-wide would reduce the chance that another program might re-enable their plugin.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Work on a such a feature is already happening in bug 738698 etc., the user facing preference is bug 776432.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Gabriela, did you accidentally reopen this?

Comment 4

[Matthias Versen [:Matti]]

Disabling all plugins on the system is a different request than asking for click to play.

Plugins can be installed as user (in Firefox-profile\plugins) and some enterprise companies may want to block that.

Comment 5

[wiki131wiki]

This is not "click to play".  This is more for complete blocking of plugins from loading.

Comment 6

[Georg Fritzsche [:gfritzsche]]

The mention of an option in user-preferences to block plugins makes it sound to me like an end-user feature; in which case i think click-to-play does the same thing privacy- and security-wise and additionally allows you to enable plugins in-page if needed.

Or do you mean blocking for enterprise/multi-user scenarios, like bug 818513?

Comment 7

[wiki131wiki]

I think "click-to-play" doesn't offer the same security benefit than completely disabling.  Click-to-play doesn't support all plugins and will leave some plugins automatically enabled.

Also, having click-to-play runs the risk of any bugs in the implementation of it as well, as the end-user either accidentally clicking it or being tricked by a malicious site to click.

Comment 8

[Georg Fritzsche [:gfritzsche]]

(In reply to wiki131wiki from comment #7)
> I think "click-to-play" doesn't offer the same security benefit than
> completely disabling.  Click-to-play doesn't support all plugins and will
> leave some plugins automatically enabled.

There are no plans to automatically enable any plugins that i'm aware of.

> Also, having click-to-play runs the risk of any bugs in the implementation
> of it as well, as the end-user either accidentally clicking it or being
> tricked by a malicious site to click.

I see, sorry for the misunderstanding. Let's see if anyone else can chime on wether this can be done.

Note that there is a (unsupported AFAIK) hidden pref plugin.disable that should disable most of the plugin code paths

Comment 10

[Justin Dolske [:Dolske]]

This seems like a good idea to me. Should probably live in the addons manager UI, since that's where one enables/disables individual plugins.

[Alternatively, I could be convinced that there's no need for such a pref, because one can already disable all plugins through that existing UI. But I think it's work considering as a belt-and-suspenders protection, unless we're really sure that we won't accidentally reenable an (updated?) plugin, or silently start using a newly-installed one.]

Comment 11

[Redo Stuff]

Attachment: Belt-and-suspenders protection, adds UI for hidden pref plugin.disable

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8571009 [details] [diff] [review]
Belt-and-suspenders protection, adds UI for hidden pref plugin.disable

With apologies to Dave's review queue, but I'm swamped.

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 8571009 [details] [diff] [review]
Belt-and-suspenders protection, adds UI for hidden pref plugin.disable

This patch doesn't appear to be generated against hg (files don't have full paths and various .orig additional extensions). This makes it hard to test and review. Can you please provide a patch against the current source. We also prefer to get a full 8 lines of context to see what is going on around the changes.

Comment 14

[Georg Fritzsche [:gfritzsche]]

You can find instructions on the patch setup here:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 15

[Redo Stuff]

folks, I have to download terabytes of code to process and mod it using terabytes of ram, so to change ten lines in a way you able to review? sorry, I have no such resources it seems. feel free to drop this patch to a floor.

Comment 16

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8571009 [details] [diff] [review]
Belt-and-suspenders protection, adds UI for hidden pref plugin.disable

De-obsoleting as someone else may pick it up.

Comment 17

[Redo Stuff]

Comment on attachment 8571009 [details] [diff] [review]
Belt-and-suspenders protection, adds UI for hidden pref plugin.disable

From 589ff28758523a0f8fa76733cf0aef0a40c87f77 Mon Sep 17 00:00:00 2001
From: git-patch
Date: Mon, 23 Mar 2015 00:11:07 +0000
Subject: Update nsPluginHost.cpp

Notify observers about plugins was loaded or unloaded after plugin.disable was changed
---
 dom/plugins/base/nsPluginHost.cpp | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/dom/plugins/base/nsPluginHost.cpp b/dom/plugins/base/nsPluginHost.cpp
index ea591ab..7dc3984 100644
--- a/dom/plugins/base/nsPluginHost.cpp
+++ b/dom/plugins/base/nsPluginHost.cpp
@@ -3373,6 +3373,7 @@ NS_IMETHODIMP nsPluginHost::Observe(nsISupports *aSubject,
     sInst->Release();
   }
   if (!strcmp(NS_PREFBRANCH_PREFCHANGE_TOPIC_ID, aTopic)) {
+    NS_ConvertUTF16toUTF8 prefName(someData);
     mPluginsDisabled = Preferences::GetBool("plugin.disable", false);
     mPluginsClickToPlay = Preferences::GetBool("plugins.click_to_play", false);
     // Unload or load plugins as needed
@@ -3381,6 +3382,16 @@ NS_IMETHODIMP nsPluginHost::Observe(nsISupports *aSubject,
     } else {
       LoadPlugins();
     }
+    if (prefName.Equals("plugin.disable")) {
+      nsCOMPtr<nsIObserverService> obsService =
+        mozilla::services::GetObserverService();
+      if (obsService) {
+        nsAutoString pluginPolicy;
+        pluginPolicy = mPluginsDisabled ? NS_LITERAL_STRING("disabled")
+                                        : NS_LITERAL_STRING("enabled");
+        obsService->NotifyObservers(nullptr, "plugin-policy-changed", pluginPolicy.get());
+      }
+    }
   }
   if (!strcmp("blocklist-updated", aTopic)) {
     nsPluginTag* plugin = mPlugins;

From 3e8a673f388697ae1b3ac0d9607256c0322d146d Mon Sep 17 00:00:00 2001
From: git-patch
Date: Mon, 23 Mar 2015 00:11:07 +0000
Subject: Update AddonManager.jsm

Add support of callManagerListeners to AddonManagerInternal
---
 toolkit/mozapps/extensions/AddonManager.jsm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/toolkit/mozapps/extensions/AddonManager.jsm b/toolkit/mozapps/extensions/AddonManager.jsm
index 684373a..c4e43be 100644
--- a/toolkit/mozapps/extensions/AddonManager.jsm
+++ b/toolkit/mozapps/extensions/AddonManager.jsm
@@ -2560,6 +2560,11 @@ this.AddonManagerPrivate = {
     AddonManagerInternal.callAddonListeners.apply(AddonManagerInternal, aArgs);
   },
 
+  callManagerListeners: function AMP_callManagerListeners(...aArgs) {
+    AddonManagerInternal.callManagerListeners.apply(AddonManagerInternal,
+                                                    aArgs);
+  },
+
   AddonAuthor: AddonAuthor,
 
   AddonScreenshot: AddonScreenshot,

From f2d004e935e01ab64513671025f4e4863d66b5db Mon Sep 17 00:00:00 2001
From: git-patch
Date: Mon, 23 Mar 2015 00:11:07 +0000
Subject: Update PluginProvider.jsm

Update plugin list if plugin-policy-changed and inform add-on manager about it.
---
 toolkit/mozapps/extensions/internal/PluginProvider.jsm | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/toolkit/mozapps/extensions/internal/PluginProvider.jsm b/toolkit/mozapps/extensions/internal/PluginProvider.jsm
index 7602dd0..31e198c 100644
--- a/toolkit/mozapps/extensions/internal/PluginProvider.jsm
+++ b/toolkit/mozapps/extensions/internal/PluginProvider.jsm
@@ -55,6 +55,7 @@ var PluginProvider = {
   plugins: null,
 
   startup: function PL_startup() {
+    Services.obs.addObserver(this, "plugin-policy-changed", false);
     Services.obs.addObserver(this, LIST_UPDATED_TOPIC, false);
     Services.obs.addObserver(this, AddonManager.OPTIONS_NOTIFICATION_DISPLAYED, false);
   },
@@ -67,6 +68,7 @@ var PluginProvider = {
     this.plugins = null;
     Services.obs.removeObserver(this, AddonManager.OPTIONS_NOTIFICATION_DISPLAYED);
     Services.obs.removeObserver(this, LIST_UPDATED_TOPIC);
+    Services.obs.removeObserver(this, "plugin-policy-changed");
   },
 
   observe: function(aSubject, aTopic, aData) {
@@ -95,6 +97,12 @@ var PluginProvider = {
       if (this.plugins)
         this.updatePluginList();
       break;
+    case "plugin-policy-changed":
+      if (!this.plugins)
+        this.plugins ={};
+      this.updatePluginList();
+      AddonManagerPrivate.callManagerListeners("onPluginPolicyChanged");
+      break;
     }
   },
 

From 1bef3ad4b574747f598bc4fa0fd71ac369d96722 Mon Sep 17 00:00:00 2001
From: git-patch
Date: Mon, 23 Mar 2015 00:11:07 +0000
Subject: Update extensions.xul

Add buttons to enabled and to disable loading of plugins.
(it's incomplete, all new strings for buttons is hard-coded yet)
---
 toolkit/mozapps/extensions/content/extensions.xul | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/toolkit/mozapps/extensions/content/extensions.xul b/toolkit/mozapps/extensions/content/extensions.xul
index 3ba2585..d04ea2c 100644
--- a/toolkit/mozapps/extensions/content/extensions.xul
+++ b/toolkit/mozapps/extensions/content/extensions.xul
@@ -86,6 +86,8 @@
     <command id="cmd_forward"/>
     <command id="cmd_enableCheckCompatibility"/>
     <command id="cmd_pluginCheck"/>
+    <command id="cmd_pluginEnable"/>
+    <command id="cmd_pluginDisable"/>
     <command id="cmd_enableUpdateSecurity"/>
     <command id="cmd_toggleAutoUpdateDefault"/>
     <command id="cmd_resetAddonAutoUpdate"/>
@@ -358,6 +360,14 @@
                       command="cmd_pluginCheck"/>
               <spacer flex="5000"/> <!-- Necessary to allow the message to wrap -->
             </hbox>
+            <vbox id="plugin-disable-button" class="global-info" flex="1"
+                  align="end">
+              <button class="button-plugin-disable"
+                      label="Disable plugins"
+                      tooltiptext="Click to stop searching and using plugins"
+                      command="cmd_pluginDisable"/>
+              <spacer flex="5000"/>
+            </vbox>
           </hbox>
           <hbox class="view-header global-info-container experiment-info-container">
             <hbox class="global-info" flex="1" align="center">
@@ -387,6 +397,17 @@
             </vbox>
             <spacer class="alert-spacer-after"/>
           </vbox>
+          <vbox id="plugin-enable-button" class="alert-container" flex="1"
+                hidden="true">
+            <spacer class="alert-spacer-before"/>
+            <vbox class="alert">
+              <label value="Click to search for installed plugins"/>
+              <button class="button-plugin-enable"
+                      label="Enable plugins"
+                      command="cmd_pluginEnable"/>
+            </vbox>
+            <spacer class="alert-spacer-after"/>
+          </vbox>
           <richlistbox id="addon-list" class="list" flex="1"/>
         </vbox>
 

From 4fa92eac02862abdbde4c0d2ef01cd8a54fe6a03 Mon Sep 17 00:00:00 2001
From: git-patch
Date: Mon, 23 Mar 2015 00:11:07 +0000
Subject: Update extensions.js

Show actual elements for about:addons if plugin.disable pref was changed
---
 toolkit/mozapps/extensions/content/extensions.js | 68 ++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/toolkit/mozapps/extensions/content/extensions.js b/toolkit/mozapps/extensions/content/extensions.js
index 2892b61..7e2e95c 100644
--- a/toolkit/mozapps/extensions/content/extensions.js
+++ b/toolkit/mozapps/extensions/content/extensions.js
@@ -829,6 +829,20 @@ var gViewController = {
       }
     },
 
+    cmd_pluginEnable: {
+      isEnabled: function cmd_pluginEnable_isEnabled() true,
+      doCommand: function cmd_pluginEnable_doCommand() {
+        Services.prefs.setBoolPref("plugin.disable", false);
+      }
+    },
+
+    cmd_pluginDisable: {
+      isEnabled: function cmd_pluginDisable_isEnabled() true,
+      doCommand: function cmd_pluginDisable_doCommand() {
+        Services.prefs.setBoolPref("plugin.disable", true);
+      }
+    },
+
     cmd_toggleAutoUpdateDefault: {
       isEnabled: function cmd_toggleAutoUpdateDefault_isEnabled() true,
       doCommand: function cmd_toggleAutoUpdateDefault_doCommand() {
@@ -2570,12 +2584,16 @@ var gListView = {
   node: null,
   _listBox: null,
   _emptyNotice: null,
+  _pluginEnableButton: null,
+  _pluginHeader: null,
   _type: null,
 
   initialize: function gListView_initialize() {
     this.node = document.getElementById("list-view");
     this._listBox = document.getElementById("addon-list");
     this._emptyNotice = document.getElementById("addon-list-empty");
+    this._pluginEnableButton = document.getElementById("plugin-enable-button");
+    this._pluginHeader = document.getElementsByClassName("plugin-info-container")[0];
 
     var self = this;
     this._listBox.addEventListener("keydown", function listbox_onKeydown(aEvent) {
@@ -2587,6 +2605,11 @@ var gListView = {
     }, false);
   },
 
+  shutdown: function gListView_shutdown() {
+    AddonManager.removeAddonListener(this);
+    AddonManager.removeManagerListener(this);
+  },
+
   show: function gListView_show(aType, aRequest) {
     if (!(aType in AddonManager.addonTypes))
       throw Components.Exception("Attempting to show unknown type " + aType, Cr.NS_ERROR_INVALID_ARG);
@@ -2594,6 +2617,8 @@ var gListView = {
     this._type = aType;
     this.node.setAttribute("type", aType);
     this.showEmptyNotice(false);
+    this.showPluginHeader(false);
+    this.showPluginEnableButton(false);
 
     while (this._listBox.itemCount > 0)
       this._listBox.removeItemAt(0);
@@ -2621,14 +2646,19 @@ var gListView = {
         for (let element of elements)
           self._listBox.appendChild(element);
       }
+      self.showPluginButton();
 
       gEventManager.registerInstallListener(self);
       gViewController.updateCommands();
       gViewController.notifyViewChanged();
+      AddonManager.addAddonListener(self); /* for onUninstalled */
+      AddonManager.addManagerListener(self); /* for onPluginPolicyChanged */
     });
   },
 
   hide: function gListView_hide() {
+    AddonManager.removeAddonListener(this);
+    AddonManager.removeManagerListener(this);
     gEventManager.unregisterInstallListener(this);
     doPendingUninstalls(this._listBox);
   },
@@ -2679,6 +2709,44 @@ var gListView = {
     }
   },
 
+  onUninstalled: function gListView_onUninstalled()  {
+    this.showEmptyNotice(this._listBox.itemCount == 0);
+  },
+
+  showPluginEnableButton: function gListView_showPluginEnableButton(aShow) {
+    if (this._pluginEnableButton)
+      this._pluginEnableButton.hidden = !aShow;
+  },
+
+  showPluginHeader: function gListView_showPluginHeader(aShow) {
+    if (this._pluginHeader)
+      this._pluginHeader.hidden = !aShow;
+  },
+
+  showPluginButton: function gListView_showPluginButton() {
+    if (this._type == "plugin") {
+      var plugin_disable = false;
+
+      try {
+        plugin_disable = Services.prefs.getBoolPref("plugin.disable")
+      } catch (e) {}
+
+      if (plugin_disable == true) {
+        this.showPluginHeader(false);
+        this.showEmptyNotice(false);
+        this.showPluginEnableButton(true);
+      } else {
+        this.showPluginHeader(true);
+        this.showEmptyNotice(this._listBox.itemCount == 0);
+        this.showPluginEnableButton(false);
+      }
+    }
+  },
+
+  onPluginPolicyChanged: function gListView_onPluginPolicyChanged() {
+    this.showPluginButton();
+  },
+
   addItem: function gListView_addItem(aObj, aIsInstall) {
     if (aObj.type != this._type)
       return;

Comment 18

[Mike Perry]

For clarity, the specific problem that this patch is trying to solve is to provide a way for users to prevent plugins from being loaded into the browser address space at all. Currently, all of the XPCOM and UI-based mechanisms of disabling plugins still allow them to be loaded into the address space before the enable/disable decision is made. This means their DllMain() and any static initializers will still execute, even if they are 'disabled'.

There are at least two main reasons to block plugins even before they are loaded:

1. Some plugins (especially antivirus programs, as well as adware/toolbars) will modify the Firefox process space during their DllMain() and/or static initializers, which means their code can still have effects on Firefox independent of the enable/disable status in the plugins UI.

2. Some plugins are not compiled with hardening options such as ASLR, SEH, or DEP. This makes them juicy and easy targets to use in the ROP chains of exploits of unrelated vulnerabilities, since their exports, imports, strings and code will be at predictable locations in the address space during exploitation. Bug 677797 is meant to improve this situation, but that bug keeps stalling due to stability concerns.

Comment 19

[Georg Koppen]

Attachment: patch for optionally disabling all plugins

In order to avoid bit-rot I applied the patch cleanly to mozilla-central. A try build can be found on: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b24b25846b2b

Comment 20

[Dave Townsend [:mossop]]

Comment on attachment 8596091 [details] [diff] [review]
patch for optionally disabling all plugins

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

There's a lot of good work here but I think we can simplify it a lot. I'd like UX to look over what this looks like too so getting some screenshots of this would be great.

::: toolkit/mozapps/extensions/content/extensions.js
@@ +2744,5 @@
> +        this.showEmptyNotice(this._listBox.itemCount == 0);
> +        this.showPluginEnableButton(false);
> +      }
> +    }
> +  },

This isn't right, it assumes that all plugins come from PluginProvider but we now have at least the Open H264 and EME video rendering plugins that come from GMPProvider. We'll either have to make GMPProvider respect plugin.disable or rethink the UI a little here. Either way I'd like someone from UX to look over screenshots of what this looks like.

Assuming we update GMPProvider the login here should just go into showEmptyNotice since other code may call into it directly and undo the changes this does.

::: toolkit/mozapps/extensions/content/extensions.xul
@@ +363,5 @@
> +            <vbox id="plugin-disable-button" class="global-info" flex="1"
> +                  align="end">
> +              <button class="button-plugin-disable"
> +                      label="Disable plugins"
> +                      tooltiptext="Click to stop searching and using plugins"

These strings need to be localised in extensions.dtd like others in the file.

@@ +402,5 @@
> +            <spacer class="alert-spacer-before"/>
> +            <vbox class="alert">
> +              <label value="Click to search for installed plugins"/>
> +              <button class="button-plugin-enable"
> +                      label="Enable plugins"

Same here.

::: toolkit/mozapps/extensions/internal/PluginProvider.jsm
@@ +98,5 @@
>          this.updatePluginList();
>        break;
> +    case "plugin-policy-changed":
> +      if (!this.plugins)
> +        this.plugins ={};

Style nit: space after =

@@ +99,5 @@
>        break;
> +    case "plugin-policy-changed":
> +      if (!this.plugins)
> +        this.plugins ={};
> +      this.updatePluginList();

nsPluginHost::LoadPlugins will have already sent plugins-list-updated in the case where plugins have become enabled so there isn't any point in doing this again. We can avoid needing to rebuild the list here entirely if we make nsPluginHost:UnloadPlugins consistent and send plugins-list-updated as well.

At that point I don't think it is worth adding the plugin-policy-changed notification and the piping to get that through to extensions.js. Instead just observe plugin.disable directly there.

Comment 21

[Dave Townsend [:mossop]]

Stephen, what is your thought on whether GMPProvider should obey the plugin.disable pref?

Comment 22

[Stephen A Pohl [:spohl]]

(I don't feel like I have any authority to make a call on this, so just take this as my personal opinion.) 

Conceptually, I think GMPs and regular NPAPI plugins are quite different: Plugins get installed by third-parties, with their own installers and updaters. In the GMP case, we take on the responsibility of downloading, installing and updating them ourselves. I think of GMPs as an extension of Firefox, the binary itself. They are modules that are essentially part of Firefox, but typically have a different update schedule, which is why they're split out. To get the true "Firefox experience", these GMPs are expected to be present and installed on disk. The same does not apply to NPAPI plugins. This is a long and convoluted way of saying: no, personally, I don't think GMPProvider should obey the plugin.disable pref. But I'm happy to get overruled if there's a good argument against it.

Comment 23

[Georg Fritzsche [:gfritzsche]]

I'll add a "no" too. We decided to show GMPs as part of the addon manager plugin category, but otherwise they are very separate.

Using the plugin.disable pref for GMPs would a) overload its meaning and b) mean we don't have a separate switch to disable NPAPI plugins.

Comment 24

[Dave Townsend [:mossop]]

So sounds like we need to agree on UI for the plugin enable/disable button since using the empty list won't work.

Comment 25

[Benjamin Smedberg]

I don't think we should expose UX for this in preferences. Having that choice spread out over the addon manager and the pref pane is going to be confusing. I also don't think there's a clear use-case for exposing this at all. Having a hidden pref which can be used by torbrowser (for example) makes sense, but once a user is in the addon manager, they can already accomplish this task.

Comment 26

[Justin Dolske [:Dolske]]

To pile on: I'd agree that we don't need UI for this, and that this can just be an about:config pref that's specific to NPAPI plugins.

Comment 27

[Dave Townsend [:mossop]]

Well if we don't want UI for this then the option to disable all plugins already exists. It doesn't update the add-ons manager UI correctly when toggled at runtime so I guess we could morph this bug to that, seems pretty low importance though.

Comment 28

[Arthur Edelstein [:arthur]]

For reference, here is a link that tracks the latest version of the Tor Browser patch:
https://torpat.ch/3547

Comment 29

[Arthur Edelstein [:arthur]]

Also, in case it's useful (though the consensus seems to be against this solution), here's the UI patch currently included in Tor Browser:
https://torpat.ch/10280

Comment 30

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.