Block storage access for third-parties on the tracking protection list

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P2
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

(Depends on 4 bugs, Blocks 1 bug)

unspecified
Firefox 62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(7 attachments, 5 obsolete attachments)

7.36 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
8.55 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
3.69 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
4.25 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
44.39 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
3.73 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
3.01 KB, patch
Ehsan
: review+
mayhemer
: review+
Details | Diff | Splinter Review
Assignee

Description

a year ago
If a third-party context is on the Tracking Protection list, we should block its storage access.
This feature should be behind pref.
Priority: -- → P2
Summary: Block storage access for third-parties → Block storage access for third-parties on the tracking protection list
Summary: Block storage access for third-parties on the tracking protection list → [meta] Block storage access for third-parties on the tracking protection list
Depends on: 1456488
Depends on: 1461515
Depends on: 1466249
Depends on: 1466247
Depends on: 1462432
Depends on: 1461822
Depends on: 1460755
Depends on: 1461534
Assignee

Comment 1

11 months ago
Assignee: nobody → amarchesini
Attachment #8985459 - Flags: review?(ehsan)
Assignee

Comment 2

11 months ago
Attachment #8985460 - Flags: review?(ehsan)
Assignee

Comment 3

11 months ago
Attachment #8985462 - Flags: review?(ehsan)
Assignee

Comment 4

11 months ago
Missing parts are HTTP cache, cookies, etags
Attachment #8985463 - Flags: review?(ehsan)

Comment 5

11 months 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

11 months 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

11 months 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

11 months 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

11 months ago
Attachment #8985463 - Flags: review?(ehsan) → review+

Comment 9

11 months ago
Thanks very much, Andrea!

Updated

11 months 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
(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

11 months 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

11 months ago
Posted patch part 5 - cookies (obsolete) — Splinter Review
Attachment #8985555 - Flags: review?(ehsan)
Assignee

Comment 13

11 months ago
Attachment #8985555 - Attachment is obsolete: true
Attachment #8985555 - Flags: review?(ehsan)
Attachment #8985597 - Flags: review?(ehsan)
Assignee

Comment 14

11 months ago
Posted patch part 6 - image cache (obsolete) — Splinter Review
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

11 months ago
Posted patch part 7 - network cache (obsolete) — Splinter Review
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

11 months 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

11 months 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

11 months 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+
(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

11 months 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

11 months 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 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

11 months ago
Posted patch part 7 - network cache (obsolete) — Splinter Review
Attachment #8985988 - Flags: review?(honzab.moz)
Attachment #8985988 - Flags: review?(ehsan)
Assignee

Comment 24

11 months ago
Posted patch part 7 - network cache (obsolete) — Splinter Review
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)
Attachment #8985598 - Flags: review?(aosmond) → review-
(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

11 months 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

11 months 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

11 months 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

11 months 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

11 months ago
Attachment #8986307 - Flags: review?(honzab.moz)
Attachment #8986307 - Flags: review?(ehsan)
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+
Attachment #8986290 - Flags: review?(aosmond) → review+

Comment 32

11 months 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

11 months 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
Assignee

Updated

11 months ago
Blocks: 1470108

Comment 35

11 months 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)

Updated

11 months ago
Depends on: 1470578
Assignee

Updated

11 months ago
Flags: needinfo?(amarchesini)

Updated

11 months ago
Blocks: cookierestrictions
No longer blocks: antitracking

Updated

11 months ago
Depends on: 1475708

Updated

10 months ago
No longer depends on: 1461822
No longer depends on: 1461515
You need to log in before you can comment on or make changes to this bug.