Sandbox needs to handle userContextId correctly

RESOLVED FIXED in Firefox 49

Status

()

P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: huseby, Assigned: timhuang)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
Component: DOM → DOM: Security
(Assignee)

Updated

3 years ago
Assignee: nobody → tihuang
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
(Reporter)

Updated

3 years ago
Whiteboard: [userContextId] → [userContextId][OA]
Whiteboard: [userContextId][OA] → [userContextId][OA][domsecurity-active]
(Assignee)

Comment 1

3 years ago
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)
(Reporter)

Comment 2

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

Comment 3

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

Comment 5

3 years ago
Created attachment 8748042 [details] [diff] [review]
WIP - Removing the function of creating a sandbox from a string URL.
(Assignee)

Comment 6

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

Comment 9

3 years ago
Created attachment 8748100 [details] [diff] [review]
Add a 'userContextId' in the sandbox options, and sandbox will reference this when creating from string url.
Attachment #8748100 - Flags: review?(bobbyholley)
(Assignee)

Updated

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

Comment 11

3 years ago
Created attachment 8748451 [details] [diff] [review]
patch-1260917-v2
Attachment #8748451 - Flags: review?(bobbyholley)
(Assignee)

Updated

3 years ago
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+
(Assignee)

Comment 13

3 years ago
Created attachment 8748990 [details] [diff] [review]
Patch-1260917-v3

Fix comments from comment 12. And r=me because bholley already r+ed.
Attachment #8748990 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8748451 - Attachment is obsolete: true
(Assignee)

Comment 15

3 years ago
Treeherder shows everything is good.
Keywords: checkin-needed

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d2dc69a7eda3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox49: --- → fixed
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.