Last Comment Bug 762409 - localStorage throws "The operation is insecure." error with document.domain
: localStorage throws "The operation is insecure." error with document.domain
Status: RESOLVED FIXED
: china-p1
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 13 Branch
: All All
: -- major (vote)
: ---
Assigned To: Honza Bambas (:mayhemer)
:
Mentors:
: 765497 (view as bug list)
Depends on:
Blocks: 495337
  Show dependency treegraph
 
Reported: 2012-06-07 02:04 PDT by Longhao Luo
Modified: 2014-09-03 04:51 PDT (History)
16 users (show)
Ms2ger: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix
+
fixed
+
verified
+
fixed


Attachments
2012-02-23 nightly screenshot of show.qq.com (758.49 KB, image/png)
2012-06-13 01:57 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
2012-02-24 nightly screenshot of show.qq.com (156.03 KB, image/png)
2012-06-13 02:00 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
v1 (1.06 KB, patch)
2012-06-13 14:59 PDT, Honza Bambas (:mayhemer)
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
honzab.moz: checkin+
Details | Diff | Review
Reduced version of js used in show.qq.com (1.62 KB, application/x-javascript)
2012-06-13 19:22 PDT, Hector Zhao [:hectorz]
no flags Details

Description Longhao Luo 2012-06-07 02:04:36 PDT
User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/536.5 (KHTML, like Gecko) Chrome/19.0.1084.52 Safari/536.5

Steps to reproduce:

1. create two HTML files.

1) http://sub.test.com/storage.html
----------------
<!DOCTYPE html>
<meta charset="utf-8" />
<title>nothing here</title>
<script>
document.domain = 'test.com';
</script>


2) http://test.com/cross-storage-test.html
----------------
<!doctype html>
<html>
	<head>
		<meta charset="utf-8" />
		<title>localStorage Test</title>
	</head>
	<body>
	</body>
	<script>
		document.domain = 'test.com';
		window.onload = function() {
			var iframe = document.createElement('iframe');
			iframe.src = 'http://sub.test.com/storage.html';
			iframe.onload = function() {
				var win = iframe.contentWindow;
				console.log(win);
				win.localStorage.setItem('hello', '1234');
				console.log(win.localStorage.getItem('hello'));
			};
			document.body.appendChild(iframe);
		};
	</script>
</html>

2. open http://test.com/cross-storage-test.html URL.



Actual results:

The operation is insecure.
[Break On This Error]

win.localStorage.setItem('hello', '1234');


Expected results:

print "1234" in console.
Comment 1 Boris Zbarsky [:bz] 2012-06-07 08:57:28 PDT
I believe per spec the storage stuff completely ignores document.domain, so you're trying to setItem across domains...
Comment 2 Honza Bambas (:mayhemer) 2012-06-07 08:59:42 PDT
(In reply to Boris Zbarsky (:bz) from comment #1)
> I believe per spec the storage stuff completely ignores document.domain, so
> you're trying to setItem across domains...

Exactly.
Comment 3 Longhao Luo 2012-06-07 19:56:04 PDT
Thanks for your reply. but the above code works fine in other browsers and Firefox <= 12.

There is one exception to the same origin rule. A script can set the value of document.domain to a suffix of the current domain. If it does so, the shorter domain is used for subsequent origin checks. For example, assume a script in the document at http://store.company.com/dir/other.html executes the following statement:

document.domain = "company.com";

After that statement executes, the page would pass the origin check with http://company.com/dir/page.html

https://developer.mozilla.org/en/DOM/document.domain
https://developer.mozilla.org/en/Same_origin_policy_for_JavaScript
Comment 4 Boris Zbarsky [:bz] 2012-06-07 20:50:56 PDT
> There is one exception to the same origin rule.

I'm well aware of how document.domain works.  It affects _some_ same-origin checks, but not all.  For example, it doesn't affect the same-origin checks that XMLHttpRequest performs.  See the difference between "origin" and "effective script origin" in the HTML5 specs.

Honza, was the behavior change here due to a recent spec change?
Comment 5 Boris Zbarsky [:bz] 2012-06-08 08:09:52 PDT
It doesn't look like a recent change, and the discussion in bug 495337 (where the behavior change happened) sure sounds like it shouldn't work in other browsers either....  See Adam Barth's comments in particular.
Comment 6 Gary Kwong [:gkw] [:nth10sd] 2012-06-12 23:58:48 PDT
This seems to be the problem happening on http://show.qq.com/ (QQ is a widely used site in China).

show.qq.com does not work properly in the latest Firefox, I tested on Nightly v16, I've heard that 12 works but not 13, and QQ was in contact mentioning that it might be this bug.

Confirming the issue (not sure if it's a site problem or a Firefox problem yet). I'll try to get a regression window.
Comment 7 Gary Kwong [:gkw] [:nth10sd] 2012-06-13 01:06:37 PDT
Confirming that show.qq.com works in:

Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:11.0a1) Gecko/20111219 Firefox/11.0a1

mozilla-central changeset d75ebb37080e

===

but fails in:

Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:16.0) Gecko/16.0 Firefox/16.0a1

(2012-06-12)

mozilla-central changeset 131961e5e0d1

I'll be narrowing this window down.
Comment 8 Gary Kwong [:gkw] [:nth10sd] 2012-06-13 01:33:38 PDT
Confirming that the site works in 20120223 but not in 20120224 nightly:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5e756e59a794&tochange=cd120efbe4c6

Bug 495337 is in the regression window and may indeed be the cause.
Comment 9 Gary Kwong [:gkw] [:nth10sd] 2012-06-13 01:35:45 PDT
I checked Chromium version 21.0.1173.0 (Developer Build 141642) and Safari 5.1.6 and both seem to render the site properly.
Comment 10 Gary Kwong [:gkw] [:nth10sd] 2012-06-13 01:46:07 PDT
Nominating tracking flags for all known versions - show.qq.com is essentially unusable with this bug.
Comment 11 Gary Kwong [:gkw] [:nth10sd] 2012-06-13 01:57:27 PDT
Created attachment 632604 [details]
2012-02-23 nightly screenshot of show.qq.com

Works in 2012-02-23 nightly.
Comment 12 Gary Kwong [:gkw] [:nth10sd] 2012-06-13 02:00:20 PDT
Created attachment 632605 [details]
2012-02-24 nightly screenshot of show.qq.com

Fails to work in 2012-02-24 nightly.

I don't think I'd expect this to be firedrilled for Firefox 13 though..
Comment 13 Boris Zbarsky [:bz] 2012-06-13 07:59:58 PDT
Honza, can you double-check the behavior wrt document.domain of other UAs?  It's possible the spec just needs to change to reflect market reality...
Comment 14 Honza Bambas (:mayhemer) 2012-06-13 08:05:27 PDT
I'll take a look today.
Comment 15 Honza Bambas (:mayhemer) 2012-06-13 14:59:26 PDT
Created attachment 632897 [details] [diff] [review]
v1

It turns out that the correct check is when at least one of these passes:
- if code base of the storage's principal is the same as the code base of the content page ; we currently do that with SubsumesIgnoringDomain()
- if code base is the same or domain is set to the same value (or not set on both) ; I'm adding Subsumes() additional check

so, finally we have canAccess = subsumesIgnoringDomain || subsumes;

I feel this is right, but Boris, please check this.  I may not catch all implications.
Comment 16 Boris Zbarsky [:bz] 2012-06-13 15:21:34 PDT
Honza: can you link to the relevant part of the spec?  Or is this not in the spec?
Comment 17 Honza Bambas (:mayhemer) 2012-06-13 16:02:13 PDT
(In reply to Boris Zbarsky (:bz) from comment #16)
> Honza: can you link to the relevant part of the spec?  Or is this not in the
> spec?

http://dev.w3.org/html5/webstorage/

--- BEGIN OF CITE ---
4.3.1 Security

  User agents must throw a SecurityError exception whenever any of the members of a Storage object originally returned by the localStorage attribute are accessed by scripts whose effective script origin is not the same as the origin of the Document of the Window object on which the localStorage attribute was accessed.

  This means Storage objects are neutered when the document.domain attribute is used.
--- END OF CITE ---


I will need help to translate the last sentence...

If it is not a typo and I correctly understand, then the correct check should be:
subject.codeBase == storage.codeBase || subject.domain == storage.codeBase

Les cryptic: 
subject.effectiveOrigin == storage.origin

Changes on the document.domain of the document whom localStorage a script tries to access should not be taken into effect.  However, that doesn't make much sense to me.
Comment 18 Honza Bambas (:mayhemer) 2012-06-13 16:04:46 PDT
I broke my rule to not refresh bug pages!  Sorry for reverting the tracking flags, but I don't have the right to put them back...
Comment 19 Boris Zbarsky [:bz] 2012-06-13 16:42:28 PDT
Comment on attachment 632897 [details] [diff] [review]
v1

OK.  So the spec does say what comment 17 says it does, yeah.  That seems pretty broken to me.  Honza, could you test whether this is what other implementations actually do?

Specifically, per the spec given a page at subdomain.foo.com that sets document.domain to foo.com and a page at foo.com, per spec the former should be able to access the localStorage of the latter (but it can only get to it, of course, if the latter also sets document.domain to foo.com).

But given a page at subdomain.foo.com that sets document.domain to foo.com and a page a othersubdomain.foo.com that sets document.domain to foo.com, per spec neither one should be able to access the other's localStorage (nor indeed their own localStorage).

The attached patch doesn't do that, I believe: it would allow access across the two pages in the second example.

I think that's OK as a temporary measure, honestly, and would be pretty safe to ship; I don't expect it to cause any security problems that don't already exist once you've set document.domain to the same value in our existing impl (where we would allow a page to touch its own localStorage after it sets document.domain).

Now _that_ itself might be a security issue, which is why the spec disallows that.  But this patch doesn't make it worse.
Comment 20 Alex Keybl [:akeybl] 2012-06-13 16:52:00 PDT
Pre-approving for Aurora/Beta since we are considering this patch for 13.0.1.
Comment 21 Honza Bambas (:mayhemer) 2012-06-13 17:16:30 PDT
(In reply to Boris Zbarsky (:bz) from comment #19)
> Comment on attachment 632897 [details] [diff] [review]
> v1
> 
> OK.  So the spec does say what comment 17 says it does, yeah.  That seems
> pretty broken to me.  Honza, could you test whether this is what other
> implementations actually do?

Doesn't this actually mean that a page that changes its document.domain would never more be able to access its own local/sessionStorage?  I've already tested that this works in all current browsers, so it is against this spec.

> 
> Specifically, per the spec given a page at subdomain.foo.com that sets
> document.domain to foo.com and a page at foo.com, per spec the former should
> be able to access the localStorage of the latter (but it can only get to it,
> of course, if the latter also sets document.domain to foo.com).
> 
> But given a page at subdomain.foo.com that sets document.domain to foo.com
> and a page a othersubdomain.foo.com that sets document.domain to foo.com,
> per spec neither one should be able to access the other's localStorage (nor
> indeed their own localStorage).

This seems controversial and actually disallows any cross-origin access what I don't observe in Fx or other browser.
Comment 22 Hector Zhao [:hectorz] 2012-06-13 17:22:27 PDT
show.qq.com loads all content(iframes) / scripts from imgcache.qq.com, with document.domain = 'qq.com' set on both sides, as I understand.
Comment 23 Gary Kwong [:gkw] [:nth10sd] 2012-06-13 17:28:57 PDT
(In reply to Alex Keybl [:akeybl] from comment #20)
> Pre-approving for Aurora/Beta since we are considering this patch for 13.0.1.

So there is a possibility of a firedrill! Nominating for tracking-firefox13 as well.
Comment 24 Gary Kwong [:gkw] [:nth10sd] 2012-06-13 17:30:38 PDT
Increasing severity since this breaks a major site quite badly.
Comment 25 Gary Kwong [:gkw] [:nth10sd] 2012-06-13 18:03:30 PDT
Helping move this along by setting checkin-needed for mozilla-inbound since it has r+, but not sure if it's been try'ed.
Comment 27 Honza Bambas (:mayhemer) 2012-06-13 18:06:42 PDT
(In reply to Gary Kwong [:gkw, :nth10sd] from comment #25)
> not sure if it's been try'ed.

It wasn't, just locally.
Comment 28 Boris Zbarsky [:bz] 2012-06-13 18:57:51 PDT
> Doesn't this actually mean that a page that changes its document.domain would never more
> be able to access its own local/sessionStorage?

Per spec, yes, unless it changes it to the original value of its domain.

> I've already tested that this works in all current browsers

Then the spec probably needs to change.

> show.qq.com loads all content(iframes) / scripts from imgcache.qq.com, with
> document.domain = 'qq.com' set on both sides, as I understand.

Is it trying to access the qq.com localStorage, or the imgcache.qq.com one?
Comment 29 Hector Zhao [:hectorz] 2012-06-13 19:22:53 PDT
Created attachment 632994 [details]
Reduced version of js used in show.qq.com

(In reply to Boris Zbarsky (:bz) from comment #28)
> > show.qq.com loads all content(iframes) / scripts from imgcache.qq.com, with
> > document.domain = 'qq.com' set on both sides, as I understand.
> 
> Is it trying to access the qq.com localStorage, or the imgcache.qq.com one?

It is trying to access the localStorage of top window, I think.

Attached is a reduced version of http://imgcache.qq.com/ac/qqshow/qsfl/2_54/core.js, which is referenced in both top show.qq.com and framed imgcache.qq.com pages.
Comment 30 Gary Kwong [:gkw] [:nth10sd] 2012-06-13 20:16:12 PDT
> https://hg.mozilla.org/mozilla-central/rev/b30e903d23a0

tbpl is largely green! \o/

https://tbpl.mozilla.org/?rev=b30e903d23a0
Comment 31 Alex Keybl [:akeybl] 2012-06-14 08:11:13 PDT
It appears that http://show.qq.com/ has worked around this issue, so we do not plan to take a fix for FF13. Any other sites that run into this issue could do the same, but we're not aware of any at this point.
Comment 32 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-18 11:29:02 PDT
If this is something we want to get out sooner to users, let's get the beta landing in today before tomorrow's go to build.
Comment 33 Honza Bambas (:mayhemer) 2012-06-19 08:56:09 PDT
(In reply to Lukas Blakk [:lsblakk] from comment #32)
> If this is something we want to get out sooner to users, let's get the beta
> landing in today before tomorrow's go to build.

It has already landed on beta, see comment 26.
Comment 34 Honza Bambas (:mayhemer) 2012-06-19 11:00:03 PDT
*** Bug 765497 has been marked as a duplicate of this bug. ***
Comment 35 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-20 16:24:44 PDT
Updating the status flags to remove confusion about what has landed where.
Comment 36 Ioana (away) 2012-08-07 06:26:11 PDT
Verified as fixed with the STR from comment 0 on:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:15.0) Gecko/20100101 Firefox/15.0
Mozilla/5.0 (Windows NT 6.1; rv:15.0) Gecko/20100101 Firefox/15.0
Build identifier: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20100101 Firefox/15.0

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