Torbutton breaks Session Manager addon

added GeorgKoppen201702 TorBrowserTeam201703R component::applications/tor browser owner::tbb-team priority::medium reporter::HolD resolution::fixed severity::normal status::closed tbb-6.5-regression type::defect labels

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

Trac:
Username: HolD

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/

Trac:
Status: new to needs_information
Keywords: N/A deleted, tbb-6.5-regression added

6.5a1 - no issue, 6.5a2 - has issue. Steps to reproduce:

install Tor Browser,
install addon https://addons.mozilla.org/en-US/firefox/addon/session-manager/,
confirm information popup about private browsing mode,
Session Manager button should immediately appear on toolbar (no restart or changing preferences is required).

Trac:
Username: HolD

Trac:
Keywords: N/A deleted, TorBrowserTeam201702 added

Trac:
Keywords: N/A deleted, GeorgKoppen201702 added

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 (moved). 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?

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 (moved). 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).

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

Trac:
Username: HolD

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 (moved). 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.

Trac:
Cc: N/A to lnl

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.

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.

Trac:
Keywords: TorBrowserTeam201702 deleted, TorBrowserTeam201703R added
Status: needs_information to needs_review

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:

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

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:

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. :(

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.

Adding braces wouldn't hurt.

Fixed.

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

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

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.

Trac:
Status: closed to reopened
Resolution: fixed to N/A

Trac:
Status: reopened to needs_review
Cc: lnl to lnl, mcs, brade

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 (moved).

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 (moved).

Did you find something out?

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

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

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 (moved).

Did you find something out?

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

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.

Trac:
Username: HolD

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.

closed

mentioned in issue #21627 (moved)

mentioned in issue #21681 (moved)

moved to tpo/applications/tor-browser#21396 (closed)

mentioned in issue tpo/applications/tor-browser#21627 (closed)

Torbutton breaks Session Manager addon

Child items ...

Activity