Last Comment Bug 536509 - localStorage does not obey "third-party cookies" pref
: localStorage does not obey "third-party cookies" pref
Status: RESOLVED FIXED
[adv-main43-]
: dev-doc-complete, privacy, sec-want, site-compat
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: P3 major with 15 votes (vote)
: mozilla43
Assigned To: Michael Layzell [:mystor]
:
Mentors:
http://scarybeastsecurity.blogspot.co...
: 844659 (view as bug list)
Depends on: 1184973 1230563
Blocks: 744466 1147820 1183968
  Show dependency treegraph
 
Reported: 2009-12-22 22:24 PST by Jesse Ruderman
Modified: 2016-02-27 16:47 PST (History)
48 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch (1.81 KB, patch)
2012-03-02 11:09 PST, Nathan Froyd [:froydnj]
no flags Details | Diff | Review
patch (12.92 KB, patch)
2012-03-21 11:30 PDT, Nathan Froyd [:froydnj]
no flags Details | Diff | Review
patch (17.92 KB, patch)
2012-03-27 14:03 PDT, Nathan Froyd [:froydnj]
honzab.moz: feedback+
Details | Diff | Review
patch (15.48 KB, patch)
2012-04-06 11:35 PDT, Nathan Froyd [:froydnj]
no flags Details | Diff | Review
patch (22.37 KB, patch)
2012-04-25 14:06 PDT, Nathan Froyd [:froydnj]
honzab.moz: feedback+
Details | Diff | Review
Make localStorage obey the third-party cookies pref (11.56 KB, patch)
2015-07-14 20:43 PDT, Michael Layzell [:mystor]
no flags Details | Diff | Review
Update localStorage to use common StorageAllowedForWindow logic (12.55 KB, patch)
2015-07-17 08:15 PDT, Michael Layzell [:mystor]
no flags Details | Diff | Review
Update localStorage to use common StorageAllowedForWindow logic (13.16 KB, patch)
2015-07-31 21:51 PDT, Michael Layzell [:mystor]
ehsan: review+
Details | Diff | Review
Update localStorage to use common StorageAllowedForWindow logic (13.41 KB, patch)
2015-08-09 15:59 PDT, Michael Layzell [:mystor]
ehsan: review-
Details | Diff | Review
Update localStorage to use common StorageAllowedForWindow logic (13.26 KB, patch)
2015-08-20 13:25 PDT, Michael Layzell [:mystor]
no flags Details | Diff | Review

Description Jesse Ruderman 2009-12-22 22:24:41 PST
http://scarybeastsecurity.blogspot.com/2009/12/bypassing-intent-of-blocking-third.html claims localStorage does not obey the third-party cookies pref.
Comment 1 Honza Bambas (:mayhemer) 2009-12-26 09:07:14 PST
Confirming.

We only take care of a complete rejection preference, not about just third-party rejection. It is explicitly expressed in the spec to allow third-party storage blocking, best by following the cookie preferences, see [1].

Going to work on this ASAP.

[1] http://dev.w3.org/html5/webstorage/#user-tracking
Comment 2 Jonas Sicking (:sicking) 2011-08-22 23:08:34 PDT
There hasn't been any action here for a long time.

The way we should do this is to completely disable localStorage in frames where the parent frame chain contains third party frames, and the user has disabled third party cookies.
Comment 3 Mike Cardwell 2012-03-01 12:46:12 PST
This privacy issue needs to be addressed. Chrome does the correct thing and disables third party DOM storage when the user disables third party cookies. Firefox does not.

People disable third party cookies because they don't want to be tracked across sites. They can still be easily tracked if third party DOM storage is enabled.
Comment 4 Jonas Sicking (:sicking) 2012-03-01 16:22:40 PST
Honza: Any chance you'd have the cycles to work on this?
Comment 5 Honza Bambas (:mayhemer) 2012-03-01 17:01:38 PST
(In reply to Jonas Sicking (:sicking) from comment #4)
> Honza: Any chance you'd have the cycles to work on this?

Since this is a major issue and there is some pressure and also it may be simple to fix I'll move this higher in my todo list and reclaim the assignee field back when I start working on it.  Give me some two weeks, tops.

However, if there is anyone who wants to take this now and work on it, then please just do (change the assignee field to your self).
Comment 6 Nathan Froyd [:froydnj] 2012-03-02 10:06:45 PST
Is this merely a matter of adding mozIThirdPartyUtil checking to nsGlobalWindow::GetLocalStorage, similar to the check in GetMozIndexedDB?  (I don't see any checking for cookies there.)  Or is there some subtlety required in nsDOMStorage::CanUseStorage, which, to my untrained eyes, already checks third-party cookie preferences?
Comment 7 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-03-02 10:08:27 PST
The indexedDB thing is something completely different (it's checking to see if the current window is a third-party iframe).
Comment 8 Nathan Froyd [:froydnj] 2012-03-02 10:12:17 PST
Then the indexedDB check is just a superset of the check we want to do here, right?  (Since the bug is to only disable with appropriate pref.)  Or would it be reasonable to just drop third-party localStorage on the floor entirely for consistency's sake?
Comment 9 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-03-02 10:14:42 PST
No, the IndexedDB check is totally orthogonal to this.
Comment 10 Nathan Froyd [:froydnj] 2012-03-02 10:27:28 PST
Why is it orthogonal?  It sounds like it's doing exactly the check Jonas suggested in comment 2.
Comment 11 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-03-02 10:28:45 PST
What Jonas suggested in comment 2 is not what the bug summary is about, and has serious web compat implications (it'll totally break BrowserID, for instance).
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-03-02 10:30:15 PST
Oh, strike that, I misread.  Yeah, that is what comment 2 suggests.
Comment 13 Nathan Froyd [:froydnj] 2012-03-02 10:39:57 PST
So, just to be clear: the indexedDB check is just doing third-party frames, it's not checking preferences?

And we don't want to do the same thing for localStorage because it'd break a lot of things?  (Doesn't that mean we're sort of stuck with localStorage for use cases like BrowserID?)
Comment 14 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-03-02 10:47:09 PST
What comment 2 wants to do is the same as the IndexedDB check, but only if the relevant preference is set.

Turning off localStorage in third-party iframes unconditionally will break things.  Doing it conditionally on the pref will break things for users that set that pref ... but presumably they're willing to live with that.
Comment 15 Nathan Froyd [:froydnj] 2012-03-02 11:09:26 PST
Created attachment 602417 [details] [diff] [review]
patch

So here's a patch which I think DTRT.  I realize this it is a little ugly to check it here rather than in nsDOMStorage::CanUseStorage, but I didn't see a good way of either getting third-partiness from inside that function or consistently passing in third-party status from outside.  That might just be due to my ignorance, though.
Comment 16 Honza Bambas (:mayhemer) 2012-03-15 19:02:16 PDT
Comment on attachment 602417 [details] [diff] [review]
patch

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

This is mostly good but the decision code should live in nsDOMStorage::CanUseStorage.

nsDOMStorage::CanUseStorage should take bool aThirdParty arg.  In nsGlobalWindow::GetLocalStorage you'll fill it with result of IsThirdPartyWindow, in nsDOMStorage class (actually, in that file) you will fill it with a flag carried on the storage.  You can send the info about being third-party to the storage instance via nsPIDOMStorage.  Just add a method setThirdParty() to that private interface to set the flag.

There is more to do, though.  There is also network.cookie.thirdparty.sessionOnly pref.  This should turn the storage instance to session-only (nsDOMStorage::CanUseStorage should return *aSessionOnly = true and allow the storage use).
Comment 17 Nathan Froyd [:froydnj] 2012-03-20 09:47:06 PDT
(In reply to Honza Bambas (:mayhemer) from comment #16)
> nsDOMStorage::CanUseStorage should take bool aThirdParty arg.  In
> nsGlobalWindow::GetLocalStorage you'll fill it with result of
> IsThirdPartyWindow, in nsDOMStorage class (actually, in that file) you will
> fill it with a flag carried on the storage.

Hm, OK.  But what do you pass for these calls:

- http://mxr.mozilla.org/mozilla-central/source/dom/src/storage/nsDOMStorage.cpp#824

I assume false here?

- http://mxr.mozilla.org/mozilla-central/source/dom/src/storage/nsDOMStorage.cpp#2062

I assume that you want to do third-party checking between requestedDomain and currentDomain, but I'm not sure how you get to something that mozIThirdPartyUtil is happy with.
Comment 18 Honza Bambas (:mayhemer) 2012-03-20 10:05:30 PDT
(In reply to Nathan Froyd (:froydnj) from comment #17)
> (In reply to Honza Bambas (:mayhemer) from comment #16)
> > nsDOMStorage::CanUseStorage should take bool aThirdParty arg.  In
> > nsGlobalWindow::GetLocalStorage you'll fill it with result of
> > IsThirdPartyWindow, in nsDOMStorage class (actually, in that file) you will
> > fill it with a flag carried on the storage.
> 
> Hm, OK.  But what do you pass for these calls:
> 
> -
> http://mxr.mozilla.org/mozilla-central/source/dom/src/storage/nsDOMStorage.
> cpp#824

You have mOwner.  When you implement the flag, you have access to it here.

> 
> I assume false here?
> 
> -
> http://mxr.mozilla.org/mozilla-central/source/dom/src/storage/nsDOMStorage.
> cpp#2062

This is currently unused (bug 687579) and will be removed completely (bug 732708).  Looks I have to give it even more priority..

So far just pass false.

> 
> I assume that you want to do third-party checking between requestedDomain
> and currentDomain, but I'm not sure how you get to something that
> mozIThirdPartyUtil is happy with.
Comment 19 Nathan Froyd [:froydnj] 2012-03-20 10:09:05 PDT
(In reply to Honza Bambas (:mayhemer) from comment #18)
> (In reply to Nathan Froyd (:froydnj) from comment #17)
> > Hm, OK.  But what do you pass for these calls:
> > 
> > -
> > http://mxr.mozilla.org/mozilla-central/source/dom/src/storage/nsDOMStorage.
> > cpp#824
> 
> You have mOwner.  When you implement the flag, you have access to it here.

Well, yes, but mOwner is null if we're making that CanUseStorage call.
Comment 20 Nathan Froyd [:froydnj] 2012-03-21 11:30:18 PDT
Created attachment 608035 [details] [diff] [review]
patch

Updated patch that I think addresses comments, though I'm not sure if there should be some extra checks against network.cookies.thirdparty.sessionOnly.  Compile-only tested at this point.
Comment 21 Honza Bambas (:mayhemer) 2012-03-22 17:30:05 PDT
Comment on attachment 608035 [details] [diff] [review]
patch

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

Generally looks good, but please see the comments.

::: dom/src/storage/nsDOMStorage.cpp
@@ +822,5 @@
>    // If this is a cross-process situation, we don't have a real storage owner.
>    // All the correct checks have been done on the child, so we just need to
>    // make sure that our session-only status is correctly updated.
>    if (!mOwner)
> +    return nsDOMStorage::CanUseStorage(&mSessionOnly, false);

Ah, it is !mOwner branch... Then you will need to carry the flag from the child process to the parent and store it also on DOMStorageImpl.

Add here as a new arg to Init() event: [1] [2] [3].

Problem is that you don't have the flag set at the moment Init gets sent.  We will then have to modify nsIDOMStorageManager.GetLocalStorageForPrincipal and the InitAsLocalStorage method (and also don't have the new SetThirdParty method).  Sorry I didn't spot this the last time...

I can provide more info how to do this correctly if you don't know your self.


[1] http://mxr.mozilla.org/mozilla-central/source/dom/src/storage/PStorage.ipdl#71
[2] http://mxr.mozilla.org/mozilla-central/source/dom/src/storage/StorageChild.h
[3] http://mxr.mozilla.org/mozilla-central/source/dom/src/storage/StorageParent.h

@@ +1456,5 @@
>  
>    if (perm == nsIPermissionManager::DENY_ACTION)
>      return false;
>  
> +  PRUint32 thirdPartySessionOnly = Preferences::GetBool(kCookiesThirdPartySessionOnly);

Do this just before you need it.  This has a performance impact.

Actually, we should cache prefs and watch for changes, but that is for some other bug, not this one.

@@ +2080,5 @@
>                                        currentDomain);
>      NS_ENSURE_SUCCESS(*aResult, nsnull);
>  
>      bool sessionOnly;
> +    if (!nsDOMStorage::CanUseStorage(&sessionOnly, false)) {

This is ok, it shouldn't be used unless we want this patch for branches, what I don't expect.
Comment 22 Honza Bambas (:mayhemer) 2012-03-22 17:31:20 PDT
Comment on attachment 608035 [details] [diff] [review]
patch

Actually f flag should be emptied, according my suggestion this was OK.
Comment 23 Nathan Froyd [:froydnj] 2012-03-27 14:03:19 PDT
Created attachment 609875 [details] [diff] [review]
patch

New patch which moves the thirdParty bits to GetLocalStorageForPrincipal and InitAsLocalStorage.  SetThirdParty is no more.  Needed to update some tests for the change to GetLocalStorageForPrincipal; passing false seemed reasonable.

Compile-tested only.
Comment 24 Honza Bambas (:mayhemer) 2012-03-31 13:06:40 PDT
Comment on attachment 609875 [details] [diff] [review]
patch

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

There are other places:
http://mxr.mozilla.org/mozilla-central/search?string=getLocalStorageForPrincipal

::: dom/interfaces/storage/nsIDOMStorageManager.idl
@@ +64,2 @@
>     * This method ensures there is always only a single instance
>     * for a single origin.

Please also fix (actually remove) the other part of the comment: the method always creates a new instance, there is no hash table to store existing instances for the origin.

@@ +65,5 @@
>     * for a single origin.
>     */
>    nsIDOMStorage getLocalStorageForPrincipal(in nsIPrincipal aPrincipal,
> +                                            in DOMString aDocumentURI,
> +                                            in bool aIsThirdParty);

Change UUID of the interface.

aIsThirdParty could be default as "false".  This way you don't to modify that much code.

::: dom/src/storage/nsDOMStorage.cpp
@@ +823,5 @@
>    // If this is a cross-process situation, we don't have a real storage owner.
>    // All the correct checks have been done on the child, so we just need to
>    // make sure that our session-only status is correctly updated.
>    if (!mOwner)
> +    return nsDOMStorage::CanUseStorage(&mSessionOnly, false);

We may fix this in a followup to separate e10s specifics.  This will allow in some cases to store the data on disk in Fennec and B2G... in other words, this bug will not be fixed for e10s when you just pass false here.

I play with an idea to rather carry the mSessionOnly flag then mIsThirdParty... but that needs checking.

@@ +1482,5 @@
> +    if (aThirdParty && cookieBehavior == BEHAVIOR_REJECTFOREIGN)
> +      return false;
> +
> +    PRUint32 thirdPartySessionOnly
> +      = Preferences::GetBool(kCookiesThirdPartySessionOnly);

= aThirdParty && Preferences::GetBool(...) and please add a comment it's a opt only.

@@ +2075,5 @@
>                                        currentDomain);
>      NS_ENSURE_SUCCESS(*aResult, nsnull);
>  
>      bool sessionOnly;
> +    if (!nsDOMStorage::CanUseStorage(&sessionOnly, false)) {

So, this will be removed by your patch from bug 732708.
Comment 25 Nathan Froyd [:froydnj] 2012-04-04 12:39:34 PDT
Thanks for the prompt feedback!

(In reply to Honza Bambas (:mayhemer) from comment #24)
> There are other places:
> http://mxr.mozilla.org/mozilla-central/
> search?string=getLocalStorageForPrincipal

Doh, I rgrep'd just in dom/.

> ::: dom/interfaces/storage/nsIDOMStorageManager.idl
> @@ +64,2 @@
> >     * This method ensures there is always only a single instance
> >     * for a single origin.
> 
> Please also fix (actually remove) the other part of the comment: the method
> always creates a new instance, there is no hash table to store existing
> instances for the origin.

The full comment reads:

  /**
   * Returns instance of localStorage object for aDocumentURI's origin.
   * This method ensures there is always only a single instance
   * for a single origin.
   */

I assume that you want me to remove the "This method ensures..." sentence, despite what you wrote above seeming to imply that the other sentence should be removed?

> @@ +1482,5 @@
> > +    if (aThirdParty && cookieBehavior == BEHAVIOR_REJECTFOREIGN)
> > +      return false;
> > +
> > +    PRUint32 thirdPartySessionOnly
> > +      = Preferences::GetBool(kCookiesThirdPartySessionOnly);
> 
> = aThirdParty && Preferences::GetBool(...) and please add a comment it's a
> opt only.

What is "a opt only" here?
Comment 26 Honza Bambas (:mayhemer) 2012-04-04 17:07:00 PDT
(In reply to Nathan Froyd (:froydnj) from comment #25)
> Thanks for the prompt feedback!
> 
> (In reply to Honza Bambas (:mayhemer) from comment #24)
> I assume that you want me to remove the "This method ensures..." sentence,
> despite what you wrote above seeming to imply that the other sentence should
> be removed?

Yes exactly, splinter usually leaves more lines... I didn't know how it is going to look at the bugzilla comment after publish.

> > = aThirdParty && Preferences::GetBool(...) and please add a comment it's a
> > opt only.
> 
> What is "a opt only" here?

That it is intended only for optimization purposes (to not read the pref until really necessary).  Sorry :)
Comment 27 Nathan Froyd [:froydnj] 2012-04-06 11:35:34 PDT
Created attachment 612954 [details] [diff] [review]
patch

OK, I think I've addressed all the feedback, and the patch is running on try:

https://tbpl.mozilla.org/?tree=Try&rev=66e19e675f16

Asking for actual review, and hoping that the patch looks good on try.
Comment 28 Honza Bambas (:mayhemer) 2012-04-11 10:20:59 PDT
(In reply to Nathan Froyd (:froydnj) from comment #27)
> Created attachment 612954 [details] [diff] [review]
> patch
> 
> OK, I think I've addressed all the feedback, and the patch is running on try:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=66e19e675f16
> 
> Asking for actual review, and hoping that the patch looks good on try.

How did the try go?  I cannot see any results and don't remember what it was.
Comment 29 Honza Bambas (:mayhemer) 2012-04-11 10:45:34 PDT
Comment on attachment 612954 [details] [diff] [review]
patch

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

So, this only needs a test (sorry I didn't mention this earlier).

Pre-r+, but a test is a review condition here.

::: dom/interfaces/storage/nsIDOMStorageManager.idl
@@ +64,4 @@
>     */
>    nsIDOMStorage getLocalStorageForPrincipal(in nsIPrincipal aPrincipal,
> +                                            in DOMString aDocumentURI,
> +                                            [optional] in bool aIsThirdParty);

Please specify default value explicitly.

::: dom/src/storage/nsDOMStorage.cpp
@@ +822,5 @@
>    // If this is a cross-process situation, we don't have a real storage owner.
>    // All the correct checks have been done on the child, so we just need to
>    // make sure that our session-only status is correctly updated.
>    if (!mOwner)
> +    return nsDOMStorage::CanUseStorage(&mSessionOnly, false);

Please refer with an XXX comment bug 744466 here.
Comment 30 Nathan Froyd [:froydnj] 2012-04-11 11:01:42 PDT
(In reply to Honza Bambas (:mayhemer) from comment #28)
> How did the try go?  I cannot see any results and don't remember what it was.

Oh, sorry, it required a different push because try ran into problems that day:

https://tbpl.mozilla.org/?tree=Try&rev=7419b2d45d2f

In any event, there were some M-oth failures that need to be debugged.

(In reply to Honza Bambas (:mayhemer) from comment #29)
> ::: dom/interfaces/storage/nsIDOMStorageManager.idl
> @@ +64,4 @@
> >     */
> >    nsIDOMStorage getLocalStorageForPrincipal(in nsIPrincipal aPrincipal,
> > +                                            in DOMString aDocumentURI,
> > +                                            [optional] in bool aIsThirdParty);
> 
> Please specify default value explicitly.

I think you are thinking of C++ and not IDL.
Comment 31 Nathan Froyd [:froydnj] 2012-04-12 10:56:38 PDT
The M-oth failures were straightforward; we ought to consider chrome windows as always first party and skip the IsThirdPartyWindow checks.

(In reply to Honza Bambas (:mayhemer) from comment #29)
> So, this only needs a test (sorry I didn't mention this earlier).

What's the right way to write a test for this?  Are there existing tests for inspiration?  The only way I'm aware of to test this is with actual websites, and accessing those from tests is a no-go.
Comment 32 Honza Bambas (:mayhemer) 2012-04-12 11:41:08 PDT
(In reply to Nathan Froyd (:froydnj) from comment #31)
> The M-oth failures were straightforward; we ought to consider chrome windows
> as always first party and skip the IsThirdPartyWindow checks.
> 

Hmm.. so, chrome windows may return IsThirdPartyWindow == true?  Strange.  But I don't know the code well enough here.

Worth to check with Boris.



> What's the right way to write a test for this?  

You got me...  We may check only whether we are not able to get the storage (or store to it) in a third party iframe, that's simple.  You have bunch of example.com, example.org and so origins in mochitest to work with.  Just load an iframe from example.com and check you cannot access.

For third party session only pref you can inspire with http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/test/unit/test_cookies_thirdparty_session.js?force=1


> Are there existing tests for
> inspiration?  

Many under dom/tests/mochitest


> The only way I'm aware of to test this is with actual
> websites, and accessing those from tests is a no-go.

Not necessary
Comment 33 Nathan Froyd [:froydnj] 2012-04-12 11:51:04 PDT
(In reply to Honza Bambas (:mayhemer) from comment #32)
> (In reply to Nathan Froyd (:froydnj) from comment #31)
> > The M-oth failures were straightforward; we ought to consider chrome windows
> > as always first party and skip the IsThirdPartyWindow checks.
> > 
> 
> Hmm.. so, chrome windows may return IsThirdPartyWindow == true?  Strange. 
> But I don't know the code well enough here.

chrome windows will return errors from IsThirdPartyWindow, causing us to return early when accessing window.localStorage.  See http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#8330 for the mozIndexedDB version.

I didn't realize we had a couple domains available to us during testing; I'll check those out.  Thanks.
Comment 34 Honza Bambas (:mayhemer) 2012-04-12 12:00:01 PDT
(In reply to Nathan Froyd (:froydnj) from comment #33)
> chrome windows will return errors from IsThirdPartyWindow, causing us to
> return early when accessing window.localStorage.  See
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.
> cpp#8330 for the mozIndexedDB version.

You must not disable access to storage for chrome windows.  At least not for about: and moz-save-about: schemes.
Comment 35 Honza Bambas (:mayhemer) 2012-04-12 12:02:00 PDT
(In reply to Honza Bambas (:mayhemer) from comment #34)
> You must not disable access to storage for chrome windows.  At least not for
> about: and moz-save-about: schemes.

Ah, the goal is to just bypass the 3rd-party check.  Then it's ok.  I wasn't aware.
Comment 36 Honza Bambas (:mayhemer) 2012-04-25 13:50:31 PDT
Nathan, any updates here?  The patch will bitroth soon...
Comment 37 Nathan Froyd [:froydnj] 2012-04-25 13:51:52 PDT
Just in the middle of debugging tests.  I should have something ready tomorrow.
Comment 38 Nathan Froyd [:froydnj] 2012-04-25 14:06:03 PDT
Created attachment 618427 [details] [diff] [review]
patch

Now with tests that we disallow localStorage in third-party windows and with chrome correctly permitted to access localStorage.
Comment 39 Nathan Froyd [:froydnj] 2012-04-25 19:37:54 PDT
Comment on attachment 618427 [details] [diff] [review]
patch

OK, so this version still has issues with chrome tests:

https://tbpl.mozilla.org/?tree=Try&rev=0b89f51659ad

but I'm not sure what the exact problem is.  Apparently some non-chrome windows (!IsChromeWindow) aren't sufficiently complete as to make it through mozIThirdPartyUtil without throwing errors.  There are apparently checks for this sort of thing (nsDOMStorage::GetDomainURI, http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#8286 (?), etc.) elsewhere, but I'm not sure how to handle those cases...

Downgrading r? to f? because things obviously aren't quite ready yet, but Honza probably knows what's going on here.
Comment 40 Honza Bambas (:mayhemer) 2012-04-27 12:42:59 PDT
(In reply to Nathan Froyd (:froydnj) from comment #39)
> Honza probably knows what's going on here.

No I don't.  This is out of scope of my knowledge.  Even the outer window is not nsGlobalChromeWindow, regardless a chrome: URL.  

However, the test is an xhtml document, we may need to turn it to xul.
Comment 41 Honza Bambas (:mayhemer) 2012-04-27 12:50:49 PDT
Comment on attachment 618427 [details] [diff] [review]
patch

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

I didn't much check the tests, will look at them later.

::: dom/src/storage/nsDOMStorage.cpp
@@ +1414,5 @@
>  }
>  
>  //static
>  bool
> +nsDOMStorage::CanUseStorage(bool* aSessionOnly, bool aThirdParty)

I hate this method's signature :)

I've proposed in bug 722857 (that btw collide with this one, you'll have to merge) to pass DOMStorageBase as a single param to this method.  And make it optional.

For your purpose, however, we need to determine the result while not having a storage yet.

So, I'd like to ask you to add an overload (or opt param, up to you), in pseudo code:

bool nsDOMStorage::CanUseStorage(bool 3rdParty)
{
  return CanUserStorage(null, 3rdParty);
}

bool nsDOMStorage::CanUseStorage(DOMStorageBase*, bool 3rdParty)
{
  do the stuff as now and set properties directly on the storage, while accepting it to be null (hence the proposal to move the state to DOMStorageBase ; I also have some future plans that fit it)
}

::: dom/src/storage/nsDOMStorage.h
@@ +418,5 @@
>    // this storage also in mLocalStorages hash table.
>    nsDOMStorageType mStorageType;
>  
> +  // true if this storage is associated with a third-party window
> +  bool mThirdParty;

Let this live on DOMStorageBase.

::: dom/tests/mochitest/localstorage/frameThirdPartyCookies2.html
@@ +5,5 @@
> +<html>
> +<head>
> +  <title>localStorage Test</title>
> +
> +  <script type="text/javascript;version=1.7">

You may know more then me, but isn't just <script> (no attributes) enough?

@@ +19,5 @@
> +        let ls = localStorage;
> +        success = !!ls;
> +      }
> +      catch (e) {
> +      }

Please:

var exceptionThrew;
try {
   ... do stuff ...
   exceptionThrew = false;
}
catch (e) {
   exceptionThrew = true;
}
Comment 42 Nathan Froyd [:froydnj] 2012-12-04 08:58:34 PST
Not working on this; the localStorage tweaks/rewrites in progress are likely to make whatever work was done here obsolete anyway.
Comment 43 Honza Bambas (:mayhemer) 2012-12-04 09:15:07 PST
Security check and session only state determination is still the same, just the DB and API implementation is about to completely change.  So I think this will be applicable.
Comment 44 [:mmc] Monica Chew (no longer reading bugmail) 2013-02-25 08:42:31 PST
*** Bug 844659 has been marked as a duplicate of this bug. ***
Comment 45 wind 2013-04-10 04:37:11 PDT Comment hidden (off-topic)
Comment 46 David Bruant 2013-06-12 05:46:33 PDT
It appears to me that the context around this bug has changed enough that this bug needs some  re-evaluation.

Before the recent context, let's start with busting a common idea which can be read in lots of places and seems obvious to a lot of people, but isn't so. The article cited in first comment says:
"In concept, HTML5 Local Storage is very similar to cookies. On a per-origin basis, there is a set of disk-persisted name / value pairs."

This is obviously omitting a serious difference which is that cookies are sent alongside *every* HTTP request while localStorage data isn't. This makes a huge difference in terms of tracking and control over the tracking. 


On the more recent context:
* The default value of the third-cookie pref is about to change. I don't think it's been properly evaluated whether disabling third-party localStorage at scale would break the web for users
* Website owners can sandbox iframes with the new iframe@sandbox attribute. If they want to protect their users privacy against content they embed, they can set the sandbox attribute and choose to disable JavaScript and/or choose to make the iframe live in its own one-time unique unforgeable origin. Each of these features removes the opportunity of tracking the user using local storage. This sort of control never existed with third-party cookies.


Given all of that, I would recommend either giving up on this idea or at the very least re-evaluate it to make sure it doesn't break the web (and I'm personally not optimistic on that topic).
Comment 47 Daniel Veditz [:dveditz] 2013-06-16 20:12:34 PDT
(In reply to David Bruant from comment #46)
> * The default value of the third-cookie pref is about to change. I don't
> think it's been properly evaluated whether disabling third-party
> localStorage at scale would break the web for users

True, we should add telemetry before making any such change.

One alternative to simply preventing localStorage calls would be to give 3rd-party iframes a blank memory-only storage space (on domains which are not whitelisted). That way in case the frame uses the storage computationally the code will still work, it just won't persist the data and can't use it for tracking. This is similar to what we do when in Private Browsing mode (although that lasts for the session and we'd want this to have a shorter lifetime, or be unique per parent+child domain.

> * Website owners can sandbox iframes with the new iframe@sandbox attribute.

3rd party cookie blocking was not invented because website owners didn't have the tools to protect user privacy -- the website owners are the ones including all the tracking beacons!
Comment 48 David Bruant 2013-06-17 02:12:26 PDT
(In reply to Daniel Veditz [:dveditz] from comment #47)
> One alternative to simply preventing localStorage calls would be to give
> 3rd-party iframes a blank memory-only storage space (on domains which are
> not whitelisted). That way in case the frame uses the storage
> computationally the code will still work, it just won't persist the data and
> can't use it for tracking. This is similar to what we do when in Private
> Browsing mode (although that lasts for the session and we'd want this to
> have a shorter lifetime, or be unique per parent+child domain.
That's an interesting idea. I wonder how much non-tracking use case that would break, though.

> > * Website owners can sandbox iframes with the new iframe@sandbox attribute.
> 
> 3rd party cookie blocking was not invented because website owners didn't
> have the tools to protect user privacy -- the website owners are the ones
> including all the tracking beacons!
That's a part of the threat model that hasn't been always clear to me. Are the website and the tracking 3rd party cooperating in tracking?
If they are not, a common use case is a website linking to an image/iframe/etc.; that sends cookies to the image domain without the website being able to do anything against it. 3rd-party cookie blocking makes a difference here.
If they are cooperating, I don't see how Firefox can prevent tracking since the website can just decide to embed a script from the 3rd-party or why not even host it so that it appears as same-origin.
What is the exact threat being fought against?
Comment 49 Brian Kardell 2013-08-28 08:37:56 PDT
> That's an interesting idea. I wonder how much non-tracking use case that
> would break, though.

I can tell you that I have seen and even developed things that do exactly this to share localstorage in data between cooperating domains (see below) via postmessage/messagechannel with a pre-established protocol and explicit trust on both ends (the one providing storage, for example denies requests that aren't from cooperating clients and then only fulfills a specific API (he doesn't actually expose write at all, rather works more like a cache/proxy).  All of these cases are currently according to specs and work in all browsers - some are definitely used by many users.


> If they are cooperating, I don't see how Firefox can prevent tracking since
> the website can just decide to embed a script from the 3rd-party or why not
> even host it so that it appears as same-origin.
> What is the exact threat being fought against?

It's also not clear to me that they *should*... Seems what you want here is cooperation under mutual suspicion which seems to me like what we should be building in here - a real good way to actually do that.  Currently this is piecemeal and not pleasant to do when you can do it - but it seems to me that there are 3 parties here:  The one who provides a "service" (a) the one who says "yes, I'm ok using that service" (b) and the user who trustingly uses b (c).  

The real problem seems to be more that getting that right is currently a problem/difficult or even impossible with some things... Seems there aren't some of the right underlying mechanisms to make this workable/safe as a general concept.  Still, even with what is there today - it is worrying to think of breaking things that people have built according to spec, invested time and money in and had running successfully in modern browsers for some time.
Comment 50 Brad Lassey [:blassey] (use needinfo?) 2014-07-18 12:30:49 PDT
*** Bug 744466 has been marked as a duplicate of this bug. ***
Comment 51 Michael Layzell [:mystor] 2015-07-14 20:43:11 PDT
Created attachment 8633877 [details] [diff] [review]
Make localStorage obey the third-party cookies pref

This patch makes localStorage obey the BEHAVIOR_REJECTFOREIGN behaviour preference for cookieBehavior. This corresponds to setting "Accept third-party cookies" to "Never".

I think that making localStorage obey BEHAVIOUR_LIMITFOREIGN ("From visited") is outside the scope of this bug, as we will need to determine a defintion as to what a "visited" website means for localStorage.

I'm opening a follow up bug to make sessionStorage obey cookies preferences consistently with localStorage.
Comment 52 David Bruant 2015-07-15 03:41:46 PDT
Comment 47 suggested telemetry should be added before making a change. Did it happen? If so, what portion of the web would break from the change? How important is it?
Comment 53 :Ehsan Akhgari (busy, don't ask for review please) 2015-07-16 12:58:57 PDT
(In reply to David Bruant from comment #52)
> Comment 47 suggested telemetry should be added before making a change. Did
> it happen? If so, what portion of the web would break from the change? How
> important is it?

We're not looking to change the default value for the third-party cookie pref, so this will not change anything for users who don't have that pref set to anything other than the default.  I am not aware of any telemetry with regards to changing the default value of that pref.
Comment 54 Michael Layzell [:mystor] 2015-07-17 08:15:33 PDT
Created attachment 8635316 [details] [diff] [review]
Update localStorage to use common StorageAllowedForWindow logic

Updated version of the patch which instead uses the StorageAllowedForWindow logic. This logic is being implemented in bug 1184789.

Test coverage on this functionality hasn't been checked yet, but new tests for the behavior will likely have to be written.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=204836d73b40
Comment 55 Michael Layzell [:mystor] 2015-07-24 12:06:07 PDT
Comment on attachment 8635316 [details] [diff] [review]
Update localStorage to use common StorageAllowedForWindow logic

clearing this because I'm working on some fairly major changes to nsContentUtils::StorageAllowedForWindow, and this review would have to probably be re-done.
Comment 56 Michael Layzell [:mystor] 2015-07-31 21:51:26 PDT
Created attachment 8641985 [details] [diff] [review]
Update localStorage to use common StorageAllowedForWindow logic

Updated version of patch. Part 2 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 57 :Ehsan Akhgari (busy, don't ask for review please) 2015-08-07 09:23:04 PDT
Comment on attachment 8641985 [details] [diff] [review]
Update localStorage to use common StorageAllowedForWindow logic

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

Great work, this increases the readability of these checks a lot!

::: dom/storage/DOMStorage.cpp
@@ +246,5 @@
>    if (!mozilla::Preferences::GetBool(kStorageEnabled)) {
>      return false;
>    }
>  
> +  EnumSet<nsContentUtils::StorageAccessibility> accessibility;

"accessibility" means something very specific in our code.  Can you please rename these to "acceess"?

@@ +248,5 @@
>    }
>  
> +  EnumSet<nsContentUtils::StorageAccessibility> accessibility;
> +  if (aWindow) {
> +    nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow);

Why not make this method accept a nsPIDOMWindow* instead?
Comment 58 Michael Layzell [:mystor] 2015-08-09 15:59:43 PDT
Created attachment 8645502 [details] [diff] [review]
Update localStorage to use common StorageAllowedForWindow logic
Comment 59 :Ehsan Akhgari (busy, don't ask for review please) 2015-08-19 20:26:09 PDT
Comment on attachment 8645502 [details] [diff] [review]
Update localStorage to use common StorageAllowedForWindow logic

This breaks DOMStorage methods for sessionStorage, while the window.sessionStorage getter is allowed.  Can you please fix it to that sessionStorage fully works, and add a test for it as well?
Comment 60 Michael Layzell [:mystor] 2015-08-20 13:25:23 PDT
Created attachment 8650665 [details] [diff] [review]
Update localStorage to use common StorageAllowedForWindow logic

Updated due to changes in StorageAllowedFor* logic
Comment 63 Ryan VanderMeulen [:RyanVM] 2015-08-28 12:33:19 PDT
https://hg.mozilla.org/mozilla-central/rev/d67c4fd50c16
Comment 65 Chris Mills (Mozilla, MDN editor) [:cmills] 2015-09-14 09:19:29 PDT
(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #64)
> Please update
> https://developer.mozilla.org/en-US/docs/Mozilla/Gecko/
> DOM_Storage_implementation_notes#Access_security_checks

I've updated that document; I've also added a note to the main Web Storage documentation at

https://developer.mozilla.org/en-US/docs/Web/API/Web_Storage_API

And I've added a note to the relevant release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/43#Security

Let me know if these read ok. Thanks!
Comment 66 Ian Nartowicz 2015-09-14 09:56:58 PDT
The release note is a bit scary.  Could it be qualified to indicate that only *certain* accesses are blocked when the pref is set?
Comment 67 Chris Mills (Mozilla, MDN editor) [:cmills] 2015-09-14 10:19:42 PDT
(In reply to Ian Nartowicz from comment #66)
> The release note is a bit scary.  Could it be qualified to indicate that
> only *certain* accesses are blocked when the pref is set?

Argh, you're right. I've missed out a vital detail. Does this sound better:

"Access to Web Storage (i.e. localStorage and sessionStorage) from sites containing third party frames is now denied if the user has disabled third-party cookies"

?
Comment 68 Michael Layzell [:mystor] 2015-09-14 10:21:44 PDT
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #67)
> (In reply to Ian Nartowicz from comment #66)
> > The release note is a bit scary.  Could it be qualified to indicate that
> > only *certain* accesses are blocked when the pref is set?
> 
> Argh, you're right. I've missed out a vital detail. Does this sound better:
> 
> "Access to Web Storage (i.e. localStorage and sessionStorage) from sites
> containing third party frames is now denied if the user has disabled
> third-party cookies"
> 
> ?

It's not disabled for the site, it's disabled for the third-party iframe.

"Access to Web Storage (i.e. localStorage and sessionStorage) from third-party iframes is now denied if the user has disabled third-party cookies" would probably be better.
Comment 69 Chris Mills (Mozilla, MDN editor) [:cmills] 2015-09-14 10:32:52 PDT
(In reply to Michael Layzell [:mystor] from comment #68)

> It's not disabled for the site, it's disabled for the third-party iframe.
> 
> "Access to Web Storage (i.e. localStorage and sessionStorage) from
> third-party iframes is now denied if the user has disabled third-party
> cookies" would probably be better.

Ah, I get it now. OK, all pages updated to fix the messaging. Thanks for your help folks.
Comment 70 Tanvi Vyas - behind on reviews [:tanvi] 2015-09-14 11:56:42 PDT
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #65)
> (In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #64)
> > Please update
> > https://developer.mozilla.org/en-US/docs/Mozilla/Gecko/
> > DOM_Storage_implementation_notes#Access_security_checks
> 
> I've updated that document; I've also added a note to the main Web Storage
> documentation at
> 
> https://developer.mozilla.org/en-US/docs/Web/API/Web_Storage_API
> 
In the above two links, can you add something like "Starting with Firefox 43,..." so that readers of this page don't assume this is the behavior on their current Firefox release?
Comment 71 Chris Mills (Mozilla, MDN editor) [:cmills] 2015-09-14 12:18:24 PDT
(In reply to Tanvi Vyas [:tanvi] from comment #70)
> In the above two links, can you add something like "Starting with Firefox
> 43,..." so that readers of this page don't assume this is the behavior on
> their current Firefox release?

Good call; updated.
Comment 73 Stefan Plewako [:stef] 2015-12-03 15:45:43 PST
Does this change take into account "Accept third-party cookies" and exceptions list for the "Accept cookies from sites" preferences?
Comment 74 Michael Layzell [:mystor] 2015-12-03 15:51:10 PST
(In reply to Stefan Plewako [:stef] from comment #73)
> Does this change take into account "Accept third-party cookies" and
> exceptions list for the "Accept cookies from sites" preferences?

Yes, it should
Comment 75 Stefan Plewako [:stef] 2015-12-03 16:35:43 PST
With my limited testing, Firefox 43 beta and latest Nightly (OS X 10.11) ignore exceptions list for the "Accept cookies from sites" when "Keep until: I close Firefox" is set.

Steps to reproduce:
1. With fresh Nightly profile go to privacy settings and change preference to use custom settings for history, set cookies to be kept until closing the browser and add following domains as allowed to the exceptions: https://splewako.com, https://www.splewako.com, https://hg.mozilla.org
2. Visit https://www.splewako.com/backlog/mozilla.l10n.beta
3. Check if there are items in localStorage (from console or on https://www.splewako.com/backlog/)
4. Restart the browser

Actual result: localStorage content is gone
Expected result: localStorage content is present
Comment 76 Stefan Plewako [:stef] 2015-12-04 05:13:56 PST
On 42 it seems to work as expected.
Michael, could you confirm and advice what to do next?
Comment 77 Michael Layzell [:mystor] 2015-12-04 07:55:06 PST
I've opened bug 1230563 to fix this, and posted my best guess as to where the problem is coming from.
Comment 78 singpolyma 2016-02-24 09:22:43 PST
Does this pref make firefox ban reading from localStorage, or only writing to it (for the third-party frame).
Comment 79 Michael Layzell [:mystor] 2016-02-27 16:47:22 PST
(In reply to singpolyma from comment #78)
> Does this pref make firefox ban reading from localStorage, or only writing
> to it (for the third-party frame).

It causes the window.localStorage accessor to throw, denying both reading and writing.

Note You need to log in before you can comment on or make changes to this bug.