Bug 93390 – Allow blocking of third-party localStorage and sessionStorage

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Vicki Pfau]

Add a method for allowing blocking of localStorage and sessionStorage in third-party contexts, similar to how third-party cookies behave.

<rdar://problem/10214943>

[Vicki Pfau]

Created attachment 157023 [details]
Patch

[Adam Barth]

Comment on attachment 157023 [details]
Patch

Although it's tempting to use the topDocument as the "first party" context, that doesn't match what we do for cookies.  For cookies, we use the firstPartyForCookies, which takes into account redirects, etc.  We probably want to be consistent in what we treat as the first party for these storage interactions.

[Vicki Pfau]

(In reply to comment #2)
> (From update of attachment 157023 [details])
> Although it's tempting to use the topDocument as the "first party" context, that doesn't match what we do for cookies.  For cookies, we use the firstPartyForCookies, which takes into account redirects, etc.  We probably want to be consistent in what we treat as the first party for these storage interactions.

I'm not entirely sure how they're different. Most of the time, they appear to be identical. I think I see a few cases where they could conceivably be different, but I don't know how they could be triggered. Can you give me some examples of cases that might pop up?

[Adam Barth]

Sure.  When the load of the main frame involves a redirect, I believe first party for cookies retains the URL of the start of the redirect chain rather than the final URL in the redirection chain.  Historically, there have also been other variations involving whether the frame was created as a result of a POST request.

I haven't studied the issue in a while, so I'm not sure which of these deviations are still present.  As a general principle, however, we should use some canonical piece of state to represent the "first party."  If firstPartyForCookies always ends up being the URL of the topDocument, then we shouldn't keep a separate member variable and instead just make it an accessor for topDocument()->url().

[Alexey Proskuryakov]

I think that the idea of "first party" is "what site the user thinks they are at". Which is admittedly vague, as most users pay little attention to address bar, but is the best we have to judge their intentions.

Programmatically opened windows and redirects are probably the most interesting cases.

[Maciej Stachowiak]

(In reply to comment #4)
> Sure.  When the load of the main frame involves a redirect, I believe first party for cookies retains the URL of the start of the redirect chain rather than the final URL in the redirection chain.  Historically, there have also been other variations involving whether the frame was created as a result of a POST request.
> 
> I haven't studied the issue in a while, so I'm not sure which of these deviations are still present.  As a general principle, however, we should use some canonical piece of state to represent the "first party."  If firstPartyForCookies always ends up being the URL of the topDocument, then we shouldn't keep a separate member variable and instead just make it an accessor for topDocument()->url().

I'm not sure it's actually appropriate or necessary to handle redirects the same way for cookies as for other kinds of storage (though I have not thought it through in detail). The reason cookies look at the pre-redirect URL is to stop sites from evading the policy by redirecting to third party tracker that sets a cookie, and then redirects back. I don't think these same concerns apply to LocalStorage or similar things. For LocalStorage, it is arguably more relevant to base decisions on what the URL field is showing right now, than what you originally typed or what was originally present in the link you clicked. The tradeoffs are different for protocol-level tracking-usable storage than for script-accessed storage.

[Adam Barth]

We talked about this in #webkit for a bit and Maciej reminded me that Safari only blocks writing cookies and not reading cookies.  This patch would block both read and writing local storage, which means it's already going to behave significantly differently from cookies.

IMHO, we should try to avoid proliferating a bunch of different notions about who the "first party".  If we're going to use something other than firstPartyForCookies, let's try to find a notion that will work well for a bunch of uses in the future.

[Adam Barth]

Comment on attachment 157023 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157023&action=review

> Source/WebCore/page/SecurityOrigin.cpp:406
> +    if (isUnique() || other->isUnique())
> +        return true;

We probably also want to handle the case with |this| has universal access.  In that case, you probably want to be first-party with everyone.

> Source/WebCore/page/SecurityOrigin.cpp:409
> +    if (equal(other))
> +        return false;

Does this ignore document.domain?  We should definitely test the document.domain case.

[Adam Barth]

Comment on attachment 157023 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157023&action=review

> Source/WebCore/page/DOMWindow.cpp:749
> -    if (!document->securityOrigin()->canAccessLocalStorage()) {
> +    if (!document->securityOrigin()->canAccessLocalStorage() || (document->settings()->thirdPartyStorageBlockingEnabled() && document->securityOrigin()->isThirdParty(document->topDocument()->securityOrigin()))) {

Can we fold these checks together?  For example, we could pass the topDocument as an argument to canAccessLocalStorage.  That way we won't forget to add this check if/when we add more callers of canAccessLocalStorage.

[Vicki Pfau]

(In reply to comment #9)
> (From update of attachment 157023 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157023&action=review
> 
> > Source/WebCore/page/DOMWindow.cpp:749
> > -    if (!document->securityOrigin()->canAccessLocalStorage()) {
> > +    if (!document->securityOrigin()->canAccessLocalStorage() || (document->settings()->thirdPartyStorageBlockingEnabled() && document->securityOrigin()->isThirdParty(document->topDocument()->securityOrigin()))) {
> 
> Can we fold these checks together?  For example, we could pass the topDocument as an argument to canAccessLocalStorage.  That way we won't forget to add this check if/when we add more callers of canAccessLocalStorage.

This definitely sounds like a good idea to me. I can revise my patch to reflect this, although I think we probably want to get more of this settled before I submit a second revision.

[Vicki Pfau]

Created attachment 157780 [details]
Patch

[Adam Barth]

Comment on attachment 157780 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157780&action=review

> Source/WebCore/page/SecurityOrigin.cpp:414
> +bool SecurityOrigin::isThirdParty(const SecurityOrigin* other) const

other -> child

> Source/WebCore/page/SecurityOrigin.h:143
> +    bool isThirdParty(const SecurityOrigin*) const;

This function is asymmetric, but it's not clear which argument is which.  Perhaps we should label the parameter "child" to make that clear?

Also, can we move this function to the "private" section?  It's better if callers use a "can" function rather than calling this function directly.

> Source/WebCore/testing/InternalSettings.cpp:626
> +    settings()->setThirdPartyStorageBlockingEnabled(enabled);

I guess this setting already exists?

[Adam Barth]

Thanks.  This looks a lot better.

[Vicki Pfau]

(In reply to comment #12)
> (From update of attachment 157780 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157780&action=review
> 
> > Source/WebCore/page/SecurityOrigin.cpp:414
> > +bool SecurityOrigin::isThirdParty(const SecurityOrigin* other) const
> 
> other -> child
> 
> > Source/WebCore/page/SecurityOrigin.h:143
> > +    bool isThirdParty(const SecurityOrigin*) const;
> 
> This function is asymmetric, but it's not clear which argument is which.  Perhaps we should label the parameter "child" to make that clear?

It's not currently asymmetric, but I intentionally invoked the function in canAccessLocalStorage in such a way that if it became asymmetric, it would still make sense. I suppose the parameter should be renamed as such beforehand though.

> 
> > Source/WebCore/testing/InternalSettings.cpp:626
> > +    settings()->setThirdPartyStorageBlockingEnabled(enabled);
> 
> I guess this setting already exists?

It was added a few days ago, but this is the first thing to use it.

[Adam Barth]

> It's not currently asymmetric,

It's asymmetric because of universal access.

[Vicki Pfau]

Committed r125335: <http://trac.webkit.org/changeset/125335>