Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#13742 closed defect (fixed)

Isolating the cache to the URL bar domain is broken in Tor Browser 4.x

Reported by: gk Owned by: tbb-team
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: tbb-linkability, tbb-4.5-alpha, TorBrowserTeam201411, tbb-testcase
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The cache entries are not bound to the URL bar domain in Tor Browser 4.x anymore.

Child Tickets

Change History (11)

comment:1 Changed 5 years ago by gk

The new cache backend introduced in https://bugzilla.mozilla.org/show_bug.cgi?id=913807 is not active in ESR 31 (see #13035 for looking at it for ESR 38). However, that bug did not only provide a new backend but rather a lot of cache logic changes as well. And those changes are not disabled. It turns our that this breaks our cache isolation patch.

Now there is no OpenNormalCacheEntry() anymore constructing an HTTPCacheQuery with our modified cacheKey. We now only have AsyncOpenURI() which adds either a POST ID or nothing (see: https://mxr.mozilla.org/mozilla-esr31/source/netwerk/protocol/http/nsHttpChannel.cpp#2614). We need to modify its second parameter adding the URL bar domain it seems. I have not looked if that is sufficient to catch all corner cases though (redirects? favicons?)

comment:2 Changed 5 years ago by gk

Keywords: 4.5-alpha-1 added

comment:3 Changed 5 years ago by gk

Keywords: tbb-4.5-alpha-1 added; 4.5-alpha-1 removed

comment:4 Changed 5 years ago by gk

Keywords: tbb-4.5-alpha added; tbb-4.5-alpha-1 removed

comment:5 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201411 tbb-testcase added

This doesn't block 4.5-alpha-1. That ship has sailed, and we need to exercise the updater anyway. However, we should fix this ASAP for 4.0.1 and 4.5-alpha-1.

We've been wanting to move this functionality out of Torbutton and into C++ for some time now. Maybe now is the time?

We should also protect this with a unittest, as this is not the first time this has broken, and with the new HTTP cache due in FF38ESR, it won't be the last.

Version 0, edited 5 years ago by mikeperry (next)

comment:6 Changed 5 years ago by mikeperry

Though, after looking at the code, it does seem relatively straight-forward to re-introduce our old domain hack, but there are some wrinkles we need to figure out first. nsHttpChannel::GenerateCacheKey() still uses our domain logic, and is used in a cached redirect corner case... We'll want to ensure consistency here, and I'm not seeing for sure how nsICacheService::AsyncOpenURI() translates its aIdExtension parameter and uri into a complete key yet.

comment:7 Changed 5 years ago by mikeperry

There is also another codepath here involving "fallback keys" and fallback channels. I think the fallback is only used with the application cache (aka the offline cache), which should be disabled. It would be nice if we could handle this, too though.

It also looks like that GenerateCacheKey routine is not the same type of cache key used by the new nsICacheService, and is its own internal buffer. I think we can leave it in place safely.

comment:8 Changed 5 years ago by mikeperry

And there's another bug. We set privacy.thirdparty.isolate to 1, but this means that all third party tracking protection is disabled if the channel is not a private browsing mode channel. We should be setting this pref to 2.

comment:9 Changed 5 years ago by mikeperry

Ok, I think I have a patch that fixes this and also takes Torbutton's CacheDomain code out of the loop (it directly uses our third party APIs instead). Testing locally.

comment:10 Changed 5 years ago by mikeperry

Resolution: fixed
Status: newclosed

Ok, I tested this and it works. I looked into making a mochitest, but the mochitests seem broken with our current build environment. I will create a new ticket for testing all network.thirdparty.isolate functionality.

I think we can go ahead and rebuild for this, as it will help us get it out quicker in 4.0. I've pushed and will retag shortly.

comment:11 Changed 5 years ago by mikeperry

Ok, filed #13749 for the tests for this.

Note: See TracTickets for help on using tickets.