Last Comment Bug 796866 - (CVE-2012-4210) Arbitrary code execution from Style Inspector
(CVE-2012-4210)
: Arbitrary code execution from Style Inspector
Status: RESOLVED FIXED
[fixed-in-fx-team][adv-track-main17+]...
: csectype-priv-escalation, sec-critical
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: 15 Branch
: All All
: P1 critical (vote)
: Firefox 19
Assigned To: Dave Camp (:dcamp)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-02 03:10 PDT by Mariusz Mlynski
Modified: 2014-07-24 14:37 PDT (History)
16 users (show)
rforbes: sec‑bounty+
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix
+
verified
+
verified
+
verified
17+
verified
verified


Attachments
An exploit for Windows, opens C:\Windows\System32\calc.exe (691 bytes, application/java-archive)
2012-10-02 03:10 PDT, Mariusz Mlynski
no flags Details
Proof of concept (576 bytes, application/octet-stream)
2012-10-02 03:34 PDT, Mariusz Mlynski
no flags Details
textContent instead of innerHTML (1.24 KB, patch)
2012-10-03 12:57 PDT, Dave Camp (:dcamp)
rcampbell: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
dveditz: sec‑approval+
Details | Diff | Review
esr10 fix (1.31 KB, patch)
2012-10-29 19:47 PDT, Dave Camp (:dcamp)
akeybl: approval‑mozilla‑esr10+
Details | Diff | Review

Description Mariusz Mlynski 2012-10-02 03:10:42 PDT
Created attachment 666916 [details]
An exploit for Windows, opens C:\Windows\System32\calc.exe

It is possible to execute arbitrary code when a user inspects computed styles on a malicious website containing a specially crafted style sheet.

The problem lies in the function PropertyView_refresh in CssHtmlTree.jsm, which writes user-controlled CSS values to innerHTML without sanitizing the input. This paves the way for an HTML markup injection in the privileged context of chrome://browser/content/devtools/csshtmltree.xul
Comment 1 Mariusz Mlynski 2012-10-02 03:34:32 PDT
Created attachment 666919 [details]
Proof of concept

Here's a generic PoC for all systems, it just throws Components.classes to the error console.
Comment 3 Rob Campbell [:rc] (:robcee) 2012-10-03 12:25:03 PDT
hunh!

so we should be doing a little scrubbing of javascript: uris in stylesheets, presumably.
Comment 4 Dave Camp (:dcamp) 2012-10-03 12:57:46 PDT
Created attachment 667611 [details] [diff] [review]
textContent instead of innerHTML

Seem reasonable?

The only other uses of innerHTML in that file are to set it to "" to clear nodes.
Comment 5 Rob Campbell [:rc] (:robcee) 2012-10-03 15:15:02 PDT
Comment on attachment 667611 [details] [diff] [review]
textContent instead of innerHTML

ok by me based on our discussion earlier. gavin can extra r+ if he feels the need.
Comment 6 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-10-03 15:40:15 PDT
TL;DR - I don't think we have a systemic set of failings, but we do have some things to think about.

From `grep -r innerHTML devtools`, most of the stuff was one of:
- tests
- rvalues
- foo.innerHTML = "";

I talked a few things over with dcamp, and didn't find any obviously exploitable holes.

In GCLI, are there any commands that expose user data via string concatenation? We're mostly safe because generally commands don't do that, and when they do, they use the template engine, which makes it safe (I think). Also we use innerHTML for menu elements. I'm more sure that that's safe, but want to think more about it for the future.

Also Orion:
- 3,257: (contentsDiv = this._htmlParent).innerHTML = contents; 
- 9,497: widthDiv.innerHTML = annotation.html; 
- 9,525: lineDiv.innerHTML = annotation.html; 
- 9,572: lineDiv.innerHTML = annotation.html; 
The last 3 look safe to me. I have no idea about the first.
Comment 7 Mark Goodwin [:mgoodwin] 2012-10-04 09:55:18 PDT
(In reply to Dave Camp (:dcamp) from comment #4)
> Created attachment 667611 [details] [diff] [review]
> textContent instead of innerHTML
> 
> Seem reasonable?
> 
> The only other uses of innerHTML in that file are to set it to "" to clear
> nodes.

Yes, this is reasonable.
Comment 8 Dave Camp (:dcamp) 2012-10-04 10:43:42 PDT
Comment on attachment 667611 [details] [diff] [review]
textContent instead of innerHTML

[Security approval request comment]
How easily can the security issue be deduced from the patch?

* Pretty easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

* The code itself is a bit of a bulls-eye.

Which older supported branches are affected by this flaw?

* All supported.

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?

* Not risky

How likely is this patch to cause regressions; how much testing does it need?

* Very unlikely, existing in-tree testing should be fine.
Comment 9 Daniel Veditz [:dveditz] 2012-10-04 13:32:03 PDT
Comment on attachment 667611 [details] [diff] [review]
textContent instead of innerHTML

We can't land this a few days before the release of Firefox 16 without potentially causing a re-spin. If there is one anyway maybe akeybl will approve it, but otherwise we should wait a couple of weeks (~ Oct 24?)

We may find other instances of this anti-pattern we can fix at the same time all at once (mgoodwin is looking).
Comment 10 bhavana bajaj [:bajaj] 2012-10-04 16:16:49 PDT
tracking for 17+, assuming it will land on 17 beta . we would not do a chemspill just for this bug but will leave tracking-16 ? so that we can take it as a ride along but even if we do a respin of 16 , we would not do a chemspill esr for this.
Comment 11 Daniel Veditz [:dveditz] 2012-10-21 00:36:44 PDT
Comment on attachment 667611 [details] [diff] [review]
textContent instead of innerHTML

sec-approval+, this can start landing now.
Comment 12 Rob Campbell [:rc] (:robcee) 2012-10-23 09:51:48 PDT
Do we need to submerge this in another patch?
Comment 13 Alex Keybl [:akeybl] 2012-10-23 16:27:15 PDT
dveditz/abillings will be able to answer comment 12, but I just want to remind you that this will need to be nominated for uplift asap to make it into FF17.
Comment 14 Al Billings [:abillings] 2012-10-23 17:04:47 PDT
Rob, I don't think we need to submerge it but I wouldn't say "no" either if there was something convenient to do so.
Comment 15 Dave Camp (:dcamp) 2012-10-25 10:24:22 PDT
Comment on attachment 667611 [details] [diff] [review]
textContent instead of innerHTML

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Style Inspector initial landing
User impact if declined: Security
Testing completed (on m-c, etc.): none yet 
Risk to taking this patch (and alternatives if risky): Low-risk
String or UUID changes made by this patch: None.
Comment 16 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-25 13:36:58 PDT
Comment on attachment 667611 [details] [diff] [review]
textContent instead of innerHTML

approving for branch uplift - we'll want this for esr10 too, please nominate if this patch applies there cleanly or nom a different patch but we'll want this in the next esr10 build to ship alongside 17.
Comment 19 Dave Camp (:dcamp) 2012-10-29 19:47:52 PDT
Created attachment 676450 [details] [diff] [review]
esr10 fix

Minor rebase for esr10.
Comment 20 Dave Camp (:dcamp) 2012-11-01 10:47:45 PDT
https://hg.mozilla.org/mozilla-central/rev/aab5d5178047
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-11-03 06:39:31 PDT
https://hg.mozilla.org/releases/mozilla-esr10/rev/b271fc88cf09
Comment 22 Ryan VanderMeulen [:RyanVM] 2012-11-03 06:40:00 PDT
Should this have a test?
Comment 23 Al Billings [:abillings] 2012-11-05 14:28:45 PST
(In reply to Ryan VanderMeulen from comment #22)
> Should this have a test?

Yes.
Comment 24 Matt Wobensmith [:mwobensmith][:matt] 2012-11-06 16:07:39 PST
I am using the first PoC, to open calculator on Windows.  

Confirmed exploit on build 2012-10-2, nightly
Verified fixed on build 2012-11-6, nightly
Verified fixed on build 2012-11-6, Aurora 18
Verified fixed on build 2012-11-6, Beta 17

However, this issue did not reproduce on 2012-10-24 ESR 10.0.10.
Comment 25 Mariusz Mlynski 2012-11-06 16:18:29 PST
(In reply to Matt Wobensmith from comment #24)
> However, this issue did not reproduce on 2012-10-24 ESR 10.0.10.

Try switching between tags on the inspector bar.
Comment 26 Matt Wobensmith [:mwobensmith][:matt] 2012-11-06 16:36:07 PST
Thanks Mariusz - that was helpful. I was able to reproduce on ESR 10.0.10 with some additional work.
Comment 27 Matt Wobensmith [:mwobensmith][:matt] 2012-11-15 09:43:13 PST
Confirmed fixed ESR 10.0.11 2102-11-14.
Comment 28 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-11-15 11:55:32 PST
(In reply to Matt Wobensmith from comment #27)
> Confirmed fixed ESR 10.0.11 2102-11-14.

Thanks Matt. Can you also please check Firefox 17esr?
Comment 29 Matt Wobensmith [:mwobensmith][:matt] 2012-11-15 14:48:34 PST
Confirmed fixed ESR 17.0, 2012-11-11.
Comment 30 Raymond Forbes[:rforbes] 2013-07-19 18:42:50 PDT
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Comment 31 Tracy Walker [:tracy] 2014-01-10 10:40:46 PST
mass remove verifyme requests greater than 4 months old

Note You need to log in before you can comment on or make changes to this bug.