1031336 - Components.utils.sandbox functions strip out functions from arguments

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: Scratchpad snippet to reproduce the problem

Run the attached snippet in a chrome-context scratchpad (I did it on OS X desktop, aurora 32).

Expected results:
- The alert dialog shows that foo is a function

Actual results:
- The alert dialog shows that foo is undefined

If I change the last line of the snippet to be this:

Components.utils.evalInSandbox("GM_xhr({foo:'hi'});", sandbox);

then the alert dialog indicates 'hi' as expected. For some reason it's only functions that are getting stripped out when passing arguments to functions in a sandbox.

(Note: this is a reduced test case that I came up with based on a failing Greasemonkey user script of mine that used to work previously. I /think/ this is a regression in Fx32 but haven't tested widely enough to confirm that. The same user script works on my Ubuntu box which is running Firefox 29).

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Also, for the record, the actual function with callbacks that I'm having trouble with is at https://github.com/staktrace/bugmash/blob/master/bugtags.user.js#L87

Comment 2

[Boris Zbarsky [:bzbarsky]]

This is pretty clearly documented at <https://developer.mozilla.org/en-US/docs/Xray_vision#Xray_semantics_for_Object_and_Array>, no, including the "some reason" for it?

Comment 3

[Benjamin Smedberg]

This requires https://developer.mozilla.org/en-US/docs/Components.utils.exportFunction right?

Comment 4

[Kartikaya Gupta (email:[e-mail])]

(In reply to Boris Zbarsky [:bz] from comment #2)
> This is pretty clearly documented at
> <https://developer.mozilla.org/en-US/docs/
> Xray_vision#Xray_semantics_for_Object_and_Array>, no, including the "some
> reason" for it?

Indeed, thanks for the pointer. Waiving the X-ray wrapper like so:
    alert('param.foo: ' + Components.utils.waiveXrays(param).foo);
does seem to fix it.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> This requires
> https://developer.mozilla.org/en-US/docs/Components.utils.exportFunction
> right?

This might be another way to solve it, but my n00b attempt at it didn't work. I tried this:

var sandbox = Components.utils.Sandbox(gBrowser.tabs[0].linkedBrowser._contentWindow);
let api = {
  xhr: function(param){
    alert('param.foo: ' + Components.utils.waiveXrays(param).foo);
  }
};
Components.utils.exportFunction(function wrapper(){
  api["xhr"].apply(api, arguments);
}, sandbox, {defineAs:"GM_xhr"});
Components.utils.evalInSandbox("GM_xhr({foo:function(){}});", sandbox);

and got this:

/*
Exception: CloneNonReflectorsWrite error
@Scratchpad/2:11:9
*/

Regardless, it looks like this is something that needs to be fixed in the add-on rather than my user script. CC'ing Kris since I was asking him about this yesterday with respect to the Scriptify add-on.

Comment 5

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] from comment #2)
> This is pretty clearly documented at
> <https://developer.mozilla.org/en-US/docs/
> Xray_vision#Xray_semantics_for_Object_and_Array>, no, including the "some
> reason" for it?

Well, it is as of about two weeks ago, before which that page did not exist. Also, this behavior is quite new. It only exists as of Firefox 32, and isn't documented in https://developer.mozilla.org/en-US/Firefox/Releases/32

Comment 6

[Boris Zbarsky [:bzbarsky]]

Sure.  We fixed some security holes in 32, and this was the fix...

Comment 7

[Bobby Holley (:bholley)]

(In reply to (Away until 02Jul) Kartikaya Gupta (email:[e-mail]) from comment #4)
> (In reply to Boris Zbarsky [:bz] from comment #2)
> > This is pretty clearly documented at
> > <https://developer.mozilla.org/en-US/docs/
> > Xray_vision#Xray_semantics_for_Object_and_Array>, no, including the "some
> > reason" for it?
> 
> Indeed, thanks for the pointer. Waiving the X-ray wrapper like so:
>     alert('param.foo: ' + Components.utils.waiveXrays(param).foo);
> does seem to fix it.

Waiving Xrays is generally going to open you up to security vulnerabilities.

> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> > This requires
> > https://developer.mozilla.org/en-US/docs/Components.utils.exportFunction
> > right?
> 
> This might be another way to solve it, but my n00b attempt at it didn't
> work. I tried this:
> 
> var sandbox =
> Components.utils.Sandbox(gBrowser.tabs[0].linkedBrowser._contentWindow);
> let api = {
>   xhr: function(param){
>     alert('param.foo: ' + Components.utils.waiveXrays(param).foo);
>   }
> };
> Components.utils.exportFunction(function wrapper(){
>   api["xhr"].apply(api, arguments);
> }, sandbox, {defineAs:"GM_xhr"});
> Components.utils.evalInSandbox("GM_xhr({foo:function(){}});", sandbox);

You can't pass callbacks as arguments to exported functions. This is a limitation that I think we should fix. I'll file a bug.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

I tried this on the latest nightly (which includes the patches from bug 1032457):

var sandbox = Components.utils.Sandbox(gBrowser.tabs[0].linkedBrowser._contentWindow);
let api = {
  xhr: function(param){
    alert('param.foo: ' + param.foo);
  }
};
Components.utils.exportFunction(function wrapper(){
  api["xhr"].apply(api, arguments);
}, sandbox, {defineAs:"GM_xhr", allowCallbacks:true});
Components.utils.evalInSandbox("GM_xhr({foo:function(){}});", sandbox);

and got this error:

/*
Exception: Permission denied to pass a Function via structured clone
@Scratchpad/3:11:1
*/

Not sure if I'm doing it wrong?

Comment 9

[Bobby Holley (:bholley)]

allowCallbacks only allows you to pass a callback directly - it doesn't allow callback objects of the form {foo: function(){}}. This is to avoid accidentally exposing random content functions that are encountered in the object graph. We could easily support it, but it would likely cause more problems than it solves.

Is this a fixed GM API that we're not able to change?

Comment 10

[Anthony Lieuallen]

I'm not quite clear whether/how this relates to Greasemonkey.  If it does, I'd appreciate a reduced test case that is actually a user script, and a mention of the exact FF and GM version to reproduce the error -- and exactly what the error is?

Comment 11

[Bobby Holley (:bholley)]

kats seems to be reporting that the GM_xmlhttpRequest API is getting stymied by Object Xrays, which recently landed in bug 987111 [1]. The basic issue is that you can't pull Functions off of Object Xrays.

[1] https://developer.mozilla.org/en-US/docs/Xray_vision#Xray_semantics_for_Object_and_Array

Comment 12

[Kartikaya Gupta (email:[e-mail])]

(In reply to Anthony Lieuallen from comment #10)
> I'm not quite clear whether/how this relates to Greasemonkey.  If it does,
> I'd appreciate a reduced test case that is actually a user script, and a
> mention of the exact FF and GM version to reproduce the error -- and exactly
> what the error is?

What Bobby said in comment 11. The user script is at the URL in comment 1. It's not reduced, but it's not that long to begin with. I initially ran into this problem using the latest Scriptish addon on AMO, and it persisted when I switched to a dev version (0.1.13pre.20140511.2000.de4b50f1). I then tried using the Scriptify add-on (v 0.2.6) and was able to debug the issue better with the add-on it generated.

I don't know how mutable the GM APIs are, but it sounds like in order to use the allowCallbacks flag as per comment 9 a lot of user scripts would have to be updated.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

(If you do still want a reduced standalone user script let me know, I can probably cook one up. Anything that uses GM_xmlhttpRequest with a callback function for success/error should do the job).

Comment 14

[Bobby Holley (:bholley)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)
> 
> I don't know how mutable the GM APIs are, but it sounds like in order to use
> the allowCallbacks flag as per comment 9 a lot of user scripts would have to
> be updated.

Well, GM could probably just inject some code into the sandbox that reshuffled the arguments appropriately. It's just a question of whether we want to do that, and whether we want to use exportFunction here at all.

Given that these GM scripts are already running with nsEP, the chance that they're compromised should be small, and it's probably fine to just expose the function directly and waive Xrays on the callback object.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: Scriptify patch to waive xrays

FTR I fixed this for myself locally by unpacking the scriptify-built add-on and modifying the bootstrap.js file so that it waives X-rays on the |params| object in the XHR function. I was able to figure out how to patch scriptify itself to do this as well (patch attached) but didn't look into Scriptish or GreaseMonkey to see what changes need to be made there.

I *really* like how Scriptify makes user scripts standalone so this sort of debugging and patching is possible. Thanks Kris!

Comment 16

[Anthony Lieuallen]

A very similar change makes Greasemonkey work again.  I'm not perfectly clear on the overall security of the change, however.  I'm generally comfortable calling a function I know is from the script (i.e. as we've always done), but not so much from content.  I think expanded principals means that I can trust this won't happen.  But I'm not 100% sure.

Comment 17

[Bobby Holley (:bholley)]

(In reply to Anthony Lieuallen from comment #16)
> I think expanded principals
> means that I can trust this won't happen.  But I'm not 100% sure.

It will only happen if the script is compromised to the extent that content can grab and invoke this API.

So it shouldn't happen unless the script is written poorly, but that might be the case. Defense-in-depth is good, so it might be worth checking Cu.getObjectPrincipal on the function object, and QI-ing it to nsIExpandedPrincipal. If the QI fails, don't call the function. :-)

Comment 18

[Anthony Lieuallen]

Thanks for the suggestion Bobby, but when I waive Xrays and .getObjectPrincipal() on the callback function (in the expected case), it only has nsIPrincipal, nsISupports, and nsIClassInfo interfaces.  It does have a .origin attribute, a string "[Expanded Principal]".

I can't figure out how to generate my own principal, nor get a handle to the (expanded) principal that the sandbox generated for itself, to try to compare, either.

Comment 19

[Bobby Holley (:bholley)]

(In reply to Anthony Lieuallen from comment #18)
> Thanks for the suggestion Bobby, but when I waive Xrays and
> .getObjectPrincipal() on the callback function (in the expected case), it
> only has nsIPrincipal, nsISupports, and nsIClassInfo interfaces.  It does
> have a .origin attribute, a string "[Expanded Principal]".

Yes. My suggestion in comment 17 was to check whether the principal is an expanded principal. You can do that by QI or instanceof. If you have an nsIExpandedPrincipal, you can be sure that the function lives in the user script (and not in the untrusted content).

> I can't figure out how to generate my own principal

You shouldn't need to do that.

> , nor get a handle to the
> (expanded) principal that the sandbox generated for itself, to try to
> compare, either.

You can Cu.getObjectPrincipal on the sandbox, which will give you that, from which you can call .equals(). That might be preferable to my suggestion above, though it probably doesn't matter much.

Comment 20

[Anthony Lieuallen]

Sorry, turns out I had a typo causing my failure above.  I ended up doing just what you suggested; getting the sandbox principal and checking that it .equals() the callback principal, before calling it.  Thanks again.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

By the sounds of the discussion I believe everything that was going to be fixed has been fixed, so I'm closing this bug.

Comment 22

[Will Bamberg [:wbamberg]]

Jorge, I'm assuming the dev-doc-needed flag you added was asking to add this to Firefox 32 for developers (https://developer.mozilla.org/en-US/Firefox/Releases/32) (a bit late admittedly). So I've done that, and marked this as ddc. If there's anything else you see in here that needs dev doc, please flag it again.

Comment 23

[Jorge Villalobos [:jorgev] (he/him)]

Correct, thanks.