Last Comment Bug 484459 - nsIXPCComponents_Utils.evalInSandbox fails on FF3.1b3
: nsIXPCComponents_Utils.evalInSandbox fails on FF3.1b3
: fixed1.9.1
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: 1.9.1 Branch
: x86 Windows XP
: -- normal (vote)
: mozilla1.9.2a1
Assigned To: Boris Zbarsky [:bz]
Depends on:
Blocks: 445873
  Show dependency treegraph
Reported: 2009-03-20 13:23 PDT by John J. Barton
Modified: 2009-09-02 10:47 PDT (History)
11 users (show)
mbeltzner: blocking1.9.2+
mbeltzner: blocking1.9.1-
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

A small extension that demonstrates the problem (1.62 KB, application/x-zip-compressed)
2009-06-02 09:38 PDT, Simon Stewart
no flags Details
Fix + test (1.98 KB, patch)
2009-06-03 12:34 PDT, Boris Zbarsky [:bz]
mrbkap: review+
mrbkap: superreview+
Details | Diff | Splinter Review
Fix + test for real (3.20 KB, patch)
2009-06-03 13:27 PDT, Boris Zbarsky [:bz]
dsicore: approval1.9.1+
Details | Diff | Splinter Review

Description John J. Barton 2009-03-20 13:23:25 PDT
In Firefox 3.1b3, code something like this:

    sandbox = new Components.utils.Sandbox(win);
    sandbox.__proto__ = (win.wrappedJSObject?win.wrappedJSObject:win);
    var scriptToEval = expr;

    scriptToEval = "with (window?window:null) { " + scriptToEval + " \n};";

    result = Components.utils.evalInSandbox(scriptToEval, sandbox);


Fails with
[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXPCComponents_Utils.evalInSandbox]

I did not see any bugzilla entries about this and the MDC pages don't mention ILLEGAL_VALUE exceptions. The only files in near to xpccomponents.cpp that have ILLEGAL_VALUE are *Wrapper.cpp.
Comment 1 John J. Barton 2009-03-20 13:24:49 PDT
The test on this line:

fails and the function returns an error code.
Comment 2 John J. Barton 2009-03-20 13:31:35 PDT
I traced through the code and the error message printed does not match the
internal error code. The actual error is invalid argument, the printed message
is illegal value.
Comment 3 John J. Barton 2009-03-20 13:33:21 PDT
WRT blocking, we need to figure out this issue and decide if FF will fix or Firebug will reimplement around it.
Comment 4 John J. Barton 2009-03-20 13:47:51 PDT
I simplified our code to improve clarity, the error comes when we have to take the win.wrappedJSObject path:
        if (win.wrappedJSObject) //  XPCNativeWrapper vs  XPCSafeJSObjectWrapper
            // in FF3.1, this path fails.
            var sandbox = new Components.utils.Sandbox(win.wrappedJSObject); // Use DOM Window
            sandbox.__proto__ = win.wrappedJSObject;
            // in FF3.1, this path works
            var sandbox = new Components.utils.Sandbox(win); // Use DOM Window
            sandbox.__proto__ = win;
Comment 5 John J. Barton 2009-03-20 14:13:03 PDT
Ok I changed this:
    if (STOBJ_GET_CLASS(sandbox) != &SandboxClass)
        return NS_ERROR_INVALID_ARG;
    JSClass* sandbox_class = STOBJ_GET_CLASS(sandbox);
    if (sandbox_class != &SandboxClass)
        return NS_ERROR_INVALID_ARG;
and looked at the sandbox_class value. It is
-		sandbox_class	0x0150d060 struct JSExtendedClass sXPC_SJOW_JSClass {name=0x015028d4 "XPCSafeJSObjectWrapper" flags=66820 addProperty=0x0149d060 ...}	JSClass *
Comment 6 John J. Barton 2009-03-20 17:19:32 PDT
I changed the Firebug code to no longer call evalInSandbox.
Comment 7 Simon Stewart 2009-05-26 19:49:08 PDT
This problem also affects webdriver. It would be better if this were fixed in FF, or an alternative method to run sandboxed code was provided.
Comment 8 John J. Barton 2009-05-27 11:48:59 PDT
Simon, you will have to do something to bring this bug to the attention of FF developers.
Comment 9 Boris Zbarsky [:bz] 2009-05-28 09:50:12 PDT
Blake, can we deal with this sanely?
Comment 10 Boris Zbarsky [:bz] 2009-05-28 09:51:17 PDT
John, please don't remove blocking noms just because you work around the bug?  We would have fixed this long since, if it had been on the radar....
Comment 11 John J. Barton 2009-05-28 09:59:56 PDT
(In reply to comment #10)
> John, please don't remove blocking noms just because you work around the bug? 
> We would have fixed this long since, if it had been on the radar....

Ok, sorry. I view my blocking noms as a precious resource to be conserved for things that matter to Firebug.  I was not thinking about them as a notify mechanism, which they also are as a practical matter.
Comment 12 Mike Beltzner [:beltzner, not reading bugmail] 2009-06-02 07:13:48 PDT
Providing a simplified testcase and finding a regression range would help us understand how likely a fix is for a security and stability release, here.
Comment 13 Simon Stewart 2009-06-02 08:24:39 PDT
The last nightly where this worked was 2009-02-09. The 2009-02-10 nightly doesn't appear to work as expected. I'm working on a reduced test case and will attach it when ready.
Comment 14 Boris Zbarsky [:bz] 2009-06-02 08:35:17 PDT
Simon, what are the changeset ids of those nightly builds on your platform?  about:buildconfig should have those.
Comment 15 Simon Stewart 2009-06-02 09:38:02 PDT
Created attachment 381095 [details]
A small extension that demonstrates the problem

This extension will attempt to trigger this particular problem. If the bug doesn't cause a problem, an alert with "success!" will appear, otherwise, an alert with the text of the error thrown will be displayed. Output will also be added to the error console.
Comment 16 Simon Stewart 2009-06-02 09:44:03 PDT
(In reply to comment #14)
> Simon, what are the changeset ids of those nightly builds on your platform? 
> about:buildconfig should have those.

The build of 2009-02-10:

And of 2009-02-09:
Comment 17 Boris Zbarsky [:bz] 2009-06-02 09:44:51 PDT
Comment on attachment 381095 [details]
A small extension that demonstrates the problem

Uh.. so this claims to not have an install script (so the browser won't install it).  What exactly is one supposed to do with it?
Comment 18 Simon Stewart 2009-06-02 09:50:30 PDT
(In reply to comment #17)
> (From update of attachment 381095 [details])
> Uh.. so this claims to not have an install script (so the browser won't install
> it).  What exactly is one supposed to do with it?

I just installed it as an unpacked extension. That is, added a file to my profile's extensions directory called "{75739dec-72db-4020-aa9a-6afa67447786}" and made that contain the path to the expanded zip. I had to clear out the old extensions.cache to make this work.
Comment 19 Boris Zbarsky [:bz] 2009-06-02 09:51:37 PDT
Oh, those are 1.9.1 nightlies... Ok.  The pushlog range for comment 16 is:

Bug 445873 is a change to the sandbox code in that range, but I wouldn't have thought it'd affect the value of |sandbox|....
Comment 20 Simon Stewart 2009-06-02 10:08:52 PDT
Might the fix for Bug 467900 be part of the problem?
Comment 21 Boris Zbarsky [:bz] 2009-06-02 10:26:29 PDT
Ah, ok.  Comment 18 helps.  That was not obvious from comment 15.

And yes, this is a regression from bug 445873.
Comment 22 Boris Zbarsky [:bz] 2009-06-02 11:04:59 PDT
OK, the key is that instead of using JSVAL_TO_OBJECT we're now calling js_ValueToObject to convert the jsval into a jsobject.  This is very much NOT the same thing, because js_ValueToObject calls into class convert hooks.  In particular, JS_ConvertStub calls the valueOf method on the object.  And it looks to me like SJOW will resolve and return anything you ask it for, and in particular it hands back an SJOW around obj_valueOf in this case.  Then we call it, with the SJOW as the object, of course, and get back the SJOW.

So we need to either have a convert hook on sandbox that hands back itself or change SJOW to not wrap the stub valueOf or not use js_ValueToObject here.  Or all three.
Comment 23 Christian :Biesinger (don't email me, ping me on IRC) 2009-06-03 09:17:41 PDT
Nominating for 1.9.1, broken sandboxes aren't good.
Comment 24 Mike Beltzner [:beltzner, not reading bugmail] 2009-06-03 11:58:23 PDT
How now good? How common a pattern? Exploitable? Help me out here?
Comment 25 Boris Zbarsky [:bz] 2009-06-03 12:34:33 PDT
Created attachment 381352 [details] [diff] [review]
Fix + test

This is a very simple small fix for this specific case; basically item 1 from the second paragraph of comment 22.

I still think there's an SJOW bug here, and other uses of SJOW as a proto will still behave weirdly, but we (or rather mrbkap) can worry about that later.
Comment 26 Boris Zbarsky [:bz] 2009-06-03 12:40:23 PDT
> How now good? How common a pattern? Exploitable? Help me out here?

Frequency: unknown but we haven't had many reports.  Only extensions that attempt to set the global scope of the sandbox to look like an untrusted script object are affected.

Impact: Attempt to evalInSandbox will throw.  This should not be exploitable unless the extension is _very_ broken.
Comment 27 Blake Kaplan (:mrbkap) 2009-06-03 13:02:04 PDT
Comment on attachment 381352 [details] [diff] [review]
Fix + test

The test itself is missing from the patch, but r+sr=mrbkap anyway.
Comment 28 Boris Zbarsky [:bz] 2009-06-03 13:06:26 PDT
Oops.  I'd forgotten to qref.  Here's the test (minus mochitest boilerplate):

var x = 3;
var w = new XPCSafeJSObjectWrapper(window);"UniversalXPConnect");
var sandbox = new Components.utils.Sandbox(w);
sandbox.__proto__ = w;
is(Components.utils.evalInSandbox("x * 4", sandbox), 12,
   "Unexpected return from the sandbox");
Comment 29 Boris Zbarsky [:bz] 2009-06-03 13:27:29 PDT
Created attachment 381365 [details] [diff] [review]
Fix + test for real

I think it's worth just landing this.
Comment 30 Damon Sicore (:damons) 2009-06-03 15:32:52 PDT
Comment on attachment 381365 [details] [diff] [review]
Fix + test for real

Comment 31 Mike Beltzner [:beltzner, not reading bugmail] 2009-06-03 17:12:23 PDT
Boris: if we couldn't get this for 3.5, would you be OK getting it in 3.5.1? Just need to make the blocking decision ...
Comment 32 Mike Beltzner [:beltzner, not reading bugmail] 2009-06-03 17:41:19 PDT
As per bz over IRC, a decision I agree with.
Comment 33 Boris Zbarsky [:bz] 2009-06-03 18:26:04 PDT
Comment 34 Boris Zbarsky [:bz] 2009-06-03 20:30:18 PDT
I filed bug 496258 on what I think is the SJOW bustage here.

Comment 35 Boris Zbarsky [:bz] 2009-08-24 05:38:55 PDT
This landed before the 1.9.2 branchpoint.

Note You need to log in before you can comment on or make changes to this bug.