939696 - Need a way to safely define properties in JS from privileged scope on content objects.

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Gabor Krizsanits (INACTIVE)]

Something like Cu.safeDefineProperty(targetObject, "name", value) that avoids all the pitfalls of JS when it comes to property definition.

As a side goal here, we should make sure that the export helpers API uses the same mechanism, and that mechanism handles proxies right.

Comment 1

[Boris Zbarsky [:bzbarsky]]

What are the pitfalls you're trying to avoid, exactly?  As in, how does this differ from Object.defineProperty?

Comment 2

[Gabor Krizsanits (INACTIVE)]

So this started as a conversation about how to safely define properties in JS and Bobby's response was:

"This kind of sniffing is basically impossible in the general case due to proxies, since defining a property can run arbitrary content code even if the property is reported as having no getter/setter.

Gabor, we should add C++ checks for proxy objects for all of the JS_DefinePropertyById calls that we do for this stuff. Does this sound good? If so, can you file a bug?"

It's not entirely clear to me... so let's just ask him.

Comment 3

[Gabor Krizsanits (INACTIVE)]

Per IRC discussion with Bobby the concerns are scripted proxies. We could check for them with Cu.getClassName and adding unit tests for all 4 cases of {Callable,Uncallable} {Direct,Indirect} proxies, to make sure their class name does not change if we rely on them. Then either we do Object.defineProperty and catch or check Object.getOwnPropertyDescriptor. One more thing to do here in C++ is making Cu.getClassName available from sandboxes when wantExportHelpers is set.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> One more thing to do here in C++ is making Cu.getClassName available from
> sandboxes when wantExportHelpers is set.

At the point when we're already adding more C++ to deal with this, I think we should add an isProxy instead of doing a string comparison.

Comment 5

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #4)
> At the point when we're already adding more C++ to deal with this, I think
> we should add an isProxy instead of doing a string comparison.

isScriptableProxy you mean? I would not like this thing to beep on windows/documents/wrappers/whatever else we have implemented on top of proxies, only for the proxies we care about here. Is it easier to identify them in C++? Anyway I'm absolutely not against doing it in C++

Comment 6

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5)
> (In reply to Bobby Holley (:bholley) from comment #4)
> > At the point when we're already adding more C++ to deal with this, I think
> > we should add an isProxy instead of doing a string comparison.
> 
> isScriptableProxy you mean? I would not like this thing to beep on
> windows/documents/wrappers/whatever else we have implemented on top of
> proxies, only for the proxies we care about here. Is it easier to identify
> them in C++? Anyway I'm absolutely not against doing it in C++

Right. I think we'd want this thing to unwrap wrappers before doing the check, but we'd want to specifically check for scripted proxies. Though TBH, I think we should still call it isProxy, because the fact that we use proxies for other things internally is an implementation detail that I don't think we need to expose to script.

So I'd vote Cu.isProxy(obj) with the following semantics:
* all wrappers are unwrapped before doing the test
* we only return true for ScriptedDirectProxies and ScriptedIndirectProxies

Boris, does this sound good?

Comment 7

[Bobby Holley (:bholley)]

Gabor, we should also file a bug for the second part of comment 2, which is a different issue (making exportFunction and friends aware of scripted proxies). Unless we don't care about that case?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Seems ok to me.

Comment 9

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #7)
> Gabor, we should also file a bug for the second part of comment 2, which is
> a different issue (making exportFunction and friends aware of scripted
> proxies). Unless we don't care about that case?

I kind of planned doing that part here as well since I want to use the same isProxy in the export helpers. Can file a separate bug if you think it's needed.

(In reply to Bobby Holley (:bholley) from comment #6)
> check, but we'd want to specifically check for scripted proxies. Though TBH,
> I think we should still call it isProxy, because the fact that we use
> proxies for other things internally is an implementation detail that I don't
> think we need to expose to script.

As I want to use this function from C++ as well for exportFunction/createObjectIn isProxy is kind of misleading there. How about an internal C++ function called IsScriptableProxy and exposing it via Cu as isProxy?

Comment 10

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9)
> As I want to use this function from C++ as well for
> exportFunction/createObjectIn isProxy is kind of misleading there. How about
> an internal C++ function called IsScriptableProxy and exposing it via Cu as
> isProxy?

Yes. But s/Scriptable/Scripted/.

Comment 11

[Gabor Krizsanits (INACTIVE)]

Attachment: part1: IsScriptedProxy. v1

I could just compare the singletons of the handlers against the one in the proxy objects slot, but that could be done only in jsproxy.cpp and we need to use this from xpconnect... So I came up with this version, but open for better ideas.

Comment 12

[Gabor Krizsanits (INACTIVE)]

Attachment: part2: isProxy for Cu. v1

Comment 13

[Gabor Krizsanits (INACTIVE)]

Attachment: part3: isProxy for export helpers. v1

Comment 14

[Gabor Krizsanits (INACTIVE)]

Attachment: part4: Tests for consumers of IsScriptedProxy. v1

Comment 15

[Bobby Holley (:bholley)]

Comment on attachment 8336676 [details] [diff] [review]
part1: IsScriptedProxy. v1

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

r=bholley with comments addressed.

::: js/src/jsproxy.h
@@ +364,5 @@
>      JS_ASSERT(n <= 1);
>      SetReservedSlot(obj, PROXY_EXTRA_SLOT + n, extra);
>  }
>  
> +inline bool IsScriptedProxy(JSObject *obj)

Please declare this immediately under IsProxy (also in this file).

@@ +369,5 @@
> +{
> +    const Class *clasp = GetObjectClass(obj);
> +    if (clasp != CallableProxyClassPtr &&
> +        clasp != UncallableProxyClassPtr)
> +        return false;

Replace all this with:

if (!IsProxy(obj))
    return false;

@@ +373,5 @@
> +        return false;
> +
> +    BaseProxyHandler *handler = GetProxyHandler(obj);
> +    JS_ASSERT(handler);
> +    return handler->isScripted();

replace this with:

return GetProxyHandler(obj)->isScripted();

::: js/xpconnect/src/Sandbox.cpp
@@ +278,5 @@
>  
> +        if (js::IsScriptedProxy(targetScope)) {
> +            JS_ReportError(cx, "Defining property on proxy object is not allowed");
> +            return false;
> +        }

This should move further up in this function, immediately after the CheckedUnwrap and subsequent null-check. In general, we should try to do input validation before taking any actions with side-effects.

::: js/xpconnect/src/XPCComponents.cpp
@@ +3038,5 @@
> +        if (!JSID_IS_VOID(options.defineAs)) {
> +            if (js::IsScriptedProxy(scope)) {
> +                JS_ReportError(cx, "Defining property on proxy object is not allowed");
> +                return false;
> +            }

This should also move up to under the CheckedUnwrap and null-check.

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 8336678 [details] [diff] [review]
part2: isProxy for Cu. v1

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

::: js/xpconnect/idl/xpccomponents.idl
@@ +309,5 @@
>  
>      /*
> +     * To be called from JS only.
> +     *
> +     * Returns the true if the object is a (scripted) proxy.

Make a note that wrappers are stripped off before performing the check.

::: js/xpconnect/src/XPCComponents.cpp
@@ +3063,5 @@
> +        return NS_OK;
> +    }
> +
> +    RootedObject obj(cx, &vobj.toObject());
> +    obj = js::CheckedUnwrap(obj);

Please add /* stopAtOuter = */ false. It doesn't actually matter, but it's more consistent from a conceptual standpoint.

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 8336679 [details] [diff] [review]
part3: isProxy for export helpers. v1

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

r=bholley with comments addressed.

::: js/xpconnect/src/Sandbox.cpp
@@ +227,5 @@
> +        JS_ReportError(cx, "Function requires at least 1 argument");
> +        return false;
> +    }
> +    if (!args[0].isObject()) {
> +        *vp = JS::BooleanValue(false);

drop the JS:: prefixing.

@@ +232,5 @@
> +        return true;
> +    }
> +
> +    RootedObject obj(cx, &args[0].toObject());
> +    obj = js::CheckedUnwrap(obj);

/* stopAtOuter = */ false.

@@ +235,5 @@
> +    RootedObject obj(cx, &args[0].toObject());
> +    obj = js::CheckedUnwrap(obj);
> +    NS_ENSURE_TRUE(obj, false);
> +
> +    *vp = JS::BooleanValue(js::IsScriptedProxy(obj));

drop the JS:: prefixing.

Comment 18

[Tom S [:evilpie]]

Comment on attachment 8336679 [details] [diff] [review]
part3: isProxy for export helpers. v1

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

::: js/xpconnect/src/Sandbox.cpp
@@ +227,5 @@
> +        JS_ReportError(cx, "Function requires at least 1 argument");
> +        return false;
> +    }
> +    if (!args[0].isObject()) {
> +        *vp = JS::BooleanValue(false);

Actually: args.rval().setBoolean(false);

@@ +235,5 @@
> +    RootedObject obj(cx, &args[0].toObject());
> +    obj = js::CheckedUnwrap(obj);
> +    NS_ENSURE_TRUE(obj, false);
> +
> +    *vp = JS::BooleanValue(js::IsScriptedProxy(obj));

args.rval().setBoolean(IsScriptedProxy(obj))

Comment 19

[Bobby Holley (:bholley)]

Oh right, thanks Tom. I should have caught that :-\

Comment 20

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #15)
> > +inline bool IsScriptedProxy(JSObject *obj)
> 
> Please declare this immediately under IsProxy (also in this file).

The reason I didn't do that is that it is using GetProxyHandler which is defined a few lines further down. Now I can forward declare GetProxyHandler but since it's an inline function I didn't really want to (probably I should not care, but I'm not sure if that would prevent the compiler to inline it). Anyway will just do it then.

> ::: js/xpconnect/src/XPCComponents.cpp
> @@ +3038,5 @@
> > +        if (!JSID_IS_VOID(options.defineAs)) {
> > +            if (js::IsScriptedProxy(scope)) {
> > +                JS_ReportError(cx, "Defining property on proxy object is not allowed");
> > +                return false;
> > +            }
> 
> This should also move up to under the CheckedUnwrap and null-check.

Even if that comes with the added side effect that createObjectIn will not work for proxies at all? Instead of it will not work only if defineAs is set?

Comment 21

[Gabor Krizsanits (INACTIVE)]

Anyway, I have a weird build error on try (only on try not locally)
XPCShellImpl.o
../../../js/src/jsproxy.cpp:784:48: error: 'override' does not name a type
../../../js/src/jsproxy.cpp:1105:48: error: 'override' does not name a type

How can this be? All the other MOZ_OVERRIDE in the file just work fine but not this two new I've added...
https://tbpl.mozilla.org/?tree=Try&rev=09548dff34e0

Is this some build system bug? A clobber might fixes it I guess, but first I would like to understand what the hell is going on here... Who might know more about this?

Comment 22

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #16)
> > +    obj = js::CheckedUnwrap(obj);
> 
> Please add /* stopAtOuter = */ false. It doesn't actually matter, but it's
> more consistent from a conceptual standpoint.

Uhh... if this is the preferred way why does the flag have a default value on the first place (instead of a comment)? More then half of the consumers use it without this, but I see no consistency at all. I will do this, but I think if consistency matters at all here we should file a bug and stop using a default value, so we force people adding this false.

Comment 23

[Gabor Krizsanits (INACTIVE)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #21)
> ../../../js/src/jsproxy.cpp:784:48: error: 'override' does not name a type
> ../../../js/src/jsproxy.cpp:1105:48: error: 'override' does not name a type

Oh... I was just being dumb, never-mind.

Comment 24

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #20)
> > This should also move up to under the CheckedUnwrap and null-check.
> 
> Even if that comes with the added side effect that createObjectIn will not
> work for proxies at all? Instead of it will not work only if defineAs is set?

You can just test defineAs when you're doing the check, no?

Comment 25

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #22)
> (In reply to Bobby Holley (:bholley) from comment #16)
> Uhh... if this is the preferred way why does the flag have a default value
> on the first place (instead of a comment)? More then half of the consumers
> use it without this, but I see no consistency at all. I will do this, but I
> think if consistency matters at all here we should file a bug and stop using
> a default value, so we force people adding this false.

It just depends on what the use case is. |stopAtOuter = true| strips off wrappers until you hit the identity object. |stopAtOuter = false| strips off all wrappers, even those that indicate a unique identity.

The reason I think it makes sense here is that the algorithm is most cleanly stated as the following:
(1) Strip all proxies that identify themselves as a wrapper.
(2) Check if the result is a scripted proxy.

Leaving the outer intact between (1) and (2) requires the reader to understand that outer windows will never be scripted proxies (which is obvious to you and me, but not necessarily everyone). It just seems cleaner here to strip off all wrappers, period.

I don't feel strongly about this one though, so feel free to do what you want.

Comment 26

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #25)
> I don't feel strongly about this one though, so feel free to do what you
> want.

Actually I mixed up something here... I thought you meant only a semantical change (though for a second that false is the default flag here). Hmm... I don't mind using the false flag in general, it doesn't matter much and your logic makes sense, sorry for the added confusion.

Comment 27

[Gabor Krizsanits (INACTIVE)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/27327f3e2943
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/017167e2056a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6728f7b4e467
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ecf073f6394b

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/27327f3e2943
https://hg.mozilla.org/mozilla-central/rev/017167e2056a
https://hg.mozilla.org/mozilla-central/rev/6728f7b4e467
https://hg.mozilla.org/mozilla-central/rev/ecf073f6394b