Opened 6 years ago

Closed 6 years ago

#9454 closed defect (fixed)

The nsIPluginTag Interface was changed, Torbutton is broken for next Firefox ESR

Reported by: cypherpunks Owned by: mikeperry
Priority: High Milestone:
Component: TorBrowserButton Version:
Severity: Keywords: ff24-esr, MikePerry201311R
Cc: g.koppen@…, mcs, brade Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Torbrowser loads system-wide installed flash plugin, disables it during init but plugin was loaded to browser's address space already.
Unless user changed security settings, browser shouldn't load anything.

Child Tickets

Change History (19)

comment:1 Changed 6 years ago by cypherpunks

Proposed fix includes changes to Torbutton code.

--- torbutton.origin.js
+++ torbutton.js
@@ -1523,12 +1523,7 @@
 // toggles plugins: true for disabled, false for enabled
 function torbutton_toggle_plugins(disable_plugins) {
   if (m_tb_tbb) {
-    var PH=Cc["@mozilla.org/plugin/host;1"].getService(Ci.nsIPluginHost);
-    var P=PH.getPluginTags({});
-    for(var i=0; i<P.length; i++) {
-        if (P[i].disabled != disable_plugins)
-          P[i].disabled=disable_plugins;
-    }
+    m_tb_prefs.setBoolPref("plugin.disable", disable_plugins);
   }
 }

"plugin.disable" must be true by default.

comment:2 in reply to:  description Changed 6 years ago by cypherpunks

Replying to cypherpunks:

plugin was loaded to browser's address space already.

It's not how a Gecko Plug-in works. https://developer.mozilla.org/en-US/docs/Gecko_Plugin_API_Reference/Plug-in_Basics#Understanding_the_Runtime_Model

No any code loaded in to memory till it required, according this document. It's absolutely safe to enumerate any system-wide installed plug-ins as long as Torbutton disables plug-in with exist code. If any code can bypass Torbutton protections then it can bypass Torbutton entirely and do even worse things than it.

The only concern may keep is monitoring of plug-in existence in add-ons list. But that is paranoia on a basis of insufficient information.

I suggest to close this bug as wontfix or notabug.

comment:3 Changed 6 years ago by cypherpunks

Summary: Torbrowser shouldn't load any plugins if user didn't changed security settingsThe nsIPluginTag Interface was changed, Torbutton is broken for next Firefox ESR

During fix of https://bugzilla.mozilla.org/show_bug.cgi?id=854467 the nsIPluginTag interface was changed so used attribute now is readonly. It will break Torbutton for next Firefox ESR (24ESR planned for september?)
It's possible to use enabledState attribute, but this will make transitions from current 17ESR more hard and unstable as no such attribute present here.

comment:4 Changed 6 years ago by gk

Cc: g.koppen@… added

comment:5 Changed 6 years ago by gk

Keywords: ff24-esr added

comment:6 Changed 6 years ago by mcs

Cc: mcs brade added
Status: newneeds_review

comment:7 Changed 6 years ago by mikeperry

At a glance this looks ok. Do we know for sure if FF24 allows us to modify this new field and have it take effect immediately? Was this tested at all?

comment:8 in reply to:  7 Changed 6 years ago by mcs

Replying to mikeperry:

At a glance this looks ok. Do we know for sure if FF24 allows us to modify this new field and have it take effect immediately? Was this tested at all?

Yes, we tested on Mac OS. The changes take effect immediately (e.g., plugins are disabled upon next page load).

comment:9 Changed 6 years ago by cypherpunks

Why need extra logic, new and old attributes? If plugin.disable exists. What purpose to show any plugin if user don't want it? What your suggestions for system-wide installed flash?

comment:10 Changed 6 years ago by mikeperry

Keywords: MikePerry201311R added

comment:11 Changed 6 years ago by mikeperry

Ok, I've gone ahead and merged this. The check for the existence of "enabledState" seems sane to me.

cypherpunks: We need to use this attribute to be consistent with the individual plugin enable/disable UI in the Tools->Addons->Plugins pane, and to ensure the appropriate observer notifications get fired in both cases.

However, I am curious why mcs chose to check the state before setting the attribute? Does this reduce spurious 'xpcom-category-entry-added' observer events? Or some other reason? If it is in fact needed for some reason, we should comment the code with that reason.

comment:12 Changed 6 years ago by mikeperry

Status: needs_reviewneeds_revision

Setting this to needs_revision so we remember to update the comments in the source (or simplify it).

comment:13 in reply to:  11 Changed 6 years ago by mcs

Replying to mikeperry:

However, I am curious why mcs chose to check the state before setting the attribute? Does this reduce spurious 'xpcom-category-entry-added' observer events? Or some other reason? If it is in fact needed for some reason, we should comment the code with that reason.

We basically followed the intent behind the older code (check that a real change is being made before setting the plugin state). I do think this approach will avoid some notifications; maybe 'plugin-info-updated'? Kathy and I will verify and then add an appropriate comment.

comment:14 Changed 6 years ago by mikeperry

Resolution: fixed
Status: needs_revisionclosed

Ok. Well, the issues here are mostly cosmetic (and the Torbutton code is a rather ancient pile of hideous cruft anyway ;).

If you feel the urge to clean this up, feel free to reopen with a patch, but there are probably better uses of your time at this point. In particular, my questions in #9570 probably are more important to investigate.

Thanks!

comment:15 Changed 6 years ago by cypherpunks

Priority: normalcritical
Resolution: fixed
Status: closedreopened

This wrong way to disable plugin. Browser loads random code outside of TBB and runs it with such "disable".

comment:16 Changed 6 years ago by cypherpunks

Status: reopenednew
Summary: The nsIPluginTag Interface was changed, Torbutton is broken for next Firefox ESRTorbrowser shouldn't load any plugins if user didn't changed security settings

comment:17 Changed 6 years ago by cypherpunks

Priority: criticalblocker

comment:18 in reply to:  16 Changed 6 years ago by mcs

Replying to cypherpunks:

https://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginsDirUnix.cpp#319
It loads!

If we want to protect against "rogue" plugins that may be installed on the user's computer, it probably makes sense to set plugin.disable = true. We will still want to set enabledState=STATE_CLICKTOPLAY on each plugin when the user enables plugins.

Mike, is this something we want to do?

comment:19 Changed 6 years ago by mikeperry

Priority: blockermajor
Resolution: fixed
Status: newclosed
Summary: Torbrowser shouldn't load any plugins if user didn't changed security settingsThe nsIPluginTag Interface was changed, Torbutton is broken for next Firefox ESR

This bug is turning into a schizophrenic train wreck. It has now changed title and purpose twice. I am retitling it back to something that reflects the commit that was merged, and I have created #10280 to house further discussion about the flash issue. Making the Firefox 24 TBB plugin behavior match the Firefox 17 behavior is what this bug should be about, since that was what we merged, and that is what we need to have in place for next week.

Note: See TracTickets for help on using tickets.