window.document needs to be [Unforgeable]

VERIFIED FIXED in Firefox 33, Firefox OS v2.0

Status

()

Core
XPConnect
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: karthik.bhargavan, Assigned: bholley)

Tracking

({sec-moderate})

22 Branch
mozilla32
x86
Mac OS X
sec-moderate
Points:
---

Firefox Tracking Flags

(firefox33 verified, firefox34 verified, firefox-esr31 wontfix, b2g-v1.4 wontfix, b2g-v2.0 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 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.
(Reporter)

Comment 1

5 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.
(Reporter)

Comment 2

5 years ago
Other POC variants:
<script>
Object.defineProperty(this,"location",{href:"http://xxx.com"});
Object.defineProperty(window,"document",{location:{href:"http://xxx.com"}})
</script>
Component: Untriaged → XPConnect
Product: Firefox → Core
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.
(Reporter)

Comment 4

5 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.
> 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?
(Reporter)

Comment 7

5 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
Flags: needinfo?(karthik.bhargavan)
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.
(Reporter)

Comment 10

5 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.
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...
(Assignee)

Comment 12

5 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.
Keywords: sec-moderate
Summary: Object.defineProperty can (still) shadow window.location → window.document needs to be [Unforgeable]
(Assignee)

Comment 13

5 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?
(Assignee)

Updated

5 years ago
Assignee: nobody → bobbyholley+bmo
> 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.
(Assignee)

Comment 15

5 years ago
Created attachment 772142 [details] [diff] [review]
Make 'document' in nsIDocument::WrapObject. v1
Attachment #772142 - Flags: review?(bzbarsky)
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_?).
Attachment #772142 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 20

5 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.
> because |obj| is outerized by the time it hits the propertyOp

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

Comment 22

5 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
(Assignee)

Comment 23

5 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?
Flags: needinfo?(bzbarsky)
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...
Flags: needinfo?(bzbarsky)
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 25

4 years ago
This was fixed by Window WebIDL. \o/
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 789261
"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.
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → fixed
status-firefox33: --- → fixed
status-firefox34: --- → fixed
status-firefox-esr31: --- → affected
Depends on: 789261
Resolution: DUPLICATE → FIXED
Target Milestone: --- → mozilla32
Is this going to be wontfix for esr31/b2g30?
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 28

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

Yes.
Flags: needinfo?(bobbyholley)
status-b2g-v1.4: affected → wontfix
status-firefox-esr31: affected → wontfix
Confirmed bug in Fx33 build from 2014-04-29.
Verified fixed in Fx33 release build and Fx34 RC.
Status: RESOLVED → VERIFIED
status-firefox33: fixed → verified
status-firefox34: fixed → verified

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.