Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#21396 closed defect (fixed)

Torbutton breaks Session Manager addon

Reported by: HolD Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-6.5-regression, TorBrowserTeam201703R, GeorgKoppen201702
Cc: lnl, mcs, brade Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Tor Browser 6.5, Torbutton 1.9.6.12. After installing Session Manager (https://addons.mozilla.org/en-US/firefox/addon/session-manager/) it is not visible in Tools menu or as a toolbar button. This is a regression. Issue is confirmed:
https://blog.torproject.org/comment/reply/1301/232647
https://blog.torproject.org/comment/reply/1301/233041
https://blog.torproject.org/comment/reply/1301/233344

Child Tickets

Change History (20)

comment:1 Changed 2 years ago by HolD

Bug presents in Tor Browser 6.5a6, 6.5, 7.0a1. Bug doesn't present in Tor Browser 6.0.8.

comment:2 in reply to:  1 Changed 2 years ago by gk

Keywords: tbb-6.5-regression added
Status: newneeds_information

Replying to HolD:

Bug presents in Tor Browser 6.5a6, 6.5, 7.0a1. Bug doesn't present in Tor Browser 6.0.8.

Thanks for the ticket. Which alpha in the 6.5 series is the first that breaks things for you? Older versions are on https://archive.torproject.org/tor-package-archive/torbrowser/

comment:3 Changed 2 years ago by HolD

6.5a1 - no issue, 6.5a2 - has issue.
Steps to reproduce:
1) install Tor Browser,
2) install addon https://addons.mozilla.org/en-US/firefox/addon/session-manager/,
3) confirm information popup about private browsing mode,
4) Session Manager button should immediately appear on toolbar (no restart or changing preferences is required).

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

comment:4 Changed 2 years ago by gk

Keywords: TorBrowserTeam201702 added

comment:5 Changed 2 years ago by gk

Keywords: GeorgKoppen201702 added

comment:6 Changed 2 years ago by gk

Session Manager does not like it that chrome://sessionmanager/locale/sessionmanager.dtd and chrome://sessionmanager/locale/options.dtd are blocked by our fix for #8725. Things like &toolbar.tooltip need to be available for content after the first installation. HolD: does adding the toolbar buttons manually to the toolbar work for you?

comment:7 in reply to:  6 ; Changed 2 years ago by yawning

Replying to gk:

Session Manager does not like it that chrome://sessionmanager/locale/sessionmanager.dtd and chrome://sessionmanager/locale/options.dtd are blocked by our fix for #8725. Things like &toolbar.tooltip need to be available for content after the first installation. HolD: does adding the toolbar buttons manually to the toolbar work for you?

What's the origin URI when the requests make it to the content policy? If this is one of the cases where aRequestOrigin can basically be anything, the only way to solve this would be to whitelist the relevant URIs. Note that, doing so will make it trivial for sites to fingerprint if the addon is present or not (then again, people installing extra addons/plugins void the non-existent warranty in the first place).

comment:8 Changed 2 years ago by HolD

gk, buttons are not available at all, even in Customize mode. Also, menu items under Tools are not available too.

comment:9 in reply to:  7 ; Changed 2 years ago by gk

Cc: lnl added

Replying to yawning:

Replying to gk:

Session Manager does not like it that chrome://sessionmanager/locale/sessionmanager.dtd and chrome://sessionmanager/locale/options.dtd are blocked by our fix for #8725. Things like &toolbar.tooltip need to be available for content after the first installation. HolD: does adding the toolbar buttons manually to the toolbar work for you?

What's the origin URI when the requests make it to the content policy? If this is one of the cases where aRequestOrigin can basically be anything, the only way to solve this would be to whitelist the relevant URIs. Note that, doing so will make it trivial for sites to fingerprint if the addon is present or not (then again, people installing extra addons/plugins void the non-existent warranty in the first place).

moz-nullprincipal:{1f22744b-c4db-41b6-8d6e-3d06c176578e}. Looking at the docs it seems like checking for that one would be okay. But this is not a solution that scales well. I wonder if we should just add a preference extensions.torbutton_resource_and_chrome_uri_fingerprinting and set that to false by default allowing users to override it and to disable the content policy hack. Maybe UX folks have an idea.

comment:10 in reply to:  9 Changed 2 years ago by yawning

Replying to gk:

moz-nullprincipal:{1f22744b-c4db-41b6-8d6e-3d06c176578e}. Looking at the docs it seems like checking for that one would be okay. But this is not a solution that scales well. I wonder if we should just add a preference extensions.torbutton_resource_and_chrome_uri_fingerprinting and set that to false by default allowing users to override it and to disable the content policy hack. Maybe UX folks have an idea.

An alternative approach would be to have a pref that changes the CSP behavior from being whitelist based to blacklist based so that it will still defend against say... loading torbutton resources. The current behavior is more robust and more resilient against this particular fingerprinting method (since it protects all resources), so it should be the default regardless of how the actual pref ends up being implemented.

Installing extra addons never has been something considered safe, or part of Tor Browser's threat model as far as I am aware, and people that chose to do so should be doing it with the understanding that it may open them up to various fingerprinting attacks.

The real fix would be for upstream Firefox to plug the chrome/resource URI scheme issues correctly, naturally.

comment:11 Changed 2 years ago by gk

Keywords: TorBrowserTeam201703R added; TorBrowserTeam201702 removed
Status: needs_informationneeds_review

What about bug_21396 (https://gitweb.torproject.org/user/gk/torbutton.git/commit/?h=bug_21396&id=3a285294fe1db84b140db456900cdbf9a62bab0c) in my public Torbutton repo? I tested it with the Session Manager extension and flipping the pref seems to get it usable. Moreover, I tested on https://browserleaks.com/firefox whether flipping the pref back and forth in a session has the respective outcome and it does: once the pref is flipped to true values get extracted but that stops as soon as it is flipped back to false and the page is reloaded.

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

Replying to gk:

What about bug_21396 (https://gitweb.torproject.org/user/gk/torbutton.git/commit/?h=bug_21396&id=3a285294fe1db84b140db456900cdbf9a62bab0c) in my public Torbutton repo?

The patch looks okay. Kathy and I tested it with browserleaks.com but not with the Session Manager add-on. Just a few small things that you may want to fix inside the shouldLoad() function:

  1. It is inconsistent to not prefix uriFingerprinting with this. (but the code seems to work).
  2. It might be better to check this.uriFingerprinting first since the schemeIs() calls must take longer to execute than checking a Boolean JS variable.
  3. Adding braces wouldn't hurt.

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

Resolution: fixed
Status: needs_reviewclosed

Replying to mcs:

Replying to gk:

What about bug_21396 (https://gitweb.torproject.org/user/gk/torbutton.git/commit/?h=bug_21396&id=3a285294fe1db84b140db456900cdbf9a62bab0c) in my public Torbutton repo?

The patch looks okay. Kathy and I tested it with browserleaks.com but not with the Session Manager add-on. Just a few small things that you may want to fix inside the shouldLoad() function:

  1. It is inconsistent to not prefix uriFingerprinting with this. (but the code seems to work).

Oops, that's a remaining part of my first patch idea. :(

  1. It might be better to check this.uriFingerprinting first since the schemeIs() calls must take longer to execute than checking a Boolean JS variable.

Fixed.

  1. Adding braces wouldn't hurt.

Fixed.

All those fixed are in commit 84ff007519fa475d36800e979169f961fe0c072e on master. Backported to maint-1.9.6 with fc00aef1fd552784dfca72dcdbb78e72e2f9932e. Thanks!

comment:14 Changed 2 years ago by gk

Resolution: fixed
Status: closedreopened

Gah, I should have thought more about uriFingerprinting being available (and not have followed your advice blindly ;) ). See my fixup in my bug_21396_v3 (https://gitweb.torproject.org/user/gk/torbutton.git/commit/?h=bug_21396_v3&id=2049d704d464907b6c58123f8c98df976f4f36e4).

uriFingerprinting was available because it got put into global scope by this.uriFingerprinting inside the handler function. Does that and the fixup make sense? The new code fixes that problem and works now as well.

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

comment:15 Changed 2 years ago by gk

Cc: mcs brade added
Status: reopenedneeds_review

comment:16 Changed 2 years ago by mcs

r=brade, r=mcs
The revised fix looks good and seems to work. I am sorry for our contribution to the bustage.

We did notice that a lot of messages like the following are logged to the Browser Console, but these are not new:

15:09:57.836 NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIHttpChannel.getResponseHeader]1 content-policy.js:99:0

Maybe something on https://browserleaks.com/firefox is returning a 3xx response code without a Location header? Kathy and I will take a look after we spend some time on #21514.

comment:17 in reply to:  16 ; Changed 2 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Replying to mcs:

r=brade, r=mcs
The revised fix looks good and seems to work. I am sorry for our contribution to the bustage.

We had to rebuild anyway and I caught it while doing "Q&A", so all is good. :)

We did notice that a lot of messages like the following are logged to the Browser Console, but these are not new:

15:09:57.836 NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIHttpChannel.getResponseHeader]1 content-policy.js:99:0

Maybe something on https://browserleaks.com/firefox is returning a 3xx response code without a Location header? Kathy and I will take a look after we spend some time on #21514.

Did you find something out?

FWIW I applied it to master as commit 2049d704d464907b6c58123f8c98df976f4f36e4 and to maint-1.9.6 as commit 1ef040450840591b9e6b1855114e71a481cb2590.

comment:18 in reply to:  17 Changed 2 years ago by mcs

Replying to gk:

Replying to mcs:

Maybe something on https://browserleaks.com/firefox is returning a 3xx response code without a Location header? Kathy and I will take a look after we spend some time on #21514.

Did you find something out?

Yes, although we did not create a fix yet. I just filed #21627 to track the issue.

comment:19 Changed 2 years ago by HolD

I'm sorry, is there any build with fix that I can use? Or where to watch for it? Right now I have to use old version, but I want to be up to date.

comment:20 Changed 2 years ago by gk

6.5.1/7.0a2/7.0a2-hardened will contain a way for you to fix that. They'll be released later today/tomorrow (depending on your timezone). Note, though, this will still be broken by default in those releases but you will be able to work around that by setting extensions.torbutton.resource_and_chrome_uri_fingerprinting to true.

Note: See TracTickets for help on using tickets.