Current release and beta builds vulnerable (13 and 14). 1. Open the attached POC 2. In the Web Console, inspect the paragraph element: > document.getElementById("p"); HUDService.jsm plays it loose with (wrapperless) eval results. JST_execute passes the eval result into formatResult, which allows the content-supplied object to override toSource or toString to return whatever it likes. From there, it hits writeOutputJS and createMessageNode. (Side note: for fun, starting with writeOutputJS, follow the call graph and check out all the places the unsafe aOutputObject can visit: <http://mxr.mozilla.org/mozilla-beta/source/browser/devtools/webconsole/HUDService.jsm#5059>.) If the toSource/toString override returned a DOM node, that node eventually gets inserted into the browser.xul document(!). From there, we can do the ol' "exploit on* event attributes to run with chrome privileges". Plausible attack: Take something interesting, like a prouget demo, and insert a variation of this exploit. Use Proxies (those are available to content, right?) to ensure that virtually any expression that the user enters will result in an object with a malicious toSource/toString. This POC doesn't work with Aurora and nightly builds, due to the async console split (bug 673148) and further changes. It's possible that HUDService-content.js is vulnerable, though; I simply haven't looked into it. Sorry for the drive-by and hurried comment. It's late and I have early morning obligations.

> that node eventually gets inserted into the browser.xul document Uh... we don't just treat what we think is a string as a _string_? Why the heck not???

That is correct. Confirmed. The proof of concept works - in Firefox 14. Going to look into potential issues with Firefox Nightly as well. I assume we want a fix for Firefox 14, is this correct? I'll look into doing that as well. Colby: can you also please analyze HUDService-content.js for any potential issues? Thank you very much!

Did some analysis and the security problem seems to be caused by a number of problems in the HUDService.jsm code. 1. we take the JS eval result and try to pretty-print it - we use toString, toSource, etc. 2. then we give whatever we have to the createMessageNode() method which *can* output nodes, not just strings. When it sees a node, the node is added into the Web Console output as-is - hence we have the security issue. However, do note we pretty-print also any object we get from console.log() calls (or almost any other window.console API call). I tested the POC with a small change and I can see toSource() is also executed, but in the logConsoleAPIMessage() we make sure we always give a string to createMessageNode(), so the exploit of adding a page-injected node into the Web Console output does not work. I'd like to know which problems do we need to mitigate to consider this security issue fixed? Problem 2 *and* problem 1? or just problem 2? Problem 2 looks easy to fix. For problem 1, as explained above, we would need a way to avoid executing *any* page script. Is there a way to xray-wrap sandbox eval results? That would help us with fixing this problem. Thank you!

This is the proposed fix, with a test included. Do note this applies, currently, only for mozilla-beta. As explained in comment 4 this patch fixes only "problem 2". Looking forward for your review and feedback. Thank you!

At a guess, problem 2 is the only one that we need to fix. Problem 1 is not an issue because the toSource or whatever should run with the page's principal....

Thanks Boris! Then the attached patch should fix the security issue. I hope there are no other attack vectors left open.

The need to actually inspect something in the console mitigates the severity of this somewhat, but it's still remote code execution with a relatively low bar, so I think sec-critical fits. We should probably avoid landing the test in this bug until we can sort out our landing story for beta. Or at the very least, better mask the severity of the problem by using a different filename and test comments. Is ESR affected? How about 15?

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #8) > The need to actually inspect something in the console mitigates the severity > of this somewhat, but it's still remote code execution with a relatively low > bar, so I think sec-critical fits. > > We should probably avoid landing the test in this bug until we can sort out > our landing story for beta. Or at the very least, better mask the severity > of the problem by using a different filename and test comments. > > Is ESR affected? How about 15? ESR, afaik, doesn't include async web console patches. IIِANM, many versions until Firefox 14 beta are affected (including ESR). Can you please point me to the ESR repo? We need to test, to make sure. Based on short testing, Firefox 15 does not seem affected due to the async web console patches which change a lot of code (need to do more testing!). The problem doesn't manifest itself. However, the formatResult() has the same problem - it doesn't guarantee that a string is returned. So, I'd like to port the fix to newer versions - including the test. How do you propose we mask the test? New filename and comments suggestions? Thank you!

The ESR code is https://hg.mozilla.org/releases/mozilla-esr10/, there are nightly builds at http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-esr10/ . To mask the test, you should talk in vague terms about what the actual bug is, and avoid mentioning things like "eval allows chrome-privileged content script" and "test content script injection into chrome code". Instead, you can make generic statements like "ensure console output is properly formatted as a string". Basically, try to make it as hard as possible for someone looking at the patch to figure out how to exploit users.

(In reply to Mihai Sucan [:msucan] from comment #9) > ESR, afaik, doesn't include async web console patches. IIِANM, many versions > until Firefox 14 beta are affected (including ESR). Can you please point me > to the ESR repo? We need to test, to make sure. ESR is affected.

Comment on attachment 640056 [details] [diff] [review] proposed fix Review of attachment 640056 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/webconsole/HUDService.jsm @@ +5153,5 @@ > } > break; > } > > + return String(output); This can throw if neither output's toString nor valueOf return a primitive value. It's a very minor edge case, but you might want to handle it nonetheless.

(In reply to Mihai Sucan [:msucan] from comment #3) > Colby: can you also please analyze HUDService-content.js for any potential > issues? Thank you very much! Tomorrow evening for this is a possibility, but unlikely. More realistic for me would be Tuesday morning. Given that you know the code, though, you should be able to pick up on the issue if it's there. Just trace the code paths that unsafe objects take, and verify that what you think what you're dealing *should* be can *only* be that thing. For example, that formatResults's return value can only ever be a string.

Colby: thank you for your feedback. Looking forward for your analysis of HUDService-content.js. Will update the patch I posted and I'll make patches for fx-team and for ESR.

Updated the patch to address Colby's feedback and to make wording more discreet, as Gavin suggested. Please let me know if further changes are needed. Thank you!

Rebased the patch for mozilla-esr10.

Comment on attachment 640232 [details] [diff] [review] updated patch for beta looks good. I might add a typeof "string" check in the test to be extra specific.

This is the patch for fx-team - forward-ported the fix from beta to latest code. Do note that the exploit does not work since bug 673148 (async web console patches). However, I do want us to have the formatResult() method fix and the test in the latest code as well. Thanks! Colby: I looked into HUDService-content.js and I didn't find anything obvious. Since I am not a security expert, I trust your analysis will be more conclusive. Thanks!

ok, ignore the typeof request. Unnecessary.

Comment on attachment 640232 [details] [diff] [review] updated patch for beta Try push: https://tbpl.mozilla.org/?tree=Try&rev=4af3c51e434e

Comment on attachment 640232 [details] [diff] [review] updated patch for beta [Approval Request Comment] Bug caused by (feature/regressing bug #): see comment #0 and comment #4. Problem is not caused by any recent patch. User impact if declined: users are susceptible to an exploit that can inject content scripts into the browser. Testing completed (on m-c, etc.): green try push. https://tbpl.mozilla.org/?tree=Try&rev=4af3c51e434e Risk to taking this patch (and alternatives if risky): minimal risks, close to none. String or UUID changes made by this patch: none.

Comment on attachment 640243 [details] [diff] [review] patch for esr10 (canceling review request, since it's pretty much the same patch as the one already reviewed by Rob) [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: User impact if declined: users are susceptible to an exploit that can inject content scripts into the browser. Fix Landed on Version: will land in beta once approved. will land in fx-team ASAP. Risk to taking this patch (and alternatives if risky): minimal risks, close to none. String or UUID changes made by this patch: none.

Comment on attachment 640255 [details] [diff] [review] patch for fx-team Canceling review request, since it's pretty much the same patch as the one already reviewed by Rob.

Gavin: how do we go about landing this patch? I could land this patch in fx-team, but I wonder if there's any special procedure I need to follow. Thanks!

As agreed with Rob, I opened bug 772506 and landed the patch in fx-team.

The PoC doesn't seem to work in aurora or nightly, did something change in our wrappers that happened to fix this (we have taken a couple xpconnect fixes)? Or was it accidentally masked?

Comment on attachment 640232 [details] [diff] [review] updated patch for beta [Triage Comment] This missed our final beta, so it will only be considered as part of a 14.0.2 (if there is one).

Setting ESR tracking flag back to nomination -- if there's a chance we take this on a 14.0.2 I wouldn't want to miss this because the flag was set to 15+

(In reply to Daniel Veditz [:dveditz] from comment #27) > The PoC doesn't seem to work in aurora or nightly, did something change in > our wrappers that happened to fix this (we have taken a couple xpconnect > fixes)? Or was it accidentally masked? It's unsafe handling of unwrapped objects in Web Console code, not buggy wrappers. On Aurora, lots of the HUDService implementation changed, so it's no longer an issue. See comment 0 and comment 9.

(In reply to Mihai Sucan [:msucan] from comment #19) > Colby: I looked into HUDService-content.js and I didn't find anything > obvious. Since I am not a security expert, I trust your analysis will be > more conclusive. Thanks! :D I'm no security expert, or any type of security… guy. Again, I don't want you coming away thinking that, "The Web console got an in-depth security audit, and it passed." Really. I looked at HUDService-content.js. Even though we already knew that it wouldn't be affected in the same way, nothing stood out to me that indicates it would allow the same type of attack with other kinds of specially-crafted objects, since console stuff gets turned into JSON and back when it passes through the message manager. Having said that, let me say now that: 1. Foremost, all the relevant code is new to me, and I'm seeing it for the first time, so it's hard for me to follow. I get lost somewhere around the _objectCache stuff. 2. From what I've looked at and understood of the async code, there are similar types of bugs to this one, where even if they don't turn out to be exploitable security concerns (and they still may!), they have the potential to cause the Web Console to behave ways that people would call it buggy. Let me explain. I found the bug I described in comment 0 and that we're discussing now when I happened to be looking at the HUDService code, and after checking formatResult, this error was staring me in the face: > let resultString = this.formatResult(result); http://mxr.mozilla.org/mozilla-beta/source/browser/devtools/webconsole/HUDService.jsm#4524 After that, I spent some time following things through to see if this bug propagated itself into anything exploitable, and it did. This was caused by a mismatch between what you assume formatResult has to return, and the actual range of values it can, given its implementation. There are similar bad assumptions that I found in the async code, like getResultType: > /** > * Determine the type of the jsterm execution result. > * > * @param mixed aResult > * The evaluation result object you want to check. > * @return string > * Constructor name or type: string, number, boolean, regexp, date, > * function, object, null, undefined... > */ > getResultType: function WCU_getResultType(aResult) > { > let type = aResult === null ? "null" : typeof aResult; > if (type == "object" && aResult.constructor && aResult.constructor.name) { > type = aResult.constructor.name; > } > > return type.toLowerCase(); > }, Here, you assume that aResult.constructor is a function, and aResult.constructor.name will be a string, and that if you call toLowerCase() on it, that it will give you back a string that matches the semantics of String.prototype.toLowerCase. But that can be overridden, and it may very well end up returning an object that behaves in ways you didn't expect when you pass it back to the caller. Boris, the evalInSandbox page on MDN <https://developer.mozilla.org/en/Components.utils.evalInSandbox#Security> says: > To understand how the security risk arises, let's take the example of > (x == 1). When this is evaluated, the x.valueOf() method gets called by > chrome code. When this happens, unsafe code can create a String object in > the chrome code's global scope. > > If the chrome code has added a function that has chrome privileges to > String.prototype, Object.prototype, or Function.prototype, then unsafe > code can abuse that function. Is this still a problem with the wrappers and compartmentalization we have today? What about when passing chrome stuff to a potentially unsafe object's method? <http://mxr.mozilla.org/mozilla-beta/source/browser/devtools/webconsole/HUDService.jsm#4524> (I think XOWs take care of this one.)

(In reply to Colby Russell :crussell from comment #31) > I looked at HUDService-content.js. Even though we already knew that it > wouldn't be affected in the same way, nothing stood out to me that indicates > it would allow the same type of attack with other kinds of specially-crafted > objects, since console stuff gets turned into JSON and back when it passes > through the message manager. > > Having said that, let me say now that: > 1. Foremost, all the relevant code is new to me, and I'm seeing > it for the first time, so it's hard for me to follow. I get > lost somewhere around the _objectCache stuff. > 2. From what I've looked at and understood of the async code, > there are similar types of bugs to this one, where even if > they don't turn out to be exploitable security concerns (and > they still may!), they have the potential to cause the Web > Console to behave ways that people would call it buggy. Let > me explain. > > I found the bug I described in comment 0 and that we're discussing now when > I happened to be looking at the HUDService code, and after checking > formatResult, this error was staring me in the face: > > let resultString = this.formatResult(result); > > http://mxr.mozilla.org/mozilla-beta/source/browser/devtools/webconsole/ > HUDService.jsm#4524 Yep, good point. This is why I also made a patch for mozilla-central. > After that, I spent some time following things through to see if this bug > propagated itself into anything exploitable, and it did. This was caused by > a mismatch between what you assume formatResult has to return, and the > actual range of values it can, given its implementation. > > There are similar bad assumptions that I found in the async code, like > getResultType: > > > /** > > * Determine the type of the jsterm execution result. > > * > > * @param mixed aResult > > * The evaluation result object you want to check. > > * @return string > > * Constructor name or type: string, number, boolean, regexp, date, > > * function, object, null, undefined... > > */ > > getResultType: function WCU_getResultType(aResult) > > { > > let type = aResult === null ? "null" : typeof aResult; > > if (type == "object" && aResult.constructor && aResult.constructor.name) { > > type = aResult.constructor.name; > > } > > > > return type.toLowerCase(); > > }, > > Here, you assume that aResult.constructor is a function, and > aResult.constructor.name will be a string, and that if you call > toLowerCase() on it, that it will give you back a string that matches the > semantics of String.prototype.toLowerCase. But that can be overridden, and > it may very well end up returning an object that behaves in ways you didn't > expect when you pass it back to the caller. Another good point. Thank you! What I tried to follow, when looking into HUDService-content.js was if the assumptions we make are exploitable. At some point we need to make some assumptions - anyway. If things break - as you said - behavior just appears to be buggy. Unfortunately, simply trying to enumerate the JS result can result in arbitrary content script execution, which can throw weird/random exceptions or it can do unexpected things. My main concerns are more of the nature: will that code execute with content or chrome privileges? If it only executes with content privileges, then that's fine. Does it badly break the Web Console functionality? If yes, let's have bug reports and fixes, but catering to all corner-cases will probably not be effective. > Boris, the evalInSandbox page on MDN > <https://developer.mozilla.org/en/Components.utils.evalInSandbox#Security> > says: > > To understand how the security risk arises, let's take the example of > > (x == 1). When this is evaluated, the x.valueOf() method gets called by > > chrome code. When this happens, unsafe code can create a String object in > > the chrome code's global scope. > > > > If the chrome code has added a function that has chrome privileges to > > String.prototype, Object.prototype, or Function.prototype, then unsafe > > code can abuse that function. > > Is this still a problem with the wrappers and compartmentalization we have > today? > > What about when passing chrome stuff to a potentially unsafe object's > method? > <http://mxr.mozilla.org/mozilla-beta/source/browser/devtools/webconsole/ > HUDService.jsm#4524> (I think XOWs take care of this one.) Thank you for raising these important questions. I'd like us to have some kind of JS sandbox eval result wrapper that prevents accidental arbitrary code evaluation. Boris, can you please provide us with more details about these things? Thanks!

Comment on attachment 640243 [details] [diff] [review] patch for esr10 since this isn't going out with beta 14, we can't take it on ESR yet. re-nom if this goes into a 14.0.2 or when it's time for 15.

(In reply to Lukas Blakk [:lsblakk] from comment #33) > or when it's time for 15. Ignore that part, I see 15 is unaffected.

Comment on attachment 640232 [details] [diff] [review] updated patch for beta Review of attachment 640232 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/webconsole/HUDService.jsm @@ +5156,5 @@ > } > break; > } > > + return output + ""; This can throw for the same reasons String(output) would, by the way: js> var fum = ({toString: function() this, valueOf: function() this}); fum + "" typein:1: TypeError: can't convert fum to primitive type

(In reply to Colby Russell :crussell from comment #35) > Comment on attachment 640232 [details] [diff] [review] > updated patch for beta > > Review of attachment 640232 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/devtools/webconsole/HUDService.jsm > @@ +5156,5 @@ > > } > > break; > > } > > > > + return output + ""; > > This can throw for the same reasons String(output) would, by the way: > > js> var fum = ({toString: function() this, valueOf: function() this}); fum + > "" > typein:1: TypeError: can't convert fum to primitive type Sure, but you are trying just a part of the formatResult() method, so your result is not the same to that in the Web Console. Here's what I get in the Web Console: [20:59:45.383] foo = {foo:"bar", omg: "test", toSource: function() this, toString: function() this, valueOf: function() this} [20:59:45.389] TypeError: can't convert Object to string And that result is clickable - you can inspect the object. I display the error, since I had no idea what to display when we can't figure out a string representation for the given object. Thanks for looking into the code.

is there anything left to do with this bug? Can it be closed?

Does the patch here apply to ESR since it is affected?

Comment on attachment 640243 [details] [diff] [review] patch for esr10 re-noming for ESR 15+

Comment on attachment 640243 [details] [diff] [review] patch for esr10 [Triage comment] Let's get this landed on ESR now that merge is over and it will ship with Firefox 15.

(In reply to Al Billings [:abillings] from comment #39) > Does the patch here apply to ESR since it is affected? Yes, the patch for esr10 is what we can land in ESR10. See attachment 640243 [details] [diff] [review]. Daniel, thanks for renominating my patch for esr. Lukas, thanks for the approval! I am confused on two problems: 1. how shall I land this patch? 2. esr10 or esr15? I prepared a patch for esr10, not esr15. Firefox 15 is unaffected. Thank you!

"esr10" is the current ESR. It is version 15+ of that, coming up.

The flag is "tracking-firefox-esr10" to indicate the esr10 branch; the value "15+" means "tracking+ for the 10.0.x that ships when Firefox 15 does". We considered using the value of x but then everyone has to remember or keep looking up that (10.0.)6+ matches Firefox 14, 7+ matches Fx15, etc. Then the offset changes when we have a "chemspill" release and everyone has to learn a new pattern.

(In reply to Daniel Veditz [:dveditz] from comment #44) > The flag is "tracking-firefox-esr10" to indicate the esr10 branch; the value > "15+" means "tracking+ for the 10.0.x that ships when Firefox 15 does". > > We considered using the value of x but then everyone has to remember or keep > looking up that (10.0.)6+ matches Firefox 14, 7+ matches Fx15, etc. Then the > offset changes when we have a "chemspill" release and everyone has to learn > a new pattern. Thanks for your explanation. If I get it right my next action should be to land in the esr10 repository the patch I have for Firefox 14 (which is esr10) - that would be attachment 640243 [details] [diff] [review]. Is this correct? And this will ship to esr10 users when Firefox 15 will be released. (sorry, I am trying to figure out what I need to do)

Yes, all you have to do is land it in the repository https://hg.mozilla.org/releases/mozilla-esr10/ (which is based on Firefox 10) like any other branch and the rest will be taken care of. :)

(well, also set the status-firefox-esr10 to "fixed" in this bug)

Thank you! Will land the patch tomorrow (it's late here).

Do I use a cover up bug? Shall I reuse bug 772506 or open a new bug?

Dan said it doesn't matter too much, but you can just use the cover bug since you have it anyways.

(In reply to Andrew McCreight [:mccr8] from comment #50) > Dan said it doesn't matter too much, but you can just use the cover bug > since you have it anyways. Due to delays and me being away towards the evening, I didn't get to land the patch in esr10 yet - as I wanted. I will try again to land it tomorrow. Apologies for that.

Updated the patch to mention bug 772506 - the cover-up bug. Landed: https://hg.mozilla.org/releases/mozilla-esr10/rev/bd76cc13e6a8

I'm knocking down the rating from sec-critical to sec-high because it can't be used for a drive-by attack, only a subset of users (the curious ones) could fall victim. Still qualifies for a Bug Bounty though.

Firefox 14 flag says it is affected but Firefox 15 is unaffected? I read through the comments but I'm missing how we fixed this for 15 without actually fixing it. Can someone explain? I see a mozilla-central checkin in comment 37 but that's it.

Lots of the web console code was rewritten for bug 673148. The method implementations and even the methods themselves that comprise the offending code paths don't exist anymore in 15+.

Does this need a test in-testsuite or a manual test?

Verified fixed using ESR 10.0.11.