Closed Bug 683361 Opened 9 years ago Closed 8 years ago

audit JS engine and mozilla js::Class-based checks which are broken by proxies


(Core :: JavaScript Engine, defect)

Not set





(Reporter: luke, Assigned: luke)




(6 files, 1 obsolete file)

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'))
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?
Blocks: cpg
Duplicate of this bug: 623102
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"))
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.
Thanks, Jesse, that was fast!  Let's ignore E4X for now :)
Attached patch fix GetPrimitiveThis (obsolete) — Splinter Review
Is this the right way to go about this Andreas?
Attachment #557386 - Flags: review?(gal)
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 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.
Attachment #557386 - Flags: review+
(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 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);
    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.
Attachment #557386 - Flags: review?(gal)
Attached patch s/js::JS/js::/Splinter Review
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.
Attachment #557386 - Attachment is obsolete: true
Attachment #559377 - Flags: review?(gal)
Attachment #559377 - Flags: review?(gal) → review+
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().
Attachment #559378 - Flags: review?(jwalden+bmo)
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.
Attachment #559379 - Flags: review?(mrbkap)
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!
Attachment #559380 - Flags: review?(jwalden+bmo)
Oops, pretend I didn't remove JS_InstanceOf in jsapi.h... getting a bit ahead of myself :)
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...
Attachment #559379 - Flags: review?(mrbkap) → review+
(In reply to Blake Kaplan (:mrbkap) from comment #14)
Thanks Blake.  The uses are in the next patch.
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];


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


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


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

Attachment #559378 - Flags: review?(jwalden+bmo) → review+
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
(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().
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.
Attachment #561346 - Flags: review?(jwalden+bmo)
Blocks: 688069
Duplicate of this bug: 654646
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 Error]" && === "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 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.
Attachment #559380 - Flags: review?(jwalden+bmo) → review+
(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.
File a bug to fix the harness on that, please.  (Keep doing that for now, tho, if it's the fast path to happy.)
Filed bug 689402.
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.


::: 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.
Attachment #561346 - Flags: review?(jwalden+bmo) → review+
Looks like this caused Jetpack tests to burn in mozilla-central

Assertion failure: OperationInProgress(cx, proxy), at ../../../js/src/jsproxy.cpp:319
CCing some jetpack people for comment 29.
Oops, forgot to add the Proxy:: layer that AutoPendingProxyOperation's.  (Also forgot to rename the ProxyHandler method to match ObjectClassIs, so fixing that too.)
Attachment #563083 - Flags: review?(jwalden+bmo)
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.
Thanks! I believe the fix will be landed today.
Attachment #563083 - Flags: review?(jwalden+bmo) → review+
Depends on: 693124
You need to log in before you can comment on or make changes to this bug.