User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.116 Safari/537.36 Steps to reproduce: Is this yet another variant of bug 756719 - (CVE-2012-1956)? Although window.location is marked as non-configurable/non-writable in Firefox 22, the global window object itself is configurable. So, non-configurable window.* objects can be shadowed by Object.defineProperty, amplifying XSS attacks and enabling origin-spoofing. POC: The following works in FF 22 <script> Object.defineProperty(this,"window",{value:{location:{href:"http://xxx.com"}}}) alert(window.location.href); </script> Similar attacks as bug 756719 apply: fooling plugins/loaded scripts into thinking they are on the wrong origin; we found this attack when trying to fix a CSRF middleware solution.

I should clarify that the global "location" object is not shadowed. It is only when scripts rely "window.location" that the problem appears. Actually, we first tried to use document.domain (and then document.location, but it turned out that window.document was configurable too, and so document.domain could be shadowed, and so could the supposedly read-only document.location.

Other POC variants: <script> Object.defineProperty(this,"location",{href:"http://xxx.com"}); Object.defineProperty(window,"document",{location:{href:"http://xxx.com"}}) </script>

This might be more of a DOM bug... In any case, we should resolve this as a permanent readonly prop on the window; the "window" property on Window is [Unforgeable] in the spec.

Making "this.window" readonly sounds like a good idea. More generally, in my tests, FF is the only browser where (this !== window), that is the global object is not the same as the window object. Is this design deliberate? This makes such tampering possible in a variety of ways, including defineProperty and: --- function window(){} window.location = {href:"http://xxx.com"} --- Another impact of (this !== window) for web applications is that if they want to place a non-configurable non-writable property in the global space, they should do it on "this" rather than on "window" even though both would have the same impact. E.g. try ---- Object.defineProperty(window,"foo",{value:1, configurable: false, writable: false}); alert(foo); Object.defineProperty(this,"foo",{value:2}); alert(foo); ---- A seemingly non-configurable non-writable property in the global namespace has been overwritten. This behavior (only in FF as far as I can test) can be super-confusing for a developer, and likely lead to security bugs in cross-domain scenarios.

> More generally, in my tests, FF is the only browser where (this !== window), Er... it should be normally! The "function window" or "var window" case is an interesting one: does that call resolve hooks?

Hm, I can't reproduce these issues at all, either in Nightly or Release. Here are live versions of the PoCs: PoC 1 (throws): http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cscript%3E%0AObject.defineProperty%28this%2C%22window%22%2C{value%3A{location%3A{href%3A%22http%3A%2F%2Fxxx.com%22}}}%29%0Aalert%28window.location.href%29%3B%0A%3C%2Fscript%3E%0A PoC 2 (throws): http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cscript%3E%0AObject.defineProperty%28this%2C%22location%22%2C{href%3A%22http%3A%2F%2Fxxx.com%22}%29%3B%0AObject.defineProperty%28window%2C%22document%22%2C{location%3A{href%3A%22http%3A%2F%2Fxxx.com%22}}%29%0A%3C%2Fscript%3E this === window (alerts 'true'): http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cscript%3E%0Aalert%28this%20%3D%3D%3D%20window%29%0A%3C%2Fscript%3E%0A Karthik, do these URLs do something different for you? If not, what's different about the testcases you're using? Are you using the web console or something? If so, that's not an accurate depiction of security behavior. :-)

Yikes, this is embarrassing. I tested two of these on the web and the rest by Web Console. So, here is what works, maybe you can confirm. - Object.defineProperty(document,"domain",{value:"xxx.com"}); - Object.defineProperty(window,"document",{value:{location:"http://xxx.com"}}); The latter is surprising because window.document.location is set as non-configurable, but the whole document object can in fact be shadowed. I apologize for any claims of (this !== window); the Web Console made me do it! I need to run more tests for window.location, but as of now, I don't know of ways to shadow that, only window document

Demo: http://prosecco.gforge.inria.fr/shadowing.html

Yes, window.document doesn't seem to be a permanent prop. See nsIDocument::WrapObject which just uses JSPROP_READONLY | JSPROP_ENUMERATE. We really need to finish converting Window to WebIDL... document.domain is not unforgeable in the spec. It's not clear to me what the win is from making it unforgeable, though.

I don't know if it counts as a win, but we stumbled upon this document.domain behaviour when analyzing OWASP CSRFGuard 3's token injection script, which relies upon document.domain to protect against the leak of CSRF tokens to malicious websites. We were able to bypass this check and steal tokens from any website that uses this feature of OWASP CSRFGuard. For their code, see: https://github.com/esheri3/OWASP-CSRFGuard/blob/master/Owasp.CsrfGuard/conf/Owasp.CsrfGuard.js Yes, the design is a bit fragile, and maybe they shouldn't be embedding secrets in scripts on publicly accessible URLs, and they have other problems in their code, but I am guessing that this is not the only security-critical application that relies on the supposedly up-domain-only nature of document.domain or the unforgeable nature of document.location.

document.location needs to be unforgeable, for sure. Is document.domain unforgeable in any other UA? If we think it should be unforgeable, that should be escalated to a spec issue, but other UAs would need to be convinced too...

(In reply to Boris Zbarsky (:bz) (reading mail, but on paternity leave) from comment #11) > document.location needs to be unforgeable, for sure. Ok. Morphing the bug, since this appears to be the only real security issue here. Do we know of any plugins using document.location? I'm going to go with sec-moderate for now, but feel free to bump if I'm wrong.

(In reply to Boris Zbarsky (:bz) (reading mail, but on paternity leave) from comment #9) > Yes, window.document doesn't seem to be a permanent prop. See > nsIDocument::WrapObject which just uses JSPROP_READONLY | JSPROP_ENUMERATE. That's actually only for Xrays. In the old world, we depended on nsDocumentSH::PostCreate to define it on the window (though spelunking indicates that we lacked JSPROP_PERMANENT there, too), but that's gone now. So we actually hit the NewResolve hook _every time_ someone does |document|. Yikes. That might be performance-sensitive, yeah?

> That's actually only for Xrays. You're thinking about the JS_DefineProperty in nsWindowSH::NewResolve, I think, but I'm talking about nsIDocument::WrapObject. > In the old world, we depended on nsDocumentSH::PostCreate to define it on the window And now we depend on nsIDocument::WrapObject to do it. And yes, it just copied the pre-existing nsDocumentSH::PostCreate code, which lacked JSPROP_PERMANENT.

Comment on attachment 772142 [details] [diff] [review] Make 'document' in nsIDocument::WrapObject. v1 r=me if you add the missing bits in the commit comment (make 'document' _what_?).

Backed out for widespread mochitest orange. https://hg.mozilla.org/integration/mozilla-inbound/rev/22636554e79b

Oh, darn. This is breaks with PropertyOps, because |obj| is outerized by the time it hits the propertyOp, meaning that if the window has navigated, we'll get the wrong inner (and therefore the wrong document). This means that |document| is broken in any navigated-away-from scope. I'm going to try a JSNative with a reserved slot pointing to the inner, which is probably closer to what we'll have with WebIDL anyway.

> because |obj| is outerized by the time it hits the propertyOp Er, is that true for a JSNative getter/setter too???

Looks like try was reset. Repushing to get access to the failures from comment 19: https://tbpl.mozilla.org/?tree=Try&rev=6b9e7f4c4d03

Ugh. So removing the code that clears mDoc in FreeInnerObjects made the old failures go away, but created new ones. In particular, we seem to have various tests with fragile dependencies on the document becoming inaccessible at certain moments, and events and whatnot not being dispatched. I'm not really sure where to go from here, other than comprehending and fixing the tests one by one, which is a huge timesuck. :-( What do you think, Boris? Are we going to need something like this eventually anyway? Is it worth pouring time into, or should we find some simpler interim solution?

I really don't know whether we'll ever _need_ to not drop this reference, but I'm having a hard time figuring out how a per-spec WebIDL .document would work with us dropping it...

This was fixed by Window WebIDL. \o/

"fixed by" is not the same as "duplicate", and this bug is nothing like the "Window WebIDL" one. This is especially important for tracking when one is a security bug and the other isn't.

Is this going to be wontfix for esr31/b2g30?

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #27) > Is this going to be wontfix for esr31/b2g30? Yes.

Confirmed bug in Fx33 build from 2014-04-29. Verified fixed in Fx33 release build and Fx34 RC.