1515665 - Granting a content blocking exception to a page doesn't make the localStorage API work on it

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[(no longer active)]

This is similar to bug 1510860 in nature.  When we grant a content blocking exception to a page, the localStorage API doesn't start to work on that page...

The reason is that this check <https://searchfox.org/mozilla-central/rev/2e5e28f518524f8af5b158ddb605022b6a2d68cf/dom/storage/Storage.cpp#46> is done using the principal object.  Internally this maps to <https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/toolkit/components/antitracking/AntiTrackingCommon.cpp#1225> in the antitracking back-end which is oblivious to the content blocking allow list because it can't walk up the window hierarchy.

Comment 1

[(no longer active)]

Attachment: Bug 1515665 - Ensure that DOM Storage checks the content blocking allow list when performing storage access checks;

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/338f4fbbfddf
Ensure that DOM Storage checks the content blocking allow list when performing storage access checks; r=baku

Comment 3

[Eliza Balazs [:ebalazs_]]

Backed out changeset 338f4fbbfddf (Bug 1515665) for bc failures browser/components/sessionstore/test/browser_339445.js CLOSED TREE

Push with multiple failures, storage issue: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=superseded%2Ctestfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=3a328e80b02cf3c11a36e060380277396e211cd9&selectedJob=218353178

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=218353178&repo=autoland&lineNumber=3240
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=218355469&repo=autoland&lineNumber=10989
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=218355472&repo=autoland&lineNumber=2054
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=218351107&repo=autoland&lineNumber=26378

Backout push: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=218351107&repo=autoland&lineNumber=26378

Comment 4

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/540b4829fd57
Backed out changeset 338f4fbbfddf for bc failures browser/components/sessionstore/test/browser_339445.js CLOSED TREE

Comment 5

[(no longer active)]

I'm still working on this...

SessionStorage has been badly busted, possibly for years.  :-(

Did people realize that mIsSessionOnly <https://searchfox.org/mozilla-central/rev/64ef7bc9fb478ba7c292f9fa57813d3fe864201e/dom/storage/Storage.cpp#53> is *never* set to true?

Comment 6

[(no longer active)]

Attachment: Bug 1515665 - Ensure that we perform localStorage access checks with a window always, and the sessionStorage access checks with a principal always;

Comment 7

[(no longer active)]

[Tracking Requested - why for this release]:
Actually given how drastic the fix has turned out to be, I no longer think this is something that we want to land on 65, the risk of backporting the fix would be way too high and we don't really have any user reported bugs that we need to backport this for.  I was hoping to get this landed on 65 for completeness but I don't think it's the end of the world if it misses that train...

Comment 8

[Ryan VanderMeulen [:RyanVM]]

We're already past early beta, so I'm perfectly fine with letting this ride with 66 if the patch is riskier than you're comfortable with.

Comment 9

[(no longer active)]

Andrea, what is the meaning of mIsSessionOnly exactly?  This test (which fails with my patch, as you would expect) makes me believe that mIsSessionOnly means we have a session-only permission, not that the Storage object is a session storage object: https://searchfox.org/mozilla-central/source/dom/tests/mochitest/sessionstorage/test_cookieSession.html

I'm super confused by all of this code now...

Comment 10

[Andrea Marchesini [:baku]]

> Did people realize that mIsSessionOnly
> <https://searchfox.org/mozilla-central/rev/
> 64ef7bc9fb478ba7c292f9fa57813d3fe864201e/dom/storage/Storage.cpp#53> is
> *never* set to true?

mIsSessionOnly is set to true when 'cookie' permission is set to session-only. test_cookieSession.html check exactly this. right? LocalStorage and SessionStorage can both have mIsSessionOnly set to true.
 
If you want to check the 'nature' of the storage, you should use |StorageType Storage::Type() const|:

https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/dom/storage/Storage.h#39

Comment 11

[Andrea Marchesini [:baku]]

I'll work on this.

Comment 12

[(no longer active)]

I think you should be able to use parts of my patch still, but all of the argument passing through the constructors etc should be changed to ultimately set mIsSessionOnly instead of what it currently does.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 1 - partitioned storage renamed

Comment 14

[Andrea Marchesini [:baku]]

Attachment: part 2 - partitioned sessionStorage

Here is the scenario I want to fix (I can move this in a separate bug if you think it's better):

1. top-level page is example.com with 2 iframes: tracker.com/inDisconnectedList and tracker.com/notInDisconnectedList
2. the 2 iframes share the same sessionStorage, but they should not.

I want to use the same storage class used for localStorage, also for sessionStorage.

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 3 - reject_cookie policy

Comment 16

[Andrea Marchesini [:baku]]

Attachment: part 4 - tests

Comment 17

[(no longer active)]

Part 2 here needs our documentation to be updated.

Comment 18

[(no longer active)]

Comment on attachment 9036318 [details] [diff] [review]
part 3 - reject_cookie policy

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

::: dom/storage/LocalStorage.cpp
@@ +63,5 @@
> +  // aWindow can be null when preloading data.
> +  if (!nsContentUtils::IsSystemPrincipal(aPrincipal) && aWindow) {
> +    DebugOnly<nsContentUtils::StorageAccess> access =
> +        nsContentUtils::StorageAllowedForWindow(aWindow);
> +    MOZ_ASSERT(access > nsContentUtils::StorageAccess::eDeny);

Better wrap the entire block in #ifdef DEBUG.

::: dom/storage/SessionStorage.cpp
@@ +43,5 @@
> +
> +  if (!nsContentUtils::IsSystemPrincipal(aPrincipal) && aWindow) {
> +    DebugOnly<nsContentUtils::StorageAccess> access =
> +        nsContentUtils::StorageAllowedForWindow(aWindow);
> +    MOZ_ASSERT(access > nsContentUtils::StorageAccess::eDeny);

Ditto.

Comment 19

[Andrea Marchesini [:baku]]

Attachment: part 1 - ePartitionedOrDeny for trackers

This is what we want:

1. when storage access is denied by permissions or cookie-policy, SessionStorage must throw security errors.
2. for normal pages and trackers, sessionStorage must work as usual.

In order to achieve this configuration, I need to return ePartitionedOrDeny only when rejectedReason is 'TRACKERS'.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: part 2 - SessionStorage and ePartitionedOrDeny

Comment 21

[Andrea Marchesini [:baku]]

Attachment: part 3 - tests

Comment 22

[Andrea Marchesini [:baku]]

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

Not green enough on try.

Comment 23

[Andrea Marchesini [:baku]]

Attachment: part 1 - exposed PartitionedOrDeny differently

Finally, a green result. This patch exposed rejectedReason in StorageAllowedForNewWindow. This will be mainly used in the next patches.
It also moves the privacy.restrict3rdpartystorage.partitionedHosts check in the localStorage code.

Comment 24

[Andrea Marchesini [:baku]]

Attachment: part 2 - SessionStorage

sessionstorage works when rejectedReason is STATE_COOKIES_BLOCKED_FOREIGN and when StorageAccess is ePartitionedOrDeny

Comment 25

[Andrea Marchesini [:baku]]

Attachment: part 3 - no window in SessionStorage.jsm

No window when restoring a window.

Comment 26

[Andrea Marchesini [:baku]]

Attachment: part 4 - tests

Comment 27

[(no longer active)]

Comment on attachment 9037674 [details] [diff] [review]
part 1 - exposed PartitionedOrDeny differently

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

::: dom/base/nsContentUtils.cpp
@@ +8102,5 @@
> +                                              channel, aRejectedReason);
> +  }
> +
> +  // No document? Let's return a generic rejected reason.
> +  *aRejectedReason = nsIWebProgressListener::STATE_COOKIES_BLOCKED_ALL;

No, you shouldn't make up non-existing errors out of thin air.  When you aren't sure why you're rejecting, you're supposed to set rejected reason to 0.

@@ +8330,5 @@
>                                                     nsIChannel* aChannel,
>                                                     nsIPrincipal* aPrincipal,
> +                                                   nsIURI* aURI,
> +                                                   uint32_t* aRejectedReason) {
> +  MOZ_ASSERT(aRejectedReason);

Why not make aRejectedReason a reference here so that you can remove this assertion and the weird dereferences below?

@@ +8363,2 @@
>    MOZ_ASSERT(aPrincipal);
> +  MOZ_ASSERT(aRejectedReason);

Ditto.

@@ +8367,5 @@
>  
>    // We don't allow storage on the null principal, in general. Even if the
>    // calling context is chrome.
>    if (aPrincipal->GetIsNullPrincipal()) {
> +    *aRejectedReason = nsIWebProgressListener::STATE_COOKIES_BLOCKED_ALL;

Again, minting synthetic rejection reasons isn't allowed.

@@ +8375,5 @@
>    if (aWindow) {
>      // If the document is sandboxed, then it is not permitted to use storage
>      Document* document = aWindow->GetExtantDoc();
>      if (document && document->GetSandboxFlags() & SANDBOXED_ORIGIN) {
> +      *aRejectedReason = nsIWebProgressListener::STATE_COOKIES_BLOCKED_ALL;

Here too.

@@ +8449,5 @@
>    }
>  
> +  // We want to have a partitioned storage only for trackers.
> +  if (*aRejectedReason ==
> +      nsIWebProgressListener::STATE_COOKIES_BLOCKED_TRACKER) {

This change will break the privacy.restrict3rdpartystorage.partitionedHosts pref for non-trackers...  I suppose that is fine, but at the very least you need to document this change in semantics here: https://searchfox.org/mozilla-central/rev/dac799c9f4e9f5f05c1071cba94f2522aa31f7eb/modules/libpref/init/all.js#1403

Comment 28

[Andrea Marchesini [:baku]]

Attachment: part 1 - exposed PartitionedOrDeny differently

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/700bc7185e64
StorageAccess::ePartitionedOrDeny must be used only for trackers, rehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/188f4b17a553
SessionStorage should be allowed when StorageAccess is ePartitionedOrDeny, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b3ea360251
no window passwed when creating SessionStorage in SessionStorage.jsm, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a1c4140f479
Tests for SessionStorage and cookie-behavior REJECTED, r=ehsan

Comment 31

[Steven Englehardt [:englehardt]]

Docs updated https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Privacy/Storage_access_policy$compare?from=1438241&to=1454297

Comment 32

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/700bc7185e64
https://hg.mozilla.org/mozilla-central/rev/188f4b17a553
https://hg.mozilla.org/mozilla-central/rev/c6b3ea360251
https://hg.mozilla.org/mozilla-central/rev/6a1c4140f479