911400 - MakeWrappable implementation still needs work to permit use with functions accepting objects as arguments

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

After discussion with till and bholley, it sounds like MakeWrappable support doesn't quite work in certain cases.  Full details here:

http://logbot.glob.com.au/?c=mozilla%23jsapi&s=30+Aug+2013&e=31+Aug+2013#c244025

(And it seemed so simple at the time!  :-) )

The gist of the problem is that we've kind of sort of in effect introduced a new kind of wrapper, by exposing SHG functions that are cross-compartment-wrapped to completely arbitrary globals.  JSCompartment::wrap needs updating for this, to avoid calling any of the embedder-provided wrapping callbacks, that understandably don't know about this and really probably shouldn't have to know about it.  (Although, I haven't taken the time to fully understand some of the later details in that discussion, so my summary may be a bit too cursory.)

Comment 1

[Jeff Walden [:Waldo]]

For the record, the symptom that was being hit, with the patch in bug 899361, was

00:45:59 INFO - Assertion failure: handler->isSafeToUnwrap() == AccessCheck::subsumes(target, origin), at ../../../../js/xpconnect/wrappers/WrapperFactory.cpp:336

occurring any time (I think) the Intl code got initialized.

Comment 2

[Till Schneidereit [:till]]

Attachment: Remove redundant check from WrapperFactory::Rewrap

This gets rid of proxyProto and the `proxyProto == wrappedProto` check in WrapperFactory::Rewrap. proxyProto isn't ever set to anything other than its default of wrapperProto.

Comment 3

[Till Schneidereit [:till]]

Attachment: Always create CrossCompartmentWrappers for wrapped self-hosted objects

I didn't change WrapperFactory::Rewrap to return the handler and reuse the code for creating or refreshing the wrapper after all: doing that would require making Rewrap return a js::Wrapper, which would in turn require changing the signature of JSPreWrapCallback, introducing a js::-return value into jsapi.h. Or casting the returned JSObject* to js::Wrapper*.

Both seemed undesirable if the only gain is to save the duplication of three fairly-straightforward lines of code.

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 798271 [details] [diff] [review]
Remove redundant check from WrapperFactory::Rewrap

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

Yeah, this is an artifact from when we used to COW prototype remapping in Rewrap (it now happens in PrepareForWrapping).

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 798272 [details] [diff] [review]
Always create CrossCompartmentWrappers for wrapped self-hosted objects

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

As mentioned on IRC, |global| here is the global we're wrapping _into_, not the global of the object we're wrapping. So this grants the SHG unfettered access to any compartment in the system, which is unacceptable IMO.

This does mean, however, that we can never support invoking CCW-ed functions in the SHG, at least ones with object arguments. Is that ok on your side?

Also, I don't think we should be invoking _any_ of the embedder callbacks in the SHG case - this stuff should be invisible to the embedder. That means we need to check for the SHG much earlier, and avoid calling prewrap or wrapForSameCompartment. This starts to get really messy though.

The best option I can think of is to group the embedder callbacks into a struct, and then create a pointer to that struct at the top if ::wrap. If we detect the SHG, we can override that pointer to point to a different struct with no-op wrap callbacks. Does that make sense?

::: js/src/jscompartment.cpp
@@ +362,5 @@
> +    } else {
> +        /*
> +         * We hand in the original wrapped object into the wrap hook to allow
> +         * the wrap hook to reason over what wrappers are currently applied
> +         * to the object.

This comment is totally obsolete btw - can you remove it?

Comment 6

[Jeff Walden [:Waldo]]

(In reply to Bobby Holley (:bholley) from comment #5)
> As mentioned on IRC, |global| here is the global we're wrapping _into_, not
> the global of the object we're wrapping. So this grants the SHG unfettered
> access to any compartment in the system, which is unacceptable IMO.

What could possibly go wrong?

> This does mean, however, that we can never support invoking CCW-ed functions
> in the SHG, at least ones with object arguments. Is that ok on your side?

My experience with self-hosting hasn't given me sufficient clairvoyance to say we definitely won't need that.  But we can burn that bridge if we ever reach it; it seems quite possible we never will.

Comment 7

[Till Schneidereit [:till]]

Attachment: Always create CrossCompartmentWrappers for wrapped self-hosted objects

So I decided to do all the special-casing in JSCompartment::wrap, after all. Looking through the code carefully, I'm convinced that having identity-preserving implementations of JSPreWrapCallback and JSSameCompartmentWrapObjectCallback is functionally the same to not calling them at all. So "all" we have to duplicate is the unwrapping stuff, really, which doesn't seem too bad to me. Well, and for the wrapping itself, the goal is to always create a CCW, right?

Doing the refactoring to a struct for the callbacks and passing that in to JSCompartment::wrap, and dealing with the ensuing ownership issues seems more of a hassle than it'd be worth it, to me. This being compartments stuff, I fully expect overlooking something big, though. :(

Comment 8

[Till Schneidereit [:till]]

Optimistically try-servering here: https://tbpl.mozilla.org/?tree=Try&rev=92ade2d72f9a

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8338473 [details] [diff] [review]
Always create CrossCompartmentWrappers for wrapped self-hosted objects

(In reply to Till Schneidereit [:till] from comment #7)
> So I decided to do all the special-casing in JSCompartment::wrap, after all.
> Looking through the code carefully, I'm convinced that having
> identity-preserving implementations of JSPreWrapCallback and
> JSSameCompartmentWrapObjectCallback is functionally the same to not calling
> them at all.

Yes. The question is how to write this in the cleanest, most robust way.

> So "all" we have to duplicate is the unwrapping stuff, really,

And the multiple "are we in our own compartment" bailouts. And we always have to check the two versions against each other whenever we alter the semantics of one of them.

> 
> Doing the refactoring to a struct for the callbacks and passing that in to
> JSCompartment::wrap

You wouldn't pass it into JSCompartment::wrap (though you'd have to pass it to WrapForSameCompartment, which should be fine). You'd do something like this at the top of JSCompartment::wrap():

JSWrapCallbacks *cb = cx->runtime()->isSelfHostingGlobal(global) ? &SelfHostingWrapCallbacks;

Then you'd just replace all the |cx->runtime()->fooCallback(...)| calls with |cb->fooCallback(...)|.

, and dealing with the ensuing ownership issues

There are no ownership issues. They'd both be statically-declared. JS_SetWrapObjectCallbacks would just take a pointer to a caller-allocated struct. XPCJSRuntime would statically declare that struct and pass it in. This is the same way other things like JSStructuredCloneCallbacks work.

Comment 10

[Till Schneidereit [:till]]

Attachment: Always create CrossCompartmentWrappers for wrapped self-hosted objects. v3

Ok, I'm convinced. This patch does what you described, and passed a try run here: https://tbpl.mozilla.org/?tree=Try&rev=41c55d446ef1

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8339986 [details] [diff] [review]
Always create CrossCompartmentWrappers for wrapped self-hosted objects. v3

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

::: js/src/jscompartment.cpp
@@ +272,5 @@
> +static JSObject *
> +SelfHostingWrapObjectCallback(JSContext *cx, HandleObject existing, HandleObject obj,
> +                              HandleObject proto, HandleObject parent, unsigned flags)
> +{
> +    CrossCompartmentWrapper *handler = &CrossCompartmentWrapper::singleton;

Let's MOZ_ASSERT here that |obj| is in the SHG here (and anything else we might be able to say about obj).

@@ +290,5 @@
> +SelfHostingPreWrapObjectCallback(JSContext *cx, HandleObject scope, HandleObject obj,
> +                                 unsigned flags)
> +{
> +    return obj;
> +}

I just noticed that we actually null-check the last two callbacks. So they can just be null, and we can just nix the implementations.

@@ +323,5 @@
>       */
>      HandleObject global = cx->global();
>      JS_ASSERT(global);
>  
> +    JSWrapObjectCallbacks *cb = cx->runtime()->isSelfHostingGlobal(global) ?

Am I crazy, or is this actually the same security problem that I complained about in comment 5?

Comment 12

[Till Schneidereit [:till]]

Attachment: Always create CrossCompartmentWrappers for wrapped self-hosted objects. v4

(In reply to Bobby Holley (:bholley) from comment #11)
> ::: js/src/jscompartment.cpp
> @@ +272,5 @@
> > +static JSObject *
> > +SelfHostingWrapObjectCallback(JSContext *cx, HandleObject existing, HandleObject obj,
> > +                              HandleObject proto, HandleObject parent, unsigned flags)
> > +{
> > +    CrossCompartmentWrapper *handler = &CrossCompartmentWrapper::singleton;
> 
> Let's MOZ_ASSERT here that |obj| is in the SHG here (and anything else we
> might be able to say about obj).

I added that assert and one for obj->global == cx->global. What else did you have in mind?

> 
> @@ +290,5 @@
> > +SelfHostingPreWrapObjectCallback(JSContext *cx, HandleObject scope, HandleObject obj,
> > +                                 unsigned flags)
> > +{
> > +    return obj;
> > +}
> 
> I just noticed that we actually null-check the last two callbacks. So they
> can just be null, and we can just nix the implementations.

Done

> 
> @@ +323,5 @@
> >       */
> >      HandleObject global = cx->global();
> >      JS_ASSERT(global);
> >  
> > +    JSWrapObjectCallbacks *cb = cx->runtime()->isSelfHostingGlobal(global) ?
> 
> Am I crazy, or is this actually the same security problem that I complained
> about in comment 5?

Well, yes. But as discussed on IRC, we have to have the ability to call CCW-ed functions in the SHG with objects as arguments. The only current use case for MakeWrappable requires precisely this.

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8340642 [details] [diff] [review]
Always create CrossCompartmentWrappers for wrapped self-hosted objects. v4

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

(In reply to Till Schneidereit [:till] from comment #12)
> Well, yes. But as discussed on IRC, we have to have the ability to call
> CCW-ed functions in the SHG with objects as arguments. The only current use
> case for MakeWrappable requires precisely this.

That wasn't what this bug was originally about. This was about making the stuff in the SHG global accessible to all compartments (to share ICC tables IIRC). Making all compartments accessible to the SHG, especially in tandem with the latter, is a totally different thing that's basically unacceptable IMO (and Waldo agrees, see comment 6).

So. First question. Do we still want to do the table sharing?

Comment 14

[Jeff Walden [:Waldo]]

Uh, I'm not sure I know enough to competently agree or disagree here, or to say that comment 6 took a stand on this.  :-)

We need a way to mark every object -- of any class -- as Intl-initialized, such that that marking occurs wrt every global.  We don't have private properties, and reserved slots are out because of the class thing, so we track initialization in a per-global WeakMap of (obj => initialization data).  An object is Intl-initialized if it's a key in that WeakMap.  But as there's one WeakMap per global, an object appears Intl-initialized only in the global where the initialization happened.

The solution is to use one WeakMap to track this association for all globals.  It's natural for it to be in the SHG, indirectly exposed via self-hosted functions (exposed to globals by cross-compartment wrapper).  That works fine -- except we have to pass objects from all those globals into those wrapper functions, as arguments.  Wrapping those objects into the SHG is what goes wrong.

All that's needed is for the SHG to be able to have passed into its functions (or at least the MakeWrappable ones) objects from other globals.  The function in the SHG, that's been wrapped, can see a CCW for such objects.  It won't do anything to that object except for performing identity/equality checks on it.

Sharing Intl data tables across globals, rather than requiring large-table cloning, is another possible way to use this.  But as those mechanisms deal (I think) only in string primitives, there's no need to be able to pass objects across the self-hosting boundary there.  And it wasn't the original motivation for this bug.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #14)
> We need a way to mark every object -- of any class -- as Intl-initialized,
> such that that marking occurs wrt every global.  We don't have private
> properties, and reserved slots are out because of the class thing, so we
> track initialization in a per-global WeakMap of (obj => initialization
> data).  An object is Intl-initialized if it's a key in that WeakMap.  But as
> there's one WeakMap per global, an object appears Intl-initialized only in
> the global where the initialization happened.

I don't grok why this is an issue. Can't the Intl-initialized-ness of an object live in the weakmap of its  home compartment, and have everyone else just unwrap to the canonical object when they want to set or query this information?

> The solution is to use one WeakMap to track this association for all
> globals.  It's natural for it to be in the SHG, indirectly exposed via
> self-hosted functions (exposed to globals by cross-compartment wrapper). 
> That works fine -- except we have to pass objects from all those globals
> into those wrapper functions, as arguments.  Wrapping those objects into the
> SHG is what goes wrong.
>
> All that's needed is for the SHG to be able to have passed into its
> functions (or at least the MakeWrappable ones) objects from other globals. 
> The function in the SHG, that's been wrapped, can see a CCW for such
> objects.  It won't do anything to that object except for performing
> identity/equality checks on it.

If identity is all we care about, I'm happy to generate opaque wrappers in this case. That doesn't present a security issue. But given what I wrote above, is there any benefit to this approach in comparison to having the data be per-compartment?

Comment 16

[Till Schneidereit [:till]]

(In reply to Bobby Holley (:bholley) from comment #15)
> I don't grok why this is an issue. Can't the Intl-initialized-ness of an
> object live in the weakmap of its  home compartment, and have everyone else
> just unwrap to the canonical object when they want to set or query this
> information?

But wouldn't you then need to give everyone else this capability - plus access to either a function in the object's home compartment or to the weakmap itself? How would that be better than having a tightly-controlled, extra-carefully reviewed function in the self-hosting compartment be the only actor to be able to access the object (or, it's CCW, rather)? Maybe I'm just not fully understanding what you're proposing.

> If identity is all we care about, I'm happy to generate opaque wrappers in
> this case. That doesn't present a security issue. But given what I wrote
> above, is there any benefit to this approach in comparison to having the
> data be per-compartment?

For this case, identity is all we care about, yes. I don't currently know of any cases where we'd want to do anything else with objects (not primitives), so this seems fine to me. And vastly preferable to the above-mentioned alternative if I understand it correctly.

Comment 17

[Bobby Holley (:bholley)]

(In reply to Till Schneidereit [:till] from comment #16)
> Maybe I'm
> just not fully understanding what you're proposing.

I was more referring to doing this in C++.

> For this case, identity is all we care about, yes. I don't currently know of
> any cases where we'd want to do anything else with objects (not primitives),
> so this seems fine to me. And vastly preferable to the above-mentioned
> alternative if I understand it correctly.

I'm generally concerned about exposing reference edges in and outside of the SHG. But per IRC discussion, the fact that MakeWrappable is an explicit tightly-controlled opt-in makes me more comfortable with it.

So opaque wrappers would be ok here. And it sounds like this is the direction we want to go.

We'll need two opaque wrappers, one for SHG->World wrappers (which deny everything), and one for World->SHG wrappers (which allow calls but nothing else). These wrappers should inherit SecurityWrapper<CrossCompartmentWrapper>, and define an |enter| trap to disallow everything (except for |call| in the case of World->SHG). This should all probably go in vm/SelfHosting.{cpp,h}.

Comment 18

[Till Schneidereit [:till]]

Attachment: Create OpaqueWrappers when wrapping objects for use in the self-hosting global and OpaqueWrapperWithCalls when wrapping self-hosted function for use in other compartments

This should be what we talked about. It can be tested by adding this code to, say, builtin/Utilities.js:

var map = new Map();
function testWrappers(o) {
  map.set(o, true);
  return map.get(o);
}
MakeWrappable(testWrappers);

and, after a rebuild, running this code in the shell:
getSelfHostedValue('testWrappers')({});

When modifying testWrappers to do anything else but using it as a key with o, an exception is thrown as expected.

Comment 19

[Till Schneidereit [:till]]

Try run here: https://tbpl.mozilla.org/?tree=Try&rev=05d2c3958765

Comment 20

[Bobby Holley (:bholley)]

Comment on attachment 8343950 [details] [diff] [review]
Create OpaqueWrappers when wrapping objects for use in the self-hosting global and OpaqueWrapperWithCalls when wrapping self-hosted function for use in other compartments

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

Test coverage is crucial for security invariants. Please add automated test coverage for the wrapper behavior here (making sure that all the proper operations fail, and that call works in the right situations), and flag me for review on that as a separate patch. This shouldn't land until that's done.

This is top-notch work! I appreciate the attention to detail here. r=bholley with comments addressed and test coverage added.

::: js/src/jscompartment.cpp
@@ +280,5 @@
> +static JSWrapObjectCallbacks SelfHostingWrapObjectCallbacks = {
> +    SelfHostingWrapObjectCallback,
> +    nullptr,
> +    nullptr
> +};

This should move into vm/SelfHosting.cpp and be declared in vm/SelfHosting.h.

::: js/src/vm/SelfHosting.cpp
@@ +1038,5 @@
> +OpaqueWrapperWithCall OpaqueWrapperWithCall::singleton;
> +
> +JSObject *
> +js::SelfHostingWrapObjectCallback(JSContext *cx, HandleObject existing, HandleObject obj,
> +                              HandleObject proto, HandleObject parent, unsigned flags)

indentation

::: js/src/vm/SelfHosting.h
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef selfhosting_h_
> +#define selfhosting_h_

This does not appear to follow the naming convention for include guards. It should be |vm_SelfHosting_h|. Here twice and once below.

@@ +12,5 @@
> +namespace js {
> +
> +JSObject *
> +SelfHostingWrapObjectCallback(JSContext *cx, HandleObject existing, HandleObject obj,
> +                              HandleObject proto, HandleObject parent, unsigned flags);

As noted above, this should become private to SelfHosting.cpp, and we can just expose

extern JSWrapObjectCallbacks SelfHostingWrapObjectCallbacks.

Comment 21

[Bobby Holley (:bholley)]

(In reply to Till Schneidereit [:till] from comment #19)
> Try run here: https://tbpl.mozilla.org/?tree=Try&rev=05d2c3958765

(Also note that this is busted)

Comment 22

[Till Schneidereit [:till]]

Attachment: Create OpaqueWrappers when wrapping objects for use in the self-hosting global and OpaqueWrapperWithCalls when wrapping self-hosted function for use in other compartments. v2

Addressed comments.

(In reply to Bobby Holley (:bholley) from comment #20)
> Test coverage is crucial for security invariants. Please add automated test
> coverage for the wrapper behavior here (making sure that all the proper
> operations fail, and that call works in the right situations), and flag me
> for review on that as a separate patch. This shouldn't land until that's
> done.

I wanted to test this, of course, but there's no way to do that I'm comfortable with. The only way I see is to do something along the lines of comment 18: Add a wrappable function to the self-hosted code that's only used for testing MakeWrappable, and write a test for it using the shell-only `getSelfHostedValue(name)` function. Adding this kind of code to self-hosting purely for testing reasons doesn't seem right to me, though. Maybe we could make it debug-only?

Comment 23

[Bobby Holley (:bholley)]

(In reply to Till Schneidereit [:till] from comment #22)
> Adding this kind of code to
> self-hosting purely for testing reasons doesn't seem right to me, though.

In general, this is what we do.

> Maybe we could make it debug-only?

But then we don't catch regressions in the bits we actually ship to users.

At least on the Gecko side, it's a commonly-accepted practice to add test-only code behind a pref that we can flip during automation. We still ship these bits, but there's generally no way for content to trigger them.

Assuming we're super careful about self-hosting code anyway, adding a test-only function to the self-hosting global doesn't seem like a big deal to me. It's possible that there are SHG-related considerations I don't fully understand, though - Waldo, what are your thoughts?

Comment 24

[Jeff Walden [:Waldo]]

Some sort of SHG-only function that tests MakeWrappable doesn't seem too horrible to me.  I'd kind of like it to be debug-only and fuzzing-unsafe as safety precautions, tho.

Comment 25

[Till Schneidereit [:till]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #24)
> Some sort of SHG-only function that tests MakeWrappable doesn't seem too
> horrible to me.  I'd kind of like it to be debug-only and fuzzing-unsafe as
> safety precautions, tho.

getSelfHostedValue is already fuzzing-unsafe, and it's all we need on the content compartment side to test this. As for debug-only: I agree with bholley about testing the bits we ship. However, we can only test this in the shell in any case, because getSelfHostedValue is shell-only. Adding it to the browser seems insane to me.

Comment 26

[Bobby Holley (:bholley)]

(In reply to Till Schneidereit [:till] from comment #25)
> getSelfHostedValue is already fuzzing-unsafe, and it's all we need on the
> content compartment side to test this. As for debug-only: I agree with
> bholley about testing the bits we ship. However, we can only test this in
> the shell in any case, because getSelfHostedValue is shell-only. Adding it
> to the browser seems insane to me.

Yes, testing it in the shell is fine. The point is to be testing the SpiderMonkey we ship, as well as is possible.

Comment 27

[Till Schneidereit [:till]]

Attachment: Create OpaqueWrappers when wrapping objects for use in the self-hosting global and OpaqueWrapperWithCalls when wrapping self-hosted function for use in other compartments. v3

Now with tests and exceptions being thrown by the wrappers instead of them just failing silently. Requesting re-review just for those two parts, the rest is unchanged.

The last linux-only try run was green, but since I've been burned by this before, here's another all-platforms run:
https://tbpl.mozilla.org/?tree=Try&rev=7c71537001a2

Comment 28

[Till Schneidereit [:till]]

Attachment: Create OpaqueWrappers when wrapping objects for use in the self-hosting global and OpaqueWrapperWithCalls when wrapping self-hosted function for use in other compartments. v4

And now, the patch actually contains the tests ...

Comment 29

[Bobby Holley (:bholley)]

Comment on attachment 8346489 [details] [diff] [review]
Create OpaqueWrappers when wrapping objects for use in the self-hosting global and OpaqueWrapperWithCalls when wrapping self-hosted function for use in other compartments. v4

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

Looks good in general - just rminusing because I want to look at the tests again. I can't overstate the importance of thorough tests for security invariants.

Also, I'd like Waldo to have a quick look at the changes to js/src/builtin/Utilities.js, since I've never touched this stuff before and wouldn't know if I was missing something obvious. Flagging him for a quick feedback? on that part when you upload the next patch is fine.

::: js/src/builtin/Utilities.js
@@ +164,5 @@
> +
> +
> +/********** Testing code **********/
> +
> +// This code enables testing of the custom allow-nothing wrappers used for 

trailing whitespace.

@@ +170,5 @@
> +// Functions marked as wrappable won't be cloned into content compartments;
> +// they're called inside the self-hosting compartment itself. Calling is the
> +// only valid operation on them. In turn, the only valid way they can use their
> +// object arguments is as keys in maps. Doing anything else with them throws.
> +var wrappersTestMap = new Map();

So, this function runs in the scope of the SHG, rather than being cloned, right? So the Map() call here is safe?

@@ +173,5 @@
> +// object arguments is as keys in maps. Doing anything else with them throws.
> +var wrappersTestMap = new Map();
> +function testWrappersAllowUseAsKey(o) {
> +  wrappersTestMap.set(o, o);
> +  return wrappersTestMap.get(o);

This is going to leak any global that calls this function, right? I can see that not mattering too much, since presumably we'll mostly be using this in shell runs, where we start a new process for each test, but if this ever ends up in jsreftest, it'll be a problem.

We should use a WeakMap, or have the function remove the wrapper from the map immediately after the get(). Even better - do both!

@@ +180,5 @@
> +  try {
> +    var result = o.prop;
> +  } catch (e) {
> +    // Got the expected exception.
> +    return true;

In order to be more sure here, you should do:

return /denied/.test(e);

This is a really important technique to make sure that security tests are testing what they intend. Otherwise, platform changes can cause other exceptions to be thrown, and we won't notice, and then we won't notice if we stop throwing the security exception (this has happened many times).

@@ +182,5 @@
> +  } catch (e) {
> +    // Got the expected exception.
> +    return true;
> +  }
> +  return false;

We should also test a set, a call, and an Object.getOwnPropertyDescriptor(Object.prototype, '__proto__').set.call(o, new Object()). The last part (munging the prototype with the __proto__ setter) tests to make sure that nativeCall is denied.

::: js/src/jit-test/tests/self-hosting/makewrappable.js
@@ +6,5 @@
> +assertEq(testWrappersAllowUseAsKey(testObject), testObject);
> +
> +var testWrappersForbidAccess = getSelfHostedValue('testWrappersForbidAccess');
> +assertEq(typeof testWrappersForbidAccess, 'function');
> +assertEq(testWrappersForbidAccess(testObject), true);
\ No newline at end of file

We should also be checking that we throw for all of the cases we expect when touching properties on the wrapped self-hosting-global function.

::: js/src/vm/SelfHosting.cpp
@@ +1022,5 @@
> +                       Wrapper::Action act, bool *bp) MOZ_OVERRIDE
> +    {
> +        JS_ReportError(cx, "Permission denied to access properties of content object from "
> +                       "self-hosted code");
> +        *bp = false;

So, looking at this code again, I realized that the instructions I was giving were pre bug 836301. In particular, the new convention is that the enter() traps themselves do _not_ throw, but just set *bp to false (which causes the AutoEnterPolicy to throw, if appropriate).

So please delete the JS_ReportError here and below. The tests should still pass. Sorry for the confusion!

Comment 30

[Till Schneidereit [:till]]

Attachment: Create OpaqueWrappers when wrapping objects for use in the self-hosting global and OpaqueWrapperWithCalls when wrapping self-hosted function for use in other compartments. v5

> > +var wrappersTestMap = new Map();
> 
> So, this function runs in the scope of the SHG, rather than being cloned,
> right? So the Map() call here is safe?

Yep, that's correct. Actually, it'd be safe in non-wrapped self-hosted functions, too: the name would be looked up in the current global's intrinsicsHolder and, upon not being found, cloned over from the self-hosting compartment. It'd be highly inefficient and might not work in all cases for all I know, though.

> We should use a WeakMap, or have the function remove the wrapper from the
> map immediately after the get(). Even better - do both!

Good point, I did both.

> In order to be more sure here, you should do:
> 
> return /denied/.test(e);

Much nicer indeed!

> We should also test a set, a call, and an
> Object.getOwnPropertyDescriptor(Object.prototype, '__proto__').set.call(o,
> new Object()). The last part (munging the prototype with the __proto__
> setter) tests to make sure that nativeCall is denied.

Added.

> We should also be checking that we throw for all of the cases we expect when
> touching properties on the wrapped self-hosting-global function.

Doh, I forgot about that indeed. Added in the same way the wrappers are tested in Utilities.js

Comment 31

[Till Schneidereit [:till]]

Comment on attachment 8346818 [details] [diff] [review]
Create OpaqueWrappers when wrapping objects for use in the self-hosting global and OpaqueWrapperWithCalls when wrapping self-hosted function for use in other compartments. v5

Waldo, can you take a look at the test code in Utilities.js?

Comment 32

[Bobby Holley (:bholley)]

Comment on attachment 8346818 [details] [diff] [review]
Create OpaqueWrappers when wrapping objects for use in the self-hosting global and OpaqueWrapperWithCalls when wrapping self-hosted function for use in other compartments. v5

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

\o/ r=bholley

Comment 33

[Jeff Walden [:Waldo]]

Comment on attachment 8346818 [details] [diff] [review]
Create OpaqueWrappers when wrapping objects for use in the self-hosting global and OpaqueWrapperWithCalls when wrapping self-hosted function for use in other compartments. v5

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

::: js/src/builtin/Utilities.js
@@ +184,5 @@
> +      case 'get': var result = o.prop; break;
> +      case 'set': o.prop2 = 'value'; break;
> +      case 'call': o(); break;
> +      case '__proto__':
> +        Object.getOwnPropertyDescriptor(Object.prototype, '__proto__').set.call(o, new Object());

I'm kind of leery about having this accessing the self-hosted global's "Object" global property.  It just seems iffy, given that most self-hosted code always acts in the context of the page that uses it.  But I don't know of an actual problem to doing so, assuming proper care is taken.

::: js/src/jsapi-tests/testBug604087.cpp
@@ +62,5 @@
>  {
>      return js::Wrapper::New(cx, obj, proto, parent, &js::CrossCompartmentWrapper::singleton);
>  }
>  
> +static JSWrapObjectCallbacks WrapObjectCallbacks = {

Can this be const?

::: js/src/jsapi.cpp
@@ +917,5 @@
>      rt->compartmentNameCallback = callback;
>  }
>  
> +JS_PUBLIC_API(void)
> +JS_SetWrapObjectCallbacks(JSRuntime *rt, JSWrapObjectCallbacks *callbacks)

const JSWOC*?

::: js/src/jsapi.h
@@ +838,5 @@
>   */
>  typedef JSObject *
>  (* JSSameCompartmentWrapObjectCallback)(JSContext *cx, JS::Handle<JSObject*> obj);
>  
> +struct JSWrapObjectCallbacks {

{ on new line.

@@ +1641,5 @@
>  extern JS_PUBLIC_API(void)
>  JS_SetCompartmentNameCallback(JSRuntime *rt, JSCompartmentNameCallback callback);
>  
> +extern JS_PUBLIC_API(void)
> +JS_SetWrapObjectCallbacks(JSRuntime *rt, JSWrapObjectCallbacks *callbacks);

const JSWOC*

::: js/src/jscompartment.cpp
@@ +183,5 @@
>  }
>  #endif
>  
>  static bool
> +WrapForSameCompartment(JSContext *cx, MutableHandleObject obj, JSWrapObjectCallbacks *cb)

const JSWOC*

@@ +290,5 @@
>       * This loses us some transparency, and is generally very cheesy.
>       */
>      HandleObject global = cx->global();
> +    RootedObject objGlobal(cx, &obj->global());
> +    JS_ASSERT(global && objGlobal);

Two asserts, so if the first half fails, it's clear that half failed.  (The same's true, if less obviously, for the second half of it.)

@@ +295,5 @@
> +
> +    JSWrapObjectCallbacks *cb;
> +
> +    if (cx->runtime()->isSelfHostingGlobal(global) ||
> +        cx->runtime()->isSelfHostingGlobal(objGlobal))

Hum, this doesn't fit in 100ch?  Surprising.

::: js/src/vm/Runtime.cpp
@@ +112,5 @@
>      JS_ASSERT(CurrentThreadCanAccessRuntime(runtime_));
>      removeFrom(runtime_->threadList);
>  }
>  
> +static JSWrapObjectCallbacks DefaultWrapObjectCallbacks = {

const

::: js/src/vm/Runtime.h
@@ +1556,5 @@
>  
>      /* Tables of strings that are pre-allocated in the atomsCompartment. */
>      js::StaticStrings   staticStrings;
>  
> +    JSWrapObjectCallbacks                  *wrapObjectCallbacks;

const JSWOC*

::: js/src/vm/SelfHosting.cpp
@@ +1064,5 @@
> +        return Wrapper::New(cx, obj, proto, parent, handler);
> +}
> +
> +JSWrapObjectCallbacks
> +js::SelfHostingWrapObjectCallbacks = {

const

::: js/src/vm/SelfHosting.h
@@ +17,5 @@
> + * When wrapping functions from the self-hosting compartment for use in other
> + * compartments, we create an OpaqueWrapperWithCall that only allows calls,
> + * nothing else.
> + */
> +extern JSWrapObjectCallbacks SelfHostingWrapObjectCallbacks;

const

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2941,5 @@
>          return true;
>      }
>  };
>  
> +static JSWrapObjectCallbacks WrapObjectCallbacks = {

const

Comment 34

[Till Schneidereit [:till]]

Comment on attachment 798271 [details] [diff] [review]
Remove redundant check from WrapperFactory::Rewrap

The changes here were entirely, perfectly contained in some other patch. Without even a merge conflict or anything.

Comment 35

[Till Schneidereit [:till]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/adfa9fa90fd9

\o/

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/adfa9fa90fd9

Comment 38

[Florent Fayolle]

Can you backport the patch? This has caused this issue with Firebug:
https://bugzilla.mozilla.org/show_bug.cgi?id=941707

Florent

Comment 39

[Jeff Walden [:Waldo]]

Redirecting to appropriate person, I have nil to do with this patch any more.  :-)

Comment 40

[Till Schneidereit [:till]]

Attachment: Aurora uplift

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Exact changeset unclear, but caused by the landing of some self-hosted JS function
User impact if declined: crashes when using Firebug
Testing completed (on m-c, etc.): yes, extensively
Risk to taking this patch (and alternatives if risky): low; no real alternatives
String or IDL/UUID changes made by this patch: none

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2c72a842fb18