1493897 - Considering using user-interaction permission for nsICookieService::BEHAVIOR_LIMIT_FOREIGN

Status: ASSIGNED

(Core :: DOM: Security, enhancement)

Description

[Andrea Marchesini [:baku]]

So far, we use cookies to consider a 3rd party context visited or not. What I propose here is the use of the new user-interaction permission.

There are 2 important changes compared with the current behavior:

1. a web-site is considered visited if it was previously loaded as first-party and interacted by the user.
2. the current user-interaction permission doesn't consider eTLD+1. Probably this part should change.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: visited_1.patch

Comment 2

[Andrea Marchesini [:baku]]

I think we should change the permission to be stored for the eTLD+1 instead of the full origin.

Comment 3

[(no longer active)]

Comment on attachment 9011699 [details] [diff] [review]
visited_1.patch

Review of attachment 9011699 [details] [diff] [review]:
-----------------------------------------------------------------

Two things here.

1. Please check in with Tanvi about this change in behavior to make sure she is OK with it too.
2. This shouldn't land without tests...  A cursory look around the tree seems to suggest this cookie behavior almost has no tests, and we should really have some...

I'd like to look at another version of the patch addressing the comments below.  The eTLD+1 issue is important though, let me know if you disagree with what I said below.

::: netwerk/cookie/nsICookieService.idl
@@ +89,5 @@
>    const uint32_t BEHAVIOR_REJECT         = 2; // reject all cookies
> +  const uint32_t BEHAVIOR_LIMIT_FOREIGN  = 3; // reject third-party cookies
> +                                              // unless the site has been
> +                                              // previously visited and
> +                                              // user-interacted.

Nit: and interacted with.

::: toolkit/components/antitracking/AntiTrackingCommon.cpp
@@ +569,5 @@
>        LOG(("Allowing access even though our behavior is reject foreign"));
>        return true;
>      }
> +
> +    LOG(("Nothing more to do due to the behavior code %d",

Nit: please hardcode BEHAVIOR_REJECT_FOREIGN in the string to make it easier to read.

@@ +578,5 @@
>  
> +  if (behavior == nsICookieService::BEHAVIOR_LIMIT_FOREIGN) {
> +    nsCOMPtr<nsIPrincipal> principal =
> +      nsGlobalWindowInner::Cast(aWindow)->GetPrincipal();
> +    if (!principal || !AntiTrackingCommon::HasUserInteraction(principal)) {

In the beginning of this method we have the following code:

  nsGlobalWindowInner* innerWindow = nsGlobalWindowInner::Cast(aWindow);
  nsIPrincipal* toplevelPrincipal = innerWindow->GetTopLevelPrincipal();
  if (!toplevelPrincipal) {
    // We are already the top-level principal. Let's use the window's principal.
    LOG(("Our inner window lacks a top-level principal, use the window's principal instead"));
    toplevelPrincipal = innerWindow->GetPrincipal();
  }

  if (!toplevelPrincipal) {
    // This should not be possible, right?
    LOG(("No top-level principal, this shouldn't be happening! Bail out early"));
    return false;
  }

Please change it like this:

  nsGlobalWindowInner* innerWindow = nsGlobalWindowInner::Cast(aWindow);
  nsIPrincipal* windowPrincipal = innerWindow->GetPrincipal();
  if (!windowPrincipal) {
    LOG(("Our window has no principal"));
    return false;
  }
  nsIPrincipal* toplevelPrincipal = innerWindow->GetTopLevelPrincipal();
  if (!toplevelPrincipal) {
    // We are already the top-level principal. Let's use the window's principal.
    LOG(("Our inner window lacks a top-level principal, use the window's principal instead"));
    toplevelPrincipal = windowPrincipal
  }

  MOZ_ASSERT(toplevelPrincipal);

Once you have made this change, please remove the definition of the |principal| variable here, and use |windowPrincipal| instead.

@@ +579,5 @@
> +  if (behavior == nsICookieService::BEHAVIOR_LIMIT_FOREIGN) {
> +    nsCOMPtr<nsIPrincipal> principal =
> +      nsGlobalWindowInner::Cast(aWindow)->GetPrincipal();
> +    if (!principal || !AntiTrackingCommon::HasUserInteraction(principal)) {
> +      LOG(("Nothing more to do due to the behavior code %d",

Same nit as above.

@@ +585,5 @@
> +      *aRejectedReason = nsIWebProgressListener::STATE_COOKIES_BLOCKED_FOREIGN;
> +      return false;
> +    }
> +
> +    LOG(("Allowing access for visited origin."));

Nit: for visited origin that has been interacted with.

@@ +784,5 @@
>        LOG(("Allowing access even though our behavior is reject foreign"));
>        return true;
>      }
> +
> +    LOG(("Nothing more to do due to the behavior code %d",

Same nit as above.

@@ +792,5 @@
>    }
>  
> +  if (behavior == nsICookieService::BEHAVIOR_LIMIT_FOREIGN) {
> +    if (!AntiTrackingCommon::HasUserInteraction(channelPrincipal)) {
> +      LOG(("Nothing more to do due to the behavior code %d",

Same nit as above.

@@ +798,5 @@
> +      *aRejectedReason = nsIWebProgressListener::STATE_COOKIES_BLOCKED_FOREIGN;
> +      return false;
> +    }
> +
> +    LOG(("Allowing access for visited origin."));

Same nit as above.

@@ +1194,5 @@
> +  }
> +
> +  uint32_t result = 0;
> +  nsresult rv =
> +    pm->TestPermissionFromPrincipal(aPrincipal, USER_INTERACTION_PERM, &result);

I would like to know more about why you think the permission should be stored at the eTLD+1 level.

Note that the permission manager already tests all of the subdomain components of the principal being tested (see https://searchfox.org/mozilla-central/rev/881a3c5664ede5e08ee986d76433bc5c4b5680e6/extensions/cookie/nsPermissionManager.cpp#318), so if the user interaction permission is granted to https://foo.bar.example, https://my.baz.foo.bar.example will be able to set third-party cookies, but https://bar.example wouldn't.  That model makes sense to me.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 0 - ETLD+1

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 1 - visited websites

I'm still working on a test. I'll submit a patch tomorrow.

Comment 6

[(no longer active)]

Comment on attachment 9012408 [details] [diff] [review]
part 0 - ETLD+1

Review of attachment 9012408 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/antitracking/AntiTrackingCommon.cpp
@@ +1148,5 @@
> +    if (!scheme.EqualsLiteral("http") && !scheme.EqualsLiteral("https")) {
> +      return;
> +    }
> +
> +    nsCOMPtr<nsIEffectiveTLDService> eTLDService(do_GetService(

It'd be nice to not refetch this from the XPCOM service manager over and over again.

Do you mind making this a global StaticRefPtr and obtain the reference once and ClearOnShutdown() it?

@@ +1156,5 @@
> +    }
> +
> +    nsAutoCString etld1;
> +    rv = eTLDService->GetBaseDomain(uri, 0, etld1);
> +    if (rv == NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS || NS_WARN_IF(NS_FAILED(rv))) {

Nit: please break this into two lines.

@@ +1169,5 @@
> +    // Let's recreate the URL with scheme + base domain.
> +    nsAutoCString etld1String;
> +    etld1String.Append(scheme);
> +    etld1String.AppendLiteral("://");
> +    etld1String.Append(etld1);

You are losing the port here.

Instead of reconstructing the URL manually like this, you should use NS_MutateURI to mutate the URL, set the host on it to the etld+1 and SetPathQueryRef("/").

@@ +1172,5 @@
> +    etld1String.AppendLiteral("://");
> +    etld1String.Append(etld1);
> +
> +    nsCOMPtr<nsIURI> etld1URI;
> +    rv = ios->NewURI(etld1String, nullptr, nullptr, getter_AddRefs(etld1URI));

Why not use NS_NewURI so that you don't have to access nsIIOService?

@@ +1179,5 @@
> +    }
> +
> +    nsCOMPtr<nsIPrincipal> principal =
> +      BasePrincipal::CreateCodebasePrincipal(etld1URI,
> +                                             BasePrincipal::Cast(aPrincipal)->OriginAttributesRef());

Nit: 80 columns please.

Comment 7

[(no longer active)]

I asked this in comment 3, perhaps you didn't see my question.  Besides just reviewing the code, I would like to understand *why* you think we should store this permission at the eTLD+1 level.  This means that if the user for example only has a YouTube account and interacts with accounts.google.com as a first-party but uses duckduckgo as their search engine, google.com would get tracking privileges on them.  I don't think this is desirable.

Can you please explain why we want this?

Comment 8

[(no longer active)]

Comment on attachment 9012409 [details] [diff] [review]
part 1 - visited websites

Review of attachment 9012409 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! (But please also submit a patch with tests before landing.)

Comment 9

[Andrea Marchesini [:baku]]

> Can you please explain why we want this?

I wrote that patch to keep the current behavior, where the LIMIT_FOREIGN uses eTLD+1.
I think it's a common approach to have 3rd party elements in separate hosts: cdn.example.com, images.example.com and so on. Maybe we should consider those URLs as visited too?

Comment 10

[(no longer active)]

Hmm, yes, that is true.  I guess it's fine to allow eTLD+1 based on this reasoning for now, until we come up with an in idea on how to make it stricter.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 0 - ETLD+1

Comment 12

[(no longer active)]

Comment on attachment 9012598 [details] [diff] [review]
part 0 - ETLD+1

Review of attachment 9012598 [details] [diff] [review]:
-----------------------------------------------------------------

Much nicer, thank you.

::: toolkit/components/antitracking/AntiTrackingCommon.cpp
@@ +1007,5 @@
>    escaped.Append(temp);
>  
>    // Stuff the whole thing back into a URI for the permission manager.
>    nsCOMPtr<nsIURI> topWinURI;
> +  rv = NS_NewURI(getter_AddRefs(topWinURI), escaped, nullptr, nullptr);

No need to pass in the nullptr's explicitly! https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/netwerk/base/nsNetUtil.h#73

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 3 - cookie tests

Please, take a close look at this change. It seems correct to me, but the test has these numbers which are not always so easy to follow.

Comment 14

[(no longer active)]

Comment on attachment 9018600 [details] [diff] [review]
part 3 - cookie tests

Review of attachment 9018600 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: extensions/cookie/test/unit/test_cookies_thirdparty.js
@@ +77,5 @@
>    Services.cookies.removeAll();
>    do_set_single_http_cookie(uri1, channel1, 1);
>    do_set_cookies(uri1, channel2, true, [2, 3, 4, 5]);
>    Services.cookies.removeAll();
> +  Services.perms.remove(uri1, "storageAccessAPI");

Nit: Services.perms.removeAll();

@@ +125,3 @@
>    do_set_cookies(uri1, channel1, true, [0, 1, 1, 2]);
>    Services.cookies.removeAll();
>    // No preference has been set for uri2, but it should act as if

Please modify this comment to say that no preference or user-interaction permission has been set for uri2.

@@ +135,5 @@
>    Services.cookies.removeAll();
>    do_set_single_http_cookie(uri1, channel1, 1);
>    do_set_cookies(uri1, channel2, true, [1, 1, 1, 1]);
>    Services.cookies.removeAll();
> +  Services.perms.remove(uri1, "storageAccessAPI");

Can you please add a similar case to this chunk above where you add the "storageAccessAPI" permission for both uri1 and uri2?  Thanks!

@@ +144,4 @@
>    // LIMIT_THIRD_PARTY overrides
> +  Services.perms.add(uri1, kPermissionType,
> +                     Ci.nsICookiePermission.ACCESS_LIMIT_THIRD_PARTY);
> +  do_set_cookies(uri1, channel1, true, [0, 1, 1, 2]);

Looks like this change due to the user-interaction permission not being present?  Please add a comment above it to state that.

@@ +167,5 @@
>  
>    // Test per-site 3rd party cookie limiting with 3rd party cookies disabled
> +  Services.prefs.setIntPref("network.cookie.cookieBehavior",
> +                            Ci.nsICookieService.BEHAVIOR_REJECT_FOREIGN);
> +  do_set_cookies(uri1, channel1, true, [0, 1, 1, 2]);

Same as the above comment (assuming this is the reason for the change, otherwise please state the reason.)

@@ +182,5 @@
>  
>    // Test per-site 3rd party cookie limiting with 3rd party cookies limited
> +  Services.prefs.setIntPref("network.cookie.cookieBehavior",
> +                            Ci.nsICookieService.BEHAVIOR_LIMIT_FOREIGN);
> +  do_set_cookies(uri1, channel1, true, [0, 1, 1, 2]);

Also here.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:baku, could you have a look please?

Comment 16

[Andrea Marchesini [:baku]]

This set of patches is postponed.

Comment 17

[Tanvi Vyas[:tanvi]]

baku, is there anything else still needed here, or can I clear my needinfo?

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

Clear a needinfo that is pending on an inactive user.

For more information, please visit auto_nag documentation.