Closed
Bug 1461921
Opened 7 years ago
Closed 7 years ago
Block storage access for third-parties on the tracking protection list
Categories
(Firefox :: Protections UI, enhancement, P2)
Firefox
Protections UI
Tracking
()
RESOLVED
FIXED
Firefox 62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: baku, Assigned: baku)
References
(Depends on 4 open bugs, Blocks 1 open bug)
Details
Attachments
(7 files, 5 obsolete files)
7.36 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
8.55 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
3.69 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
4.25 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
44.39 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
3.73 KB,
patch
|
aosmond
:
review+
|
Details | Diff | Splinter Review |
3.01 KB,
patch
|
ehsan.akhgari
:
review+
mayhemer
:
review+
|
Details | Diff | Splinter Review |
If a third-party context is on the Tracking Protection list, we should block its storage access.
This feature should be behind pref.
Updated•7 years ago
|
Priority: -- → P2
Updated•7 years ago
|
Summary: Block storage access for third-parties → Block storage access for third-parties on the tracking protection list
Updated•7 years ago
|
Summary: Block storage access for third-parties on the tracking protection list → [meta] Block storage access for third-parties on the tracking protection list
Assignee | ||
Comment 1•7 years ago
|
||
Assignee: nobody → amarchesini
Attachment #8985459 -
Flags: review?(ehsan)
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8985460 -
Flags: review?(ehsan)
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8985462 -
Flags: review?(ehsan)
Assignee | ||
Comment 4•7 years ago
|
||
Missing parts are HTTP cache, cookies, etags
Attachment #8985463 -
Flags: review?(ehsan)
Comment 5•7 years ago
|
||
Comment on attachment 8985459 [details] [diff] [review]
part 1 - pref and blocking
Review of attachment 8985459 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/base/nsContentUtils.cpp
@@ +512,5 @@
> // Let the EventListenerManagerMapEntry clean itself up...
> lm->~EventListenerManagerMapEntry();
> }
>
> +bool
Nit: static
@@ +517,5 @@
> +IsThirdPartyWindowOrChannel(nsPIDOMWindowInner* aWindow,
> + nsIChannel* aChannel,
> + nsIURI* aURI)
> +{
> + // In the absence of a window or channel, we assume that we are first-party.
Can you please add an assertion to ensure at least one of aWindow or aChannel has been provided?
Attachment #8985459 -
Flags: review?(ehsan) → review+
Comment 6•7 years ago
|
||
Comment on attachment 8985460 [details] [diff] [review]
part 2 - First tests for DOM storages
Review of attachment 8985460 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/components/antitracking/test/browser/browser_blockingResources.js
@@ +62,5 @@
> + }
> + });
> + });
> +
> +AntiTracking.runTest("localStorage",
Hmm, should you also add a test to demonstrate that sessionStorage would continue to work?
Attachment #8985460 -
Flags: review?(ehsan) → review+
Comment 7•7 years ago
|
||
Comment on attachment 8985459 [details] [diff] [review]
part 1 - pref and blocking
Review of attachment 8985459 [details] [diff] [review]:
-----------------------------------------------------------------
::: modules/libpref/init/StaticPrefList.h
@@ +1020,5 @@
> +// Anti-Tracking prefs
> +//---------------------------------------------------------------------------
> +
> +VARCACHE_PREF(
> + "security.antitracking.enabled",
BTW, sorry to nitpick, but I think this should be called "privacy.antitracking.enabled". This pref has nothing to do with security. :-)
Comment 8•7 years ago
|
||
Comment on attachment 8985462 [details] [diff] [review]
part 3 - BroadcastChannel
Review of attachment 8985462 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/broadcastchannel/BroadcastChannel.cpp
@@ +301,5 @@
> }
> +
> + nsContentUtils::StorageAccess access =
> + nsContentUtils::StorageAllowedForWindow(window);
> + if (access == nsContentUtils::StorageAccess::eDeny) {
Isn't the right check here:
if (access != nsContentUtils::StorageAccess::eAllow)
?
I know that it doesn't make any difference here, but still...
In the worker case we are actually inconsistent right now in workers vs service workers :-(
https://searchfox.org/mozilla-central/rev/42930ab9634ebf3f62aed60f7d1c1bf25c0bf00c/dom/workers/WorkerPrivate.cpp#3081
https://searchfox.org/mozilla-central/rev/42930ab9634ebf3f62aed60f7d1c1bf25c0bf00c/dom/serviceworkers/ServiceWorkerPrivate.cpp#1973
Do you mind filing a follow-up to fix that?
Attachment #8985462 -
Flags: review?(ehsan) → review+
Updated•7 years ago
|
Attachment #8985463 -
Flags: review?(ehsan) → review+
Comment 9•7 years ago
|
||
Thanks very much, Andrea!
Updated•7 years ago
|
Summary: [meta] Block storage access for third-parties on the tracking protection list → Block storage access for third-parties on the tracking protection list
Comment 10•7 years ago
|
||
(In reply to :Ehsan Akhgari from comment #7)
> Comment on attachment 8985459 [details] [diff] [review]
> part 1 - pref and blocking
>
> Review of attachment 8985459 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: modules/libpref/init/StaticPrefList.h
> @@ +1020,5 @@
> > +// Anti-Tracking prefs
> > +//---------------------------------------------------------------------------
> > +
> > +VARCACHE_PREF(
> > + "security.antitracking.enabled",
>
> BTW, sorry to nitpick, but I think this should be called
> "privacy.antitracking.enabled". This pref has nothing to do with security.
> :-)
And sorry for the drive-by, but it seems confusing to have both privacy.trackingprotection.* and privacy.antitracking.enabled.
Maybe this should be privacy.trackingprotection.storagerestriction.enabled ?
Assignee | ||
Comment 11•7 years ago
|
||
> In the worker case we are actually inconsistent right now in workers vs
> service workers :-(
This is actually done on purpose: we don't want to expose ServiceWorkers in privateBrowsing mode, but only Dedicated Workers and SharedWorkers.
Assignee | ||
Comment 12•7 years ago
|
||
Attachment #8985555 -
Flags: review?(ehsan)
Assignee | ||
Comment 13•7 years ago
|
||
Attachment #8985555 -
Attachment is obsolete: true
Attachment #8985555 -
Flags: review?(ehsan)
Attachment #8985597 -
Flags: review?(ehsan)
Assignee | ||
Comment 14•7 years ago
|
||
We can have a unique image cache key for tracking documents. We have a similar approach for ServiceWorker controlled documents.
Attachment #8985598 -
Flags: review?(ehsan)
Assignee | ||
Comment 15•7 years ago
|
||
Let's disable the cache for tracking resources' channels, in 3rd party contexts. Note that SetIsTrackingResource(), if called, runs always before BeginConnectActual().
Attachment #8985599 -
Flags: review?(ehsan)
Comment 16•7 years ago
|
||
(In reply to François Marier [:francois] from comment #10)
> > ::: modules/libpref/init/StaticPrefList.h
> > @@ +1020,5 @@
> > > +// Anti-Tracking prefs
> > > +//---------------------------------------------------------------------------
> > > +
> > > +VARCACHE_PREF(
> > > + "security.antitracking.enabled",
> >
> > BTW, sorry to nitpick, but I think this should be called
> > "privacy.antitracking.enabled". This pref has nothing to do with security.
> > :-)
>
> And sorry for the drive-by, but it seems confusing to have both
> privacy.trackingprotection.* and privacy.antitracking.enabled.
>
> Maybe this should be privacy.trackingprotection.storagerestriction.enabled ?
I don't think the names of the prefs matter all that much, but I think making this a subset of the TP prefs is more confusing IMO. The reason is that right now the feature uses the tracking protection list to decide what domains to restrict, but this is something that is subject to change in the future, as we may augment that list with others, or decide to use other heuristics, etc.
In my mind, "trackingprotection" is just a name for the feature that blocks resource loads from tracking domains, and "antitracking" is a name for this other feature, and both are about protecting users from tracking, so the names we're using are certainly confusing. :-)
My 2 cents...
Comment 17•7 years ago
|
||
Comment on attachment 8985597 [details] [diff] [review]
part 5 - cookies
Review of attachment 8985597 [details] [diff] [review]:
-----------------------------------------------------------------
Looks great, thanks!
::: dom/base/nsContentUtils.cpp
@@ +8981,5 @@
> }
>
> if ((behavior == nsICookieService::BEHAVIOR_REJECT_FOREIGN ||
> + behavior == nsICookieService::BEHAVIOR_LIMIT_FOREIGN) &&
> + IsThirdPartyWindowOrChannel(aWindow, aChannel, aURI)) {
It's a bit wasteful to compute IsThirdPartyWindowOrChannel() twice here (once here and once in StorageDisabledByAntiTracking). How about adding an optional bool* out argument to StorageDisabledByAntiTracking() called aThirdParty and set it if it's passed in when that function determines the third-partiness?
Then you can pass the address of a local variable in this call site and nullptr in the other call site.
::: dom/html/nsHTMLDocument.cpp
@@ +1090,5 @@
>
> + if (nsContentUtils::StorageDisabledByAntiTracking(GetInnerWindow(), nullptr,
> + nullptr)) {
> + return;
> + }
You also need to do the same in SetCookie(), otherwise third-party trackers will be able to write cookies to their first-party cookie jar, which is pretty bad.
::: netwerk/cookie/nsCookieService.cpp
@@ +4179,5 @@
> return STATUS_REJECTED_WITH_ERROR;
> }
>
> + // No cookies allowed if this request comes from a tracker, in a 3rd party
> + // context, when anti-tracking protection is enabled.
Please also document that this overrides cookie permissions.
@@ +4182,5 @@
> + // No cookies allowed if this request comes from a tracker, in a 3rd party
> + // context, when anti-tracking protection is enabled.
> + if (aIsForeign && aIsTrackingResource &&
> + StaticPrefs::privacy_trackingprotection_storagerestriction_enabled()) {
> + return STATUS_REJECTED;
Please log this rejection, similar to <https://searchfox.org/mozilla-central/rev/6eea08365e7386a2b81c044e7cc8a3daa51d8754/netwerk/cookie/nsCookieService.cpp#4182>.
Attachment #8985597 -
Flags: review?(ehsan) → review+
Comment 18•7 years ago
|
||
Comment on attachment 8985598 [details] [diff] [review]
part 6 - image cache
Review of attachment 8985598 [details] [diff] [review]:
-----------------------------------------------------------------
Looks fine to me, Andrew do you mind having a quick look as well?
Attachment #8985598 -
Flags: review?(ehsan)
Attachment #8985598 -
Flags: review?(aosmond)
Attachment #8985598 -
Flags: review+
Comment 19•7 years ago
|
||
(In reply to :Ehsan Akhgari from comment #16)
> I don't think the names of the prefs matter all that much, but I think
> making this a subset of the TP prefs is more confusing IMO. The reason is
> that right now the feature uses the tracking protection list to decide what
> domains to restrict, but this is something that is subject to change in the
> future, as we may augment that list with others, or decide to use other
> heuristics, etc.
>
> In my mind, "trackingprotection" is just a name for the feature that blocks
> resource loads from tracking domains, and "antitracking" is a name for this
> other feature, and both are about protecting users from tracking, so the
> names we're using are certainly confusing. :-)
All good points. It basically comes down to whether we want privacy.tp.* to continue to refer to just the Disconnect-related features or whether we want to extend that to anything that's protecting users against tracking.
At the UI level, we're going to be bundling all of these protections together under the same TBD name (current working name: "Smart Blocking"). We don't have to, of course, but maybe it makes sense to also expose it in a similar way in our prefs.
Comment 20•7 years ago
|
||
Comment on attachment 8985599 [details] [diff] [review]
part 7 - network cache
Review of attachment 8985599 [details] [diff] [review]:
-----------------------------------------------------------------
We certainly don't want to disable the cache for all third-party trackers completely! That has massive performance implications which I think would be unacceptable. I think you should use an approach very similar to the image cache (aka, partition, but perhaps using the top-level domain instead of generating a unique namespace for each load since that defeats the purpose of caching.)
It shouldn't be too hard to do this actually. I think the right place for doing this is this function which computes the cache entry keys: <https://searchfox.org/mozilla-central/rev/6eea08365e7386a2b81c044e7cc8a3daa51d8754/netwerk/cache2/CacheFileUtils.cpp#203>. We would need to extend nsILoadContextInfo with another boolean bit indicating whether the load context belongs to a tracking channel or not... This is a rough idea though, please check the details with Honza, I think he is the best person to guide you through the cache code for this purpose. :-)
Attachment #8985599 -
Flags: review?(ehsan) → review-
Comment 21•7 years ago
|
||
(In reply to François Marier [:francois] from comment #19)
> All good points. It basically comes down to whether we want privacy.tp.* to
> continue to refer to just the Disconnect-related features or whether we want
> to extend that to anything that's protecting users against tracking.
Indeed. Renaming prefs is A Hard Problem (since you'd need to actually migrate them to ensure people who have the old pref names set won't have a broken experience) so my personal tendency is to leave existing prefs be at all costs. :-)
> At the UI level, we're going to be bundling all of these protections
> together under the same TBD name (current working name: "Smart Blocking").
> We don't have to, of course, but maybe it makes sense to also expose it in a
> similar way in our prefs.
Yeah, I was thinking of suggesting that too, except that we may want to change the name of the feature to something else, and then the pref name would be inconsistent again!
But maybe there is a way, perhaps we could start a new pref namespace:
privacy.protections.*
Then this one could be called something like privacy.protections.3rdpartystorage.enabled or some such, and later on we can have privacy.protections.fingerprinting.enabled, etc. And maybe some day we'd rename the TP pref to privacy.protections.crosssitetracking.enabled or something like that... Thoughts?
(I really don't feel all that strongly about this, happy to leave the final decision to Andrea and you.)
Comment 22•7 years ago
|
||
Comment on attachment 8985598 [details] [diff] [review]
part 6 - image cache
Review of attachment 8985598 [details] [diff] [review]:
-----------------------------------------------------------------
::: image/ImageCacheKey.cpp
@@ +78,5 @@
> + aDocument->GetChannel(),
> + aURI)) {
> + nsPrintfCString p("%p", aDocument);
> + ptr.Append(p);
> + }
We already add the document pointer to the hash a little further up in the constructor. I think it would make more sense to expand to check in ImageCacheKey::GetControlledDocumentToken to yield the document pointer in this additional case; that will also ensure the document's match in ImageCacheKey::operator== in the event of a hash collision.
Assignee | ||
Comment 23•7 years ago
|
||
Attachment #8985988 -
Flags: review?(honzab.moz)
Attachment #8985988 -
Flags: review?(ehsan)
Assignee | ||
Comment 24•7 years ago
|
||
Attachment #8985988 -
Attachment is obsolete: true
Attachment #8985988 -
Flags: review?(honzab.moz)
Attachment #8985988 -
Flags: review?(ehsan)
Attachment #8985989 -
Flags: review?(honzab.moz)
Attachment #8985989 -
Flags: review?(ehsan)
Updated•7 years ago
|
Attachment #8985598 -
Flags: review?(aosmond) → review-
Comment 25•7 years ago
|
||
(In reply to :Ehsan Akhgari from comment #21)
> Then this one could be called something like
> privacy.protections.3rdpartystorage.enabled or some such, and later on we
> can have privacy.protections.fingerprinting.enabled, etc. And maybe some
> day we'd rename the TP pref to privacy.protections.crosssitetracking.enabled
> or something like that... Thoughts?
Thinking more about it, I think the main issue I had was with the name "antitracking". That's feels too generic for a specific feature. How about simply `privacy.restrict3rdpartystorage.enabled`?
We could punt on the namespace and the moving of existing prefs.
Comment 26•7 years ago
|
||
(In reply to François Marier [:francois] from comment #25)
> (In reply to :Ehsan Akhgari from comment #21)
> > Then this one could be called something like
> > privacy.protections.3rdpartystorage.enabled or some such, and later on we
> > can have privacy.protections.fingerprinting.enabled, etc. And maybe some
> > day we'd rename the TP pref to privacy.protections.crosssitetracking.enabled
> > or something like that... Thoughts?
>
> Thinking more about it, I think the main issue I had was with the name
> "antitracking". That's feels too generic for a specific feature. How about
> simply `privacy.restrict3rdpartystorage.enabled`?
>
> We could punt on the namespace and the moving of existing prefs.
Fine by me!
Comment 27•7 years ago
|
||
Comment on attachment 8985989 [details] [diff] [review]
part 7 - network cache
Review of attachment 8985989 [details] [diff] [review]:
-----------------------------------------------------------------
::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +3737,5 @@
> }
>
> + if (StaticPrefs::privacy_trackingprotection_storagerestriction_enabled() &&
> + mIsTrackingResource) {
> + extension.Append("-TR");
Nit: maybe use lowercase here to distinguish from the tag used for TRR?
Attachment #8985989 -
Flags: review?(ehsan) → review+
Comment 28•7 years ago
|
||
Comment on attachment 8985989 [details] [diff] [review]
part 7 - network cache
Review of attachment 8985989 [details] [diff] [review]:
-----------------------------------------------------------------
Changing my review to r- per in-person discussion. Your patch is missing two things:
* Third-partiness check
* Partitioning the cache based on the top-level page's domain
Attachment #8985989 -
Flags: review+ → review-
Assignee | ||
Comment 29•7 years ago
|
||
Attachment #8985598 -
Attachment is obsolete: true
Attachment #8985599 -
Attachment is obsolete: true
Attachment #8985989 -
Attachment is obsolete: true
Attachment #8985989 -
Flags: review?(honzab.moz)
Attachment #8986290 -
Flags: review?(aosmond)
Assignee | ||
Comment 30•7 years ago
|
||
Attachment #8986307 -
Flags: review?(honzab.moz)
Attachment #8986307 -
Flags: review?(ehsan)
![]() |
||
Comment 31•7 years ago
|
||
Comment on attachment 8986307 [details] [diff] [review]
part 7 - network cache
Review of attachment 8986307 [details] [diff] [review]:
-----------------------------------------------------------------
r+ with comments
::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +3755,5 @@
> + nsAutoString topWindowOrigin;
> + rv = nsContentUtils::GetUTFOrigin(topWindowURI, topWindowOrigin);
> + NS_ENSURE_SUCCESS(rv, rv);
> +
> + extension.Append("-TR:");
instead of "-TR" having "-tracker:" (or something) may be better, it's way more descriptive.
Attachment #8986307 -
Flags: review?(honzab.moz) → review+
Updated•7 years ago
|
Attachment #8986290 -
Flags: review?(aosmond) → review+
Comment 32•7 years ago
|
||
Comment on attachment 8986307 [details] [diff] [review]
part 7 - network cache
Review of attachment 8986307 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with the comments addressed. Thanks!
::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +3739,5 @@
>
> + if (StaticPrefs::privacy_trackingprotection_storagerestriction_enabled() &&
> + mIsTrackingResource) {
> + nsCOMPtr<mozIThirdPartyUtil> thirdPartyUtil =
> + do_GetService(THIRDPARTYUTIL_CONTRACTID);
Would it be possible to cache this somewhere in order to avoid calling do_GetService() repeatedly? This code may be performance sensitive and the hashtable lookups under do_GetService() regularly show up in profiles.
Perhaps you could add a lazy service getter to mozilla::services? Or add a helper to nsNetUtil.h? Up to you.
Bonus points if you also convert this call site over to use it as well: https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/netwerk/protocol/http/HttpBaseChannel.cpp#2382
@@ +3740,5 @@
> + if (StaticPrefs::privacy_trackingprotection_storagerestriction_enabled() &&
> + mIsTrackingResource) {
> + nsCOMPtr<mozIThirdPartyUtil> thirdPartyUtil =
> + do_GetService(THIRDPARTYUTIL_CONTRACTID);
> + MOZ_ASSERT(thirdPartyUtil);
Nope, you do need to null check here, I'm afraid, unless you can guarantee that this won't run past shutdown. Otherwise do_GetService() will happily return null.
Attachment #8986307 -
Flags: review?(ehsan) → review+
Comment 33•7 years ago
|
||
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d519616a1d4c
Block storage access for third-parties on the tracking protection list - part 1 - Pref and Blocking check, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba91bc6240f3
Block storage access for third-parties on the tracking protection list - part 2 - First tests for DOM storages, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9c37c6ca437
Block storage access for third-parties on the tracking protection list - part 3 - BroadcastChannel, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/c34c37fe5f9d
Block storage access for third-parties on the tracking protection list - part 4 - ServiceWorkers, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8b4f415c043
Block storage access for third-parties on the tracking protection list - part 5 - Cookies, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1401b747619
Block storage access for third-parties on the tracking protection list - part 6 - Image cache, r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ffa02850c04
Block storage access for third-parties on the tracking protection list - part 7 - Network cache, r=mayhemer, r=ehsan
Comment 34•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d519616a1d4c
https://hg.mozilla.org/mozilla-central/rev/ba91bc6240f3
https://hg.mozilla.org/mozilla-central/rev/c9c37c6ca437
https://hg.mozilla.org/mozilla-central/rev/c34c37fe5f9d
https://hg.mozilla.org/mozilla-central/rev/a8b4f415c043
https://hg.mozilla.org/mozilla-central/rev/f1401b747619
https://hg.mozilla.org/mozilla-central/rev/0ffa02850c04
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Comment 35•7 years ago
|
||
(In reply to :Ehsan Akhgari from comment #26)
> (In reply to François Marier [:francois] from comment #25)
> > (In reply to :Ehsan Akhgari from comment #21)
> > > Then this one could be called something like
> > > privacy.protections.3rdpartystorage.enabled or some such, and later on we
> > > can have privacy.protections.fingerprinting.enabled, etc. And maybe some
> > > day we'd rename the TP pref to privacy.protections.crosssitetracking.enabled
> > > or something like that... Thoughts?
> >
> > Thinking more about it, I think the main issue I had was with the name
> > "antitracking". That's feels too generic for a specific feature. How about
> > simply `privacy.restrict3rdpartystorage.enabled`?
> >
> > We could punt on the namespace and the moving of existing prefs.
>
> Fine by me!
Andrea, did you forget to rename the pref?
Flags: needinfo?(amarchesini)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(amarchesini)
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•