karthik.bhargavan Reporter
	Description • 11 years ago

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.

	karthik.bhargavan Reporter
	Comment 1 • 11 years ago

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.

	karthik.bhargavan Reporter
	Comment 2 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 11 years ago

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.

	karthik.bhargavan Reporter
	Comment 4 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 11 years ago

> 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?

	Bobby Holley (:bholley) Assignee
	Comment 6 • 11 years ago

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. :-)

	karthik.bhargavan Reporter
	Comment 7 • 11 years ago

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

	karthik.bhargavan Reporter
	Comment 8 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 11 years ago

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.

	karthik.bhargavan Reporter
	Comment 10 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 11 years ago

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...

	Bobby Holley (:bholley) Assignee
	Comment 12 • 11 years ago

(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.

	Bobby Holley (:bholley) Assignee
	Comment 13 • 11 years ago

(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?

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 11 years ago

> 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.

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 11 years ago

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_?).

	Bobby Holley (:bholley) Assignee
	Comment 17 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/4903ca450b3f

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 11 years ago

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

	Bobby Holley (:bholley) Assignee
	Comment 19 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=9801641135f9

	Bobby Holley (:bholley) Assignee
	Comment 20 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 11 years ago

> because |obj| is outerized by the time it hits the propertyOp

Er, is that true for a JSNative getter/setter too???

	Bobby Holley (:bholley) Assignee
	Comment 22 • 11 years ago

Looks like try was reset. Repushing to get access to the failures from comment 19:

https://tbpl.mozilla.org/?tree=Try&rev=6b9e7f4c4d03

	Bobby Holley (:bholley) Assignee
	Comment 23 • 11 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 11 years ago

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...

	Bobby Holley (:bholley) Assignee
	Comment 25 • 10 years ago

This was fixed by Window WebIDL. \o/

	Daniel Veditz [:dveditz]
	Comment 26 • 10 years ago

"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.

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 10 years ago

Is this going to be wontfix for esr31/b2g30?

	Bobby Holley (:bholley) Assignee
	Comment 28 • 10 years ago

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

Yes.

	Matt Wobensmith [:mwobensmith][:matt:]
	Comment 29 • 10 years ago

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