421593 - Firebug "command-line code" regression

Status: RESOLVED FIXED

(Core :: Security, defect, P2)

Description

[Justin Dolske [:Dolske]]

[Filing by request of mrbkap...]

In bug 344751 comment 40, moz_bug writes:

> * When Firebug evaluates a command line code, content can access __scope__.api
> and abuse it.
> 
> By the way, there is a regression: the command line stuff no longer works,
> since it tries to access a privileged object via SJOW in a sandbox and fails. 
> (But, an exploit code can work even with this regression.)

Regressions bad. :(

moz_bug: can you clarify what "command line code" you're talking about here?

Comment 1

[moz_bug_r_a4]

Attachment: Screenshot of Firebug Console

When Firebug evaluates a command line code, __win__ is a SJOW, and
__win__.__scope__ is a privileged object.

commandLine.js:

// XXXjjb FF3  needs win.__scope__ because we eval in sandbox

const evalScript = "with (__win__.__scope__.vars) { with (__win__.__scope__.api) { with (__win__.__scope__.userVars) { with (__win__) {" +
    "try {" +
        "__win__.__scope__.callback(eval(__win__.__scope__.expr));" +
    "} catch (exc) {" +
        "__win__.__scope__.callback(exc, true);" +
    "}" +
"}}}}";

Firebug.CommandLine = extend(Firebug.Module,
{
...
    evaluate: function(expr, context, userVars, thisValue)
    {
...
        var scriptToEval = thisValue ? evalScriptWithThis : evalScript;
...
                Components.utils.evalInSandbox(scriptToEval, sandbox);

Comment 2

[Justin Dolske [:Dolske]]

From http://code.google.com/p/fbug/issues/detail?id=507:

1. open firebug's JS console
2. type alert('test');
3. get error:

commandLine.evaluate FAILS: [Exception... "Security Manager vetoed action"
nsresult: "0x80570027 (NS_ERROR_XPC_SECURITY_MANAGER_VETO)" location: "JS
frame :: chrome://firebug/content/commandLine.js :: <TOP_LEVEL> :: line
100" data: no]

Was reported on  with FB 1.1.0b12 & Gecko/2008030607 Minefield/3.0b5pre

Comment 3

[Johnny Stenback (:jst)]

Marking this a blocker, but if this is something that can be fixed in Firebug then this shouldn't be a Firefox blocker.

Comment 4

[Brian Crowder]

Breaking into the debugger at the time this exception is thrown:

The subjPrincipal:  codebase = 0x1bd84a48 "http://www.google.com/"
The objPrincipal:   codebase = 0x65b488 "[System Principal]"

Comment 5

[Brian Crowder]

The problem here is this:  chrome has installed a property __scope__ (chrome principals) on __win__ and is then trying to access that property from within a sandbox (content principals) because of the complicated |with| expression:

const evalScript = "with (__win__.__scope__.vars) { with (__win__.__scope__.api) { with (__win__.__scope__.userVars) { with (__win__) {" +
    "try {" +
        "__win__.__scope__.callback(eval(__win__.__scope__.expr));" +
    "} catch (exc) {" +
        "__win__.__scope__.callback(exc, true);" +
    "}" +
"}}}}";

Comment 6

[Brian Crowder]

Thanks to jst for walking me through the logic of this, btw.

Comment 7

[Brian Crowder]

To continue remarks from comment #5: in essence this is a firebug bug.  Firebug formerly engaged in the highly-unsafe behavior of hooking an object with chrome privileges onto a content object, and then accessing that chrome-object from within a content script.  We no longer allow this dangerous behavior, and so firebug is foiled.  We need a fix -in- the firebug source.

Comment 8

[John J. Barton]

(In reply to comment #4)
> Breaking into the debugger at the time this exception is thrown:
> 
> The subjPrincipal:  codebase = 0x1bd84a48 "http://www.google.com/"
> The objPrincipal:   codebase = 0x65b488 "[System Principal]"
> 
This is the kind of information that extension developers need in the exception message. I have known about this problem since b5pre started but I had no clue how to even think about fixing it.

Comment 9

[John J. Barton]

(In reply to comment #7)
> To continue remarks from comment #5: in essence this is a firebug bug.  Firebug
> formerly engaged in the highly-unsafe behavior of hooking an object with chrome
> privileges onto a content object, and then accessing that chrome-object from
> within a content script.  We no longer allow this dangerous behavior, and so
> firebug is foiled.  We need a fix -in- the firebug source.
> 

I don't see how to save evalInSandbox under these constraints. Firebug needs to create an environment for the command line eval.  The act of creation will be in chrome code. The objects in this environment are (or should be) safe (they are from the user page), but since the environment is created in chrome and objects from chrome are fragile, the eval cannot be safe.  

Does anyone see a way to create the environment safely?

(I have to say that evalInSandbox was already on the top of my list of things to get rid of, since creating the correct environment has proven difficult).

Comment 10

[John J. Barton]

(In reply to comment #0)
> [Filing by request of mrbkap...]
> 
> In bug 344751 comment 40, moz_bug writes:
> 
> > * When Firebug evaluates a command line code, content can access __scope__.api
> > and abuse it.
> > 
> > By the way, there is a regression: the command line stuff no longer works,
> > since it tries to access a privileged object via SJOW in a sandbox and fails. 

There was no regression. FF3b5 changed the semantics of evalInSandbox and firebug failed. 

> > (But, an exploit code can work even with this regression.)

How? You said the command line failed.

Comment 11

[John J. Barton]

(In reply to comment #3)
> Marking this a blocker, but if this is something that can be fixed in Firebug
> then this shouldn't be a Firefox blocker.
> 
IMO the blocker should be 423890, since if the exception had adequate information I would have already fixed the command line.

Comment 12

[moz_bug_r_a4]

(In reply to comment #10)
> > > (But, an exploit code can work even with this regression.)
> 
> How? You said the command line failed.
> 

The exploit code runs before Firebug calls evalInSandbox().

Comment 13

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I think https://bugzilla.mozilla.org/show_bug.cgi?id=423890#c3 is relevant here (I didn't see this bug in my list at the time, because I wasn't logged in).

Comment 14

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Marking as a beta blocker, though if this is solved on the Firebug side per bug 423890 comment 3 then we can deprioritize here.

Comment 15

[Justin Dolske [:Dolske]]

Attachment: Patch, work in progress

This is a patch against Firebug's commandLine.js. Still a work-in-progress, but the command line seems to work now. Firebug.Debugger.evaluate() will probably need similar treatment.

Comment 16

[Mike Schroepfer]

Justin - is this just on the firebug side or does it require core code?

Comment 17

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

It looks like this case is Firebug-side, unless we hit a wall with this approach tonight.

Comment 18

[Mike Schroepfer]

Ok - if so I'm setting priority/TM to match this *not* a b5 blocker.  Please reset if you get new data...

Comment 19

[John J. Barton]

(In reply to comment #15)
> Created an attachment (id=311469) [details]
> Patch, work in progress
> 
> This is a patch against Firebug's commandLine.js. Still a work-in-progress, but
> the command line seems to work now.

That's what I thought the first time I got evalInSandbox to eval something. But the stuff you commented out is the stuff that is the problem ;-)

Comment 20

[John J. Barton]

Since the result of evalInSandbox will be mixed with other readouts from the page, see also https://bugzilla.mozilla.org/show_bug.cgi?id=350558

Comment 21

[Justin Dolske [:Dolske]]

Attachment: Patch, work in progress 2

This cleans up the debugger side, and removes some other places that were using |Firebug.CommandLine.evalScript|.

Bits I'm still unsure about:

* These places checking for indexOf(FB.CL.evalScript), when were they triggered? I'm not sure what they did, so I'm not sure if we need a different workaround.

* Is it safe to do |sandbox.foo = bar|, if untrusted script has already run in the sandbox. (|foo| could be a setter?)

* The evaluate() in debugger.js previously allowed for a case where frame.exec() was invoked with |this| set to some different object. I'm not sure how this case would be useful? If it's needed, there needs to be a place for Firebug to stick its |thisValue|, so it can wrap the expression with function.apply()... I'm not sure what to twiddle on the jsdIStackFrame to do something like that. [Or if that even makes sense -- why would you want to change |this| when evaluating something in a backtrace?]

Comment 22

[Justin Dolske [:Dolske]]

bug 423890 comment 6 mentions a few firebug bugs which may have been depending on the __scope__ hack. With my patched Firebug, most are working now. [http://code.google.com/p/fbug/issues/detail?id=###]

521: pass - Works.
513: pass - Works.
459: fail - "new XMLHttpRequest();" throws NS_ERROR_XPC_NOT_ENOUGH_ARGS
428: pass - seems to work (bug is kind of vague)
379: pass - I don't think this is a bug (iframe security checks).

Comment 23

[John J. Barton]

(In reply to comment #21)
> Created an attachment (id=311512) [details]
> Patch, work in progress 2
> 
> This cleans up the debugger side, and removes some other places that were using
> |Firebug.CommandLine.evalScript|.
> 
> Bits I'm still unsure about:
> 
> * These places checking for indexOf(FB.CL.evalScript), when were they
> triggered? I'm not sure what they did, so I'm not sure if we need a different
> workaround.

When the javascript debugger is on the stack.

> 
> * Is it safe to do |sandbox.foo = bar|, if untrusted script has already run in
> the sandbox. (|foo| could be a setter?)

Also what's the meaning of "foo" if bar is a wrapper of some sort.

> 
> * The evaluate() in debugger.js previously allowed for a case where
> frame.exec() was invoked with |this| set to some different object. I'm not sure
> how this case would be useful? If it's needed, there needs to be a place for
> Firebug to stick its |thisValue|, so it can wrap the expression with
> function.apply()... I'm not sure what to twiddle on the jsdIStackFrame to do
> something like that. [Or if that even makes sense -- why would you want to
> change |this| when evaluating something in a backtrace?]
> 

When the javascript debugger is on the stack and someone edits an object, eventually Firebug will do 
    object[name] = Firebug.CommandLine.evaluate(value, this.context, null, object);

Comment 24

[John J. Barton]

I applied this patch (well except for the removal of .evalCommandLine) and tried to use it to evaluate an object onto the page eg,
 window.console = new Foo();
The code executes and window.console is correct. But then when I look into the page later there is no window.console object. 

So I continue to not understand evalInSandbox.  I guess now that anything you do in the sandbox is not real or something?

Comment 25

[John J. Barton]

The window object needs to be the unwrapped js object:
                sandbox.window = win.wrappedJSObject;

Comment 26

[Justin Dolske [:Dolske]]

Attachment: Patch, work in progress 2.1

Small update.

(In reply to comment #25)
> The window object needs to be the unwrapped js object:
>                 sandbox.window = win.wrappedJSObject;

I don't think that's needed now (at least with this patch). One does need to be careful with getSandboxByWindow(), because it's comparing windows against sandbox.window... If nothing matches, a new sandbox is created, and things that were only stored on the old sandbox seem to go away. [It might be good to look at just removing the sandbox caching, that would eliminate a possible route for memory leaks.]

Comment 27

[Justin Dolske [:Dolske]]

Attachment: Testcase #1, v.1

This is a testcase to try things through the evalInSandbox() path (ie, when the JS debugger isn't being run).

Issues:

#4 - If a evaled snipped returns |undefined|, Firebug doesn't seem to show that properly in the console.

#6.5 - eIS("foo = bar") is apparently setting |foo| on the sandbox object, and not the page's global object... So, if the page's script then looks at |foo|, it doesn't see the value that was assigned.

#11 - |var xhr = new XMLHttpRequest();| throws NS_ERROR_XPC_NOT_ENOUGH_ARGS. I don't understand why, haven't traced into it.

Comment 28

[Justin Dolske [:Dolske]]

Attachment: Testcase #2, v.1

This is a testcase to try things through the frame.eval() path (ie, when the JS debugger is running).

Issues:

#4, #6 - (same |undefined| issue the other test.)

#12 - |setTimeout(function(){ alert("test"); }, 2000);| never seems to invoke the alert(), even after continuing script execution.

#13 - (same XHR issue as in the other test.)

Comment 29

[John J. Barton]

(In reply to comment #27)

> #6.5 - eIS("foo = bar") is apparently setting |foo| on the sandbox object, and
> not the page's global object... So, if the page's script then looks at |foo|,
> it doesn't see the value that was assigned.

This is the symptom that caused me to set 
 sandbox.window = win.wrappedJSObject;
because otherwise the values set on sandbox.window stay in the sandbox on the wrapper and don't punch through to the page object.

Comment 30

[John J. Barton]

What is the API for objects returned by evalInSandBox?

Comment 31

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

(Patch v.2.1 has already been rolled into Firebug)

Two changes here:

* Wraps the expression for the sandbox with "with(window) { ... }". This fixes the problem ("#6.5") I described in comment 27.

* When the CommandLine.evaluate() was called with a thisObject, is was using "(function {...}).apply(thisObject)". Added a return in there so that the expression's value actually gets returned to the caller.

Also, a patch I sent to JJB separately fixes Firebug to show |undefined| values in the console (the issues mentioned in comment 27/28, which were not regressions anyway). So, the only testcase problem not working is the "new XHR()" thing, which didn't work previously either.

Comment 32

[John J. Barton]

There are two bits of functionality missing, corresponding to the two parts of __scope__:
                api       : context.commandLineAPI,
                vars      : getInspectorVars(context),
I suppose commandLineAPI needs to be injected into the sandbox the way I did the console.

The inspector vars need to be "with", analyzed for wrappers and security.

Comment 33

[John J. Barton]

I am returning to my former position that evalInSandbox does not work. The new command line in Firebug 1.2 can handle simple cases, but I keep hitting problems. For example these two expressions fail:

var time = new Date().getTime();

Array.prototype

This leads me to believe that the environment in evalInSandbox is not equivalent to the web page and hence won't work for the command line.

Comment 34

[John J. Barton]

I now have a (possibly overly complicated) prototype for the command line that only relies on evalInSandbox to get itself loaded. 

In operation the command line sets the eval expr in an attribute of a firebug-added element in the web page, then issues an event on that element. The page has an event handler to eval() the expr and return the result through the console object.  This seems to work and expressions like
  var time = new Date().getTime(); time;
work.

The command line is added on-demand when the user sets the cursor on the command line (focus event handler). All of this event and script inject stuff is complicated but secure I suppose.

The current implementation requires users to issue "loadFirebugConsole()" before the first use of the console object.  We'll pay for that with lots of complaints but I don't know how to avoid it.  This function is injected into the window with evalInSandbox using a string script.

Comment 35

[Johnny Stenback (:jst)]

Doesn't seem like there's anything here that we need to hold the *Firefox* release for. Given that this needs to be fixed in Firebug, and that work is already ongoing, I'm reassigning this bug to you, John, and removing the blocking1.9+ flag.

Comment 36

[Brian Crowder]

This bug is both a) fixed by recent firebug evolution and b) recent changes that cause evalInSandbox to work as it should -- not clear what those are.