Opened 2 years ago

Closed 6 weeks ago

Last modified 4 weeks ago

#22538 closed defect (fixed)

Changing circuit for page with error switches catch-all circuit instead

Reported by: gk Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-linkability, TorBrowserTeam201904R
Cc: dmr, h1n1, creideiki+tor-trac@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by gk)

Go to https://pinning-test.badssl.com/ and change the circuit for this site. The expected behavior would be to use a new circuit keyed to badssl.com. But what happens is that the catch-all circuit gets changed. Found in #22513.

This is not only certificate related the timeout error page exhibits the same problem.

Child Tickets

Change History (22)

comment:1 Changed 2 years ago by gk

Description: modified (diff)
Summary: Changing circuit for page with certificate error switches catch-all circuit insteadChanging circuit for page with error switches catch-all circuit instead

comment:2 Changed 20 months ago by cypherpunks

Keywords: tbb-usability-website added; tbb-linkability removed

No linkability implications.

comment:3 Changed 20 months ago by gk

Keywords: tbb-linkability added; tbb-usability-website removed
Status: newneeds_information

The usability of the website is not affected by that. Could you explain your reasoning behind the keyword change? (Reverting it back to the original one meanwhile)

comment:4 in reply to:  3 Changed 20 months ago by gk

Replying to gk:

The usability of the website is not affected by that. Could you explain your reasoning behind the keyword change? (Reverting it back to the original one meanwhile)

Expanding on that: I think the option to change a circuit for a particular domain might indeed perceived as an easy way to reduce linkability (which it actually even is). So, if that's not working but the old circuit is still used as only the catch-all one gets rotated that's bad.

comment:5 Changed 20 months ago by cypherpunks

Status: needs_informationnew

Changing circuit for page with error switches catch-all circuit instead

The page with error usually is about:*, so that's technically correct to switch its circuit (catch-all), but it's not what is expected. If some error page is caused by bad circuit, then it's necessary to change it, but it's impossible. Thus, tbb-usability-website.
TBB has no feature to remove linkability by switching circuits, everything is perfectly linkable by cookies.

comment:6 in reply to:  5 ; Changed 20 months ago by gk

Replying to cypherpunks:

Changing circuit for page with error switches catch-all circuit instead

The page with error usually is about:*, so that's technically correct to switch its circuit (catch-all), but it's not what is expected. If some error page is caused by bad circuit, then it's necessary to change it, but it's impossible. Thus, tbb-usability-website.
TBB has no feature to remove linkability by switching circuits, everything is perfectly linkable by cookies.

Switching circuits is part of the solution to the linkability problem which includes dealing with a bunch of different techniques (like cookies, DOM storage etc.).

That said thanks for you explanation. I think we can better tag this ticket with tbb-usability.

comment:7 in reply to:  6 Changed 20 months ago by cypherpunks

Replying to gk:

Switching circuits is part of the solution to the linkability problem which includes dealing with a bunch of different techniques (like cookies, DOM storage etc.).

It was. Before FPI. Now it is used and useful for the catch-all circuit only. But it's not a panacea, just a workaround.

That said thanks for you explanation. I think we can better tag this ticket with tbb-usability.

tbb-usability is used for minor things e.g. in GUI, something to think about by ux-team, but not for functionality loss.

comment:8 Changed 18 months ago by gk

A somewhat related issue is mentioned in comment:16:ticket:15897. We might be able to solve that one in this bug, too.

comment:9 Changed 18 months ago by gk

#24493 is a duplicate.

comment:10 Changed 14 months ago by sysrqb

#25670 is a duplicate.

comment:11 Changed 9 months ago by dmr

Cc: dmr added

comment:12 Changed 6 months ago by gk

Cc: h1n1 added

#28376 is a duplicate.

comment:13 Changed 7 weeks ago by creideiki

Cc: creideiki+tor-trac@… added

comment:14 Changed 7 weeks ago by acat

torbutton uses gBrowser.contentPrincipal.originAttributes.firstPartyDomain to get the active firstPartyDomain to change circuit. This is set to about.ef2a7dd5-93bc-417f-a698-142c3116864f.mozilla for all about:* pages, including about:neterror and about:certerror which are the cases here. So, if I'm not wrong, changing the circuit when there is a network or ssl error currently changes the circuit for all about:* pages (not the catch-all circuit).

I did not find an obvious way to get the firstPartyDomain causing the error directly from gBrowser.*. Here is a patch that gets it from the url parameter that about:neterror and about:certerror has, which is the original url causing it: https://github.com/acatarineu/torbutton/commit/22538

Another possibility could be forcing a new circuit always when there are network or ssl errors, although I'm not sure if that's desirable.

Also not sure if this is the same as https://trac.torproject.org/projects/tor/ticket/25670, since I have only seen this behaviour either in neterror or certerror.

comment:15 Changed 7 weeks ago by gk

Keywords: TorBrowserTeam201904R added
Status: newneeds_review

comment:16 in reply to:  14 ; Changed 7 weeks ago by gk

Keywords: TorBrowserTeam201904 added; TorBrowserTeam201904R removed
Status: needs_reviewneeds_revision

Replying to acat:

torbutton uses gBrowser.contentPrincipal.originAttributes.firstPartyDomain to get the active firstPartyDomain to change circuit. This is set to about.ef2a7dd5-93bc-417f-a698-142c3116864f.mozilla for all about:* pages, including about:neterror and about:certerror which are the cases here. So, if I'm not wrong, changing the circuit when there is a network or ssl error currently changes the circuit for all about:* pages (not the catch-all circuit).

Yes, this behavior is new since Tor Browser 8 (we got that with https://bugzilla.mozilla.org/show_bug.cgi?id=1300671).

I did not find an obvious way to get the firstPartyDomain causing the error directly from gBrowser.*. Here is a patch that gets it from the url parameter that about:neterror and about:certerror has, which is the original url causing it: https://github.com/acatarineu/torbutton/commit/22538

Nice idea and I think that works for now at least. Some review comments:

1) We have been thinking about constification of code as much as possible (see: #26184) but we are not there yet. Thus, please use let for now where you don't have fixed strings. (This means everywhere besides ABOUT_URI_FIRST_PARTY_DOMAIN).

2) Please put the const at the beginning of the file using a similar naming schema as the other consts we already have.

3)

+  const knownErrors = ["about:neterror", "about:certerror"];
+  const origin = gBrowser.contentPrincipal.origin || '';
+  if (isAboutFPD && knownErrors.some(x => origin.startsWith(x))) {
+    try {

I think it's not needed to determine origin for every domain we are requesting but only for those that have the about-firstpartydomain. Thus, even if the code is slightly more complex please rewrite that to something like

+  if (isAboutFPD) {
+    let knownErrors = ["about:neterror", "about:certerror"];
+    let origin = gBrowser.contentPrincipal.origin || '';
+    if (knownErrors.some(x => origin.startsWith(x))) {
+      try {


Another possibility could be forcing a new circuit always when there are network or ssl errors, although I'm not sure if that's desirable.

Hm, there is the risk of burning a bunch of circuits that way which might not help if a website is poorly configured. So, I think I'd rather avoid that, at least for now.

Also not sure if this is the same as https://trac.torproject.org/projects/tor/ticket/25670, since I have only seen this behaviour either in neterror or certerror.

I'd assume so. What would be helpful is figuring out whether we'd fix #22513 as well with your patch. If so, we should note this in the commit message and close that bug as well.

comment:17 in reply to:  16 Changed 7 weeks ago by acat

Updated with requested changes: https://github.com/acatarineu/torbutton/commit/22538+1

Replying to gk:

I'd assume so. What would be helpful is figuring out whether we'd fix #22513 as well with your patch. If so, we should note this in the commit message and close that bug as well.

After the patch, New Circuit for this site should fix those errors. Not handled automatically as the reporter was first suggesting, but I think this is enough to close it as fixed.

comment:18 Changed 7 weeks ago by gk

Keywords: TorBrowserTeam201904R added; TorBrowserTeam201904 removed
Resolution: fixed
Status: needs_revisionclosed

Looks good, thanks! Merged to Torbutton with commit 5e3bc46165b0963bb63976f17c742da5dd5fd724.

comment:19 Changed 6 weeks ago by acat

Resolution: fixed
Status: closedreopened

While looking at #24622 I realized that this one would still not work for IP address hosts, here is the patch for that: https://github.com/acatarineu/torbutton/commit/22538+2

comment:20 Changed 6 weeks ago by acat

Status: reopenedneeds_review

comment:21 Changed 6 weeks ago by gk

Resolution: fixed
Status: needs_reviewclosed

Nice catch. Merged to master (commit a762819edb07559959c163dec4fa29768f7e021c).

comment:22 Changed 4 weeks ago by gk

#30242 is a duplicate.

Note: See TracTickets for help on using tickets.