Closed Bug 1241349 Opened 4 years ago Closed 4 years ago

Use of xpc::UnprivilegedJunkScope in maplike/setlike generated code and dictionary ToJSON is not OK

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- affected
firefox47 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(5 files)

UnprivilegedJunkScope is mainthread-only.  Nothing forces maplike/setlike to be mainthread-only.  The only good news is that there are no non-test interfaces that are maplike/setlike so far.

We should use something else here, but I'm not sure what, exactly.  Bobby, do we have some worker-thread-safe concept of "just give me a scope, dammit"?
Flags: needinfo?(bobbyholley)
Per the other bug where we discussed this, I'm fine with UnprivilegedJunkScopeOrWorkerGlobal or somesuch.
Flags: needinfo?(bobbyholley)
OK, this is annoying.  I started adding this function and writing the long detailed "watch out for this if you try to use this" comment, and realized that it's probably OK for the maplike/setlike case, but totally not OK for the dictionary ToJSON case.  

And the reason for that is that ToJSON constructs a JSObject from a dictionary, then JS_Stringifies it.  But stringification will, afaict, look at the .toJSON property on the object, and does not restrict to own properties.

Which means in the worker case the script in the worker can mess with Object.prototype to totally change how dictionaries convert to JSON.  On main thread, the use of the junk scope means we have to be careful to be side-effect-free, but at least don't have to do that in the face of an adversary that controls the standard objects...

I wonder whether we can add a lazily-allocated junk global to WorkerGlobalScope or something.
(In reply to Boris Zbarsky [:bz] from comment #2)
> I wonder whether we can add a lazily-allocated junk global to
> WorkerGlobalScope or something.

Conceptually we could, though it might open up a can of worms about multiple globals in a worker runtime - not sure if that can has been opened already. Kyle would be the person to task.
I did check with Kyle; he seemed OK with it, esp given we already have the debugger global.

That said, another option is to stick with the scary thing but add a version of JS_Stringify that skips getting .toJSON.  It's conceptually scarier, of course.

A third option is to stop round-tripping through JS for dictionary ToJSON conversion (and stick with the scary thing for maplike/setlike), but that requires writing and maintaining another serializer, which is not optimal either...

I'm really not sure what to do.  I worry about adding a function for which I had precisely two use cases, and one of them turned out to be unsafe in a way that had been non-obvious to me until I really thought deeply about its details.  So from a sane security perspective, I _really_ prefer adding a junk global to WorkerGlobalScope.
(In reply to Boris Zbarsky [:bz] from comment #4)
> I did check with Kyle; he seemed OK with it, esp given we already have the
> debugger global.
> 
> That said, another option is to stick with the scary thing but add a version
> of JS_Stringify that skips getting .toJSON.  It's conceptually scarier, of
> course.

Can you explain this part? It seems to me that having a JSAPI method that does some sort of canonical stringification without invoking methods on the object is a totally reasonable thing to add.
> Can you explain this part?

Oh, the scary part is not the change to JS_Stringify.  It's the "have a function that returns a global which is _really_ hard to use correctly, requiring a deep understanding of everything".  From my draft code comment:

// It's VERY important that any consumers of this function only do things that
// are guaranteed to be side-effect-free, even in the face of a script
// environment controlled by a hostile adversary.  This is because in the worker
// case the global is in fact the worker global, so it and its standard objects
// are controlled by the worker script.  This is why this function is in the
// binding_detail namespace.  Any use of this function MUST be very carefully
// reviewed by someone who is sufficiently devious and has a very good
// understanding of all the code that will run while we're using the return
// value, including the SpiderMonkey parts.
But this isn't really all that different than, say, JS::CurrentGlobalOrNull, or xpc::GlobalObject(someObjectThatWeFound), right?

In general, dealing with untrusted script environments is kind of par for the course for JSAPI, and the reason why SpiderMonkey has been moving away from implementing most JSAPI methods with property lookups. JS_Stringify seems a bit of a vestige from the old days...
Well, JS_Stringify just implements the spec's JSON.stringify algorithm.  And the ES spec is all unsafe-lookup-happy...

I just realized that there are other issues here too, in the general case of dictionary ToJSON against a content-controlled global: dictionary properties of "object" type that include accessors or arrays with holes, interface-typed properties whose reflector has custom accessors defined on it, etc.  I _think_ with the unprivileged junk scope all that stuff kinda works because you get opaque wrappers for all that and hence none of those end up being issues.  Right?
> But this isn't really all that different than, say, JS::CurrentGlobalOrNull, or xpc::GlobalObject(someObjectThatWeFound), right?

Oh, and to answer this: you're right.  It's not.  The problems only arise when we use JSAPI for a situation that isn't specced in terms of JS stuff.
(In reply to Boris Zbarsky [:bz] from comment #8)
> I _think_ with the unprivileged junk scope all that stuff kinda
> works because you get opaque wrappers for all that and hence none of those
> end up being issues.  Right?

Accessing an object from web content from the unprivileged junk scope doesn't work at all, because that scope has a null principal.

It seems like this algorithm really needs to just operate within the scope of the object it's parsing. Can you re-explain the reason why we can't do that?
> Accessing an object from web content from the unprivileged junk scope doesn't work at all

Right, by "works" I meant "doesn't end up running any content-defined code".

> It seems like this algorithm really needs to just operate within the scope of the object it's parsing

Dictionaries don't have a scope attached to them.  And again, we _want_ to be operating in a scope from which all the objects involved that we didn't create ourselves look opaque.
Oh, so this isn't about parsing objects into dictionaries, but rather objectifying dictionaries as an incidental intermediate processing stage? For that case, I could go either way between a lazily-instantiated junk scope and just using the worker scope and being careful.

But then I'm kind of confused about the following:

(In reply to Boris Zbarsky [:bz] from comment #8)
> I just realized that there are other issues here too, in the general case of
> dictionary ToJSON against a content-controlled global: dictionary properties
> of "object" type that include accessors or arrays with holes,
> interface-typed properties whose reflector has custom accessors defined on
> it, etc.  I _think_ with the unprivileged junk scope all that stuff kinda
> works because you get opaque wrappers for all that and hence none of those
> end up being issues.  Right?

If the dictionary is content-controlled, then you're not going to see anything at all from the junk-scope. And this sounds a lot more like "parsing objects into dictionaries" rather than the stuff discussed above.
> If the dictionary is content-controlled

It is.

> then you're not going to see anything at all from the junk-scope

I'm not sure I follow.  We'd create the object for the dictionary itself in the junk scope.  Then we'd define a bunch of properties on it, corresponding to the dictionary members.  The dictionary members that are primitives Just Work.  The members that are themselves dictionaries also Just Work, since we create objects representing them in the junk scope.  Same thing for sequences of all of those: we create the array in the junk scope.  The members that involve objects of various sorts (interface types, "object", etc) would end up with those objects being reflected into the junk scope as an opaque wrapper for the actual underlying object, right?  And then the stringify op will presumably throw.  That seems fine; for the actual use cases for this stuff we don't have such members anyway.
Summary: Use of xpc::UnprivilegedJunkScope in maplike/setlike generated code is not OK → Use of xpc::UnprivilegedJunkScope in maplike/setlike generated code and dictionary ToJSON is not OK
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8715654 - Flags: review?(bobbyholley) → review+
Attachment #8715655 - Flags: review?(bobbyholley) → review+
Attachment #8715656 - Flags: review?(bobbyholley) → review+
Attachment #8715653 - Flags: review?(bobbyholley) → review+
Comment on attachment 8715656 [details] [diff] [review]
part 5.  Start using binding_detail::UnprivilegedJunkScopeOrWorkerGlobal in dictionary ToJSON conversions

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

::: dom/bindings/Codegen.py
@@ +12232,5 @@
>          return ClassMethod(
>              "ToJSON", "bool",
>              [Argument('nsAString&', 'aJSON')],
>              body=dedent("""
>                  MOZ_ASSERT(NS_IsMainThread());

Should this patch now also remove the NS_IsMainThread() assertion here?
(In reply to Tim Taubert [:ttaubert] from comment #19)
> ::: dom/bindings/Codegen.py
> @@ +12232,5 @@
> >          return ClassMethod(
> >              "ToJSON", "bool",
> >              [Argument('nsAString&', 'aJSON')],
> >              body=dedent("""
> >                  MOZ_ASSERT(NS_IsMainThread());
> 
> Should this patch now also remove the NS_IsMainThread() assertion here?

And the same for "initFromJSONMethod" above that? OTOH, it seems that this might not be enough either, I'm hitting the following when trying the patches here on top of the WebCrypto/Workers patches:

Hit MOZ_CRASH() at /Users/tim/workspace/mozilla-central/js/xpconnect/src/xpcprivate.h:259
#01: nsXPConnect::XPConnect() (xpcprivate.h:259, in XUL)
#02: mozilla::AutoJSContext::Init(bool, mozilla::detail::GuardObjectNotifier&&) (ScriptSettings.cpp:765, in XUL)
#03: mozilla::AutoJSContext::AutoJSContext(bool, mozilla::detail::GuardObjectNotifier&&) (ScriptSettings.cpp:755, in XUL)
#04: mozilla::AutoSafeJSContext::AutoSafeJSContext(mozilla::detail::GuardObjectNotifier&&) (ScriptSettings.cpp:805, in XUL)
#05: mozilla::AutoSafeJSContext::AutoSafeJSContext(mozilla::detail::GuardObjectNotifier&&) (ScriptSettings.cpp:807, in XUL)
#06: mozilla::dom::JsonWebKey::Init(nsAString_internal const&) (SubtleCryptoBinding.cpp:3303, in XUL)
#07: mozilla::dom::ImportKeyTask::SetJwkFromKeyData() (WebCryptoTask.cpp:1433, in XUL)
> Should this patch now also remove the NS_IsMainThread() assertion here?

Yes.  Fixed locally.

> And the same for "initFromJSONMethod" above that?

No, as you noticed that method is not fixed here.  Filed bug 1246153 on that and will fix.
(In reply to Boris Zbarsky [:bz] from comment #21)
> > And the same for "initFromJSONMethod" above that?
> 
> No, as you noticed that method is not fixed here.  Filed bug 1246153 on that
> and will fix.

Did not notice, thanks for clarifying! I thought they were somehow related.
I count comment 20 as "noticing"... ;)
Comment on attachment 8715652 [details] [diff] [review]
part 1.  Introduce a JS::ToJSONMaybeSafely API that's kind of like JS_Stringify but will give you a chance of not having side-effects as long as the input is nice enough

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

*holds nose until Web Crypto isn't insane about this*

::: js/src/jsapi.cpp
@@ +5196,5 @@
> +                      JSONWriteCallback callback, void* data)
> +{
> +    AssertHeapIsIdle(cx);
> +    CHECK_REQUEST(cx);
> +    assertSameCompartment(cx, input);

Blank line after this.

@@ +5197,5 @@
> +{
> +    AssertHeapIsIdle(cx);
> +    CHECK_REQUEST(cx);
> +    assertSameCompartment(cx, input);
> +    RootedValue inputValue(cx, ObjectValue(*input));

Move this down just before the Stringify call, and add blank lines between all these |if (!)| lines and the final return.  Needs room to breathe!

::: js/src/jsapi.h
@@ +4740,5 @@
> + * side-effects when the stringification is performed.  This means it does not
> + * allow a replacer or a custom space, and has the following constraints on its
> + * input:
> + *
> + * 1) The input must be a plain object, not an abitrary value.

s/object/object or array/?

@@ +4754,5 @@
> + */
> +JS_PUBLIC_API(bool)
> +ToJSONMaybeSafely(JSContext* cx, JS::HandleObject input,
> +                  JSONWriteCallback callback, void* data);
> +} /* namespace JS */

Blank line at start/end of the namespace block.

::: js/src/json.cpp
@@ +221,5 @@
> +    // We don't want to do any preprocessing here if scx->maybeSafely,
> +    // since the stuff we do here can have side-effects.
> +    if (scx->maybeSafely) {
> +        return true;
> +    }

No braces here, we didn't drink that Kool-Aid.  :-P

@@ +396,5 @@
>           */
>          id = propertyList[i];
>          RootedValue outputValue(cx);
> +        // Might be nice to assert that if scx->maybeSafely the
> +        // property is a value property....

#ifdef DEBUG
        {
            RootedNativeObject nativeObj(cx, &obj->as<NativeObject>());
            RootedShape prop(cx);
            NativeLookupOwnPropertyNoResolve(cx, nativeObj, id, &prop);
            MOZ_ASSERT(prop->isDataDescriptor());
        }
#endif

We could probably assert more here, but that probably makes sense in PreprocessValue instead.

@@ +477,5 @@
> +#ifdef DEBUG
> +            if (scx->maybeSafely) {
> +                /**
> +                 * Assert that this array has a property at this index.
> +                 */

Just a single-line // comment.

@@ +479,5 @@
> +                /**
> +                 * Assert that this array has a property at this index.
> +                 */
> +                bool propExists;
> +                if (JS_AlreadyHasOwnElement(cx, obj, i, &propExists)) {

There's no way to do this perfectly, without trying to allocate, and without doing some crazy coding thing to synthesize on the fly a non-int-valued jsid for really large indexes.  I'm really not a fan of testing a fallible operation in DEBUG-only code.  So let's just hit the 99% case of not-insanely-large arrays and otherwise do something incomplete:

#ifdef DEBUG
            if (scx->maybeSafely) {
                MOZ_ASSERT(obj->is<ArrayObject>());
                if (i <= JSID_INT_MAX) {
                    MOZ_ASSERT(obj->containsDenseElement(i) !=
                               obj->isIndexed(),
                               "the array must either be small enough to "
                               "remain fully dense (and otherwise "
                               "un-indexed), *or* all its initially-dense "
                               "elements were sparsified and the object is "
                               "indexed");
                } else {
                    MOZ_ASSERT(obj->isIndexed());
                }
            }
#endif

@@ +562,5 @@
>          if (v.isDouble()) {
> +            if (!IsFinite(v.toDouble())) {
> +                MOZ_ASSERT(!scx->maybeSafely,
> +                           "Input JS::ToJSONMaybeSafely must not include "
> +                           "reachable non-finite numbers.");

Skip the initial caps, trailing period (a general comment, throughout).

@@ +575,5 @@
>      MOZ_ASSERT(v.isObject());
>      RootedObject obj(cx, &v.toObject());
>  
> +    MOZ_ASSERT(!scx->maybeSafely || obj->is<PlainObject>() ||
> +               obj->is<ArrayObject>(),

This probably fits in 99ch, so do that for readability.

@@ +584,5 @@
>      auto dec = mozilla::MakeScopeExit([&] { scx->depth--; });
>  
>      bool isArray;
> +    if (!IsArray(cx, obj, &isArray)) {
> +        MOZ_ASSERT(!scx->maybeSafely);

I don't quite think this works -- IsArray can fail due to overrecursion and similar things, that are reachable in "safe" JSON stuff.  I think just leave this alone.

@@ +605,5 @@
> +    /**
> +     * This uses MOZ_ASSERT, since it's actually asserting something jsapi
> +     * consumers could get wrong, so needs a better error message.
> +     */
> +    MOZ_ASSERT(!maybeSafely || vp.toObject().is<PlainObject>(),

It can't also be an object?  It's a bit stupid, but can't a JWK be array syntax?

::: js/src/json.h
@@ +27,3 @@
>  extern bool
>  Stringify(JSContext* cx, js::MutableHandleValue vp, JSObject* replacer,
> +          Value space, StringBuffer& sb, bool maybeSafely = false);

Don't use a nondescript bool here, rather

  enum class StringifyBehavior {
    Normal,
    RestrictedSafe
  };

or something like that, so it's clear what callers are doing rather than it requiring looking at the declaration.
Attachment #8715652 - Flags: review?(jwalden+bmo) → review+
> s/object/object or array/?

For my purposes the input really is restricted to plain objects.  I could loosen it to object or array if you want, of course, but I felt I might as well make this as strict as I could.

> I don't quite think this works -- IsArray can fail due to overrecursion and similar things

Can it, even given that all the objects coming through here are PlainObject or ArrayObject?

> It can't also be an object?  It's a bit stupid, but can't a JWK be array syntax?

Dunno about JWK in general, but for the case of JWK in webcrypto, we're starting with a dictionary, which is an object, and representing it as JSON.  So it will definitely be an object, not an array, here.

Will apply the rest of your comments; needinfo on these three items to make sure we agree on them.
(In reply to Boris Zbarsky [:bz] from comment #25)
> > s/object/object or array/?
> 
> For my purposes the input really is restricted to plain objects.  I could
> loosen it to object or array if you want, of course, but I felt I might as
> well make this as strict as I could.

Okay.  If that's so, please be very clear in the ToJSONMaybeSafely API documentation that only plain objects are permitted, and arrays will never appear.

> > I don't quite think this works -- IsArray can fail due to overrecursion and similar things
> 
> Can it, even given that all the objects coming through here are PlainObject
> or ArrayObject?

Sure.  "Over-recursion" isn't really even accurate, it's whether the stack depth gets too large.  Invoke these algorithms with too much on the stack and they will fail in this way.  Whether that might be true right now of the crypto code, I don't know, but I'd rather not hard-code reliance on the caller having stack to spare.

> > It can't also be an object?  It's a bit stupid, but can't a JWK be array syntax?
> 
> Dunno about JWK in general, but for the case of JWK in webcrypto, we're
> starting with a dictionary, which is an object, and representing it as JSON.
> So it will definitely be an object, not an array, here.

Okay.  So this is a replay of my first response, then.  Just be super-clear that this is *only* for object graphs containing bog-standard Object instances as its only non-primitive.
No, I think there's some confusion here.

The object graph can contains arrays inside it.  But the _root_ of the graph (the entrypoint to the stringification algorithm) will always be a PlainObject.
Per IRC discussion, will just go ahead and allow an ArrayObject in addition to PlainObject as the input argument.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.