Closed Bug 484459 Opened 15 years ago Closed 15 years ago

nsIXPCComponents_Utils.evalInSandbox fails on FF3.1b3

Categories

(Core :: XPConnect, defect)

1.9.1 Branch
x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: johnjbarton, Assigned: bzbarsky)

References

Details

(Keywords: fixed1.9.1)

Attachments

(2 files, 1 obsolete file)

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);

(see http://code.google.com/p/fbug/source/browse/branches/firebug1.4/content/firebug/commandLine.js#189)

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.
Flags: blocking-firefox3.5?
The test on this line:

http://mxr.mozilla.org/mozilla1.9.1/source/js/src/xpconnect/src/xpccomponents.cpp#3526

fails and the function returns an error code.
Component: General → XPConnect
Flags: blocking-firefox3.5?
Product: Firefox → Core
QA Contact: general → xpconnect
Version: 3.1 Branch → 1.9.1 Branch
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.
Flags: blocking1.9.1?
Whiteboard: [firebug-p1]
WRT blocking, we need to figure out this issue and decide if FF will fix or Firebug will reimplement around it.
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;
        }
        else
        {
            // in FF3.1, this path works
            var sandbox = new Components.utils.Sandbox(win); // Use DOM Window
            sandbox.__proto__ = win;
        }
Ok I changed this:
    if (STOBJ_GET_CLASS(sandbox) != &SandboxClass)
        return NS_ERROR_INVALID_ARG;
to
    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 *
I changed the Firebug code to no longer call evalInSandbox.
No longer blocks: 453978
Flags: blocking1.9.1?
Whiteboard: [firebug-p1]
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.
Simon, you will have to do something to bring this bug to the attention of FF developers.
Blake, can we deal with this sanely?
Flags: wanted1.9.1?
Flags: blocking1.9.2?
Keywords: regression
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....
(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.
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.
Flags: wanted1.9.1.x?
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.
Simon, what are the changeset ids of those nightly builds on your platform?  about:buildconfig should have those.
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.
Attachment #381095 - Attachment mime type: application/x-zip-compressed → application/x-xpinstall
(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: 
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/eb22a4fcb12d

And of 2009-02-09:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/34fff0736f57
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?
Attachment #381095 - Attachment mime type: application/x-xpinstall → application/x-zip-compressed
(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.
Oh, those are 1.9.1 nightlies... Ok.  The pushlog range for comment 16 is:

http://hg.mozilla.org/releases/mozilla-1.9.1/pushloghtml?fromchange=34fff0736f57&tochange=eb22a4fcb12d

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|....
Might the fix for Bug 467900 be part of the problem?
Ah, ok.  Comment 18 helps.  That was not obvious from comment 15.

And yes, this is a regression from bug 445873.
Blocks: 445873
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.
Keywords: late-compat
Keywords: relnote
Whiteboard: [relnote: using a wrappedJSObject as the prototype of a sandbox makes the sandbox not work]
Nominating for 1.9.1, broken sandboxes aren't good.
No longer blocks: 445873
Flags: blocking1.9.1?
How now good? How common a pattern? Exploitable? Help me out here?
Attached patch Fix + test (obsolete) — Splinter Review
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.
Attachment #381352 - Flags: superreview?(mrbkap)
Attachment #381352 - Flags: review?(mrbkap)
> 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 on attachment 381352 [details] [diff] [review]
Fix + test

The test itself is missing from the patch, but r+sr=mrbkap anyway.
Attachment #381352 - Flags: superreview?(mrbkap)
Attachment #381352 - Flags: superreview+
Attachment #381352 - Flags: review?(mrbkap)
Attachment #381352 - Flags: review+
Oops.  I'd forgotten to qref.  Here's the test (minus mochitest boilerplate):

var x = 3;
var w = new XPCSafeJSObjectWrapper(window);
netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
var sandbox = new Components.utils.Sandbox(w);
sandbox.__proto__ = w;
is(Components.utils.evalInSandbox("x * 4", sandbox), 12,
   "Unexpected return from the sandbox");
I think it's worth just landing this.
Assignee: nobody → bzbarsky
Attachment #381352 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #381365 - Flags: approval1.9.1?
Comment on attachment 381365 [details] [diff] [review]
Fix + test for real

a1.9.1+=damons
Attachment #381365 - Flags: approval1.9.1? → approval1.9.1+
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 ...
As per bz over IRC, a decision I agree with.
Flags: blocking1.9.2?
Flags: blocking1.9.2+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Pushed http://hg.mozilla.org/mozilla-central/rev/206ce59bf935
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
I filed bug 496258 on what I think is the SJOW bustage here.

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1042e4baad30
Flags: wanted1.9.1?
Whiteboard: [relnote: using a wrappedJSObject as the prototype of a sandbox makes the sandbox not work]
Flags: wanted1.9.1.x?
This landed before the 1.9.2 branchpoint.
Keywords: fixed1.9.2
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.