1260917 - Sandbox needs to handle userContextId correctly

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Dave Huseby [:huseby]]

In the file Sandbox.cpp there is a call to CreateCodebasePrincipal here:

https://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/Sandbox.cpp#1240

The comments suggest that creating a sandbox from a string is deprecated.  I think we need to force this issue.  We need to either fix this function to properly pass origin attributes or we need to remove it from the code.

Comment 1

[Tim Huang[:timhuang]]

The code here is used exclusively for creating sandbox with string URI, so I think there are two approaches to make this right depending on whether should we allow the origin suffix here. For one thing, if we do not allow the origin suffix been used when creating a sandbox, then we ought to use default contextual Id here, and it appears that the code here is correct. For another, if the origin suffix is allowed here, then we should use the populateFromOrigin to populate origin attributes from the URI.

However, I am not sure which behavior is proper here. What do you think, Dave?

Comment 2

[Dave Huseby [:huseby]]

I think the right thing to do here is to remove this function.  The comments say that it is deprecated.  In comment 1 I said that we need to force the issue.  ni? bobby and/or bz and see if we can remove this.

Comment 3

[Tim Huang[:timhuang]]

Could we remove the function of creating a sandbox from string URL?

Comment 4

[Boris Zbarsky [:bzbarsky]]

I suspect not without breaking lots of addons, but you should query the addons mxr to see how often it's used.

Comment 5

[Tim Huang[:timhuang]]

Attachment: WIP - Removing the function of creating a sandbox from a string URL.

Comment 6

[Tim Huang[:timhuang]]

It looks like many addons use this feature, but most of them use this feature to open 'about:blank' [1]. And not only addons, but also many of our test codes utilize string URLs to create sandboxes. It could break lots of things if we simply remove this function.

[1] https://mxr.mozilla.org/addons/search?string=.sandbox

Comment 7

[Boris Zbarsky [:bzbarsky]]

We can fix our test code, obviously.  The addons situation is a bigger problem.

Comment 8

[Bobby Holley (:bholley)]

Creating sandboxes from strings is one of the most common things ever, so I don't think we should deprecate it.

We should obviously allow people to pass in a userContextId in the sandbox options. But aside from that I think that creating a sandbox is a kind of primitive operation (like creating a principal) where we need the caller to tell us the right thing to do.

Comment 9

[Tim Huang[:timhuang]]

Attachment: Add a 'userContextId' in the sandbox options, and sandbox will reference this when creating from string url.

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8748100 [details] [diff] [review]
Add a 'userContextId' in the sandbox options, and sandbox will reference this when creating from string url.

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

::: js/xpconnect/src/Sandbox.cpp
@@ +1235,5 @@
>   * we use the related Codebase Principal for the sandbox.
>   */
>  bool
> +ParsePrincipal(JSContext* cx, HandleString codebase, nsIPrincipal** principal,
> +               SandboxOptions& options)

Make this arg const, and put it before the nsIPrincipal** outparam.

@@ +1572,5 @@
>                ParseJSString("addonId", &addonId) &&
>                ParseBoolean("writeToGlobalPrototype", &writeToGlobalPrototype) &&
>                ParseGlobalProperties() &&
> +              ParseValue("metadata", &metadata) &&
> +              ParseValue("userContextId", &userContextId);

This will need to be a newly-added ParseUInt32.

@@ +1655,5 @@
>          bool isArray;
>          if (!JS_IsArrayObject(cx, obj, &isArray)) {
>              ok = false;
>          } else if (isArray) {
> +            ok = GetExpandedPrincipal(cx, obj, getter_AddRefs(expanded), options);

I don't think we should support nsEP construction with a userContextId. Let's just throw in this branch if options.userContextId != 0.

::: js/xpconnect/src/xpcprivate.h
@@ +3448,5 @@
>      bool invisibleToDebugger;
>      bool discardSource;
>      GlobalProperties globalProperties;
>      JS::RootedValue metadata;
> +    JS::RootedValue userContextId;

Let's parse this directly to a unit32_t.

Comment 11

[Tim Huang[:timhuang]]

Attachment: patch-1260917-v2

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8748451 [details] [diff] [review]
patch-1260917-v2

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

r=me with those fixes.

::: js/xpconnect/src/Sandbox.cpp
@@ +1235,5 @@
>   * we use the related Codebase Principal for the sandbox.
>   */
>  bool
> +ParsePrincipal(JSContext* cx, HandleString codebase, const SandboxOptions& options,
> +               nsIPrincipal** principal)

Instead of passing the SandboxOptions, we should pass a const PrincipalOriginAttributes& aAttrs, and set them appropriately in the caller.

@@ +1659,5 @@
> +                                                     : nullptr);
> +
> +    SandboxOptions options(cx, optionsObject);
> +    if (calledWithOptions && !options.Parse())
> +        return ThrowAndFail(NS_ERROR_INVALID_ARG, cx, _retval);

We can then declare the PrincipalOriginAttributes here, assign over the userContextId, and pass it to ParsePrincipal below.

@@ +1679,5 @@
>          } else if (isArray) {
> +            if (options.userContextId == 0) {
> +                ok = GetExpandedPrincipal(cx, obj, getter_AddRefs(expanded));
> +                prinOrSop = expanded;
> +            }

Instead of doing it like this, do:

if (options.userContextId != 0) {
  // We don't support passing a userContextId with an array.
  ok = false;
} else {
  ok = GetExpandedPrincipal(cx, obj, getter_AddRefs(expanded));
  prinOrSop = expanded; 
}

Comment 13

[Tim Huang[:timhuang]]

Attachment: Patch-1260917-v3

Fix comments from comment 12. And r=me because bholley already r+ed.

Comment 14

[Tim Huang[:timhuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=72a08d4d6d91

Comment 15

[Tim Huang[:timhuang]]

Treeherder shows everything is good.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2dc69a7eda3

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Ethan Tseng [:ethan]]

Tim and Bobby, thanks for fixing this bug! Cheers!