Consider this testcase: var count = 1000000; for (var i = 0; i < count; ++i) { document; } I've been investigating what gets passed as the thisv() to the JSNative getter for the document here. When we're called from the interpreter, js::Shape::get calls js::InvokeGetterOrSetter which calls js::Invoke which outerizes the this object. So in this case we get the outer window proxy as thisv(). When we baseline-compile this code we seem to end up calling js::jit::DoGetNameFallback which ends up taking the same codepath as the interpreter. When we ion-compile this code, though, it looks like we're calling the getter directly from ioncode and it's no longer outerizing the this value in that case. The inconsistency is pretty suboptimal, because it means depending on the jit compilation level you have you might or might not get a security exception on the bareword acess. Furthermore, I would like us to generally move to the ion version here for JSNative getters, or at least for the DOM's window bindings: while we need to outerize the this for scripted getters, for JSNative ones that's just dataloss in that we can no longer reliably tell which actual global we're working with. In particular, inner windows are in a many-to-one relationship with outer ones, so it's always possible to go from an inner to an outer but the reverse is not necessarily true unless one is really careful. In fact, I think it's impossible to find the right C++ object in general in the bindings code if the outer is passed as thisv, which means that we end up having to assume that we should forward to the current inner, which can be a different object from the actual global the script is running on. That's not to mention the performance issues involved with the outerObject hook and then unwrapping the extra layers of wrappers in the binding code. ;)

Of course the particular JSNative getters bz's talking about are well-behaved. They will not expose thisv to script without outerizing. We have JSNatives that are not so careful, though. Array.prototype.pop, for example: array_pop js::GetLengthProperty(cx, obj, lengthp) JSObject::getProperty(cx, obj, receiver, id, vp) JSObject::getGeneric(cx, obj, receiver, id, vp) js::baseops::GetProperty(cx, obj, receiver, id, vp) GetPropertyHelperInline<CanGC>(cx, obj, receiver, id, vp) NativeGetInline<CanGC>(cx, obj, receiver, pobj, shape, vp) js::Shape::get(cx, receiver, obj, pobj, vp) js::InvokeGetterOrSetter(cx, obj, fval, argc, argv, rval) js::Invoke(cx, thisv, fval, argc, argv, rval) I don't see outerize happening anywhere. So I'm not sure why this test doesn't fail (I tried, it doesn't): Object.defineProperty(window, "length", { get: function() { if (this !== window) alert("FAIL"); return 0; }, set: function () {/*ignore*/} }); Object.defineProperty(window, "x", {get: [].pop}); var count = 1000000; for (var i = 0; i < count; ++i) { x; } I find I'm less certain about where the outerization boundaries are supposedly drawn than I'd like to be.

Let's pick semantics first and then talk about optimization. bz and I agree it'd be nice if bare `document` always returned the document for the Window, not the WindowProxy. I think we also agree that *other* user-defined getters on the Window should always receive the WindowProxy, never the Window, as the this value. Look how weird that is. The only ways I see to implement this are: - have a magic bit on the property - have a magic bit on the getter function - make `document`, and any others we care about, data properties on the Window.

> I don't see outerize happening anywhere. js::Invoke outerizes, no?

I agree that it's weird. Making document a data property could work, if the WindowProxy denied all attempts to reconfigure it (because I think there is no way to directly reconfigure on the Window), and we got the relevant specs changed and got other UAs to go along (less of an issue for Blink/WebKit, where it's already a fake data property). It doesn't help that the spec for WindowProxy is basically nonexistent. It's not immediately obvious to me that "document" is the only issue, or even that this is only restricted to getters. For example, bareword setTimeout() in a navigated-away-from page seems to work now that we're using WebIDL bindings (setting a timeout on the currently-loaded page!), whereas before for some reason it didn't.... and imo it shouldn't.

http://web.mit.edu/bzbarsky/www/testcases/windowproxy/no-longer-current-setTimeout.html is the testcase I'm using, btw, and I get weird results. Firefox 25 says FAIL, but a 2013-11-05 nightly says PASS and a 2013-11-06 nightly says FAIL.

Er, I guess for setTimeout the thisv is actually undefined, and the WebIDL code then gets the callee's global, which is the inner the call happened in (as opposed to the old XPConnect code, which probably did JS_ComputeThis and got outerized). But then nsGlobalWindow::InnerForSetTimeoutOrInterval becomes a no-op; we should be checking there whether we're the current inner. I'll file that separately. In any case, that gives us a somewhat bizarre difference between methods and getters/setters on the C++ level. In script, of course, JSOP_THIS outerizes.

I agree that we should not outerize |this| for non-scripted getters, setters, and methods invoked with bareword access. I don't know enough about our outerization strategy to make suggestions of how to arrange the hooks in order to achieve the semantics that we want.

It seems kind of poor to me to have foo and window.foo behave differently (or additionally, as jorendorff notes, getter.call(undefined) be different from them). Yeah, yeah, that's how it works now, in some cases. I don't think that's a good thing, or something that necessarily must be so.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8) > It seems kind of poor to me to have foo and window.foo behave differently > (or additionally, as jorendorff notes, getter.call(undefined) be different > from them). Yeah, yeah, that's how it works now, in some cases. I don't > think that's a good thing, or something that necessarily must be so. Given the semantics of Window, I think having different behavior as described above is by far the most sane and performant option. Do you have a counter-proposal?

> It seems kind of poor to me to have foo and window.foo behave differently Consider the "var foo" case. Would you want them to behave the same in that case too? I suspect that causes unacceptable performance issues...

(In reply to Boris Zbarsky [:bz] from comment #3) > > I don't see outerize happening anywhere. > > js::Invoke outerizes, no? Yes, via the thisObject hook. OK, how about Object.prototype.valueOf? It just returns ToObject(thisv); no wrapping happens in it; so if Ion passes it the inner window, I don't see why it wouldn't return the inner window to script. This didn't fail either, though. Object.defineProperty(window, "x", { get: {}.valueOf, enumerable: true }); var count = 1000000; for (var i = 0; i < count; ++i) { if (this !== x) { alert("FAIL " + i); break; } }

Oh, this is bad. The testcase in comment 11 doesn't seem to ever Ion NameIC the "x" (I'm looking into why, exactly). But this testcase shows that you can in fact get your hands on the inner using this general method: Object.defineProperty(window, "x", { get: {}.valueOf, enumerable: true }); var count = 100000; var w = (function() { var z; for (var i = 0; i < count; ++i) { z = x; } return z; })(); alert((w == this) + " | " + w + " | " + this)

I've stepped through doing alert(w) in that testcase in a debugger, by the way, and confirmed that w is an inner window object.

Before someone forgets to check the private button on some comment, I think we should just close this up. :(

Fwiw: [Abort] JSOP_THIS outside of a JSFunction. And indeed, this testcase: Object.defineProperty(window, "x", { get: {}.valueOf, enumerable: true }); (function() { var count = 1000000; for (var i = 0; i < count; ++i) { if (this !== x) { alert("FAIL " + i); break; } } })() in fact alerts "FAIL 1098" for me.

Question - in the modern world, is there actually a security problem when script gets its hands on the inner? In the old world that would allow code to bypass security checks, but post-compartments I don't think that's true anymore. We still shouldn't permit it, of course, but it may be less dire than we think.

So maybe what we should do for now is get consistency by either disabling the NameIC when the thisobj has an outerObject hook or making sure to outerize in the NameIC code when there is such a hook and file a separate bug for rejiggering the behavior for DOM bindings, since we'll want to make that change to interp and baseline as well?

> is there actually a security problem when script gets its hands on the inner? I don't know. :(

(In reply to Boris Zbarsky [:bz] from comment #18) > > is there actually a security problem when script gets its hands on the inner? > > I don't know. :( Ok, let's see if mrbkap can think of anything. We still need to fix this either way though, for the sake of web semantics.

> (In reply to Boris Zbarsky [:bz] from comment #18) > > > is there actually a security problem when script gets its hands on the inner? My recollection of the history here is that when split windows were originally implemented, we intended to remove the security check from the inner window, putting the burden of responsibility solely on the outer window. From that point of view, there would have been a security hole if there was a way of taking an outer window and getting its associated inner, as that would provide un-security-checked access to a cross-origin inner. If the bug here only deals with same-compartment inner/outer windows, then I don't think there's a security bug. > We still need to fix this either way though, for the sake of web semantics. This is definitely still the case, though.

Is there some reason you all are hiding all the comments even though this bug is closed now?

(In reply to Andrew McCreight [:mccr8] from comment #21) > Is there some reason you all are hiding all the comments even though this > bug is closed now? Probably because replying to a hidden comment creates another hidden comment. Unhiding everything.

(In reply to Bobby Holley (:bholley) from comment #16) > Question - in the modern world, is there actually a security problem when > script gets its hands on the inner? In the old world that would allow code > to bypass security checks, but post-compartments I don't think that's true > anymore. We still shouldn't permit it, of course, but it may be less dire > than we think. Hopefully Bobby is right, but if it's still a problem I bet moz_bug_r_a4 can find it.

> is there actually a security problem when script gets its hands on the inner? Please see bug 940085.

I really should know better than to say stuff like comment 20 before moz_bug_r_a4 has a chance to comment...

Boris, do you know who can work on this? Thanks.

The obvious choices are jandem or efaust or some other jit hacker. I could try, if push comes to shove...

Disabling the IC is not desirable in general, by the way, because it makes things like "performance" slow. Maybe the right short-term hack is to disable it for native getters without jitinfo.

Comment on attachment 8335475 [details] [diff] [review] hackery Jason, how much does this make you cry? Enough to try to look for something more complex (like outerizing in the ic)?

Comment on attachment 8335475 [details] [diff] [review] hackery Review of attachment 8335475 [details] [diff] [review]: ----------------------------------------------------------------- Well, I think we all agree that this is pretty much how it has to go down. (I can't say whether it's sufficient. We need an Ion reviewer, at least.) However, whatever we do must behave the same across interpreter/baseline/ion. It's user-visible which window object is passed to the getter, in the case that we've navigated, and user-visible behavior can't depend on JIT modes. And of course this needs tests. So clearing the r? flag on this patch.

I manually verified the test passes if I nix the hackery that makes document a value property. Not including a test for the actual security bug, since I don't want to check that in yet and expose the bug

Comment on attachment 8335652 [details] [diff] [review] Be consistent about the thisobj we pass to getters. Review of attachment 8335652 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Interpreter.cpp @@ +505,4 @@ > */ > + if (!fval.isObject() || !fval.toObject().is<JSFunction>() || > + !fval.toObject().as<JSFunction>().isNative() || > + !fval.toObject().as<JSFunction>().jitInfo()) { Style nit: In SM house style, the '{' goes on its own outdented line: if (!... !... !... { ... }

Comment on attachment 8338062 [details] [diff] [review] Updated to review comment [Security approval request comment] How easily could an exploit be constructed based on the patch? Unclear. The patch sort of demonstrates a technique for passing an inner window to a WebIDL getter, but then people would need to figure out what to do with it. I couldn't figure out one that was actually problematic, fwiw, which is why the test is a no-op on current trunk. I carefully avoided using any of the actually-vulnerable getters in the patch. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Unclear. The test might at least be suggestive. Which older supported branches are affected by this flaw? Everything since Ion landed, I believe. So at least back to Firefox 18. If not all supported branches, which bug introduced the flaw? IonMonkey landing. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I suspect backports should be fairly straightforward. How likely is this patch to cause regressions; how much testing does it need? I don't expect this to be very regression-prone.

Comment on attachment 8338062 [details] [diff] [review] Updated to review comment Review of attachment 8338062 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonCaches.cpp @@ +617,5 @@ > + JSFunction& getter = shape->getterValue().toObject().as<JSFunction>(); > + if (!getter.isNative()) > + return false; > + > + if (getter.jitInfo()) I think it's pretty non-obvious to most SM devs that fun.jitinfo() iff DOM. I know there's a comment below, but can we put something here?

I can give this sec-approval+ but I want to know if release management will take this on beta this cycle or not.

Added // Check for a DOM method; those are OK with both inner and outer objects. before that jitinfo check.

(In reply to Boris Zbarsky [:bz] from comment #38) > Added > > // Check for a DOM method; those are OK with both inner and outer > objects. > > before that jitinfo check. Thanks. I appreciate it.

I just realized we have jitinfo for non-DOM methods (e.g. parallel js stuff). Eric, how do you feel about a boolean flag in jitinfo for this behavior?

Is it not sufficient to use the JSJitInfo::OpType here? All the non-dom things have OpTypeNone, I'm pretty sure, since it doesn't make any sense for them to use any other designation, so this should be enough to tell. Perhaps JSJitInfo::isDOMJitInfo() should be added? As it is, I would rather they have not stashed a random pointer in a random struct hanging around and called it a day, but it seems that decision was already made.

> Is it not sufficient to use the JSJitInfo::OpType here? Hmm. Yes, it is, with a comment about implications of the OpType. I'll hide it behind isDOMJitInfo(). I do think eventually we want to use jitinfo as a general communication mechanism of metadata about methods to the jit, so the parallel JS use doesn't seem unreasonable.

Unless this is something we would respin for, and at sec-high it doesn't appear to be, we'll have to wait for FF27 on this one.

Comment on attachment 8341385 [details] [diff] [review] With the better op type check sec-approval+ for checkin on trunk on 12/23 or later. We should get branch patches at that time as well.

Al, I probably won't be able to land this until Jan 2 or so in practice. Is that OK?

My plan is to do a mid-vacation landing of at least DOM sec bugs with patches.

Boris, that's fine though if Andrew could land earlier, that would be better.

Great. The attached patch applies fine to aurora. I'll post a patch that applies to beta.

Comment on attachment 8345551 [details] [diff] [review] Patch for beta 27 [Approval Request Comment] Bug caused by (feature/regressing bug #): Been a problem for a while, probably since initial ion landing. User impact if declined: Possible to bypass some security checks. Testing completed (on m-c, etc.): Passes tests, last I tried. Risk to taking this patch (and alternatives if risky): I think risk here is fairly low. String or IDL/UUID changes made by this patch: None.

I believe b2g18 is actually unaffected, since it doesn't use ion.

Comment on attachment 8341385 [details] [diff] [review] With the better op type check Review of attachment 8341385 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonCaches.cpp @@ +618,5 @@ > + if (!getter.isNative()) > + return false; > + > + // Check for a DOM method; those are OK with both inner and outer objects. > + if (getter.jitInfo()) This just landed, but shouldn't it also have the isDOMJitInfo() check? Did we miss a spot?

needinfo bz for above comment.

Yet. That test should be checking for isDOMJitInfo() as well. Good catch.

However in practice, I expect we have no parallel native getters. So for branches this is probably ok; for trunk we should add the check to be future-safe.

https://hg.mozilla.org/releases/mozilla-aurora/rev/477aec9bca87 https://hg.mozilla.org/releases/mozilla-beta/rev/0414fb5b7ceb https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/fb1a91c262ea The esr24 patch was busted due to not having OpType_None (looks like it was added by bug 881988 for Fx25). Also, this still needs a follow-up for m-c, right?

I'll upload a new esr24 patch; all jitinfos are DOM there. I'll also post the m-c followup.

Backed out from beta for a lot of orange. https://hg.mozilla.org/releases/mozilla-beta/rev/e682394a24a8

The reason for the orange is that the fix for bug 951245 actually depends on bug 932309, which hasn't landed on beta 27. Investigating landing it there.

Comment on attachment 8355583 [details] [diff] [review] Followup fix for m-c Review of attachment 8355583 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for fixing this. r=me

OK, talked to Peter and we're just going to disable the NameIC for bareword getters on Window in 27, because we don't have a working IsActiveDocument check for inners there. That's the safest change at this point, and while it regresses the performance some, it only goes to about Firefox 22 levels, which should be bearable for one release.

Confirmed issue in FF26 using sample code in comment 15. Verified fixed in FF27, FF28 and FF29, 2014-01-18. Verified fixed in ESR24, 2014-01-19.