Opened 3 years ago

Last modified 3 years ago

#19508 new defect

Proposal to drop Tor Browser's plugin patches

Reported by: arthuredelstein Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201607, tbb-obsolete
Cc: gk, brade, mcs Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by arthuredelstein)

Tor Browser has three patches related to blocking plugins:

  • #3547 adds a function that whitelists the flash plugin only and excludes loading all other plugins
  • #8312 hides the link to "Manage plugins" when the plugin is disabled
  • #10280 adds a UI for enabling/disabling plugins in the add-ons page

These patches were introduced when Flash was still in fairly wide use. But since then, Flash has been disabled by default in Firefox, and is replaced on a substantial number of websites by HTML5 video and JavaScript. Furthermore, we want to strongly discourage users from using Flash as there is a significant risk that it will bypass the proxy or expose the user to tracking or security vulnerabilities.

First, from what I can see, when the pref plugin.disable is set to true (as it is in browser/app/profile/000-tor-browser.js), all plugins (including Flash) are blocked from ever loading into the Firefox process. Therefore the code in our #3547 is never exercised.

Second, #10280 only makes it more likely for the user to set "plugin.disable" to false, by exposing that pref in the UI.

Finally, #8312 seems unnecessary because, when "plugin.disable" is true, no "Manage plugins" link appears. Instead, the only message is "A plugin is needed to display this content." Also, various popular video sites, such as YouTube and Vimeo, now use HTML5 video without any complaints about missing Flash.

So I would suggest we can drop these three patches. Instead we might consider a couple of UI tweaks to improve user safety:

  1. Hide the Plugins section of about:addons altogether to prevent the user from even considering loading any plugins
  2. Change the plugin failure message to "A plugin would be needed to display this content. For security reasons, Tor Browser does not support plugins."

I think both of these changes could be implemented as XUL overlays in torbutton.

Finally, for extra safety, we could add an extra C++ patch that ensures that whenever an nsPluginsDir::LoadPlugin implementation is called, the plugin.disable pref is checked and, if it is true, the function loads nothing and returns an error code. I think such a patch might be upstreamable.

Child Tickets

Change History (7)

comment:1 Changed 3 years ago by arthuredelstein

Description: modified (diff)

comment:2 Changed 3 years ago by arthuredelstein

Keywords: tbb-obsolete added

comment:3 Changed 3 years ago by gk

Cc: gk added

comment:4 Changed 3 years ago by mcs

Cc: brade mcs added

I do not think Flash is disabled by default in Firefox (I tested with 49.0b3 and a clean profile). That said, I am not fan of Flash and I am in favor of dropping browser patches when we can. But I wonder if making it harder to activate Flash will generate a lot of complaints.

comment:5 in reply to:  description ; Changed 3 years ago by gk

Replying to arthuredelstein:

First, from what I can see, when the pref plugin.disable is set to true (as it is in browser/app/profile/000-tor-browser.js), all plugins (including Flash) are blocked from ever loading into the Firefox process. Therefore the code in our #3547 is never exercised.

For reference: where can you see that?

comment:6 in reply to:  5 Changed 3 years ago by gk

Replying to gk:

Replying to arthuredelstein:

First, from what I can see, when the pref plugin.disable is set to true (as it is in browser/app/profile/000-tor-browser.js), all plugins (including Flash) are blocked from ever loading into the Firefox process. Therefore the code in our #3547 is never exercised.

For reference: where can you see that?

Oh, I think I see what you mean, nevermind then.

comment:7 Changed 3 years ago by gk

Keywords: TorBrowserTeam201607 added; TorBrowserTeam201606 removed

I think https://bugzilla.mozilla.org/show_bug.cgi?id=817255 and #15842 should be considered here as well. I can see benefit from not exposing this in the UI (especially as Mozilla is not happy about that part either) but otherwise I think the right thing is to get the relevant bits upstreamed. Does that make sense? Arthur, if so, then putting it on your radar for the ESR52 deadline seems to be a good thing to me. Then we would be able to drop the three patches with the switch to the new ESR.

Note: See TracTickets for help on using tickets.