Opened 2 years ago
Closed 8 months 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 2 years ago by
Keywords: | tbb-circuit-display added |
---|
comment:2 Changed 2 years ago by
comment:3 Changed 2 years ago by
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 2 years ago by
Keywords: | tbb-regression tbb-linkability TorBrowserTeam201712 GeorgKoppen201712 added |
---|---|
Priority: | Medium → High |
Severity: | Normal → Major |
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 2 years ago by
Keywords: | tbb-7.0-issues added; tbb-circuit-display removed |
---|
comment:6 Changed 2 years ago by
Keywords: | GeorgKoppen201801 added; GeorgKoppen201712 removed |
---|
Moving my tickets to 2018
comment:7 Changed 2 years ago by
Keywords: | TorBrowserTeam201801 added; TorBrowserTeam201712 removed |
---|
Moving tickets to 2018.
comment:8 Changed 22 months ago by
Keywords: | GeorgKoppen201802 added; GeorgKoppen201801 removed |
---|
Moving my tickets to Feb.
comment:9 Changed 22 months ago by
Keywords: | TorBrowserTeam201802 added; TorBrowserTeam201801 removed |
---|
Moving tickets to Feb
comment:10 Changed 21 months ago by
Keywords: | GeorgKoppen201803 added; GeorgKoppen201802 removed |
---|
Moving my tickets to March.
comment:11 Changed 21 months ago by
Keywords: | TorBrowserTeam201803 added; TorBrowserTeam201802 removed |
---|
Adding to our March plate.
comment:12 Changed 21 months ago by
Keywords: | GeorgKoppen201804 added; GeorgKoppen201803 removed |
---|
Moving my tickets to April 2018
comment:13 Changed 20 months ago by
Keywords: | TorBrowserTeam201804 added; TorBrowserTeam201803 removed |
---|
Moving our tickets to April.
comment:14 Changed 20 months ago by
Priority: | High → Medium |
---|
comment:15 Changed 19 months ago by
Keywords: | TorBrowserTeam201805 added; TorBrowserTeam201804 removed |
---|
Moving remaining tickets to May.
comment:16 Changed 19 months ago by
Keywords: | GeorgKoppen201805 added; GeorgKoppen201804 removed |
---|
Moving my tickets.
comment:17 Changed 18 months ago by
Keywords: | GeorgKoppen201806 added; GeorgKoppen201805 removed |
---|
Moving my tickets to June 2018.
comment:18 Changed 18 months ago by
Keywords: | TorBrowserTeam201806 added; TorBrowserTeam201805 removed |
---|
Moving our tickets to June 2018
comment:20 Changed 18 months ago by
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 17 months ago by
Keywords: | TorBrowserTeam201807 added; TorBrowserTeam201806 removed |
---|
More tickets for July.
comment:22 Changed 17 months ago by
Keywords: | GeorgKoppen201807 added; GeorgKoppen201806 removed |
---|
Moving my tickets.
comment:23 Changed 17 months ago by
Keywords: | GeorgKoppen201808 added; GeorgKoppen201807 removed |
---|
Moving my tickets.
comment:24 Changed 17 months ago by
Keywords: | TorBrowserTeam201808 added; TorBrowserTeam201807 removed |
---|
Move our tickets to August.
comment:25 Changed 15 months ago by
Keywords: | GeorgKoppen201809 added; GeorgKoppen201808 removed |
---|
Moving my tickets to September.
comment:26 Changed 15 months ago by
Keywords: | TorBrowserTeam201809 added; TorBrowserTeam201808 removed |
---|
Moving our tickets to September 2018
comment:27 Changed 14 months ago by
Keywords: | TorBrowserTeam201810 added; TorBrowserTeam201809 removed |
---|
Moving tickets to October
comment:28 Changed 14 months ago by
Keywords: | GeorgKoppen201810 added; GeorgKoppen201809 removed |
---|
Moving my tickets to October.
comment:29 Changed 13 months ago by
Keywords: | TorBrowserTeam201811 added; TorBrowserTeam201810 removed |
---|
Moving our tickets to November.
comment:30 Changed 13 months ago by
Keywords: | GeorgKoppen201811 added; GeorgKoppen201810 removed |
---|
comment:31 Changed 12 months ago by
Keywords: | GeorgKoppen201812 added; GeorgKoppen201811 removed |
---|
Moving my tickets to December.
comment:32 Changed 12 months ago by
Keywords: | TorBrowserTeam201812 added; TorBrowserTeam201811 removed |
---|
Moving our tickets to December.
comment:33 Changed 11 months ago by
Keywords: | GeorgKoppen201901 added; GeorgKoppen201812 removed |
---|
Moving my tickets.
comment:34 Changed 11 months ago by
Keywords: | TorBrowserTeam201901 added; TorBrowserTeam201812 removed |
---|
Moving tickets to Jan 2019.
comment:35 Changed 10 months ago by
Keywords: | GeorgKoppen201902 added; GeorgKoppen201901 removed |
---|
Moving my tickets to Feb
comment:36 Changed 10 months ago by
Keywords: | TorBrowserTeam201902 added; TorBrowserTeam201901 removed |
---|
Moving tickets to February.
comment:37 Changed 9 months ago by
Keywords: | TorBrowserTeam201903 added; TorBrowserTeam201902 removed |
---|
Moving my tickets to March.
comment:38 Changed 9 months ago by
Keywords: | GeorgKoppen201903 added; GeorgKoppen201902 removed |
---|
Now for my keyword.
comment:39 Changed 8 months ago by
Keywords: | TorBrowserTeam201904 added; TorBrowserTeam201903 removed |
---|
Moving tickets to April.
comment:40 follow-up: 41 Changed 8 months ago by
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 Changed 8 months ago by
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:42 Changed 8 months ago by
Reported here: https://bugzilla.mozilla.org/show_bug.cgi?id=1542309
comment:43 follow-up: 45 Changed 8 months ago by
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 8 months ago by
Status: | new → needs_review |
---|
comment:45 Changed 8 months ago by
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 follow-up: 47 Changed 8 months ago by
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 Changed 8 months ago by
Replying to acat:
It was to make sure the special cases
about:
andblob:
are still handled like before. If I'm not wrong, the error on those cases would still beNS_ERROR_INSUFFICIENT_DOMAIN_LEVELS
(empty host). So the condition needs to beif (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 return
ing 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 follow-up: 49 Changed 8 months ago by
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 Changed 8 months ago by
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:50 Changed 8 months ago by
comment:51 Changed 8 months ago by
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 follow-up: 53 Changed 8 months ago by
Fixed in mozilla-central: https://bugzilla.mozilla.org/show_bug.cgi?id=1542309
comment:53 Changed 8 months ago by
Keywords: | TorBrowserTeam201904R added; TorBrowserTeam201904 removed |
---|---|
Resolution: | → fixed |
Status: | needs_review → closed |
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.
Hey, cpunk! We used
Torcrazybutton
when it did something really crazy, but never mind :)