The nsIPluginTag Interface was changed, Torbutton is broken for next Firefox ESR
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.
Activity
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.
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.
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.
Trac:
Summary: Torbrowser shouldn't load any plugins if user didn't changed security settings to The nsIPluginTag Interface was changed, Torbutton is broken for next Firefox ESR
Trac:
Cc: N/A to g.koppen@jondos.de
We committed a fix here:
Mike -- please review.
Trac:
Cc: g.koppen@jondos.de to g.koppen@jondos.de, mcs, brade
Status: new to needs_review-
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).
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.
-
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.
-
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 (moved) probably are more important to investigate.
Thanks!
Trac:
Status: needs_revision to closed
Resolution: N/A to fixed https://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginsDirUnix.cpp#319 It loads!
Trac:
Summary: The nsIPluginTag Interface was changed, Torbutton is broken for next Firefox ESR to Torbrowser shouldn't load any plugins if user didn't changed security settings
Status: reopened to new-
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?
-
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 (moved) 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.
Trac:
Status: new to closed
Summary: Torbrowser shouldn't load any plugins if user didn't changed security settings to The nsIPluginTag Interface was changed, Torbutton is broken for next Firefox ESR
Resolution: N/A to fixed
Priority: blocker to major