Boris Zbarsky [:bzbarsky] Assignee
	Description • 11 years ago

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

	Jason Orendorff [:jorendorff]
	Comment 1 • 11 years ago

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.

	Jason Orendorff [:jorendorff]
	Comment 2 • 11 years ago

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.

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

> I don't see outerize happening anywhere.

js::Invoke outerizes, no?

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

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.

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

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.

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

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.

	Bobby Holley (:bholley)
	Comment 7 • 11 years ago

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.

	Jeff Walden [:Waldo]
	Comment 8 • 11 years ago

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.

	Bobby Holley (:bholley)
	Comment 9 • 11 years ago

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

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

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

	Jason Orendorff [:jorendorff]
	Comment 11 • 11 years ago

(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;
    }
}

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

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)

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

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.

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

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

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

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.

	Bobby Holley (:bholley)
	Comment 16 • 11 years ago

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.

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

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?

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

> is there actually a security problem when script gets its hands on the inner?

I don't know.  :(

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

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

	Blake Kaplan (:mrbkap) (inactive)
	Comment 20 • 11 years ago

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

	Andrew McCreight [:mccr8]
	Comment 21 • 11 years ago

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

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

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

	Daniel Veditz [:dveditz]
	Comment 23 • 11 years ago

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

	moz_bug_r_a4
	Comment 24 • 11 years ago

> is there actually a security problem when script gets its hands on the inner?

Please see bug 940085.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 25 • 11 years ago

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

	Andrew McCreight [:mccr8]
	Comment 26 • 11 years ago

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

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

The obvious choices are jandem or efaust or some other jit hacker.

I could try, if push comes to shove...

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

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.

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

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

	Jason Orendorff [:jorendorff]
	Comment 31 • 11 years ago

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.

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

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

	Jason Orendorff [:jorendorff]
	Comment 33 • 11 years ago

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 (!...
        !...
        !...
    {
        ...
    }

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

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.

	Eric Faust [:efaust]
	Comment 36 • 11 years ago

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?

	Al Billings [:abillings - ex-MoCo]
	Comment 37 • 11 years ago

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

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

Added 

    // Check for a DOM method; those are OK with both inner and outer objects.

before that jitinfo check.

	Eric Faust [:efaust]
	Comment 40 • 11 years ago

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

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

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?

	Eric Faust [:efaust]
	Comment 42 • 11 years ago

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.

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

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

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 45 • 11 years ago

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.

	Al Billings [:abillings - ex-MoCo]
	Comment 46 • 11 years ago

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.

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

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

	Andrew McCreight [:mccr8]
	Comment 48 • 11 years ago

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

	Al Billings [:abillings - ex-MoCo]
	Comment 49 • 11 years ago

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

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

Great.

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

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

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.

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

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

	Andrew McCreight [:mccr8]
	Comment 55 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/960513b266a6

	Eric Faust [:efaust]
	Comment 56 • 11 years ago

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?

	Eric Faust [:efaust]
	Comment 57 • 11 years ago

needinfo bz for above comment.

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

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

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

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.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 60 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/960513b266a6

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

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?

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

I'll upload a new esr24 patch; all jitinfos are DOM there.

I'll also post the m-c followup.

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

https://hg.mozilla.org/releases/mozilla-esr24/rev/c859c551877b

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

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

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

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.

	Eric Faust [:efaust]
	Comment 68 • 11 years ago

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

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/451513e0b68c

	Ed Morley [:emorley]
	Comment 70 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/451513e0b68c

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

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.

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

https://hg.mozilla.org/releases/mozilla-beta/rev/92703e5c84a0

	Matt Wobensmith [:mwobensmith][:matt:]
	Comment 74 • 11 years ago

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.