Torcrazybutton can't decipher website s3.amazonaws.com

[cypherpunks]

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... 🤔

[cypherpunks]

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

[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)

[gk]

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.

[gk]

Moving my tickets to 2018

[gk]

Moving tickets to 2018.

[gk]

Moving my tickets to Feb.

[gk]

Moving tickets to Feb

[gk]

Moving my tickets to March.

[gk]

Adding to our March plate.

[gk]

Moving my tickets to April 2018

[gk]

Moving our tickets to April.

[gk]

Moving remaining tickets to May.

[gk]

Moving my tickets.

[gk]

Moving my tickets to June 2018.

[gk]

Moving our tickets to June 2018

[cypherpunks]

FWIW still happens on 8.0a9.

[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.

[gk]

More tickets for July.

[gk]

Moving my tickets.

[gk]

Moving my tickets.

[gk]

Move our tickets to August.

[gk]

Moving my tickets to September.

[gk]

Moving our tickets to September 2018

[gk]

Moving tickets to October

[gk]

Moving my tickets to October.

[gk]

Moving our tickets to November.

[gk]

Moving my tickets to December.

[gk]

Moving our tickets to December.

[gk]

Moving my tickets.

[gk]

Moving tickets to Jan 2019.

[gk]

Moving my tickets to Feb

[gk]

Moving tickets to February.

[gk]

Moving my tickets to March.

[gk]

Now for my keyword.

[gk]

Moving tickets to April.

[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?

[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.

[acat]

Reported here: ​https://bugzilla.mozilla.org/show_bug.cgi?id=1542309

[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?)

[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.

[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").

[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.

[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.

[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.

[tom]

-central: ​https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8dba9403863424caaa1f85d12095a435182a3ce&selectedJob=239086020

[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.

[acat]

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

[gk]

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.