Torcrazybutton can't decipher website s3.amazonaws.com
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... 
Activity
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?
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.
Reported here: https://bugzilla.mozilla.org/show_bug.cgi?id=1542309
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?)
-
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 aelse if (rv == NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS) {treating both error cases together. That might even be better. 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").-
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
returning both in theaboutif-clause and in theblobelseif-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 theisInsufficientDomainLevelsdeclaration 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.
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.
-
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.