626745 - privilege escalation using Web Console

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[shutdown]

Attachment: testcase

Web Console has a problem that allows web pages to execute arbitrary code
with chrome privileges. Seems to be affected only when Web Console is open.

Mozilla/5.0 (Windows NT 6.0; rv:2.0b10pre) Gecko/20110118 Firefox/4.0b10pre

Comment 1

[Johnathan Nightingale [:johnath]]

Thanks for the report - I've asked the team to look into it. We won't ship Firefox 4 without a fix here.

Comment 2

[Rob Campbell [:rc] (:robcee)]

cc'ing mrbkap.

I'm wondering (and have been wondering) if we need to wrapper the Console API in a forwarding proxy to better protect it. Not sure if that would fix this or not..

Comment 3

[David Dahl :ddahl]

Some notes:

our sandox is created like: 

 this.sandbox = new Cu.Sandbox(this._window,
      { sandboxPrototype: this._window, wantXrays: false });

Our execution happens like:

  return Cu.evalInSandbox(aString, this.sandbox, "1.8", "Web Console", 1)


in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/console/hudservice/HUDService.jsm?force=1

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

This is going to be fixed by either bug 553102, bug 625559 or bug 612835 (all in different, correct ways).

Comment 5

[Johnathan Nightingale [:johnath]]

Two of which are hardblockers - that's a promising thing.

Comment 6

[shutdown]

Attachment: testcase 2 using InstallTrigger

New testcase which uses InstallTrigger instead of Web Console.
This one works even if Web Console is not open.

Comment 7

[Rob Campbell [:rc] (:robcee)]

wow. Thanks for these, shutdown. Nice work.

Thanks for checking these out, Blake.

Comment 8

[David Dahl :ddahl]

The patch in bug 553102 kills this exploit:

JavaScript error: file:///home/ddahl/Desktop/web-console-exploit.html, line 12: Permission denied to access property 'constructor'

all dom and hudservice tests pass

Comment 9

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch.

Patch attached for the console.

I think that one of these three solutions should land before Fx 4: the current status quo is too dangerous.

Comment 10

[David Dahl :ddahl]

Comment on attachment 505256 [details] [diff] [review]
Proposed patch.

nice fix. a good stop gap measure for sure. I attempted a similar approach but from the scope of above the consoleAPI as I thought that was the scope that was broken into - as per my logging. anyway, this will do for now.

Comment 11

[Rob Campbell [:rc] (:robcee)]

the patch in bug 553102 will fix this.

Comment 12

[Kevin Dangoor]

Yes, but you're advocating not taking bug 553102 until fx 5. This patch seems straightforward and low-risk. It also seems easy to remove should bug 553102 land (a decision which will need to be made soon given the betaN+ blocker flag on that bug)

Comment 13

[David Dahl :ddahl]

(In reply to comment #12)
> Yes, but you're advocating not taking bug 553102 until fx 5. This patch seems
> straightforward and low-risk. It also seems easy to remove should bug 553102
> land (a decision which will need to be made soon given the betaN+ blocker flag
> on that bug)

There is 1 additional patch in security bug 612835 that should fix this as well. I assume that bug will make the next beta or rc.

All of the console tests pass with 553012's patch applied. No problem.

Comment 14

[Patrick Walton (:pcwalton)]

The reason I wrote this patch is that the discussion in bug 553102 seemed to favor chrome code explicitly setting __exposedProps__ on sensitive stuff exposed to content. Since this seems to be the best practice, I thought we should follow it in the Web Console code.

Comment 15

[David Dahl :ddahl]

(In reply to comment #14)
> The reason I wrote this patch is that the discussion in bug 553102 seemed to
> favor chrome code explicitly setting __exposedProps__ on sensitive stuff
> exposed to content. Since this seems to be the best practice, I thought we
> should follow it in the Web Console code.

The patch in bug 553102 does just that.

Comment 16

[Patrick Walton (:pcwalton)]

(In reply to comment #15)
> (In reply to comment #14)
> > The reason I wrote this patch is that the discussion in bug 553102 seemed to
> > favor chrome code explicitly setting __exposedProps__ on sensitive stuff
> > exposed to content. Since this seems to be the best practice, I thought we
> > should follow it in the Web Console code.
> 
> The patch in bug 553102 does just that.

It could break the web due to removing the __noSuchMethod__ fallback. I'll comment in the bug.

Comment 17

[David Dahl :ddahl]

(In reply to comment #16)
> 
> It could break the web due to removing the __noSuchMethod__ fallback. I'll
> comment in the bug.

I'm pretty sure we have tests for this now, don't we? as such, our test suite passed with this patch applied as is.

we test for these extras here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/console/hudservice/tests/browser/test-console-extras.html?force=1#19

perhaps we need to go back and add in the kinds of things developers have done when assuming the API was more extensive?

Comment 18

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 2.

New version of the patch uses proxies to emulate __noSuchMethod__.

Comment 19

[Patrick Walton (:pcwalton)]

Comment on attachment 505633 [details] [diff] [review]
Proposed patch, version 2.

Asking for feedback from gal to double-check my proxies.

Comment 20

[Andreas Gal :gal]

Comment on attachment 505633 [details] [diff] [review]
Proposed patch, version 2.

Looks good. Note that this patch also depends on default-safe __exposedProps__ because the descriptor you return has a Chrome Object.prototype proto.

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 505633 [details] [diff] [review]
Proposed patch, version 2.

>+    return Proxy.create({
>+      getOwnPropertyDescriptor: function(aName) { return descriptor; },
>+      getPropertyDescriptor: function(aName) { return descriptor; },
>+      getOwnPropertyNames: function() { return []; },
>+      getPropertyNames: function() { return []; },
>+      defineProperty: function(aName, aPropertyDescriptor) {},
>+      "delete": function(aName) { return false; },

No need to quote delete here.

Is there interest in a proxy-based implementation of __noSuchMethod__?

/be

Comment 22

[Patrick Walton (:pcwalton)]

(In reply to comment #21)
> Is there interest in a proxy-based implementation of __noSuchMethod__?
> 
> /be

Yes. :)

Comment 23

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #21)
> Comment on attachment 505633 [details] [diff] [review]
> Proposed patch, version 2.
> 
> >+    return Proxy.create({
> >+      getOwnPropertyDescriptor: function(aName) { return descriptor; },
> >+      getPropertyDescriptor: function(aName) { return descriptor; },
> >+      getOwnPropertyNames: function() { return []; },
> >+      getPropertyNames: function() { return []; },
> >+      defineProperty: function(aName, aPropertyDescriptor) {},
> >+      "delete": function(aName) { return false; },
> 
> No need to quote delete here.
> 
> Is there interest in a proxy-based implementation of __noSuchMethod__?

Definitely.

I got hung up while trying to implement a catch-all using Proxies last month and shelved it to work on other things. This'd be a great help.

Comment 24

[Patrick Walton (:pcwalton)]

Switching assignee to me, please let me know if that's an issue.

Comment 25

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 3.

Patch version 3 locks down the property descriptors. IMO we should block on bug 625559; it's very difficult to plug all the leaks of chrome eval.

Comment 26

[Patrick Walton (:pcwalton)]

Oops, some debug stuff sneaked into that patch.

Comment 27

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 5.

Comment 28

[Shawn Wilsher :sdwilsh]

Can you please label the sections of code (in the patch) with TODO REMOVE ME comments with the appropriate bug number of the bug that can remove the code because it is no longer needed?

This patch also does not seem to add any test cases.

Comment 29

[David Dahl :ddahl]

(In reply to comment #28)
> Can you please label the sections of code (in the patch) with TODO REMOVE ME
> comments with the appropriate bug number of the bug that can remove the code
> because it is no longer needed?
Perhaps once this lands you should file a bug to do the removals so we do not forget

> 
> This patch also does not seem to add any test cases.

You should try to replicate the errant code in your test contentDocument(s) that brought up the initial problems we encountered before adding __noSuchMethod__

Comment 30

[Shawn Wilsher :sdwilsh]

(In reply to comment #29)
s/[Yy]ou/Patrick/ per discussion in irc ;)

Comment 31

[Patrick Walton (:pcwalton)]

I actually can't seem to get the property descriptors on the wrapped "console" proxy object. Is getOwnPropertyDescriptor() implemented for wrapped proxy objects?

Comment 32

[Patrick Walton (:pcwalton)]

Never mind the above. I can get the property descriptors, but they don't seem to obey __exposedProps__. However, it doesn't seem to be possible to abuse the constructor that getOwnPropertyDescriptor() returns:

console.log((new (Object.getOwnPropertyDescriptor(console, "log").constructor.defineProperty.constructor)("alert(Components.classes)"))())

yields "Permission denied"

Comment 33

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 6.

Here's a new patch. It adds a test case and makes sure the console extension methods work.

Comment 34

[Shawn Wilsher :sdwilsh]

This doesn't seem to be addressed in the current patch:
> Can you please label the sections of code (in the patch) with TODO REMOVE ME
> comments with the appropriate bug number of the bug that can remove the code
> because it is no longer needed?

Also, it would be nice to explain with some detail why we are doing this in comments in the code.  Most people will not understand why without looking at hg blame.

Comment 35

[Patrick Walton (:pcwalton)]

(In reply to comment #34)
> This doesn't seem to be addressed in the current patch:
> > Can you please label the sections of code (in the patch) with TODO REMOVE ME
> > comments with the appropriate bug number of the bug that can remove the code
> > because it is no longer needed?

Misparsed that sentence. I'll get on that.

Comment 36

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 7.

Patch version 7 adds the requested comments.

Comment 37

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 8.

Patch version 8 adds more comments.

Comment 38

[David Dahl :ddahl]

Comment on attachment 506803 [details] [diff] [review]
Proposed patch, version 8.

Looks good. You should move 'let emptyArray = [];' below the long comment and use javadoc style comments for all properties in ConsoleAPI's prototype. Also, did you file a bug yet for the removal of this code? You should reference it prominently in the comment.

Comment 39

[Brendan Eich [:brendan]]

Filing bugs and citing them in "FIXME: bug NNNNNN" comments is an idea I first heard from Nat Friedman. It has worked well for us in SpiderMonkey. But, I think FIXME: prefixing is enough. No need for REMOVE THIS OMG LOL etc. ;-).

/be

Comment 40

[Shawn Wilsher :sdwilsh]

(In reply to comment #38)
> Looks good. You should move 'let emptyArray = [];' below the long comment and
> use javadoc style comments for all properties in ConsoleAPI's prototype. Also,
> did you file a bug yet for the removal of this code? You should reference it
> prominently in the comment.
The bug that actually fixes this can remove it.  I'm fine with citing that bug, and that patch author can fix it.

Comment 41

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 9.

Patch version 9 addresses feedback. Review requested.

Comment 42

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

For what it's worth, I don't think we need to do anything in this bug, at least not for FF4, what with all the fixes going in for XPConnect.

We do need to look at doing something in FF4+1 when we do switch the default for __exposedProps__

Comment 43

[Patrick Walton (:pcwalton)]

(In reply to comment #42)
> For what it's worth, I don't think we need to do anything in this bug, at least
> not for FF4, what with all the fixes going in for XPConnect.
> 
> We do need to look at doing something in FF4+1 when we do switch the default
> for __exposedProps__

I still think it's worth taking this patch for two reasons:
(a) It switches the console away from the deprecated __noSuchMethod__.
(b) It will keep our beta users safer until mrbkap gets to bug 625559.

Comment 44

[Shawn Wilsher :sdwilsh]

Comment on attachment 507038 [details] [diff] [review]
Proposed patch, version 9.

>+  /**
>+   * Creates a proxy for __exposedProps__ that exposes all properties.
>+   *
>+   * @return proxy
>+   *         The proxy for __exposedProps__.
You've got a weird mix of @param and @return formatting here.  Should just be:
 * @return the proxy for __exposedProps__.
 
>+  makeExposedPropsProxy: function CA_makeExposedPropsProxy() {
>+    // FIXME (bug 628903): This won't be needed once bug 553102 or bug 625559
>+    // lands. For now, we need to do this so that the Function constructor
>+    // isn't accessible either (a) via the "constructor" property; or (b) by
>+    // accessing the prototype via "__proto__", grabbing a method from it, and
>+    // asking for that method's constructor. If the Function constructor is
>+    // exposed to content, then that content can execute arbitrary code with
>+    // chrome privileges through new Function(string).
Like I said in comment 28 and comment 40, we don't need a new bug for this.  Just cite bug 553102 and bug 625559, and comment in those bugs asking form them to remove this.

The above two comments apply to other parts of this patch too.

>+++ b/dom/tests/browser/browser_ConsoleAPISecurityTests.js	Tue Jan 25 21:37:54 2011 -0800
>+const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
That's neat, but confusing.  I'm fine with it being in a test, but please don't ever use this in other code :)

r=sdwilsh

Comment 45

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 10.

Patch version 10 addresses review comments.

Comment 46

[Patrick Walton (:pcwalton)]

Now that bug 625559 has landed in tracemonkey, I'm going to mark this fixed-in-tracemonkey. When it lands on m-c, I'm going to mark this fixed and split out the proxification into a separate (non-blocking) bug.

Any objections?

Comment 47

[Rob Campbell [:rc] (:robcee)]

no objections here.

Comment 48

[Patrick Walton (:pcwalton)]

The proxification has been moved to bug 629607. This is now fixed:

http://hg.mozilla.org/mozilla-central/rev/9ac5cb7a9aee

Comment 50

[Raymond Forbes[:rforbes]]

rforbes-bugspam-for-setting-that-bounty-flag-20130719