426619 - evaluating "new Date()" using evalInSandbox fails

Status: VERIFIED WORKSFORME

(Core :: Security, defect, P2)

Description

[Justin Dolske [:Dolske]]

Spunoff from bug 421593.

Evaling code like "new Date()" or "new XMLHttpRequest();" in a sandbox can unexpectedly throw a NS_ERROR_XPC_NOT_ENOUGH_ARGS error.

The following is sufficient to recreate the problem (when run from a chrome context, where |domWin| is a DOM window for some untrusted page):

var sandbox = new Components.utils.Sandbox(domWin);
sandbox.__proto__ = domWin.wrappedJSObject;
var result =  Components.utils.evalInSandbox("new Date()", sandbox);

The problem seems to be a result of setting __proto__ to the wrappedJSObject. If the above code uses |sandbox.__proto__ = domWin;| it works fine. However, that isn't an acceptable workaround, because then evaluating code like "foo = 1" sets foo on the XPCNativeWrapper, where the page content can't see it.

Comment 1

[John J. Barton]

While I agree that this needs to be fixed, it does not need to block 421593 since I have an implementation for the commandLine that does not use evalInSandbox.

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Testcase

I'm not sure what the easiest way to tests this is, since you need both (1) chrome context and (2) a DOM window. So, this is rather hacky, but since I know password manager code I just stuck this in there. :-) The WebProgressListener where I stuck this gets called during page load.

Comment 3

[Damon Sicore (:damons)]

What else does this break?  Since firebug is working around this issue, can we do this in 1.9.0.x?

Comment 4

[Brian Crowder]

It seems like this would break greasemonkey at least, depending on the userscript.  I'd tend to agree that this should block.  Not sure why no one else has complained about it already.

Comment 5

[Damon Sicore (:damons)]

+'ing based on Comment #4.  

Blake, you get to pick who fixes this because there's no way you have time.  Who's it going to be?

Comment 6

[Damon Sicore (:damons)]

Fixing priority to P2.

Comment 7

[John J. Barton]

changed title so I can find it

Comment 8

[Brian Crowder]

I added dolske's patch:

var sandbox = new Components.utils.Sandbox(targetWindow);
sandbox.__proto__ = targetWindow.wrappedJSObject;
try {
  result = Components.utils.evalInSandbox(expr, sandbox)
} catch (e) {
  result = "XXX threw: " + e;
}
dump("======= result: " + result + "\n");

to the firebug1.2 commandLine code and was able to see the following result on the debug console (not the firebug console):

======= result: Thu Apr 10 2008 16:29:50 GMT-0600 (MDT)

Comment 9

[Brian Crowder]

re: comment 8, I was using the "expr" |new Date()|, of course.

Comment 10

[John J. Barton]

Hmm... just checking: you know that we re-implemented the firebug 1.2 command line so that it no longer goes thru evalInSandbox (and thus new Date() works in any case).

I'm pretty sure that dolske's handy patch gets the window state correct but not the built-in objects, so for example:
new Array().constructor == [].constructor
will be false see http://code.google.com/p/fbug/issues/detail?id=559&q=label:commandline

Comment 11

[Brian Crowder]

Yes, I'm skirting that observer path.  Just used dolske's patch in firebug because it seemed like an easy way to have something interactive.  His "result" path doesn't use your observer, just the evalInSandbox route.  Here's the result of evaluating

>>> new Array().constructor == [].constructor

from the firebug command line using dolske's hack:

======= result: true

even:

>>> new Array().constructor === [].constructor
yields
======= result: true

This is real evalInSandbox, really working.  Not the observer hack.

Comment 12

[Justin Dolske [:Dolske]]

Yeah, my testcase works now (Firebug not involved). Any idea what fixed this?

Comment 13

[Brian Crowder]

Nope, not a clue.  An anti-regression range would be nice, but I haven't got the time.