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

RESOLVED FIXED in mozilla28

Status

()

Core
XPConnect
--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gabor Krizsanits (INACTIVE), Assigned: Gabor Krizsanits (INACTIVE))

Tracking

unspecified
mozilla28
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(4 attachments)

(Assignee)

Description

5 years ago
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.
What are the pitfalls you're trying to avoid, exactly?  As in, how does this differ from Object.defineProperty?
(Assignee)

Comment 2

5 years ago
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.
Flags: needinfo?(bobbyholley+bmo)
(Assignee)

Comment 3

5 years ago
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.
Flags: needinfo?(bobbyholley+bmo)
(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.
(Assignee)

Comment 5

5 years ago
(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++
(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?
Flags: needinfo?(bzbarsky)
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?
Flags: needinfo?(gkrizsanits)
Seems ok to me.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 9

5 years ago
(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?
Flags: needinfo?(gkrizsanits)
(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/.
(Assignee)

Comment 11

5 years ago
Created attachment 8336676 [details] [diff] [review]
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.
Attachment #8336676 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 12

5 years ago
Created attachment 8336678 [details] [diff] [review]
part2: isProxy for Cu. v1
Attachment #8336678 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 13

5 years ago
Created attachment 8336679 [details] [diff] [review]
part3: isProxy for export helpers. v1
Attachment #8336679 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 14

5 years ago
Created attachment 8336681 [details] [diff] [review]
part4: Tests for consumers of IsScriptedProxy. v1
Attachment #8336681 - Flags: review?(bobbyholley+bmo)
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.
Attachment #8336676 - Flags: review?(bobbyholley+bmo) → review+
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.
Attachment #8336678 - Flags: review?(bobbyholley+bmo) → review+
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.
Attachment #8336679 - Flags: review?(bobbyholley+bmo) → review+
Attachment #8336681 - Flags: review?(bobbyholley+bmo) → review+
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))
Oh right, thanks Tom. I should have caught that :-\
(Assignee)

Comment 20

5 years ago
(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?
(Assignee)

Comment 21

5 years ago
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?
(Assignee)

Comment 22

5 years ago
(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.
(Assignee)

Comment 23

5 years ago
(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.
(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?
(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.
(Assignee)

Comment 26

5 years ago
(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.

Updated

5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.