Closed
Bug 484459
Opened 16 years ago
Closed 15 years ago
nsIXPCComponents_Utils.evalInSandbox fails on FF3.1b3
Categories
(Core :: XPConnect, defect)
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)
(deleted),
application/x-zip-compressed
|
Details | |
(deleted),
patch
|
damons
:
approval1.9.1+
|
Details | Diff | Splinter Review |
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•16 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.
Updated•16 years ago
|
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•16 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•16 years ago
|
Flags: blocking1.9.1?
Whiteboard: [firebug-p1]
Reporter | ||
Comment 3•16 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•16 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•16 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•16 years ago
|
||
I changed the Firebug code to no longer call evalInSandbox.
Comment 7•16 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•16 years ago
|
||
Simon, you will have to do something to bring this bug to the attention of FF developers.
Assignee | ||
Comment 9•16 years ago
|
||
Blake, can we deal with this sanely?
Assignee | ||
Comment 10•16 years ago
|
||
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•16 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.
Comment 12•16 years ago
|
||
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•16 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.
Assignee | ||
Comment 14•16 years ago
|
||
Simon, what are the changeset ids of those nightly builds on your platform? about:buildconfig should have those.
Comment 15•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Attachment #381095 -
Attachment mime type: application/x-zip-compressed → application/x-xpinstall
Comment 16•16 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
Assignee | ||
Comment 17•16 years ago
|
||
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•16 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.
Assignee | ||
Comment 19•16 years ago
|
||
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•16 years ago
|
||
Might the fix for Bug 467900 be part of the problem?
Assignee | ||
Comment 21•16 years ago
|
||
Ah, ok. Comment 18 helps. That was not obvious from comment 15.
And yes, this is a regression from bug 445873.
Blocks: 445873
Assignee | ||
Comment 22•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Keywords: late-compat
Assignee | ||
Updated•16 years ago
|
Keywords: relnote
Whiteboard: [relnote: using a wrappedJSObject as the prototype of a sandbox makes the sandbox not work]
Comment 23•15 years ago
|
||
Nominating for 1.9.1, broken sandboxes aren't good.
No longer blocks: 445873
Flags: blocking1.9.1?
Comment 24•15 years ago
|
||
How now good? How common a pattern? Exploitable? Help me out here?
Assignee | ||
Comment 25•15 years ago
|
||
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)
Assignee | ||
Comment 26•15 years ago
|
||
> 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•15 years ago
|
||
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+
Assignee | ||
Comment 28•15 years ago
|
||
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");
Assignee | ||
Comment 29•15 years ago
|
||
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 30•15 years ago
|
||
Comment on attachment 381365 [details] [diff] [review]
Fix + test for real
a1.9.1+=damons
Attachment #381365 -
Flags: approval1.9.1? → approval1.9.1+
Comment 31•15 years ago
|
||
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•15 years ago
|
||
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-
Assignee | ||
Comment 33•15 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Assignee | ||
Comment 34•15 years ago
|
||
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]
Updated•15 years ago
|
Flags: wanted1.9.1.x?
Assignee | ||
Comment 35•15 years ago
|
||
This landed before the 1.9.2 branchpoint.
Keywords: fixed1.9.2
Target Milestone: --- → mozilla1.9.2a1
Updated•15 years ago
|
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.
Description
•