1469993 - Grant storage access to a 3rd party, tracking resource if a opened document has user-interaction

Status: RESOLVED FIXED

(Firefox :: Security, enhancement)

Description

[Andrea Marchesini [:baku]]

This is part of the anti-tracking project.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - first user-interaction in document with a opener

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - necko

Storing the permission is still something to do, but it's not trivial.
Let's discuss this issue at the next meeting.

Comment 3

[(no longer active)]

Comment on attachment 8986571 [details] [diff] [review]
part 1 - first user-interaction in document with a opener

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

::: dom/base/nsContentUtils.cpp
@@ +8870,2 @@
>    if (aWindow) {
> +    nsCOMPtr<nsIURI> documentURI = aURI ? aURI : aWindow->GetDocumentURI();

Nit: this could be a raw nsIURI*, I think...

::: dom/base/nsGlobalWindowInner.cpp
@@ +8017,5 @@
> +
> +  nsAutoString origin;
> +  nsresult rv = nsContentUtils::GetUTFOrigin(aURI, origin);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return true;

Why return true here?  That seems like a bad assumption!  I think false is a better default assumption.

Comment 4

[(no longer active)]

Comment on attachment 8986575 [details] [diff] [review]
part 2 - necko

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

::: dom/base/nsContentUtils.cpp
@@ +8906,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return false;
> +  }
> +
> +  if (!grantedOrigin.IsEmpty()) {

Alternatively, you could do:

  if (grantedOrigin.IsEmpty()) {
    // Deny storage access
    return true;
  }

and then de-indent the rest of this block.

::: netwerk/base/LoadInfo.h
@@ +173,5 @@
>    nsWeakPtr                        mContextForTopLevelLoad;
>    nsSecurityFlags                  mSecurityFlags;
>    nsContentPolicyType              mInternalContentPolicyType;
>    LoadTainting                     mTainting;
> +  nsCString                        mStorageAccessGrantedOrigin;

Hmm, you're storing the origin in the first part as a wide string and here as a narrow string.  It seems like storing it as a narrow string here is forcing you to do a bunch of string conversions back and forth which seem quite unnecessary to me.  Can you just store it as a wide string here as well?  It seems like that should work and simplify the patch somewhat.

::: netwerk/base/nsILoadInfo.idl
@@ +1007,5 @@
> +  /**
> +   * This is the origin that has access storage granted also if 3rd party and
> +   * in the tracking protection list.
> +   */
> +  readonly attribute AUTF8String storageAccessGrantedOrigin;

Scripted callers do not need this, so [noscript] please.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 1 - first user-interaction in document with a opener

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 2 - necko

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 3 - permissions

Here permission stored.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 4 - workers

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 5 - permission removal propagated to content processes

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 6 - DOM Cache

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 7 - tests

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8987660 [details] [diff] [review]
part 5 - permission removal propagated to content processes

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

::: dom/ipc/ContentChild.cpp
@@ +2646,5 @@
> +{
> +  nsCOMPtr<nsIPermissionManager> permissionManagerIface =
> +    services::GetPermissionManager();
> +  nsPermissionManager* permissionManager =
> +    static_cast<nsPermissionManager*>(permissionManagerIface.get());

We could probably add a nsPermissionManager::Singleton() method to just get it directly.

::: extensions/cookie/nsPermissionManager.cpp
@@ +2124,5 @@
> +
> +  // Remove from memory and notify immediately. Since the in-memory
> +  // database is authoritative, we do not need confirmation from the
> +  // on-disk database to notify observers.
> +  RemoveAllFromMemory();

The comment about the on-disk database doesn't make sense in the child process, as we have no on-disk database there. The only database is the in-memory one.

@@ +2126,5 @@
> +  // database is authoritative, we do not need confirmation from the
> +  // on-disk database to notify observers.
> +  RemoveAllFromMemory();
> +
> +  return NS_OK;

In the RemoveAll method, we re-import the default entries from the defaults file before allowing any other code to run. I think this could leave the content process in an inconsistent state between calls where no permission information is present, including default permissions.

Ideally we should send down the set of default permissions with the same IPC message so we can't see that inconsistent state.

The defaults URI may also be exposed to the content process despite the sandbox and we may be able to re-read it there.

That being said, it seems like other methods (such as RemoveAllModifiedSince) also take this approach and will have the no-default-permissions state, so it's probably OK?

Perhaps add a comment explaining that it's probably okay?

Comment 13

[(no longer active)]

Comment on attachment 8987255 [details] [diff] [review]
part 2 - necko

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

::: netwerk/base/LoadInfo.cpp
@@ +1414,5 @@
> +
> +  nsAutoString origin;
> +  nsresult rv = nsContentUtils::GetUTFOrigin(aURI, origin);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return true;

Here I think false is a better assumption in the case of an error return code.

Comment 14

[(no longer active)]

Comment on attachment 8987256 [details] [diff] [review]
part 3 - permissions

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

r=me with the comments below addressed.

::: dom/base/nsGlobalWindowInner.cpp
@@ +335,5 @@
>  #define MIN_IDLE_NOTIFICATION_TIME_S 1
>  
> +// Anti-tracking permission expiration
> +#define ANTITRACKING_EXPIRATION 2592000000 // 30 days.
> +#define ANTITRACKING_PERM_KEY "3rdPartyGranted"

Nit: please call this "3rdPartyStorage" instead, I think that name better describes the permission.

@@ +6491,5 @@
>  
> +nsIPrincipal*
> +nsGlobalWindowInner::GetParentPrincipal()
> +{
> +  nsPIDOMWindowOuter* outerWindow = GetParentInternal();

This parent window may or may not be the top-level window.  I think we need to key the permission to the top-level window only.  And more specifically, for compatibility with Safari for now, I think we should only look at one level of nesting inside a top-level window (i.e. an iframe inside a top-level window, not an iframe inside another iframe).  Can you please modify this logic so that only in that case we return a non-null pointer?  I think the function could also use a better name, such as GetTopLevelStorageAreaPrincipal() or something like that...

@@ +8077,5 @@
> +    return;
> +  }
> +
> +  MaybeRestoreStorageAccessGrantedOrigins();
> +  for (const StorageGrantedOrigin& data : mStorageGrantedOrigins) {

Nit: to ensure the caller isn't doing something crazy, please Truncate() the input array before the loop body...

@@ +8088,5 @@
>  {
>    MOZ_ASSERT(aURI);
>  
> +  if (!StaticPrefs::privacy_trackingprotection_storagerestriction_enabled()) {
> +    return false;

Hmm, isn't true a better return value here if the feature is disabled?  (I doubt it matters one way or the other...)

@@ +8152,5 @@
> +  }
> +
> +  ContentChild* cc = ContentChild::GetSingleton();
> +  if (NS_WARN_IF(!cc)) {
> +    return;

This branch shouldn't really be taken, ever.  Please add an NS_WARNING here to warn accordingly.

@@ +8227,5 @@
> +  }
> +
> +  // Let's take the principal and the origin of the current window.
> +  nsIPrincipal* principal = GetPrincipal();
> +  if (NS_WARN_IF(!principal)) {

Missing return?

Comment 15

[(no longer active)]

Comment on attachment 8987659 [details] [diff] [review]
part 4 - workers

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

::: dom/workers/WorkerLoadInfo.h
@@ +102,5 @@
>    bool mReportCSPViolations;
>    bool mXHRParamsAllowed;
>    bool mPrincipalIsSystem;
>    bool mStorageAllowed;
> +  bool mStorageAccessGranted;

Hmm, where is the actual work done in this patch?  I must be missing something...  As far as I can tell, this member is never set to true anywhere.  I can't really say what impact this patch has, AFAICT it's currently no-op??

Comment 16

[(no longer active)]

Comment on attachment 8987661 [details] [diff] [review]
part 6 - DOM Cache

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

Hmm, this patch changes the behavior of the cache API, from throwing from the global.caches getter to throwing on individual operations.  I'm not quite sure why this is necessary or desired, however.  I'd appreciate if you can describe what you are trying to achieve here and why please.  Thanks!

Comment 17

[(no longer active)]

Comment on attachment 8987662 [details] [diff] [review]
part 7 - tests

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

::: toolkit/components/antitracking/test/browser/browser.ini
@@ +3,5 @@
>    head.js
>    page.html
>    3rdParty.html
> +  3rdPartyUI.html
> +  3rdPartyOpen.html

The second file is missing from the patch...

Comment 18

[(no longer active)]

Comment on attachment 8987662 [details] [diff] [review]
part 7 - tests

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

::: dom/html/nsHTMLDocument.cpp
@@ +3430,5 @@
> +
> +void
> +nsHTMLDocument::UserInteractionForTesting()
> +{
> +  ActivateByUserGesture();

BTW note that this method was removed in bug 1470346.

I find it surprising that you need to expose this to the test environment though.  Why is it not enough to synthesize a click or some such?

Comment 19

[Andrea Marchesini [:baku]]

Attachment: part 4 - workers

Comment 20

[Andrea Marchesini [:baku]]

Related to DOM Cache:


CacheStorage object is created by CacheStorage::CreateOnMainThread() at the first access WindowOrWorkerGlobalScope.caches attribute. Without my patch, the accessing of the storage is decided by nsGlobalWindowInner::GetCaches and in CacheStorage::CreateOnWorker. Here:

https://searchfox.org/mozilla-central/rev/1193ef6a61cb6e350460eb2e8468184d3cb0321d/dom/cache/CacheStorage.cpp#186-189
https://searchfox.org/mozilla-central/rev/1193ef6a61cb6e350460eb2e8468184d3cb0321d/dom/base/nsGlobalWindowInner.cpp#5198-5199
https://searchfox.org/mozilla-central/rev/1193ef6a61cb6e350460eb2e8468184d3cb0321d/dom/cache/CacheStorage.cpp#149-153

this means that, if we grant the permission, at any point, the cache will still return errors. Here an example:

https://searchfox.org/mozilla-central/rev/1193ef6a61cb6e350460eb2e8468184d3cb0321d/dom/cache/CacheStorage.cpp#356-359

My approach is to check the storage access when CacheStorage methods are triggered.

Comment 21

[(no longer active)]

(In reply to Andrea Marchesini [:baku] from comment #20)
> Related to DOM Cache:
> 
> 
> CacheStorage object is created by CacheStorage::CreateOnMainThread() at the
> first access WindowOrWorkerGlobalScope.caches attribute. Without my patch,
> the accessing of the storage is decided by nsGlobalWindowInner::GetCaches
> and in CacheStorage::CreateOnWorker. Here:
> 
> https://searchfox.org/mozilla-central/rev/
> 1193ef6a61cb6e350460eb2e8468184d3cb0321d/dom/cache/CacheStorage.cpp#186-189
> https://searchfox.org/mozilla-central/rev/
> 1193ef6a61cb6e350460eb2e8468184d3cb0321d/dom/base/nsGlobalWindowInner.
> cpp#5198-5199
> https://searchfox.org/mozilla-central/rev/
> 1193ef6a61cb6e350460eb2e8468184d3cb0321d/dom/cache/CacheStorage.cpp#149-153
> 
> this means that, if we grant the permission, at any point, the cache will
> still return errors. Here an example:
> 
> https://searchfox.org/mozilla-central/rev/
> 1193ef6a61cb6e350460eb2e8468184d3cb0321d/dom/cache/CacheStorage.cpp#356-359
> 
> My approach is to check the storage access when CacheStorage methods are
> triggered.

Makes sense, thanks!

Comment 22

[(no longer active)]

Comment on attachment 8987662 [details] [diff] [review]
part 7 - tests

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

Removing the flag because of the missing file...

Comment 23

[Andrea Marchesini [:baku]]

Attachment: part 7 - cookies

I have to store the origin with storage access granted in the nsILoadInfo in order to make document.cookie happy: that uses the document's channel and its loadInfo to retrieve the cookies.

Comment 24

[(no longer active)]

Comment on attachment 8990252 [details] [diff] [review]
part 4 - workers

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

::: dom/workers/WorkerPrivate.h
@@ +1030,5 @@
>      mLoadInfo.mXHRParamsAllowed = aAllowed;
>    }
>  
>    bool
> +  IsStorageAllowed() const;

Nit: I don't see a good reason why this should be not be inlined here, please keep it inlined unless if it can't be for a reason I'm missing...

Comment 25

[(no longer active)]

Comment on attachment 8990719 [details] [diff] [review]
part 7 - cookies

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

This will need tests too...  :-)

::: netwerk/cookie/CookieServiceChild.h
@@ +65,5 @@
>  
>    void GetCookieStringFromCookieHashTable(nsIURI *aHostURI,
>                                            bool aIsForeign,
>                                            bool aIsTrackingResource,
> +                                          bool aStorageAccessGranted,

Same here and elsewhere in the patch...

::: netwerk/cookie/PCookieService.ipdl
@@ +47,5 @@
>     *        rejected depending on user preferences; if those checks are
>     *        disabled, this parameter is ignored.
>     * @param isTrackingResource
> +   *        True if the request has been marked as tracking.
> +   * @param storageAccessGranted

Nit: please call this firstPartyStorageAccessGranted, since that explains what this flag represents much better.  Please also reword the comment below accordingly as well as the name of the parameter of the GetCookieString function.

@@ +93,5 @@
>     *        rejected depending on user preferences; if those checks are
>     *        disabled, this parameter is ignored.
>     * @param isTrackingResource
> +   *        True if the request has been marked as tracking.
> +   * @param storageAccessGranted

Ditto.

::: netwerk/cookie/nsCookieService.cpp
@@ +4203,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.

Nit: please add:

  " and when we don't have access to the first-party cookie jar".

Comment 26

[Andrea Marchesini [:baku]]

Attachment: part 8 - tests

Here tests. It turned out that synthesized events do not mark the document as "user-interacted". I kept my chrome-only function.

Comment 27

[(no longer active)]

Comment on attachment 8990746 [details] [diff] [review]
part 8 - tests

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

Great, thank you!

Comment 28

[Andrea Marchesini [:baku]]

Attachment: fix.patch

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53885d61244e
Grant storage access to a 3rd party, tracking resource if a opened document has user-interaction - part 1 - storing first user interaction in a document with an opener window, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/04ab7d6c169a
Grant storage access to a 3rd party, tracking resource if a opened document has user-interaction - part 2 - storing first user interaction in nsILoadInfo, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/12b9c8bfa41f
Grant storage access to a 3rd party, tracking resource if a opened document has user-interaction - part 3 - using permissions, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c1c838d54ba
Grant storage access to a 3rd party, tracking resource if a opened document has user-interaction - part 4 - workers, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/55499fcd9738
Grant storage access to a 3rd party, tracking resource if a opened document has user-interaction - part 5 - permission removal propagated to content processes, r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b9870995c73
Grant storage access to a 3rd party, tracking resource if a opened document has user-interaction - part 6 - DOM cache, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/37182cfe869c
Grant storage access to a 3rd party, tracking resource if a opened document has user-interaction - part 7 - cookies, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b261595099d
Grant storage access to a 3rd party, tracking resource if a opened document has user-interaction - part 8 - tests, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/e89192032fe2
Grant storage access to a 3rd party, tracking resource if a opened document has user-interaction - part 9 - fix a include issue related to window.h, r=smaug

Comment 31

[Eliza Balazs [:ebalazs_]]

Backed out 9 changesets (bug 1469993) for causing bustage in build/srcdom/base/nsGlobalWindowInner.cpp on a CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e89192032fe21bedf44aa4447f126abf6eba61ae&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=187320924&repo=mozilla-inbound

Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=766b23553adf1c310767fce603c6bdd448382254&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cceb27fec3b
Grant storage access to a 3rd party, tracking resource if a opened document has user-interaction - part 1 - storing first user interaction in a document with an opener window, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/e23c5a410a4d
Grant storage access to a 3rd party, tracking resource if a opened document has user-interaction - part 2 - storing first user interaction in nsILoadInfo, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/38efe060bca8
Grant storage access to a 3rd party, tracking resource if a opened document has user-interaction - part 3 - using permissions, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2a83ddae32a
Grant storage access to a 3rd party, tracking resource if a opened document has user-interaction - part 4 - workers, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/b10960397cd0
Grant storage access to a 3rd party, tracking resource if a opened document has user-interaction - part 5 - permission removal propagated to content processes, r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ee5d0613ed5
Grant storage access to a 3rd party, tracking resource if a opened document has user-interaction - part 6 - DOM cache, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ec47c691aa4
Grant storage access to a 3rd party, tracking resource if a opened document has user-interaction - part 7 - cookies, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b40782c2bc3
Grant storage access to a 3rd party, tracking resource if a opened document has user-interaction - part 8 - tests, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea2b433af23d
Grant storage access to a 3rd party, tracking resource if a opened document has user-interaction - part 9 - fix a include issue related to window.h, r=smaug

Comment 33

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/1cceb27fec3b
https://hg.mozilla.org/mozilla-central/rev/e23c5a410a4d
https://hg.mozilla.org/mozilla-central/rev/38efe060bca8
https://hg.mozilla.org/mozilla-central/rev/a2a83ddae32a
https://hg.mozilla.org/mozilla-central/rev/b10960397cd0
https://hg.mozilla.org/mozilla-central/rev/3ee5d0613ed5
https://hg.mozilla.org/mozilla-central/rev/2ec47c691aa4
https://hg.mozilla.org/mozilla-central/rev/7b40782c2bc3
https://hg.mozilla.org/mozilla-central/rev/ea2b433af23d