861493 - Passing dictionaries from chrome to constructors in content doesn't work

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Str:
- Open browser
- load about:blank
- open error console (ctrl+shitf+j or similar)
- run: var x = new top.opener.content.XMLHttpRequest({ mozAnon: true}); x.mozAnon;
  If you just run var x = new XMLHttpRequest({ mozAnon: true}); x.mozAnon; the value of
  mozAnon is right.

(Similar things happens with Event ctors)

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Oh, maybe XHR mozAnon is special, but Event's properties certainly aren't.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

var e = new top.opener.content.Event("test", {bubbles: true}); e.bubbles
vs.
var e = new Event("test", {bubbles: true}); e.bubbles

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Hmm, no, XHR is checking the caller, so setting mozAnon when caller is chrome should work, but
doesn't.

Comment 4

[Peter Van der Beken [:peterv]]

Obviously we get a ChromeObjectWrapper with no exposedProperties, so getting the dictionary object's properties fails.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Peter Van der Beken [:peterv] from comment #4)
> Obviously we get a ChromeObjectWrapper with no exposedProperties, so getting
> the dictionary object's properties fails.

Do we? That would only happen if we didn't go over Xray (and entered the content compartment), right? Who's waiving in this case?

Comment 6

[Peter Van der Beken [:peterv]]

DOMXrayTraits::call/construct enter the content compartment and wrap the arguments in it.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

So __exposedProps__ does help, but that is a new requirement. Do we want to force people to use that
even when chrome is just passing simple values within dictionary?

Comment 8

[Bobby Holley (:bholley)]

(In reply to Peter Van der Beken [:peterv] from comment #6)
> DOMXrayTraits::call/construct enter the content compartment and wrap the
> arguments in it.

Oh hm, that's not so great. So we more or less just lose Xray protections here? It seems like we should have a better story there. What are the exact constraints that are causing us to do this? Can we find other solutions?

(In reply to Olli Pettay [:smaug] from comment #7)
> So __exposedProps__ does help, but that is a new requirement. Do we want to
> force people to use that
> even when chrome is just passing simple values within dictionary?

Definitely not. __exposedProps__ (and content access of chrome JS in general) is deprecated, and we don't want to be depending on it for correctness.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> DOMXrayTraits::call/construct enter the content compartment and wrap the arguments in it.

Er... that's what I was proposing, I think.  So how did bug 853709 happen, then?

In any case, I can certainly change codegen to unsafe-unwrap dictionaries if we want to.  Or to only do it in certain cases (because otherwise it seems like you could exfiltrate cross-origin information by pretending your cross-origin Window is a dictionary in some API or something).

Comment 10

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #9)
> > DOMXrayTraits::call/construct enter the content compartment and wrap the arguments in it.
> 
> Er... that's what I was proposing, I think.  So how did bug 853709 happen,
> then?

Presumably because File isn't on the new bindings, and this is for DOM Xray traits.

I'm _really not_ ok with this behavior though. :-(
 
> In any case, I can certainly change codegen to unsafe-unwrap dictionaries if
> we want to.  Or to only do it in certain cases (because otherwise it seems
> like you could exfiltrate cross-origin information by pretending your
> cross-origin Window is a dictionary in some API or something).

Case in point.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

I believe we should fix this in Aurora too.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Some code relying on the old behavior 
http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.jsm#383

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Olli Pettay [:smaug] from comment #11)
> I believe we should fix this in Aurora too.

What is the user-affecting impact here? Comment 0 looks very edge casey.  If this has significant impact, who's going to take this?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

We have some code, at least http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.jsm#383 which relies on the old behavior.
(I don't know whether some b2g will be released from 22)

peterv or bz will look at this, I believe.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Comment 0 looks very edge casey.

Comment 0 are just simple steps to reproduce.  See comment 12 for an example of code that's broken right now.

In any case, I'm taking it, per meeting earlier today.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: m-c patch

The code this generates is just as horrible as you might think; I can attach what the test codegen ends up as if you want.

I'm not terribly happy about the MutableThis bits, but I couldn't think of any other sane way to do it.  :(

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Aurora patch

The dom/ bits are all the same except for s/CheckedUnwrap/UnwrapObjectChecked/.  The xray bits had to be basically rewritten because the rooting setup and arguments are different on aurora, so those need to be read over carefully.

Comment 18

[Bobby Holley (:bholley)]

Comment on attachment 741155 [details] [diff] [review]
m-c patch

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

r=bholley with comments.

> I could break this up if desired, but I figured a single diff would be easier to land on both branches...

In the future, I'd prefer for the m-c patches to be broken up whenever possible (so that they're bisectable), and then just do the branch patches as a rollup. Not a big deal in this case though, since most of the scattered changes are clearly independent of each other.

::: dom/bindings/BindingDeclarations.h
@@ +259,5 @@
>    {
>      return mImpl.ref();
>    }
>  
> +  Optional& MutableThis() const

Mmm, "This" makes me think of |this| in a JS sense. How about ConstCast or AsMutable?

@@ +381,5 @@
>    {
>      return mValue;
>    }
>  
> +  JS::Value* operator&()

Hm, should this be .address() instead, to match the convention in the rooting API? Or do you actually need to pass these interchangeably with raw jsvals such that the different semantics would be a pain?

::: dom/bindings/BindingUtils.h
@@ +1433,5 @@
>    }
> +
> +  JSObject** operator&()
> +  {
> +    // Assert if we're empty, on purpose

Hm, but aren't we allowed to be empty? Isn't it valid to return an address of an uninitialized JSObject? Or do we want to disallow that here for some reason?

Looking at the Codegen.py changes, I get the impression that we could end up with null here, but maybe I'm missing something...

::: dom/bindings/Codegen.py
@@ +4026,5 @@
>  class MethodNotCreatorError(Exception):
>      def __init__(self, typename):
>          self.typename = typename
>  
> +sequenceWrapLevel = 0

This makes me :'( a little bit. When I first saw it, I thought it had to do with indentation, and it took me a bit to figure out what the deal was. Comment it?

In general, it seems like we need some kind of name allocator...

@@ +4035,5 @@
> +    "object", or spidermonkey-interface types inside return a CGThing
> +    that will wrap them into the current compartment.
> +    """
> +    if type.isAny():
> +        assert not type.nullable()

Are |any|s not nullable per-spec? Or is this just "we'll deal with them when they happen"?

@@ +4058,5 @@
> +        if type.nullable():
> +            type = type.inner
> +            value = "%s.MutableThis().Value()" % value
> +        global sequenceWrapLevel
> +        index = "i%d" % sequenceWrapLevel

I might call this "indexName"

@@ +4071,5 @@
> +                              (index, index, value, index)),
> +                         post="\n}")
> +
> +    if type.isDictionary():
> +        assert not type.nullable()

Don't support or per spec?

@@ +4090,5 @@
> +    if type.isUnion():
> +        raise TypeError("Can't handle wrapping of unions in constructor "
> +                        "arguments yet")
> +
> +    if (type.isString() or type.isPrimitive() or type.isEnum() or

For JS-implemented DOM APIs, we always do the full jsval->AString and AString->jsval conversion, right? If we short-circuited with jsval->jsval, this would be a problem...

@@ +4098,5 @@
> +
> +    raise TypeError("Unknown type; we don't know how to wrap it in constructor "
> +                    "arguments: %s" % type)
> +
> +def wrapMaybeOptionalTypeIntoCurrentCompartment(type, value, isOptional):

I _think_ it would be cleaner and more general to have a CGThing that handles optional values, and demultiplex the cases in the caller. The caller already has to check for isOptional when calling this, so I think it's probably a net win. Fee free to ignore if it turns out to be problematic, though.

So the callers would do:
wrap = wrapTypeIntoCurrentCompartment(..)
if (wrap and arg.optional and not arg.default_value)
  wrap = CGConditionalDefault(type, wrap)

or something.

@@ +4199,5 @@
>  
> +        if isConstructor:
> +            # If we're called via an xray, we need to enter the
> +            # underlying object's compartment and then wrap up all of
> +            # our arguments into that compartment as needed.

So the idea is that by the time this code has been invoked, things like Dictionaries and stuff will already have been parsed out of the incoming jsvals, right? And now we just need to manually emulate a CrossCompartmentWrapper?

This comment could use some beefing up with the answer to that question.

::: dom/bindings/Configuration.py
@@ +472,5 @@
>      """
>      members = [m for m in descriptor.interface.members]
>      if descriptor.interface.ctor():
>          members.append(descriptor.interface.ctor())
> +    members.extend(descriptor.interface.namedConstructors)

I'm going to take your word for this one.

::: dom/tests/mochitest/chrome/test_sandbox_bindings.xul
@@ +238,5 @@
> +        var e = new eventCtor("test", { bubbles: true });
> +        is(e.bubbles, true, "Dictionary argument should work");
> +      } catch (e) {
> +        ok(false, "Should be able to construct my event " + e);
> +      }

Hm, why does a sandbox need to be involved here? Shouldn't this also be testable just by doing:

new contentWindow.Event('test', { bubbles: true})

I'm all for this piece of test coverage, but I'd like us to test the simple and non-magical cases whenever we can, and anything that involves sandboxPrototype definitely doesn't fit that bill. :-)

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1234,5 @@
> +        vp[1] = args.thisv();
> +        PodCopy(vp.begin() + 2, args.array(), args.length());
> +        if (!clasp->call(cx, args.length(), vp.begin()))
> +            return false;
> +        args.rval().set(vp[0]);

Per IRC discussion, let's just replace this Pod stuff with .base().

@@ +1235,5 @@
> +        PodCopy(vp.begin() + 2, args.array(), args.length());
> +        if (!clasp->call(cx, args.length(), vp.begin()))
> +            return false;
> +        args.rval().set(vp[0]);
> +    } else {

I would prefer to structure this as:
// Long comment explaining what the heck we're doing with all this stuff. More or less the
// five-step process you dsecribed to me on IRC.
if (clasp->flags & JSCLASS_IS_DOMIFACEANDPROTOJSCLASS) {
  if (!clasp->call)
    throw;
} else {
// Long comment explaining how this can only be legacy webidl and how we really do
// want to enter the compartment there.
MOZ_ASSERT(IsLegacyWebIDL);
return Base::call().
}

Comment 19

[Bobby Holley (:bholley)]

Comment on attachment 741156 [details] [diff] [review]
Aurora patch

Can you incorporate the feedback from the m-c patch on this patch, and then flag that for review?

Comment 20

[Boris Zbarsky [:bzbarsky]]

> Mmm, "This" makes me think of |this| in a JS sense. How about ConstCast or AsMutable?

I'll do AsMutable.

> Or do you actually need to pass these interchangeably with raw jsvals such that the
> different semantics would be a pain?

Exactly.  This allows me to just emit a & in the codegen for both raw values and these guys.

> Hm, but aren't we allowed to be empty?

In practice, not by the time the code that's using this operator runs.  The only reason it needs the ability to be empty is so it can have a default constructor, but right after that we init the dictionary members.

> Comment it?

Will do.

> Are |any|s not nullable per-spec?

Yes, since null is already a valid value for an "any".

> I might call this "indexName"

OK.

> Don't support or per spec?

Per spec, since null is already a valid value for a dictionary type.

> For JS-implemented DOM APIs, we always do the full jsval->AString and AString->jsval
> conversion, right?

Yes, absolutely, for exactly the reason you describe.

> So the callers would do:

Ah, hmm.  I'll see what that ends up looking like.

> So the idea is that by the time this code has been invoked, things like Dictionaries
> and stuff will already have been parsed out of the incoming jsvals, right?

Yes.  I'll improve the comments.

> Hm, why does a sandbox need to be involved here? Shouldn't this also be testable just
> by doing:
>   new contentWindow.Event('test', { bubbles: true})

Hmm.  Yes, I think so.  I mostly just added the test by copy-and-paste.  ;)

I'll try to add a direct test too.

>  if (!clasp->call)
>    throw;

Throwing the right exception for that case might be a bit of a pain.  I figured I'd just delegate it to whatever code normally throws them instead of trying to replicate it...

> MOZ_ASSERT(IsLegacyWebIDL);

I don't know how to assert this, honestly, short of saying that it's a WebIDL instance class with a call in the JSClass...

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch addressing comments

Jeff, can you look over the jsfriendapi bits?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated aurora patch

Comment 23

[Bobby Holley (:bholley)]

Comment on attachment 742005 [details] [diff] [review]
Updated aurora patch

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

r=bholley with comments addressed (some of these probably apply to the mc patch too)

::: dom/tests/mochitest/chrome/test_xray_event_constructor.xul
@@ +21,5 @@
> +  <![CDATA[
> +  /** Test for Bug 861493 **/
> +   SimpleTest.waitForExplicitFinish();
> +
> +   addLoadEvent(function() {

Is the initial about:blank in the type=content iframe here guaranteed to get a content principal? Please add a ok(Cu.isXrayWrapper($("t").contentWindow) to be sure.

::: js/src/jsfriendapi.cpp
@@ +1058,5 @@
> +JS_FRIEND_API(JSBool)
> +js_ReportIsNotFunction(JSContext *cx, const JS::Value& v)
> +{
> +    return ReportIsNotFunction(cx, v);
> +}

Do we really need this wrapper? I you can just put the declaration of ReportIsNotFunction in jsfriendapi.h and call it a day.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +136,5 @@
>                                      JSObject *wrapper, JSObject *holder,
>                                      jsid id, JSPropertyDescriptor *desc, unsigned flags);
>  
> +    static bool call(JSContext *cx, JS::Handle<JSObject*> wrapper,
> +                     unsigned argc, Value *vp, js::Wrapper& baseSingleton)

Just call this |base|, or |baseInstance| if you prefer. All js::Wrappers are singletons.

@@ +1295,5 @@
> +        vp[1] = JS::UndefinedValue();
> +        PodCopy(vp.begin() + 2, argv, argc);
> +        if (!clasp->construct(cx, argc, vp.begin()) && vp[0].isObject())
> +            return false;
> +        rval.set(vp[0]);

Er, can't we get rid of the Pod stuff in this function too?

Comment 24

[Boris Zbarsky [:bzbarsky]]

> Is the initial about:blank in the type=content iframe here guaranteed to get a content
> principal?

Yes, but I'll add the check.

> I you can just put the declaration of ReportIsNotFunction in jsfriendapi.h and call it a
> day.

Its optional arguments pull in various types and enums and stuff...  I was trying to minimize how much I moved to friendapi.

> Er, can't we get rid of the Pod stuff in this function too?

No, we can't, because we really are just passed in a totally separate argv+argc and rval here, as far as I can tell.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Oh, I'll do baseInstance.

Comment 26

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #24)
> No, we can't, because we really are just passed in a totally separate
> argv+argc and rval here, as far as I can tell.

Can you elaborate on this? The entry point from jsclass hook into proxy happens in proxy_{call,construct}, here:

http://mxr.mozilla.org/mozilla-central/source/js/src/jsproxy.cpp#3171

Both of them use CallArgsFromVp, and pass the result to Proxy::{call,construct}, which in turn passes them directly to the handler, which in turn passes them directly to the traits. How are the two cases different?

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Final aurora patch

Comment 28

[Boris Zbarsky [:bzbarsky]]

Hmm.  Are we guaranteed that no one ever calls these handlers in any other way?  On Aurora, prox_Construct does in fact get the argc, argv, rval out of a JSNative's vp, so if we know that this code is _always_ only entered via proxy_Construct there we can in fact hold our noses and do argv-2...

Comment 29

[Jeff Walden [:Waldo]]

Comment on attachment 741994 [details] [diff] [review]
Patch addressing comments

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

Yeah, not exposing those hairy trailing arguments seems fairly clearly the best thing to do.

Comment 30

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d85f6cc0eb4b for the trunk bits, since everyone is happy with them.

Comment 31

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #28)
> Hmm.  Are we guaranteed that no one ever calls these handlers in any other
> way?  On Aurora, prox_Construct does in fact get the argc, argv, rval out of
> a JSNative's vp, so if we know that this code is _always_ only entered via
> proxy_Construct there we can in fact hold our noses and do argv-2...

We know that it always gets invoked by Proxy::construct. It's possible that somebody else might call that (rather than the JSClass hook), but currently they don't. Can somebody actually create a CallArgs that doesn't cast back to vp?

Comment 32

[Boris Zbarsky [:bzbarsky]]

> Can somebody actually create a CallArgs that doesn't cast back to vp?

No, but on aurora what we have isn't a CallArgs, just a bunch of separate arguments....

Comment 33

[Boris Zbarsky [:bzbarsky]]

OK, I talked this over with Bobby.  I'm going to make this change from the last aurora construct() in this bug:

-        JS::AutoValueVector vp(cx);
-        if (!vp.resize(2 + argc))
-            return false;
-        vp[0] = JS::ObjectValue(*wrapper);
-        vp[1] = JS::UndefinedValue();
-        PodCopy(vp.begin() + 2, argv, argc);
-        if (!clasp->construct(cx, argc, vp.begin()) && vp[0].isObject())
+        // We're going to assume that argv == vp + 2
+        JS::Value* vp = argv - 2;
+        if (!clasp->construct(cx, argc, vp))

Comment 34

[Boris Zbarsky [:bzbarsky]]

Attachment: Final Aurora patch

Comment 35

[Boris Zbarsky [:bzbarsky]]

Attachment: Final Aurora patch

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 742119 [details] [diff] [review]
Final Aurora patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Several things together, but the most
   recent is bug 822399
User impact if declined: Extension compat issues due to event constructors not
   working right in chrome code.
Testing completed (on m-c, etc.): Passes try.
Risk to taking this patch (and alternatives if risky): I think the risk is
   medium.  I'm pretty sure this is safe and all, and that we didn't miss
   anything in the value wrapping code we're adding.  But I'm not sure I'd stake
   my life on it...  The alternative is probably to ship the broken constructors.
String or IDL/UUID changes made by this patch: None.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

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

Comment 38

[Alex Keybl [:akeybl]]

Comment on attachment 742119 [details] [diff] [review]
Final Aurora patch

If we suspect we'll need an add-on compat regression fix in FF22, we should make sure to land as early as possible. Given that, approving.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/90ee8d82a171

Comment 40

[Biraj Karmakar [:biraj]]

is there any manual verification needed, considering the automated test that is available

Comment 41

[Boris Zbarsky [:bzbarsky]]

Probably not, though the steps in comment 0 are the way to do it if you want to verify.

Comment 42

[u279076]

Marking [qa-] as per comment 41. Please remove this whiteboard tag and add the qawanted keyword if some testing is needed.