Closed Bug 762409 Opened 12 years ago Closed 12 years ago

localStorage throws "The operation is insecure." error with document.domain

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Version:
13 Branch
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox13 + wontfix
firefox14 + fixed
firefox15 + verified
firefox16 + fixed

People

(Reporter: luolonghao, Assigned: mayhemer)

References

Details

(Keywords: china-p1)

Attachments

(4 files)

2012-02-23 nightly screenshot of show.qq.com
758.49 KB, image/png
Details
2012-02-24 nightly screenshot of show.qq.com
156.03 KB, image/png
Details
v1
1.06 KB, patch
bzbarsky
: review+
akeybl
: approval-mozilla-aurora+
akeybl
: approval-mozilla-beta+
mayhemer
: checkin+
Details | Diff | Splinter Review
Reduced version of js used in show.qq.com
1.62 KB, application/x-javascript
Details
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.
Component: Untriaged → DOM
Product: Firefox → Core
QA Contact: untriaged → general
I believe per spec the storage stuff completely ignores document.domain, so you're trying to setItem across domains...
(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.
Status: UNCONFIRMED → RESOLVED
Closed: 12 years ago
Resolution: --- → INVALID
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
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
> 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?
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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
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.
I checked Chromium version 21.0.1173.0 (Developer Build 141642) and Safari 5.1.6 and both seem to render the site properly.
Blocks: 495337
OS: Windows XP → All
Hardware: x86 → All
Nominating tracking flags for all known versions - show.qq.com is essentially unusable with this bug.
tracking-firefox13: --- → ?
tracking-firefox14: --- → ?
tracking-firefox15: --- → ?
tracking-firefox16: --- → ?
Fails to work in 2012-02-24 nightly. I don't think I'd expect this to be firedrilled for Firefox 13 though..
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...
I'll take a look today.
Assignee: nobody → honzab.moz
Attached patch v1 — — Splinter Review
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.
Attachment #632897 - Flags: review?(bzbarsky)
Honza: can you link to the relevant part of the spec? Or is this not in the spec?
(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.
tracking-firefox14: + → ?
tracking-firefox15: + → ?
tracking-firefox16: + → ?
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...
Status: NEW → ASSIGNED
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.
Attachment #632897 - Flags: review?(bzbarsky) → review+
Attachment #632897 - Flags: approval-mozilla-beta+
Attachment #632897 - Flags: approval-mozilla-aurora+
Pre-approving for Aurora/Beta since we are considering this patch for 13.0.1.
(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.
show.qq.com loads all content(iframes) / scripts from imgcache.qq.com, with document.domain = 'qq.com' set on both sides, as I understand.
(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.
tracking-firefox13: --- → ?
Increasing severity since this breaks a major site quite badly.
Severity: normal → major
Helping move this along by setting checkin-needed for mozilla-inbound since it has r+, but not sure if it's been try'ed.
Keywords: checkin-needed
(In reply to Gary Kwong [:gkw, :nth10sd] from comment #25) > not sure if it's been try'ed. It wasn't, just locally.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago → 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
> 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?
(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.
Flags: in-testsuite?
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.
status-firefox13: affected → wontfix
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.
(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.
Updating the status flags to remove confusion about what has landed where.
status-firefox14: affected → fixed
status-firefox15: affected → fixed
status-firefox16: affected → fixed
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
status-firefox15: fixed → verified
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: