Jason Orendorff [:jorendorff] Reporter
	Description • 15 years ago

      No description provided.

	Jason Orendorff [:jorendorff] Reporter
	Comment 1 • 15 years ago

We've gone back and forth a dozen times now about whether this is a security hole. I believe it is. The ins_choose calls at the end of TR::getThis are no-ops if `this` is not the global object at record time. But we should be guarding against `this` later being the inner window (which can happen various ways -- trust us).

Blake is going to write a bunch of trace-tests for this.

	Jason Orendorff [:jorendorff] Reporter
	Comment 2 • 15 years ago

The fix might be to leave fp->thisp null until its first use, then call js_ComputeThis, instead of temporarily populating it with the global object and using JSFRAME_COMPUTED_THIS.

IIUC, JSOP_THIS would then be able to emit trivial code (just a stack load from fp->thisp) if thisp is not null at record time (because a TT_OBJECT stack slot will never be null at run time; we already guarded on that).

	Andreas Gal :gal
	Comment 3 • 15 years ago

#2 is pretty clever. I like it.

	Brendan Eich [:brendan]
	Comment 4 • 15 years ago

I <3 TT_NULL :-).

/be

	Julian Seward [:jseward]
	Comment 5 • 15 years ago

Kind of a red herring, but .. the do-nothing code that Jason spotted 
looks like this

    eq8 = eq ld4, inner
    cmov3 = cmov eq8 ? inner : ld4
    cmov4 = cmov eq8 ? inner : cmov3

Adding the following rewrite rules in ExprFilter::ins3

    (y == x) ? x : y  =>  y
    (x == y) ? x : y  =>  y

reduces the instruction count on v8-richards from 3499 million to
3227 million.  So it would be extremely cool (positively freezing)
if the fix to this bug also had the same effect.

	Julian Seward [:jseward]
	Comment 6 • 15 years ago

Note also, the numbers in comment #5 first require application of
the patch attached to bug 504587 comment #96.

	Jason Orendorff [:jorendorff] Reporter
	Comment 7 • 15 years ago

Blake and I had a long conversation about this.

1. Blake confirmed you can a reference to the inner object can escape this way. (Incidentally, the shell should have a way to check and see if you've got the inner object or not--Blake looked in the debugger.)

2. The plan in comment 2 would make us trace the loop twice, once with
thisp:TT_NULL and once with thisp:TT_OBJECT.

3. Blake's counteroffer was:
    this_ins = insChoose(getfslot(thisp_ins, PARENT),
                         thisp_ins,
                         INSCONST(wrapped outer window))
which is fine but adds a scary new invariant that the only parentless
object that can ever be "really exposed to script" is the parent object.

4. I think we have a better plan, about which more in a moment.

	Jason Orendorff [:jorendorff] Reporter
	Comment 8 • 15 years ago

The plan is: compute fp->thisp eagerly so the slot is always correct. JSOP_THIS should just do PUSH(fp->thisp).

I think we used to do that but it was slow. The JS_ComputeThis machinery is an attempt to optimize via laziness. Instead we should cache.

The pobj calculated by js_FindPropertyHelper (and js_FullTestPropertyCache) should be an object that can be used as thisp without any further fuss. That means unwrapping With objects, replacing Call and Block objects with the global, and outerizing split global objects. The trick is to make the property cache cover this work. If (innerWindow -> wrappedOuterWindow) is a fixed relation, we can cache the end-to-end result of the whole lookup, both pobj-as-ammended and sprop. It will require some hacking, maybe a new kind of JSPropCacheEntry or some out-of-line data.

JS_ComputeThis would go away, which would save us a function call on most fast natives. JIT would win, interpreter might win big, and it seems simpler to me overall than what we're doing.

	Andreas Gal :gal
	Comment 9 • 15 years ago

If we can cache it, we can burn it onto trace. That will be nice and fast.

	Brendan Eich [:brendan]
	Comment 10 • 15 years ago

Jason, great thinking -- JSFastNative and JS_THIS came in before the return of the property cache, and we didn't think to use it instead of the COMPUTED_THIS frame flag and laziness. You don't need any new propcache entry, just use an object-valued entry keyed by JSOP_THIS etc.

/be

	Julian Seward [:jseward]
	Comment 11 • 15 years ago

Comment on attachment 397303 [details] [diff] [review]
patch to remove silly cmov cases as per comment #5

patch is obsoleted by "patch v2" on bug 513407

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 12 • 15 years ago

I'll file a followup bug on making computing this be BLAZINGLY FAST.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 13 • 15 years ago

That would be bug 515496.

	Jason Orendorff [:jorendorff] Reporter
	Comment 14 • 15 years ago

Comment on attachment 399603 [details] [diff] [review]
Immediate fix for this bug

>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
In TraceRecorder::getThis:
>+    JSObject* wrappedGlobal = globalObj->thisObject(cx);
>+    if (!wrappedGlobal)
>+        return JSRS_ERROR;

          ABORT_TRACE_ERROR("helpful message");

This also wants a test. I think you were able to get hold of an inner object in the shell using this bug, so a trace-test seems appropriate.

r+ with those changes.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 15 • 15 years ago

bug 516826 blocks adding a test for this.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 16 • 15 years ago

http://hg.mozilla.org/tracemonkey/rev/2b8f660c66cd

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 17 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/2b8f660c66cd

	Daniel Veditz [:dveditz]
	Comment 18 • 15 years ago

If this is a security bug should it also be fixed in 1.9.2, or was it introduced after that branch? Does it need to be fixed in 1.9.1 (Firefox 3.5) also? I feel more confident in assuming it doesn't affect 1.9.0

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 19 • 15 years ago

Yeah, we should fix this on all relevant branches (which, afaict, is 1.9.1 and 1.9.2).

	Robert Sayre
	Comment 20 • 15 years ago

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5ffab6d66686