Last Comment Bug 683361 - audit JS engine and mozilla js::Class-based checks which are broken by proxies
: audit JS engine and mozilla js::Class-based checks which are broken by proxies
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: Luke Wagner [:luke]
:
: Jason Orendorff [:jorendorff]
Mentors:
: 623102 654646 (view as bug list)
Depends on: 693124
Blocks: cpg 688069
  Show dependency treegraph
 
Reported: 2011-08-30 15:57 PDT by Luke Wagner [:luke]
Modified: 2011-10-08 22:06 PDT (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix GetPrimitiveThis (3.64 KB, patch)
2011-08-31 18:01 PDT, Luke Wagner [:luke]
jorendorff: review+
Details | Diff | Splinter Review
s/js::JS/js::/ (132.52 KB, patch)
2011-09-08 21:02 PDT, Luke Wagner [:luke]
gal: review+
Details | Diff | Splinter Review
use CallArgs more (110.44 KB, patch)
2011-09-08 21:06 PDT, Luke Wagner [:luke]
jwalden+bmo: review+
Details | Diff | Splinter Review
add ProxyHandler::nativeCall (8.86 KB, patch)
2011-09-08 21:10 PDT, Luke Wagner [:luke]
mrbkap: review+
Details | Diff | Splinter Review
fix everything to do with this.[[Class]] (61.83 KB, patch)
2011-09-08 21:15 PDT, Luke Wagner [:luke]
jwalden+bmo: review+
Details | Diff | Splinter Review
fix [[Class]] == "X" queries (18.06 KB, patch)
2011-09-20 16:53 PDT, Luke Wagner [:luke]
jwalden+bmo: review+
Details | Diff | Splinter Review
Proxy::objectClassIs (7.76 KB, patch)
2011-09-28 08:51 PDT, Luke Wagner [:luke]
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2011-08-30 15:57:50 PDT
The goal is to audit all transitive uses of obj->clasp to find cases where proxies are not transparent.  With compartment-per-global (bug 650353), the web will be fully exposed to transparent JSCrossCompartmentWrappers proxies so it is critical that they are actually transparent.

IIUC, here is a shell example stemming from GetPrimitiveThis:

js> var g1 = newGlobal('same-compartment');
js> g1.eval('String.prototype.toString').call(new String('hi'))
"hi"
js> var g2 = newGlobal('new-compartment');
js> g2.eval('String.prototype.toString').call(new String('hi'))
uncaught exception: TypeError: String.prototype.toString called on incompatible Proxy

Actually, perhaps this is something fuzzing can help with!  Specifically, the fuzzer could try to find cases involving more than one global where changing 'same-compartment' to 'new-compartment' produces divergent behavior.  Class-based errors also exist (and have been found) outside the JS engine, so perhaps this could also be fuzzed in the browser (does special-powers give access to anything similar to newGlobal?).  What do you think Jesse?
Comment 1 Chris Leary [:cdleary] (not checking bugmail) 2011-08-30 16:38:36 PDT
*** Bug 623102 has been marked as a duplicate of this bug. ***
Comment 2 Jesse Ruderman 2011-08-31 16:01:17 PDT
I modified jsfunfuzz to look for this kind of bug. Here's an example of what it can find:

js> evalcx("(1, <x><y/></x>)", newGlobal("same-compartment"))
<x>
  <y/>
</x>
js> evalcx("(1, <x><y/></x>)", newGlobal("new-compartment"))  
typein:4: TypeError: can't wrap XML objects

I hasn't yet found the GetPrimitiveThis issue in this bug or the regexp issue in bug 623102.
Comment 3 Luke Wagner [:luke] 2011-08-31 16:50:17 PDT
Thanks, Jesse, that was fast!  Let's ignore E4X for now :)
Comment 4 Luke Wagner [:luke] 2011-08-31 18:01:10 PDT
Created attachment 557386 [details] [diff] [review]
fix GetPrimitiveThis

Is this the right way to go about this Andreas?
Comment 5 Luke Wagner [:luke] 2011-08-31 18:24:11 PDT
For finding more of these issues, one good indicator is "[[Class]] internal property" in the spec, that finds a whole bunch: str_{match,replace,search}, date_*, regexp_*, JSON parse, stringify, and Str().

Another good indicator is ReportIncompatibleMethod, which points to:
array_toSource, js_regexp_toString, TypedArrayTemplate::fun_*, WeakMap_*.
Comment 6 Jason Orendorff [:jorendorff] 2011-09-01 08:17:12 PDT
Comment on attachment 557386 [details] [diff] [review]
fix GetPrimitiveThis

This certainly gives the desired behavior for transparent wrappers.

It doesn't allow scripted proxies to pretend to be String objects, which seems best.

The only danger is that it will "see through" wrapper proxies that aren't meant to reveal this information. I don't foresee ever having any of those, so r=me.
Comment 7 Luke Wagner [:luke] 2011-09-01 08:38:16 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #6)
> The only danger is that it will "see through" wrapper proxies that aren't
> meant to reveal this information. I don't foresee ever having any of those,
> so r=me.

Yeah, I was worried about that.  The "if isWrapper, unwrap, look at that" pattern is just taken from a few places in jsarray that implement '[[Class]] internal property == "Array"' for proxies.  Before happening upon that, I was certain we'd have to add a JSProxyHandler callback (like JSProxyHandler::obj_toString) that is only implemented by JSCrossCompartmentWrapper.

What do you think?

(And yes, I'll factor out some new canonical helpers so that we are not ad hoc unwrapping all over the place.)
Comment 8 Luke Wagner [:luke] 2011-09-02 17:16:13 PDT
Comment on attachment 557386 [details] [diff] [review]
fix GetPrimitiveThis

Ah hah, I think I found a way to handle all these cases.  The reoccurring problem is that, as jorendorff pointed out, once we know "yes, this is a proxy for a Date", to *do* anything with that Date object, I have to enter compartments and be wary of compartment leakage, wrapping, etc.  GetPrimitiveThis is just a simple case.

So the idea is to add a new JSProxyHandler operation:

  bool liftNativeCall(JSContext *, JSObject *proxy, JSClass *, Native, CallArgs)

so that, in date_whatever we can write:

  if (!obj->isDate()) {
    if (obj->isProxy())
      return obj->getProxyHandler().liftNativeCall(cx, obj, &DateClass,
                                                   date_whatever, args);
    ReportIncompatibleMethod(...)
    return false;
  }

and then JSCrossCompartmentWrapper::liftNativeCall can just wrap a call to the given native and all the other (security) wrappers can report an error.
Comment 9 Luke Wagner [:luke] 2011-09-08 21:02:19 PDT
Created attachment 559377 [details] [diff] [review]
s/js::JS/js::/

Since I'm doing work in this area, it's been bugging me for a while that we have js::JSProxyHandler.  This patch makes it js::ProxyHandler and similarly for the other proxy/wrapper classes.
Comment 10 Luke Wagner [:luke] 2011-09-08 21:06:36 PDT
Created attachment 559378 [details] [diff] [review]
use CallArgs more

This patch does pretty simple refactoring to use CallArgs as needed by the subsequent patches.  It also includes a few renamings so that, e.g., we can write args.ct() instead of the redundant args.argc().
Comment 11 Luke Wagner [:luke] 2011-09-08 21:10:21 PDT
Created attachment 559379 [details] [diff] [review]
add ProxyHandler::nativeCall

This patch adds the ProxyHandler hook mentioned above (no uses in this patch).  Notice that nativeCall is implemented for CrossCompartmentCall and any subclass (i.e., all the security wrappers).  I thought I might need to override this (to throw an error) in security wrappers, but I think FilterWrapper::enter handles this.  But I don't really know, please double check me on this Blake and Andreas.
Comment 12 Luke Wagner [:luke] 2011-09-08 21:15:37 PDT
Created attachment 559380 [details] [diff] [review]
fix everything to do with this.[[Class]]

This patch fixes most of the issues.  The new testCrossCompartmentTransparency.js shows like 80 methods which currently do not handle wrappers transparently.

The only stragglers left are methods that do [[Class]] tests but not on thisv; I'll do that next.  I also audited mozilla for bad JS_GET_CLASS usage (and a few others) and, surprisingly, the only thing I could find was ctypes.  It looks like xpconnect ftw!
Comment 13 Luke Wagner [:luke] 2011-09-08 21:18:03 PDT
Oops, pretend I didn't remove JS_InstanceOf in jsapi.h... getting a bit ahead of myself :)
Comment 14 Blake Kaplan (:mrbkap) 2011-09-12 15:17:56 PDT
Comment on attachment 559379 [details] [diff] [review]
add ProxyHandler::nativeCall

I'm fine with this, but I don't see where it's used or how we end up in nativeCall...
Comment 15 Luke Wagner [:luke] 2011-09-13 07:33:59 PDT
(In reply to Blake Kaplan (:mrbkap) from comment #14)
Thanks Blake.  The uses are in the next patch.
Comment 16 Jeff Walden [:Waldo] (remove +bmo to email) 2011-09-19 20:55:35 PDT
Comment on attachment 559378 [details] [diff] [review]
use CallArgs more

Review of attachment 559378 [details] [diff] [review]:
-----------------------------------------------------------------

arr+ with minor tweaks noted.  Second review tomorrow, too late today to do it.

::: js/src/jsdate.cpp
@@ +1787,5 @@
>      }
> +
> +    uintN numNums = Min(args.ct(), maxargs);
> +    JS_ASSERT(numNums <= 4);
> +    double nums[4];

RangedPtr!

@@ +1805,5 @@
>      else
>          lorutime = result;
>  
> +    double *argp = nums;
> +    double *stop = argp + numNums;

RangedPtr!

@@ +1907,5 @@
>      }
> +
> +    uintN numNums = Min(args.ct(), maxargs);
> +    JS_ASSERT(1 <= numNums && numNums <= 3);
> +    double nums[3];

RangedPtr!

@@ +1934,5 @@
>          lorutime = local ? LocalTime(result, cx) : result;
>      }
>  
> +    double *argp = nums;
> +    double *stop = argp + numNums;

RangedPtr!
Comment 17 Jeff Walden [:Waldo] (remove +bmo to email) 2011-09-19 21:08:16 PDT
Although: ct()?  Really?  Really?  I will die a little inside every time I read that.  What was so wrong with argc()?  Or even (gasp!) just one more character to make it count()?  :-P
Comment 18 Luke Wagner [:luke] 2011-09-20 08:09:00 PDT
(In reply to Jeff Walden (remove +bmo to email) from comment #17)
I don't want to be killing anyone.  args.argc() sounded redundant, so I went shorter, but I could just as well do args.count().
Comment 19 Luke Wagner [:luke] 2011-09-20 16:53:14 PDT
Created attachment 561346 [details] [diff] [review]
fix [[Class]] == "X" queries

This removes the obj->isWrapper() && obj->unwrap().isArray() hacks and fixes a few more cases of wrapper non-transparency as demonstrated in the given testcase.

The only remaining case to fix are the [[Class]] == "RegExp" queries in the string regexp code.  Unfortunately, the "right" fix to this wants bug 650411 so that the regexp allocator can be hoisted up to the JSRuntime, so I'll file a separate bug for that.
Comment 20 Luke Wagner [:luke] 2011-09-20 17:12:36 PDT
*** Bug 654646 has been marked as a duplicate of this bug. ***
Comment 21 Jeff Walden [:Waldo] (remove +bmo to email) 2011-09-26 17:15:13 PDT
Comment on attachment 559380 [details] [diff] [review]
fix everything to do with this.[[Class]]

Review of attachment 559380 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/basic/testCrossCompartmentTransparency.js
@@ +1,1 @@
> +// |jit-test| error: done

Why this?

@@ +23,5 @@
> +    var threw = false;
> +    try {
> +        f(g1.eval("new Object()"));
> +    } catch (e) {
> +        threw = true;

Check more specifically than for just "threw", but check for the error being thrown being a TypeError.  |Object.prototype.toString.call(e) === "[object Error]" && e.name === "TypeError"| will do the trick.  Likewise for the rest of this.

::: js/src/jsapi.h
@@ -2175,5 @@
>  
>  extern JS_PUBLIC_API(JSBool)
> -JS_InstanceOf(JSContext *cx, JSObject *obj, JSClass *clasp, jsval *argv);
> -
> -extern JS_PUBLIC_API(JSBool)

If only...

::: js/src/jsobj.h
@@ +2175,5 @@
> + * function better have a good explanation for why proxies are being handled
> + * correctly (e.g., by IsCallable) or are not an issue (E4X).
> + */
> +void
> +ReportIncompatibleMethod(JSContext *cx, CallReceiver call, Class *clasp);

Should this be extern?

@@ +2189,5 @@
> + * args.thisv's compartment). Thus, there are three possible post-conditions:
> + *  1. thisv is an object of the given clasp: the caller may proceed;
> + *  2. there was an error: the caller must return 'false';
> + *  3. thisv wrapped an object of the given clasp and the native was reentered
> + *     and completed succesfully: the caller must return 'true'.

This all sounds horribly complicated, but it's not, really, because it almost always collapses to this idiom:

  if (!NonGenericMethodGuard(cx, args, clasp, &ok))
    return ok;

Point this out at the end of the overall description, please.

I think readability would be improved here if the paragraphs and numeric lists were separated by blank lines, and if the numeric list were indented two spaces rather than one.

@@ +2191,5 @@
> + *  2. there was an error: the caller must return 'false';
> + *  3. thisv wrapped an object of the given clasp and the native was reentered
> + *     and completed succesfully: the caller must return 'true'.
> + * Case 1 is indicated by a non-NULL return value; case 2 by a NULL return
> + * value with *ok == false; and case 3 by a NULL return value with *ok == true.

I think it worth pointing out explicitly that this guard must be used in any method before any of its (other) side effects have occurred, to avoid re-performing those side effects.  I can only think of one method where side effects occur before a ToObject(this) call (and those side effects could be eliminated in the reentrant case), but that method's generic, so it's only a small concern.  Still, going forward...

@@ +2210,5 @@
> +/*
> + * Implement the extraction of a primitive from a value according as needed for
> + * the toString, valueOf, and a few other methods of the boxed primitives
> + * classes Boolean, Number, and String (e.g., ES5 15.6.4.2). If 'true' is
> + * returned, the extracted primitive is stored unboxed in |*v|. If 'false' is

I think generally we usually quote code values with ||, a la |true| or |false| (but null instead of |NULL|, to keep you on your toes).

"unboxed" is redundant with the potential types T might be (not to mention with "extracted primitive").

@@ +2223,1 @@
>  #endif /* jsobj_h___ */

Newline before #endif.
Comment 22 Luke Wagner [:luke] 2011-09-26 17:35:38 PDT
(In reply to Jeff Walden (remove +bmo to email) from comment #21)

Good points all around.

> > +// |jit-test| error: done
> 
> Why this?

Catches return-false-but-no-error-reported errors.
Comment 23 Jeff Walden [:Waldo] (remove +bmo to email) 2011-09-26 17:43:07 PDT
File a bug to fix the harness on that, please.  (Keep doing that for now, tho, if it's the fast path to happy.)
Comment 24 Luke Wagner [:luke] 2011-09-26 17:47:50 PDT
Filed bug 689402.
Comment 25 Jeff Walden [:Waldo] (remove +bmo to email) 2011-09-26 19:28:53 PDT
Comment on attachment 561346 [details] [diff] [review]
fix [[Class]] == "X" queries

Review of attachment 561346 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/basic/testCrossCompartmentTransparency2.js
@@ +11,5 @@
> +assertEq(JSON.stringify({arr:array}), "{\"arr\":[1,2,3]}");
> +assertEq(JSON.stringify({2:'ponies', unicorns:'not real'}, array), "{\"2\":\"ponies\"}");
> +assertEq(JSON.stringify({42:true, ponies:true, unicorns:'sad'}, [number, string]), "{\"42\":true,\"ponies\":true}");
> +assertEq(JSON.stringify({a:true,b:false}, undefined, number), "{\n          \"a\": true,\n          \"b\": false\n}");
> +assertEq(JSON.stringify({a:true,b:false}, undefined, string), "{\nponies\"a\": true,\nponies\"b\": false\n}");

Can't these tests produce unicorns at least once?  You are dashing the childhood dreams of particularly up-and-coming hackers everywhere.  :-(

::: js/src/jsbool.cpp
@@ +185,5 @@
> +    /*
> +     * To respect the proxy abstraction, we can't simply unwrap and call
> +     * getPrimitiveThis on the wrapped object. All we know is that obj says
> +     * its [[Class]] is "Boolean". Boolean.prototype.valueOf is specified to
> +     * return the [[Primitive Value]] internal property, so call that instead.

[[PrimitiveValue]]

::: js/src/jsobj.h
@@ +2272,5 @@
>  inline bool
>  BoxedPrimitiveMethodGuard(JSContext *cx, CallArgs args, T *v, bool *ok);
>  
> +/*
> + * Enumeration describing possible values of the E262 [[Class]] internal

"E262" is a bit of an unusual term.  I'd rather see ECMA-262, or just omit it entirely and leave the ECMA-262 bit implied.

@@ +2284,5 @@
> + * JSObject so the caller must not assume anything about obj's representation
> + * (e.g., obj may be a proxy).
> + */
> +inline bool
> +ClassPropertyIs(JSContext *cx, JSObject &obj, ESClassValue classValue);

You say ClassPropertyIs here, and I think of it as comparing, say, |regex.[[Match]]| to a specified value -- not checking [[Class]] == "String" or similar.  ObjectHasClass seems like a better name to me.

This is an infallible method, so |cx| should be a trailing parameter.

What's the reason this isn't JSObject::hasClass(ESClassValue, JSContext*)?  Why a free-floating method rather than a method on JSObject?  Built into objects seems much better to me than this.

::: js/src/jsobjinlines.h
@@ +1742,5 @@
> +      case ESClass_Number: return obj.isNumber();
> +      case ESClass_String: return obj.isString();
> +      case ESClass_Boolean: return obj.isBoolean();
> +    }
> +}

Can you put a JS_NOT_REACHED(...) at the end of the method without triggering compiler warnings for unreachable code?

::: js/src/jswrapper.cpp
@@ +743,5 @@
>  bool
>  CrossCompartmentWrapper::nativeCall(JSContext *cx, JSObject *wrapper, Class *clasp, Native native, CallArgs srcArgs)
>  {
> +    JS_ASSERT_IF(!srcArgs.calleev().isUndefined(),
> +                 srcArgs.callee().getFunctionPrivate()->native() == native);

Tricksy.  I will let this slide, but I think there must be a better way to do this than the crazy Boolean valueOf calling, and if someone finds it, I think we should switch to it in a heartbeat.
Comment 27 Luke Wagner [:luke] 2011-09-27 23:11:20 PDT
Fix warning
https://hg.mozilla.org/integration/mozilla-inbound/rev/95bbaf6cb2a6
Comment 29 Marco Bonardo [::mak] 2011-09-28 03:54:32 PDT
Looks like this caused Jetpack tests to burn in mozilla-central

Assertion failure: OperationInProgress(cx, proxy), at ../../../js/src/jsproxy.cpp:319
Comment 30 Ed Morley [:emorley] 2011-09-28 04:07:08 PDT
CCing some jetpack people for comment 29.
Comment 31 Luke Wagner [:luke] 2011-09-28 08:51:37 PDT
Created attachment 563083 [details] [diff] [review]
Proxy::objectClassIs

Oops, forgot to add the Proxy:: layer that AutoPendingProxyOperation's.  (Also forgot to rename the ProxyHandler method to match ObjectClassIs, so fixing that too.)
Comment 32 Myk Melez [:myk] [@mykmelez] 2011-09-28 09:15:09 PDT
Alexandre Poirot is the Jetpack guy who knows the most about our use of proxies, and Gábor Krizsanits can help out with any platform work required to support that use; cc:ing them.
Comment 33 Luke Wagner [:luke] 2011-09-28 09:30:57 PDT
Thanks! I believe the fix will be landed today.
Comment 35 Michael Wu [:mwu] 2011-09-29 01:29:13 PDT
https://hg.mozilla.org/mozilla-central/rev/b626aecfddf7

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