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

RESOLVED FIXED in Firefox 63

Status

()

enhancement
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

(Blocks 2 bugs)

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

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(9 attachments, 4 obsolete attachments)

14.03 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
16.03 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
25.42 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
5.13 KB, patch
Nika
: review+
Details | Diff | Splinter Review
11.98 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
10.95 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
40.54 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
21.87 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
5.38 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Assignee

Description

11 months ago
This is part of the anti-tracking project.
Assignee

Comment 1

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

Comment 2

11 months ago
Posted patch part 2 - necko (obsolete) — Splinter Review
Storing the permission is still something to do, but it's not trivial.
Let's discuss this issue at the next meeting.
Attachment #8986575 - Flags: review?(ehsan)

Comment 3

11 months ago
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.
Attachment #8986571 - Flags: review?(ehsan) → review+

Comment 4

11 months ago
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.
Attachment #8986575 - Flags: review?(ehsan) → review+
Assignee

Comment 6

11 months ago
Attachment #8986575 - Attachment is obsolete: true
Assignee

Comment 7

11 months ago
Here permission stored.
Assignee

Comment 8

11 months ago
Posted patch part 4 - workers (obsolete) — Splinter Review
Assignee

Comment 10

11 months ago
Assignee

Comment 11

11 months ago
Posted patch part 7 - tests (obsolete) — Splinter Review
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?
Attachment #8987660 - Flags: review?(nika) → review+
Assignee

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Comment 13

11 months ago
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.
Attachment #8987255 - Flags: review?(ehsan) → review+

Comment 14

11 months ago
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?
Attachment #8987256 - Flags: review?(ehsan) → review+

Comment 15

11 months ago
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??
Attachment #8987659 - Flags: review?(ehsan)

Comment 16

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

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

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

Comment 19

11 months ago
Attachment #8987659 - Attachment is obsolete: true
Assignee

Comment 20

11 months ago
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.
Flags: needinfo?(ehsan)

Comment 21

11 months ago
(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!
Flags: needinfo?(ehsan)

Updated

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

Comment 22

11 months ago
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...
Attachment #8987662 - Flags: review?(ehsan)

Updated

11 months ago
Blocks: cookierestrictions
No longer blocks: antitracking
Assignee

Updated

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

Comment 23

11 months ago
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.
Attachment #8990719 - Flags: review?(ehsan)
Assignee

Updated

11 months ago
Attachment #8990719 - Attachment description: doc_7.patch → part 7 - cookies

Comment 24

11 months ago
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...
Attachment #8990252 - Flags: review?(ehsan) → review+

Comment 25

11 months ago
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".
Attachment #8990719 - Flags: review?(ehsan) → review+
Assignee

Comment 26

11 months ago
Here tests. It turned out that synthesized events do not mark the document as "user-interacted". I kept my chrome-only function.
Attachment #8987662 - Attachment is obsolete: true
Attachment #8990746 - Flags: review?(ehsan)

Comment 27

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

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

Great, thank you!
Attachment #8990746 - Flags: review?(ehsan) → review+
Assignee

Comment 28

11 months ago
Posted patch fix.patchSplinter Review
Attachment #8990909 - Flags: review?(bugs)
Assignee

Updated

11 months ago
Duplicate of this bug: 1460755

Updated

11 months ago
Attachment #8990909 - Flags: review?(bugs) → review+

Comment 30

11 months ago
Pushed by amarchesini@mozilla.com:
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
Assignee

Updated

11 months ago
Flags: needinfo?(amarchesini)

Comment 32

11 months ago
Pushed by amarchesini@mozilla.com:
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

Updated

11 months ago
Blocks: 1460755
Assignee

Updated

11 months ago
Blocks: 1474651
You need to log in before you can comment on or make changes to this bug.