Closed Bug 1260917 Opened 6 years ago Closed 6 years ago

Sandbox needs to handle userContextId correctly

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: huseby, Assigned: timhuang)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [userContextId][OA][domsecurity-active])

Attachments

(1 file, 3 obsolete files)

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.
Component: DOM → DOM: Security
Assignee: nobody → tihuang
Status: NEW → ASSIGNED
Whiteboard: [userContextId] → [userContextId][OA]
Whiteboard: [userContextId][OA] → [userContextId][OA][domsecurity-active]
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?
Flags: needinfo?(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.
Flags: needinfo?(huseby)
Could we remove the function of creating a sandbox from string URL?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bobbyholley)
I suspect not without breaking lots of addons, but you should query the addons mxr to see how often it's used.
Flags: needinfo?(bzbarsky)
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
We can fix our test code, obviously.  The addons situation is a bigger problem.
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.
Flags: needinfo?(bobbyholley)
Attachment #8748042 - Attachment is obsolete: true
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.
Attachment #8748100 - Flags: review?(bobbyholley) → review-
Attached patch patch-1260917-v2 (obsolete) — Splinter Review
Attachment #8748451 - Flags: review?(bobbyholley)
Attachment #8748100 - Attachment is obsolete: true
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; 
}
Attachment #8748451 - Flags: review?(bobbyholley) → review+
Attached patch Patch-1260917-v3Splinter Review
Fix comments from comment 12. And r=me because bholley already r+ed.
Attachment #8748990 - Flags: review+
Attachment #8748451 - Attachment is obsolete: true
Treeherder shows everything is good.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d2dc69a7eda3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Priority: -- → P1
Tim and Bobby, thanks for fixing this bug! Cheers!
Blocks: 1271873
You need to log in before you can comment on or make changes to this bug.