Closed Bug 1072472 Opened 7 years ago Closed 7 years ago

Sandbox shim broken by bug 1052093

Categories

(Firefox :: Extension Compatibility, defect)

34 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 35

People

(Reporter: billm, Assigned: billm)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch fix-greasemonkey-sandbox (obsolete) — Splinter Review
Bug 1052093 makes us go through security wrappers whenever the child wants to touch an object that lives in the parent. That broke the case where the parent asks the child to create a new Cu.Sandbox: the |options| parameter is passed as a CPOW pointing to the parent, and the child is no longer able to access is because of security restrictions.

The fix is easy: just copy all the options properties to an object in the child. I added a test for this in bug 1072467.
Attachment #8494689 - Flags: review?(mconley)
Comment on attachment 8494689 [details] [diff] [review]
fix-greasemonkey-sandbox

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

::: toolkit/components/addoncompat/RemoteAddonsParent.jsm
@@ +580,5 @@
>        .rootTreeItem
>        .QueryInterface(Ci.nsIInterfaceRequestor)
>        .getInterface(Ci.nsIContentFrameMessageManager);
>  
> +    if (rest.length) {

One of the options that one can send with Cu.Sandbox is "sandboxPrototype". According to the documentation:

"A prototype object for the sandbox. The sandbox will inherit the contents of this object if it's provided."

The rest of the options are strings or bools, so they transfer fine, but aren't we going to have a problem with sandboxPrototype? Will it not also be a CPOW from the parent scope that the child will have trouble accessing?
Comment on attachment 8494689 [details] [diff] [review]
fix-greasemonkey-sandbox

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

Also note that I do see you testing sandboxPrototype in your test in bug 1072467, so I'm willing to believe that it's not a problem, but I'd like to understand why.

::: toolkit/components/addoncompat/RemoteAddonsParent.jsm
@@ +583,5 @@
>  
> +    if (rest.length) {
> +      let options = rest[0];
> +      let optionsCopy = new chromeGlobal.Object();
> +      for (let prop in options) {

Could Components.utils.cloneInto be used here instead of manually copying each property? Or am I misunderstanding how cloneInto can be used?
In the Greasemonkey case, at least, sandboxPrototype points to a window in the content process. Since we're creating the sandbox in the content process, we don't have to worry about CPOWs at all there.

Cu.cloneInto does a deep clone using structured cloning. It would probably try to recurse into the sandboxPrototype and clone its individual properties, which we definitely don't want.

I can add comments about this.
Added a comment.
Attachment #8494689 - Attachment is obsolete: true
Attachment #8494689 - Flags: review?(mconley)
Attachment #8496137 - Flags: review?(mconley)
Comment on attachment 8496137 [details] [diff] [review]
fix-greasemonkey-sandbox v2

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

Ok, understood. Thanks for clearing that up. :)
Attachment #8496137 - Flags: review?(mconley) → review+
https://hg.mozilla.org/mozilla-central/rev/a029e4d3181b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.