1184973 - Add nsContentUtils::StorageAllowedFor* as a unified mechanism for determining storage avaliability

Status: RESOLVED FIXED

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

Description

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

This method provides a unified set of logic for determining whether a form of persistent storage should be avaliable to a webpage. In follow up bugs, it will be used to implement the storage permissions logic for localStorage, sessionStorage, IndexedDB, and CacheStorage.

Comment 1

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

Attachment: Add nsContentUtils::StorageAllowedForWindow as a unified mechanism for determining storage avaliability

I'm currently doing a try run with this patch, as well as my modifications to the storages in question (which will be dependant bugs on this one). You can find the try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=204836d73b40

This code could probably use some tests to ensure its correctness, but I want to first find what the current test coverage is like, to avoid unnecessary test duplication. The code paths should also, hopefully, be relatively thoroughly tested by its to-be consumers. 

I'm hoping you can look over the logic which I have implemented, and ensure that it makes sense.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

So, we're about to make a distinction in CacheStorage between:

1) The "parent principal" which is determined by the window or worker creating the CacheStorage object.
2) The "target principal" which represents the origin whose storage we actually want to work with.

For content script the parent and target principal should always be the same.

For things like devtools and about:serviceworkers, however, they may create a CacheStorage with a system parent principal and a target principal for some other origin.

How will your proposed checks work on these two principals?  Would they just apply to the parent principal?  Or if the parent principal is the system principal, do you still need to execute some of these checks on the target principal?

Comment 3

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

oops, didn't mean to clear the needinfo

Comment 4

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

(In reply to Ben Kelly [:bkelly] from comment #2)
> So, we're about to make a distinction in CacheStorage between:
> 
> 1) The "parent principal" which is determined by the window or worker
> creating the CacheStorage object.
> 2) The "target principal" which represents the origin whose storage we
> actually want to work with.
> 
> For content script the parent and target principal should always be the same.
> 
> For things like devtools and about:serviceworkers, however, they may create
> a CacheStorage with a system parent principal and a target principal for
> some other origin.
> 
> How will your proposed checks work on these two principals?  Would they just
> apply to the parent principal?  Or if the parent principal is the system
> principal, do you still need to execute some of these checks on the target
> principal?

Right now these checks perform the following test in the shared logic, before other checks:

  if (IsSystemPrincipal(SubjectPrincipal()) || IsSystemPrincipal(aPrincipal)) {
    return true;
  }

Which means that if either the target principal or the current JS context's principal are system principals, the check passes. I'm not sure if I understand completely what you need for CacheStorage, but would this cover that requirement?

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Michael Layzell [:mystor] from comment #4)
> Which means that if either the target principal or the current JS context's
> principal are system principals, the check passes. I'm not sure if I
> understand completely what you need for CacheStorage, but would this cover
> that requirement?

I guess it depends on how much of the current CacheStorage checks you remove.  For example, if parent principal is the system principal, we should still fail if the target is the null principal.

Comment 6

[(no longer active)]

The thing that is being checked here is the parent principal.  I don't think the target principal is involved here...

Comment 7

[(no longer active)]

Comment on attachment 8635314 [details] [diff] [review]
Add nsContentUtils::StorageAllowedForWindow as a unified mechanism for determining storage avaliability

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

::: dom/base/nsContentUtils.cpp
@@ +7947,5 @@
>  }
> +
> +static const char kCookiePermissionType[] = "cookie";
> +static const char kCookiesBehavior[] = "network.cookie.cookieBehavior";
> +static const char kCookiesLifetimePolicy[] = "network.cookie.lifetimePolicy";

Nit: you could make these nsContentUtils members to use elsewhere in the code.

@@ +7957,5 @@
> +                                        nsIPrincipal** aPrincipal)
> +{
> +  if (NS_WARN_IF(aWindow && aWindow->IsOuterWindow())) {
> +    aWindow = aWindow->EnsureInnerWindow();
> +  }

I think it should be the responsibility of callers to pass an inner window here.  Please assert that we're dealing with an inner window instead.

@@ +8015,5 @@
> +  // The system principal can do whatever it wants, of course it can access
> +  // storage! We also allow storage when the caller is chrome privileged, as
> +  // chrome privileged code can get access to the storage for another window,
> +  // which may not necessarially be allowed to access storage normally.
> +  if (IsSystemPrincipal(SubjectPrincipal()) || IsSystemPrincipal(aPrincipal)) {

Isn't this check wrong?  If we're being called from nsContentUtils::StorageAllowedForWindow, this will be the principal of the window, so if untrusted script for some reason gets access to it, then this check passes!

@@ +8017,5 @@
> +  // chrome privileged code can get access to the storage for another window,
> +  // which may not necessarially be allowed to access storage normally.
> +  if (IsSystemPrincipal(SubjectPrincipal()) || IsSystemPrincipal(aPrincipal)) {
> +    return true;
> +  }

In fact, I think here you would want to check to see if the window's principal is system, and return false.

@@ +8031,5 @@
> +  // About URIs can have an ENABLE_STORAGE flag, which indicates that they
> +  // should be permitted to access storage no matter what, even if they don't
> +  // have chrome privileges. This is important for, for example, about:home
> +  // which doesn't have a host or chome privileges, but needs to be able to
> +  // use IndexedDB

I think this needs to live in the IndexedDB code, unless you have a specific reason why not.

::: dom/base/nsContentUtils.h
@@ +2452,5 @@
> +   * request that storage not be persisted between browsing sessions.
> +   */
> +  static bool StorageAllowedForWindow(nsPIDOMWindow* aWindow,
> +                                      bool* aSessionOnly = nullptr,
> +                                      nsIPrincipal** aPrincipal = nullptr);

Hmm, is there a reason why you have this outvar?  This is sort of a weird API...

::: netwerk/protocol/about/nsIAboutModule.idl
@@ +50,2 @@
>       */
> +    const unsigned long ENABLE_STORAGE = (1 << 3);

Hmm, I don't think we want to block all storage in those about pages, at least not in this bug.

Comment 8

[(no longer active)]

A couple of more thoughts:

* This needs to check to see if the window is in private browsing mode too, and needs to return that to the caller.
* It's probably better to make it more obvious for the caller what they should do with it.  So perhaps define a bitfield such as

enum class StorageAccessibility {
  None,
  Allow,
  ForPrivateBrowsing,
  ForCurrentSessionOnly,
};

and then return an EnumSet<StorageAccessibility>.  This way callers can test for all of the desired properties of the return value.

Comment 9

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

Attachment: Part 1: Add nsContentUtils::StorageAllowedForWindow as a unified mechanism for determining storage avaliability

This is the first part in the new storage system. All changes can be found on github here: https://github.com/mystor/mozilla-central/tree/storage_pref

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d48360c5cc3e

Comment 10

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

Attachment: Part 2: Tests for new storage permissions model

Updated version of patch. Part 6 of new storage logic. Full tree here: https://github.com/mystor/mozilla-central/tree/storage_pref

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d48360c5cc3e

Comment 11

[(no longer active)]

Comment on attachment 8641984 [details] [diff] [review]
Part 1: Add nsContentUtils::StorageAllowedForWindow as a unified mechanism for determining storage avaliability

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

::: dom/base/nsContentUtils.cpp
@@ +196,5 @@
> +#include "nsICookiePermission.h"
> +#include "mozIThirdPartyUtil.h"
> +#include "nsICookieService.h"
> +#include "nsIAboutModule.h"
> +#include "nsAboutProtocolUtils.h"

Why do you need these two headers?

@@ +8007,5 @@
> +// static, public
> +EnumSet<nsContentUtils::StorageAccessibility>
> +nsContentUtils::StorageAllowedForWindow(nsPIDOMWindow* aWindow)
> +{
> +  MOZ_ASSERT(aWindow->IsInnerWindow());

You null check aWindow below, but not here.  You should either support it being null or not.  :-)  FWIW I suggest removing the null check.  If a caller wants to use the subject principal, they can pass it explicitly to StorageAllowedForPrincipal!

@@ +8042,5 @@
> +  // We define the deny entry here to make the return sites more explicit.
> +  EnumSet<StorageAccessibility> allow = StorageAccessibility::Allow;
> +  EnumSet<StorageAccessibility> deny;
> +
> +

Nit: unneeded line break.

@@ +8063,5 @@
> +  // If the caller is chrome privileged, then it is allowed to access any
> +  // storage it likes, no matter whether the storage for that window/principal
> +  // would normally be permitted.
> +  if (IsSystemPrincipal(SubjectPrincipal())) {
> +    return allow;

If we get to here, we haven't yet checked for session storage yet.  I think this needs to move down past that point.

@@ +8069,5 @@
> +
> +  if (IsSystemPrincipal(aPrincipal)) {
> +    NS_WARNING("A non-system principal is attempting to access "
> +               "storage for the system principal!");
> +    return deny;

What if this is evil.com trying to access foo.com's storage?  I think you should do a principal subsumption check here instead.

@@ +8072,5 @@
> +               "storage for the system principal!");
> +    return deny;
> +  }
> +
> +  // If the document is sandboxed, then it is not permitted to use storage

I think this is similar to the null principal case, and should probably be moved up right after it.

@@ +8087,5 @@
> +  nsCOMPtr<nsIURI> uri;
> +  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(aPrincipal->GetURI(getter_AddRefs(uri))));
> +  bool isAbout = false;
> +  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(uri->SchemeIs("about", &isAbout)));
> +  if (isAbout) {

What about file URIs?

@@ +8088,5 @@
> +  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(aPrincipal->GetURI(getter_AddRefs(uri))));
> +  bool isAbout = false;
> +  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(uri->SchemeIs("about", &isAbout)));
> +  if (isAbout) {
> +    return allow;

At this point we haven't checked the session only restriction yet, but perhaps this is fine for about.  Please add a comment to say explicitly that we are ignoring this.

@@ +8114,5 @@
> +
> +  // If we don't have an explicit permission either allowing or preventing
> +  // a site from accessing the storage, we have to check preferences
> +  uint32_t cookieBehavior = Preferences::GetUint(sCookiesBehaviorPref);
> +  uint32_t lifetimePolicy = Preferences::GetUint(sCookiesLifetimePolicyPref);

You should probably add a var cache for these preference accesses.  Could happen in a follow-up bug though.

::: dom/base/nsContentUtils.h
@@ +2447,5 @@
> +   * preference, and the cookies lifetime policy preference.
> +   */
> +  static const char *sCookiePermissionType;
> +  static const char *sCookiesBehaviorPref;
> +  static const char *sCookiesLifetimePolicyPref;

Nit: * belongs with the type.

Comment 12

[(no longer active)]

Comment on attachment 8641989 [details] [diff] [review]
Part 2: Tests for new storage permissions model

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

Clearing this since the behavior here may change...

Comment 13

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

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #11)
> Why do you need these two headers?

You're right, I don't need them anymore, they're from a previous iteration where I did more work with about URIs

> You null check aWindow below, but not here.  You should either support it
> being null or not.  :-)  FWIW I suggest removing the null check.  If a
> caller wants to use the subject principal, they can pass it explicitly to
> StorageAllowedForPrincipal!

Sounds good, I think the reason why I included the null check in the first place is gone now anyway.

> If we get to here, we haven't yet checked for session storage yet.  I think
> this needs to move down past that point.

So I should move the following code:
>  // If we only cookies for a session, mark the response as session only
>  if (lifetimePolicy == nsICookieService::ACCEPT_SESSION) {
>    allow += StorageAccessibility::ForCurrentSessionOnly;
>  }
above the system principal point?

> @@ +8069,5 @@
> > +
> > +  if (IsSystemPrincipal(aPrincipal)) {
> > +    NS_WARNING("A non-system principal is attempting to access "
> > +               "storage for the system principal!");
> > +    return deny;
> 
> What if this is evil.com trying to access foo.com's storage?  I think you
> should do a principal subsumption check here instead.

Sounds good.

> I think this is similar to the null principal case, and should probably be
> moved up right after it.

Sounds good.

> What about file URIs?

What about them? about: URIs are special cased because they need to use storage etc. to be functional, so preferences shouldn't be able to disable them (as that would break a lot of stuff.

I think that file URIs should be handled exactly the same way as non-file URIs, shouldn't it?

> At this point we haven't checked the session only restriction yet, but
> perhaps this is fine for about.  Please add a comment to say explicitly that
> we are ignoring this.

This will be fixed if we perform the check earlier like I was talking about above.

> You should probably add a var cache for these preference accesses.  Could
> happen in a follow-up bug though.

Sure, sounds easy enough.

> Nit: * belongs with the type.

Oops - I've been writing too many clang plugins.

Comment 14

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

Attachment: Part 1: Add nsContentUtils::StorageAllowedForWindow as a unified mechanism for determining storage avaliability

Updated version of part 1.

new try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c45031e4ca96

Comment 15

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

Attachment: Part 1 interdiff

An diff for the latest version of Part 1.

Comment 16

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

Comment on attachment 8641989 [details] [diff] [review]
Part 2: Tests for new storage permissions model

Optimistically marking this for review again :).

github branch: https://github.com/mystor/mozilla-central/tree/storage_pref

Comment 17

[(no longer active)]

Comment on attachment 8645496 [details] [diff] [review]
Part 1: Add nsContentUtils::StorageAllowedForWindow as a unified mechanism for determining storage avaliability

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

Looks good to me but I'd prefer a second pair of eyes.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8645496 [details] [diff] [review]
Part 1: Add nsContentUtils::StorageAllowedForWindow as a unified mechanism for determining storage avaliability

>+EnumSet<nsContentUtils::StorageAccessibility>
>+nsContentUtils::StorageAllowedForWindow(nsPIDOMWindow* aWindow)
>+{
>+  MOZ_ASSERT(aWindow->IsInnerWindow());
>+
>+  nsCOMPtr<nsIPrincipal> principal = aWindow->GetDoc()->NodePrincipal();
GetExtantDoc() and null check its return value.



But there will be another patch which will be hopefully less error prone. 
Allow|ForPB is error prone, if one doesn't explicitly check ForPB.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

or maybe I still misunderstand this. The API reads oddly, or how to say.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8645496 [details] [diff] [review]
Part 1: Add nsContentUtils::StorageAllowedForWindow as a unified mechanism for determining storage avaliability

>+  // If the caller is chrome privileged, then it is allowed to access any
>+  // storage it likes, no matter whether the storage for that window/principal
>+  // would normally be permitted.
>+  if (IsSystemPrincipal(SubjectPrincipal())) {
>+    return allow;
>+  }
Do we actually need this behavior?
I mean, would
if (IsSystemPrincipal(aPrincipal)) {
  return allow;
}
be enough?

I'm having trouble to understand this all since I don't know who is using the methods.

>+
>+  if (!SubjectPrincipal()->Subsumes(aPrincipal)) {
>+    NS_WARNING("A principal is attempting to access storage for a principal "
>+               "which it doesn't subsume!");
>+    return deny;
>+  }
How can this ever be possible? I mean, what kind of caller would lead to this case?

Comment 21

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

(In reply to Olli Pettay [:smaug] from comment #20)
> Do we actually need this behavior?
> I mean, would
> if (IsSystemPrincipal(aPrincipal)) {
>   return allow;
> }
> be enough?
> 
> I'm having trouble to understand this all since I don't know who is using
> the methods.
> 

This logic is also called before DOMStorage method calls. There was chrome code which would try to access DOMStorage for a principal which otherwise shouldn't have access to DOMStorage. As the code is chrome, I decided to give it access.

> How can this ever be possible? I mean, what kind of caller would lead to
> this case?

I couldn't think of a caller. If there was a caller it would be an error. I added that check out of an overabundance of caution, as if content incorrectly got access to another window's storage, it would fail (though that code path should never be hit).

Comment 22

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

Attachment: Part 1: Add nsContentUtils::StorageAllowedForWindow as a unified mechanism for determining storage avaliability

Updated version of the patch. Switched to using an enum with order as the output type rather than an enumset. Should be more clear how everything is supposed to act

Comment 23

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

Comment on attachment 8641989 [details] [diff] [review]
Part 2: Tests for new storage permissions model

Clearing the review flag because these tests are going to have to change due to the decision to close bug 1183968.

Comment 24

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

Attachment: Part 2: Tests for new storage permissions model

Updated tests which take into account the choice to drop bug 1183968.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8650630 [details] [diff] [review]
Part 1: Add nsContentUtils::StorageAllowedForWindow as a unified mechanism for determining storage avaliability

>+nsContentUtils::InternalStorageAllowedForPrincipal(nsIPrincipal* aPrincipal,
>+                                                   nsPIDOMWindow* aWindow)
>+{
>+  MOZ_ASSERT(aPrincipal);
>+  MOZ_ASSERT(!aWindow || aWindow->IsInnerWindow());
>+
>+  StorageAccess access = StorageAccess::eAllow;
>+
>+  // Check if we are in private browsing, and record that fact
>+  if (aWindow) {
>+    nsIDocument* document = aWindow->GetExtantDoc();
>+    if (IsInPrivateBrowsing(document)) {
>+      access = std::min(StorageAccess::ePrivateBrowsing, access);
Why not just
access = StorageAccess::ePrivateBrowsing;


>+  // Check if we should only allow storage for the session, and record that fact
>+  if (sCookiesLifetimePolicy == nsICookieService::ACCEPT_SESSION &&
>+      access == StorageAccess::eAllow) {
>+    access = std::min(StorageAccess::eSessionScoped, access);
Why not just
access = StorageAccess::eSessionScoped;

>+  // We don't allow storage on the null principal, in general. Even if the
>+  // calling context is chrome.
>+  bool isNullPrincipal;
>+  if (NS_WARN_IF(NS_FAILED(aPrincipal->GetIsNullPrincipal(&isNullPrincipal))) ||
>+      isNullPrincipal) {
>+    return StorageAccess::eDeny;
>+  }
I would move this up

>+
>+  // If the document is sandboxed, then it is not permitted to use storage
>+  if (aWindow) {
>+    nsIDocument* document = aWindow->GetExtantDoc();
>+    if(document->GetSandboxFlags() & SANDBOXED_ORIGIN) {
>+      return StorageAccess::eDeny;
>+    }
>+  }
And this too, so that we end up returning eDeny as early as possible
In fact, I'd combine this with the IsInPrivateBrowsing check, so that first you check for
sandbox and possibly return early, and then check for IsInPrivateBrowsing and possibly set access variable.



>+   * Checks if storage for the given window is prohibited by the user's
>+   * preferences, and the window's third-party status.

What is 'third-party' in this context?

>+   *
>+   * This logic is intended to be shared between the different forms of
>+   * persistent storage which are avaliable to web pages.
available

>+   * this logic, and security logic related to them must be updated seperately.
separately


>+  /*
>+   * Checks if storage for the given principal is prohibited by the user's
>+   * preferences. This is the same as StorageAllowedForWindow, except that
>+   * aPrincipal must be non-null, and the context is assumed to be first-party.
I have zero idea what "the context is assumed to be first-party" means in this context
(I need to read the implementation to see that is uses ThirdPartyUtil and the check what that does).
So, please clarify the comment


> 
>+  /*
>+   * Checks if storage for a given principal is prohibited by the user's
>+   * preferences. If aWindow is non-null, its principal must be passed as
>+   * aPrincipal, and the third-party and sandboxing status of the window
>+   * are also checked.
>+   *
>+   * Used in the implementation of StorageAllowedForWindow and
>+   * StorageAllowedForPrincipal.
>+   */
>+  static StorageAccess InternalStorageAllowedForPrincipal(nsIPrincipal* aPrincipal,
>+                                                                                   nsPIDOMWindow* aWindow);
Odd looking indentation for the latter param. Just put it below the first one. 

And clearly the method isn't about "if storage for a given principal is prohibited", given that the method name is about
"...StorageAllowed..."
Same with all the methods in nsContentUtils

Comment 26

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

Attachment: Part 1: Add nsContentUtils::StorageAllowedForWindow as a unified mechanism for determining storage avaliability

I did my best to update the comments & re-arrange the code in response to the feedback :)

Comment 27

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

Attachment: Part 1: Add nsContentUtils::StorageAllowedForWindow as a unified mechanism for determining storage avaliability

typo

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03dc725619b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/d49d61e97fcb

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/07f5717ac742

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f236d459320d
https://hg.mozilla.org/integration/mozilla-inbound/rev/48700cf2b4af

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f236d459320d
https://hg.mozilla.org/mozilla-central/rev/48700cf2b4af

Comment 32

[TBeholder]

Unified storage settings only make sense, but looks like having it read-only (or, say, "permanent: read-only, session: allowed all") is impossible?
In normal circumstances it's unnecessary, but there are always abnormal ones. Access by 3rd party, debugging in web-development, sites that the user may want to retain some setup, but not track IDs (or just force use of site history features, etc)...