Closed Bug 1461921 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Protections UI, enhancement, P2)

enhancement

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
: 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
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: nobody → amarchesini
Attachment #8985459 - Flags: review?(ehsan)
Attachment #8985460 - Flags: review?(ehsan)
Attachment #8985462 - Flags: review?(ehsan)
Missing parts are HTTP cache, cookies, etags
Attachment #8985463 - Flags: review?(ehsan)
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 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 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 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+
Attachment #8985463 - Flags: review?(ehsan) → review+
Thanks very much, Andrea!
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 ?
> 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.
Attached patch part 5 - cookies (obsolete) — Splinter Review
Attachment #8985555 - Flags: review?(ehsan)
Attached patch part 5 - cookiesSplinter Review
Attachment #8985555 - Attachment is obsolete: true
Attachment #8985555 - Flags: review?(ehsan)
Attachment #8985597 - Flags: review?(ehsan)
Attached 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)
Attached 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)
(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 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 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 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-
(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.
Attached patch part 7 - network cache (obsolete) — Splinter Review
Attachment #8985988 - Flags: review?(honzab.moz)
Attachment #8985988 - Flags: review?(ehsan)
Attached 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.
(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 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 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-
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)
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 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+
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
Blocks: 1470108
(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)
Depends on: 1470578
Flags: needinfo?(amarchesini)
Blocks: cookierestrictions
No longer blocks: antitracking
Depends on: 1475708
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.