nsIXPCComponents_Utils.evalInSandbox fails on FF3.1b3

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
XPConnect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: John J. Barton, Assigned: bz)

Tracking

({fixed1.9.1})

1.9.1 Branch
mozilla1.9.2a1
x86
Windows XP
fixed1.9.1
Points:
---
Bug Flags:
blocking1.9.2 +
blocking1.9.1 -
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

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

Comment 2

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

Updated

8 years ago
Flags: blocking1.9.1?
Whiteboard: [firebug-p1]
(Reporter)

Comment 3

8 years ago
WRT blocking, we need to figure out this issue and decide if FF will fix or Firebug will reimplement around it.
(Reporter)

Comment 4

8 years ago
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;
        }
(Reporter)

Comment 5

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

Comment 6

8 years ago
I changed the Firebug code to no longer call evalInSandbox.
No longer blocks: 453978
Flags: blocking1.9.1?
Whiteboard: [firebug-p1]

Comment 7

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

Comment 8

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

Comment 11

8 years ago
(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?

Comment 13

8 years ago
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.

Comment 15

8 years ago
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.
Attachment #381095 - Attachment mime type: application/x-zip-compressed → application/x-xpinstall

Comment 16

8 years ago
(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

Comment 18

8 years ago
(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|....

Comment 20

8 years ago
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?
Blocks: 445873
How now good? How common a pattern? Exploitable? Help me out here?
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.
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");
Created attachment 381365 [details] [diff] [review]
Fix + test for real

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
Last Resolved: 8 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?
Keywords: late-compat, regression, relnote → fixed1.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
status1.9.2: --- → beta1-fixed
Keywords: fixed1.9.2
You need to log in before you can comment on or make changes to this bug.