privilege escalation using Web Console

RESOLVED FIXED

Status

DevTools
General
RESOLVED FIXED
8 years ago
a month ago

People

(Reporter: shutdown, Assigned: pcwalton)

Tracking

Trunk
x86
Windows Vista
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [hardblocker][sg:critical][real solution is bug 625559][fixed-in-tracemonkey])

Attachments

(3 attachments, 8 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 504813 [details]
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
blocking2.0: --- → final+
Whiteboard: [hardblocker]
Thanks for the report - I've asked the team to look into it. We won't ship Firefox 4 without a fix here.

Updated

8 years ago
Assignee: nobody → ddahl
Whiteboard: [hardblocker] → [hardblocker][sg:critical]
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

8 years ago
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
This is going to be fixed by either bug 553102, bug 625559 or bug 612835 (all in different, correct ways).
Two of which are hardblockers - that's a promising thing.
(Reporter)

Comment 6

8 years ago
Created attachment 504935 [details]
testcase 2 using InstallTrigger

New testcase which uses InstallTrigger instead of Web Console.
This one works even if Web Console is not open.
wow. Thanks for these, shutdown. Nice work.

Thanks for checking these out, Blake.

Comment 8

8 years ago
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
(Assignee)

Comment 9

8 years ago
Created attachment 505256 [details] [diff] [review]
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.
Attachment #505256 - Flags: feedback?(ddahl)

Comment 10

8 years ago
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.
Attachment #505256 - Flags: feedback?(ddahl) → feedback+
(Assignee)

Updated

8 years ago
Attachment #505256 - Flags: review?(gavin.sharp)
the patch in bug 553102 will fix this.

Comment 12

8 years ago
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

8 years ago
(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.
(Assignee)

Comment 14

8 years ago
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.

Updated

8 years ago
Attachment #505256 - Flags: review?(gavin.sharp)

Comment 15

8 years ago
(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.
(Assignee)

Comment 16

8 years ago
(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

8 years ago
(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?
(Assignee)

Comment 18

8 years ago
Created attachment 505633 [details] [diff] [review]
Proposed patch, version 2.

New version of the patch uses proxies to emulate __noSuchMethod__.
Attachment #505633 - Flags: feedback?(ddahl)
(Assignee)

Comment 19

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

Asking for feedback from gal to double-check my proxies.
Attachment #505633 - Flags: feedback?(gal)

Comment 20

8 years ago
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.
Attachment #505633 - Flags: feedback?(gal) → feedback+
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
(Assignee)

Comment 22

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

Yes. :)
(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.
(Assignee)

Comment 24

8 years ago
Switching assignee to me, please let me know if that's an issue.
Assignee: ddahl → pwalton
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 25

8 years ago
Created attachment 506476 [details] [diff] [review]
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.
Attachment #505256 - Attachment is obsolete: true
Attachment #505633 - Attachment is obsolete: true
Attachment #506476 - Flags: review?
Attachment #506476 - Flags: feedback?(ddahl)
Attachment #505633 - Flags: feedback?(ddahl)
(Assignee)

Updated

8 years ago
Whiteboard: [hardblocker][sg:critical] → [hardblocker][sg:critical][real solution is bug 625559]
(Assignee)

Updated

8 years ago
Attachment #506476 - Flags: review? → review?(sdwilsh)
(Assignee)

Comment 26

8 years ago
Oops, some debug stuff sneaked into that patch.
(Assignee)

Updated

8 years ago
Attachment #506476 - Attachment is obsolete: true
Attachment #506476 - Flags: review?(sdwilsh)
Attachment #506476 - Flags: feedback?(ddahl)
(Assignee)

Comment 27

8 years ago
Created attachment 506479 [details] [diff] [review]
Proposed patch, version 5.
Attachment #506479 - Flags: review?(sdwilsh)
Attachment #506479 - Flags: feedback?(ddahl)
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

8 years ago
(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__
(In reply to comment #29)
s/[Yy]ou/Patrick/ per discussion in irc ;)
(Assignee)

Comment 31

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

Comment 32

8 years ago
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"
(Assignee)

Comment 33

8 years ago
Created attachment 506625 [details] [diff] [review]
Proposed patch, version 6.

Here's a new patch. It adds a test case and makes sure the console extension methods work.
Attachment #506479 - Attachment is obsolete: true
Attachment #506625 - Flags: review?(sdwilsh)
Attachment #506625 - Flags: feedback?(ddahl)
Attachment #506479 - Flags: review?(sdwilsh)
Attachment #506479 - Flags: feedback?(ddahl)
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.
(Assignee)

Comment 35

8 years ago
(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.
(Assignee)

Comment 36

8 years ago
Created attachment 506790 [details] [diff] [review]
Proposed patch, version 7.

Patch version 7 adds the requested comments.
Attachment #506625 - Attachment is obsolete: true
Attachment #506790 - Flags: review?(sdwilsh)
Attachment #506790 - Flags: feedback?(ddahl)
Attachment #506625 - Flags: review?(sdwilsh)
Attachment #506625 - Flags: feedback?(ddahl)
(Assignee)

Comment 37

8 years ago
Created attachment 506803 [details] [diff] [review]
Proposed patch, version 8.

Patch version 8 adds more comments.
Attachment #506790 - Attachment is obsolete: true
Attachment #506803 - Flags: review?(sdwilsh)
Attachment #506803 - Flags: feedback?(ddahl)
Attachment #506790 - Flags: review?(sdwilsh)
Attachment #506790 - Flags: feedback?(ddahl)

Comment 38

8 years ago
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.
Attachment #506803 - Flags: feedback?(ddahl) → feedback+
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
(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.
(Assignee)

Updated

8 years ago
Blocks: 628903
(Assignee)

Comment 41

8 years ago
Created attachment 507038 [details] [diff] [review]
Proposed patch, version 9.

Patch version 9 addresses feedback. Review requested.
Attachment #506803 - Attachment is obsolete: true
Attachment #507038 - Flags: review?(sdwilsh)
Attachment #506803 - Flags: review?(sdwilsh)
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__
(Assignee)

Comment 43

8 years ago
(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 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
Attachment #507038 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 45

8 years ago
Created attachment 507195 [details] [diff] [review]
Proposed patch, version 10.

Patch version 10 addresses review comments.
Attachment #507038 - Attachment is obsolete: true
(Assignee)

Comment 46

8 years ago
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?
Whiteboard: [hardblocker][sg:critical][real solution is bug 625559] → [hardblocker][sg:critical][real solution is bug 625559][fixed-in-tracemonkey]
no objections here.
(Assignee)

Comment 48

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

http://hg.mozilla.org/mozilla-central/rev/9ac5cb7a9aee
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Attachment #504935 - Attachment description: testcase 2 → testcase 2 using InstallTrigger
Group: core-security
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Flags: sec-bounty+

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.