Opened 4 years ago

Closed 2 years ago

Last modified 8 months ago

#14970 closed enhancement (fixed)

Don't allow third parties to block our own Tor Browser extensions

Reported by: gk Owned by: gk
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff45-esr, tbb-security, tbb-6.0a5, TorBrowserTeam201604R, GeorgKoppen201604, tbb-no-uplift
Cc: mikeperry, mcs, brade Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Right now we ship Torbutton and Tor Launcher and take care that we alone can make updates and that these updates take effect when they need to. But that might change if all extensions need to get signed beforehand by Mozilla to be accepted by the underlying browser (see: https://blog.mozilla.org/addons/2015/02/10/extension-signing-safer-experience/).

The easy solution would be to just disable that feature entirely in Tor Browser (either by flipping a preference or a build switch) but I think we should be smarter here and use the signing requirement for extensions not written by us but exempt our add-ons from it.

The tracking bug is: https://bugzilla.mozilla.org/show_bug.cgi?id=1047239

Marking this for ESR 45 as it won't take effect in ESR 38.

Child Tickets

Change History (17)

comment:1 Changed 4 years ago by gk

Type: defectenhancement

comment:2 Changed 3 years ago by mcs

I agree with this approach (just exempt our own bundled add-ons from the 3rd party signing requirement). There is also a ticket that I cannot find at the moment that proposes that we should build in Tor Launcher and Torbutton so they cannot be disabled or removed. Since the changes for #10280 landed, disabling or removing Torbutton makes it so that about:addons cannot be opened, which means that users can paint themselves into a corner (the file toolkit/mozapps/extensions/content/extensions.xul directly uses entities that are part of Torbutton's brand.dtd file).

comment:3 Changed 3 years ago by gk

Severity: Normal

https://blog.mozilla.org/addons/2016/01/22/add-on-signing-update/ for an update on the status of this feature.

comment:4 Changed 3 years ago by gk

Keywords: tbb-6.0a5 added

comment:5 Changed 3 years ago by gk

Owner: changed from tbb-team to gk
Status: newassigned

comment:6 Changed 3 years ago by gk

Keywords: GeorgKoppen201603 added

comment:8 Changed 2 years ago by gk

Keywords: TorBrowserTeam201604 added

We want that for the alpha and the ESR 45 stable series.

comment:9 Changed 2 years ago by gk

Keywords: GeorgKoppen201604 added; GeorgKoppen201603 removed

comment:10 Changed 2 years ago by gk

What do we want to do with HTTPS-Everywhere? There are basically two possible options: 1) just take the AMO .xpi as we are doing with NoScript or 2) stick to the EFF version but whitelist that one, too. I think I am leaning towards 2) at the moment.

Last edited 2 years ago by gk (previous) (diff)

comment:11 in reply to:  10 Changed 2 years ago by mcs

Replying to gk:

I think I am leaning towards 2) at the moment.

I would say "do whatever is easier" unless it is likely that we will need to update extensions to use different code than is on AMO (in which case whitelisting is better).

comment:12 Changed 2 years ago by gk

Keywords: TorBrowserTeam201604R added; TorBrowserTeam201604 removed
Status: assignedneeds_review

comment:13 Changed 2 years ago by mcs

Kathy and I review your patch and have two comments:

  1. Are you sure you need the pref override? In ESR45, browser/app/profile/firefox.js already has: pref("xpinstall.signatures.required", true);
  1. I think we may need to add whitelisting inside processPendingFileChanges() in XPIProvider.jsm near the call to mustSign(). If I remember correctly, processPendingFileChanges() will be called after an update and we want to ensure that our extensions are not blocked (since #13252 landed, our extensions are copied out of the .app into the user's profile after each update on Mac OS).

comment:14 in reply to:  13 ; Changed 2 years ago by gk

Replying to mcs:

Kathy and I review your patch and have two comments:

  1. Are you sure you need the pref override? In ESR45, browser/app/profile/firefox.js already has: pref("xpinstall.signatures.required", true);

Ah, correct. I misremembered the discussion regarding this preference on the mozilla enterprise mailing list (that one was actually about whether that preference stays while it is going to get removed soon in vanilla Firefox code). I removed that part

  1. I think we may need to add whitelisting inside processPendingFileChanges() in XPIProvider.jsm near the call to mustSign(). If I remember correctly, processPendingFileChanges() will be called after an update and we want to ensure that our extensions are not blocked (since #13252 landed, our extensions are copied out of the .app into the user's profile after each update on Mac OS).

Good catch and I think you are right. My bug_14970_v5 (https://gitweb.torproject.org/user/gk/tor-browser.git/commit/?h=bug_14970_v5&id=d65c317a541615545f8eeba6c85c05ca468e490a) should have the fixes.

comment:15 in reply to:  14 Changed 2 years ago by mcs

Replying to gk:

[snip]
My bug_14970_v5 (https://gitweb.torproject.org/user/gk/tor-browser.git/commit/?h=bug_14970_v5&id=d65c317a541615545f8eeba6c85c05ca468e490a) should have the fixes.

r=brade, r=mcs
This looks good to us.

comment:16 Changed 2 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

This is commit c444324b69dbf9c5afd630673223acc2596871d5 on tor-browser-45.0.2esr-6.x-1.

comment:17 Changed 8 months ago by arthuredelstein

Keywords: tbb-no-uplift added
Note: See TracTickets for help on using tickets.