Opened 17 months ago

Closed 4 weeks ago

#24622 closed defect (fixed)

Torcrazybutton can't decipher website s3.amazonaws.com

Reported by: cypherpunks Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Major Keywords: tbb-7.0-issues, tbb-regression, tbb-linkability, GeorgKoppen201903, TorBrowserTeam201904R
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Go to https://s3.amazonaws.com/bucket/filled/with/pile/of/onions

Then click on the Torbutton and you should see this:

-----
| @ |
-----
 |----------------------------------------------------------------------------
 | New Identity                   Ctrl+Shift+U | Tor circuit for this site   |
 | New Tor Circuit for this Site  Ctrl+Shift+L | (--unknown--)               |
 | --------------------------------------------| o This browser              |
 | Security Settings...                        | o China  (xx.xx.xx.xx)      |
 | Tor Network Settings...                     | o Russia (xx.xx.xx.xx)      |
 | --------------------------------------------| o North Korea (xx.xx.xx.xx) |
 | Check for Tor Browser Update...             | o Internet                  |
 |                                             |                             |
 -----------------------------------------------------------------------------

The relevant part is Tor circuit for this site (--unknown--). Meaning that it can't decipher amazonaws.com it seems... 🤔

Child Tickets

Change History (53)

comment:1 Changed 17 months ago by gk

Keywords: tbb-circuit-display added

comment:2 Changed 17 months ago by cypherpunks

Hey, cpunk! We used Torcrazybutton when it did something really crazy, but never mind :)

comment:3 Changed 17 months ago by cypherpunks

If you go to https://www.overleaf.com/docs/12882290qmcxzryvtwqn/atts/69956807 it will redirect you to a link with https://writelatex.s3.amazonaws.com/... and if you open the Torbutton you'll see: Tor circuit for this site (writelatex.s3.amazonaws.com)

comment:4 Changed 17 months ago by gk

Keywords: tbb-regression tbb-linkability TorBrowserTeam201712 GeorgKoppen201712 added
Priority: MediumHigh
Severity: NormalMajor

Fun, fun, this is a regression from 7.0. I wonder whether the Mozilla logic we are using now for FPI has a bug here.

comment:5 Changed 17 months ago by gk

Keywords: tbb-7.0-issues added; tbb-circuit-display removed

comment:6 Changed 17 months ago by gk

Keywords: GeorgKoppen201801 added; GeorgKoppen201712 removed

Moving my tickets to 2018

comment:7 Changed 17 months ago by gk

Keywords: TorBrowserTeam201801 added; TorBrowserTeam201712 removed

Moving tickets to 2018.

comment:8 Changed 16 months ago by gk

Keywords: GeorgKoppen201802 added; GeorgKoppen201801 removed

Moving my tickets to Feb.

comment:9 Changed 16 months ago by gk

Keywords: TorBrowserTeam201802 added; TorBrowserTeam201801 removed

Moving tickets to Feb

comment:10 Changed 15 months ago by gk

Keywords: GeorgKoppen201803 added; GeorgKoppen201802 removed

Moving my tickets to March.

comment:11 Changed 15 months ago by gk

Keywords: TorBrowserTeam201803 added; TorBrowserTeam201802 removed

Adding to our March plate.

comment:12 Changed 14 months ago by gk

Keywords: GeorgKoppen201804 added; GeorgKoppen201803 removed

Moving my tickets to April 2018

comment:13 Changed 14 months ago by gk

Keywords: TorBrowserTeam201804 added; TorBrowserTeam201803 removed

Moving our tickets to April.

comment:14 Changed 14 months ago by gk

Priority: HighMedium

comment:15 Changed 13 months ago by gk

Keywords: TorBrowserTeam201805 added; TorBrowserTeam201804 removed

Moving remaining tickets to May.

comment:16 Changed 13 months ago by gk

Keywords: GeorgKoppen201805 added; GeorgKoppen201804 removed

Moving my tickets.

comment:17 Changed 12 months ago by gk

Keywords: GeorgKoppen201806 added; GeorgKoppen201805 removed

Moving my tickets to June 2018.

comment:18 Changed 12 months ago by gk

Keywords: TorBrowserTeam201806 added; TorBrowserTeam201805 removed

Moving our tickets to June 2018

comment:19 Changed 11 months ago by cypherpunks

FWIW still happens on 8.0a9.

comment:20 Changed 11 months ago by cypherpunks

Giving another related example just to not forget about it: Circuit display shows --unknown-- for https://1.1.1.1/ and https://1.0.0.1.

comment:21 Changed 11 months ago by gk

Keywords: TorBrowserTeam201807 added; TorBrowserTeam201806 removed

More tickets for July.

comment:22 Changed 11 months ago by gk

Keywords: GeorgKoppen201807 added; GeorgKoppen201806 removed

Moving my tickets.

comment:23 Changed 10 months ago by gk

Keywords: GeorgKoppen201808 added; GeorgKoppen201807 removed

Moving my tickets.

comment:24 Changed 10 months ago by gk

Keywords: TorBrowserTeam201808 added; TorBrowserTeam201807 removed

Move our tickets to August.

comment:25 Changed 9 months ago by gk

Keywords: GeorgKoppen201809 added; GeorgKoppen201808 removed

Moving my tickets to September.

comment:26 Changed 9 months ago by gk

Keywords: TorBrowserTeam201809 added; TorBrowserTeam201808 removed

Moving our tickets to September 2018

comment:27 Changed 8 months ago by gk

Keywords: TorBrowserTeam201810 added; TorBrowserTeam201809 removed

Moving tickets to October

comment:28 Changed 8 months ago by gk

Keywords: GeorgKoppen201810 added; GeorgKoppen201809 removed

Moving my tickets to October.

comment:29 Changed 7 months ago by gk

Keywords: TorBrowserTeam201811 added; TorBrowserTeam201810 removed

Moving our tickets to November.

comment:30 Changed 7 months ago by gk

Keywords: GeorgKoppen201811 added; GeorgKoppen201810 removed

comment:31 Changed 6 months ago by gk

Keywords: GeorgKoppen201812 added; GeorgKoppen201811 removed

Moving my tickets to December.

comment:32 Changed 6 months ago by gk

Keywords: TorBrowserTeam201812 added; TorBrowserTeam201811 removed

Moving our tickets to December.

comment:33 Changed 4 months ago by gk

Keywords: GeorgKoppen201901 added; GeorgKoppen201812 removed

Moving my tickets.

comment:34 Changed 4 months ago by gk

Keywords: TorBrowserTeam201901 added; TorBrowserTeam201812 removed

Moving tickets to Jan 2019.

comment:35 Changed 4 months ago by gk

Keywords: GeorgKoppen201902 added; GeorgKoppen201901 removed

Moving my tickets to Feb

comment:36 Changed 3 months ago by gk

Keywords: TorBrowserTeam201902 added; TorBrowserTeam201901 removed

Moving tickets to February.

comment:37 Changed 3 months ago by gk

Keywords: TorBrowserTeam201903 added; TorBrowserTeam201902 removed

Moving my tickets to March.

comment:38 Changed 3 months ago by gk

Keywords: GeorgKoppen201903 added; GeorgKoppen201902 removed

Now for my keyword.

comment:39 Changed 7 weeks ago by gk

Keywords: TorBrowserTeam201904 added; TorBrowserTeam201903 removed

Moving tickets to April.

comment:40 Changed 7 weeks ago by acat

Firefox computes the firstPartyDomain with Services.eTLD.getBaseDomainFromHost('s3.amazonaws.com'), and that one throws an error with top-level domains (like s3.amazonaws.com) as defined in mozilla public suffix list https://publicsuffix.org/list/. And I assume if there's an exception then it gets set to an empty string (unless it's about:*, etc.).

Which means that all domains here will go to the same catch-all circuit: https://gitweb.torproject.org/tor-browser.git/tree/netwerk/dns/effective_tld_names.dat?h=tor-browser-60.6.1esr-8.0-1-build1. So urls like http://mycd.eu, http://s3.amazonaws.com/whatever, https://ownprovider.com/en/Main, ..., will go to the same catch-all circuit with the current firstPartyDomain implementation. The same happens with any random domain like http://foobarfoo. Also note the list in the repo is not up to date with https://publicsuffix.org/list/public_suffix_list.dat.

Shouldn't firstPartyDomain be the first-level domain on those cases (instead of empty string), or am I missing some reason why this is not a good idea?

comment:41 in reply to:  40 Changed 7 weeks ago by gk

Replying to acat:

Firefox computes the firstPartyDomain with Services.eTLD.getBaseDomainFromHost('s3.amazonaws.com'), and that one throws an error with top-level domains (like s3.amazonaws.com) as defined in mozilla public suffix list https://publicsuffix.org/list/. And I assume if there's an exception then it gets set to an empty string (unless it's about:*, etc.).

Which means that all domains here will go to the same catch-all circuit: https://gitweb.torproject.org/tor-browser.git/tree/netwerk/dns/effective_tld_names.dat?h=tor-browser-60.6.1esr-8.0-1-build1. So urls like http://mycd.eu, http://s3.amazonaws.com/whatever, https://ownprovider.com/en/Main, ..., will go to the same catch-all circuit with the current firstPartyDomain implementation. The same happens with any random domain like http://foobarfoo. Also note the list in the repo is not up to date with https://publicsuffix.org/list/public_suffix_list.dat.

Shouldn't firstPartyDomain be the first-level domain on those cases (instead of empty string), or am I missing some reason why this is not a good idea?

Interesting. My intuition goes with you here and I'd assume that's a bug. Could you file a bug at Mozilla's bug tracker blocking https://bugzilla.mozilla.org/show_bug.cgi?id=1299996? We could ask :ethan to help us finding someone at Mozilla who'd have an opinion about that.

Meanwhile, feel free to work on a patch implementing your suggestion. We should take it for Tor Browser at least.

comment:43 Changed 6 weeks ago by acat

Here is a patch: https://github.com/acatarineu/tor-browser/commit/24622

Not sure if we should wait for more feedback on https://bugzilla.mozilla.org/show_bug.cgi?id=1542309. Implemented dveditz first suggestion, the alternative I see is to try to fix it just on torbutton by looking at the special case when firstPartyOrigin is empty, and try to get it from somewhere else (nsiChannel.loadInfo.loadingPrincipal?)

comment:44 Changed 6 weeks ago by acat

Status: newneeds_review

comment:45 in reply to:  43 Changed 6 weeks ago by gk

Replying to acat:

Here is a patch: https://github.com/acatarineu/tor-browser/commit/24622

Not sure if we should wait for more feedback on https://bugzilla.mozilla.org/show_bug.cgi?id=1542309. Implemented dveditz first suggestion, the alternative I see is to try to fix it just on torbutton by looking at the special case when firstPartyOrigin is empty, and try to get it from somewhere else (nsiChannel.loadInfo.loadingPrincipal?)

I think that approach is good and we should get the fix into Firefox as Mozilla's users are affected by this problem as well.

Regarding your patch: why did you deal with the insufficient domain level case in the code block that is concerned with the scheme of the URL? It seems it does not fit there. What about having a special block before that one like the ip address error one. It could start with
if (rv == NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS) { followed by the remainder of your patch. Or maybe you use the ip address error block and do a else if (rv == NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS) { treating both error cases together. That might even be better.

comment:46 Changed 6 weeks ago by acat

It was to make sure the special cases about: and blob: are still handled like before. If I'm not wrong, the error on those cases would still be NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS (empty host). So the condition needs to be if (rv == NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS && !scheme.EqualsLiteral("about") && !scheme.EqualsLiteral("blob").

comment:47 in reply to:  46 Changed 6 weeks ago by gk

Replying to acat:

It was to make sure the special cases about: and blob: are still handled like before. If I'm not wrong, the error on those cases would still be NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS (empty host). So the condition needs to be if (rv == NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS && !scheme.EqualsLiteral("about") && !scheme.EqualsLiteral("blob").

Right. I am still not overly happy to mix this new check with scheme related ones. What about returning both in the about if-clause and in the blob elseif-clause after the checks are done and then having an insufficient_domain_levels check in an own block afterwards in case the code still has not returned? Oh, and adding a comment above the isInsufficientDomainLevels declaration would be good about why we have this one at that place at all.

I guess before we overengineer that on our side it might be worth getting this to review for Mozilla folks (I wanted to point you to try builds etc. after we have a reasonable Tor Browser patch, but it seems tjr has jumped the gun ;). So, in case tjr's try build looks good, could you request review on the Moz bug and then we'd basically take what Mozilla is happy with? Or you could post a revised patch based on my comments above (if they make sense to you) to the ticket and request review. Up to you.

comment:48 Changed 6 weeks ago by acat

Updated here addressing comments: https://github.com/acatarineu/tor-browser/commit/24622+1

Will work on hg patches for esr60 and mozilla-central meanwhile, if the above is fine.

comment:49 in reply to:  48 Changed 6 weeks ago by gk

Replying to acat:

Updated here addressing comments: https://github.com/acatarineu/tor-browser/commit/24622+1

Will work on hg patches for esr60 and mozilla-central meanwhile, if the above is fine.

Looks good to me.

comment:51 Changed 6 weeks ago by acat

Is the test failing a flaky one?

I just had added a patch (via phabricator) to https://bugzilla.mozilla.org/show_bug.cgi?id=1542309 a few minutes before. Did not see that you had updated the try submission, now this one is the same as the patch.

comment:52 Changed 5 weeks ago by acat

comment:53 in reply to:  52 Changed 4 weeks ago by gk

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

Replying to acat:

Fixed in mozilla-central: https://bugzilla.mozilla.org/show_bug.cgi?id=1542309

Okay, neat! I cherry-picked the patch that landed in mozilla-central onto tor-browser-60.6.1esr-8.5-1 (commit c722d57604db58695140d95565a78433989fe9ca). The code applied cleanly and I reformatted the tests according to the code-style in use for esr60. I think we are good here and am fine with shipping the patch on our own without any esr60 backport by Mozilla.

Note: See TracTickets for help on using tickets.