Last Comment Bug 873966 - (CVE-2013-1688) Arbitrary code execution from Profiler
(CVE-2013-1688)
: Arbitrary code execution from Profiler
Status: VERIFIED FIXED
[adv-main22+]
: csectype-priv-escalation, regression, sec-high, verifyme
Product: Firefox
Classification: Client Software
Component: Developer Tools: Performance Tools (Profiler/Timeline) (show other bugs)
: 21 Branch
: All All
: -- critical (vote)
: Firefox 24
Assigned To: Anton Kovalyov (:anton)
: Mihaela Velimiroviciu (:mihaelav)
: Greg Tatum [:gregtatum] [@gregtatum]
Mentors:
: 858507 (view as bug list)
Depends on: 858507
Blocks: 795268
  Show dependency treegraph
 
Reported: 2013-05-20 00:53 PDT by Mariusz Mlynski
Modified: 2014-07-24 16:08 PDT (History)
14 users (show)
abillings: sec‑bounty+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
fixed
+
verified
+
verified
unaffected
unaffected


Attachments
Exploit (843 bytes, text/html)
2013-05-20 00:55 PDT, Mariusz Mlynski
no flags Details
Use textNodes for escapeHTML function (3.34 KB, patch)
2013-05-20 15:42 PDT, Anton Kovalyov (:anton)
b56girard: review+
Details | Diff | Splinter Review
Use textNode instead of innerHTML (910 bytes, patch)
2013-05-20 22:07 PDT, Anton Kovalyov (:anton)
anton: review+
abillings: sec‑approval+
Details | Diff | Splinter Review
Tests for escapeHTML (2.66 KB, patch)
2013-05-20 22:08 PDT, Anton Kovalyov (:anton)
anton: review+
Details | Diff | Splinter Review
Use textNode instead of innerHTML (beta) (200 bytes, patch)
2013-05-21 13:43 PDT, Anton Kovalyov (:anton)
anton: review+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Use textNode instead of innerHTML (aurora) (200 bytes, patch)
2013-05-21 13:44 PDT, Anton Kovalyov (:anton)
anton: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Mariusz Mlynski 2013-05-20 00:53:46 PDT
It is possible to execute arbitrary code when a user examines the profiler output on a malicious website containing a specially crafted code.

The problem lies in the function |escapeHTML|, which tries to escape user-controlled strings (in this case, a URI) by writing them to the innerHTML property of a textarea element and reading it back. This approach misses the fact that the provided string can be used to close the inner text node and manipulate the DOM of the containing textarea element.
Comment 1 Mariusz Mlynski 2013-05-20 00:55:17 PDT
Created attachment 751593 [details]
Exploit

This opens C:\Windows\System32\calc.exe on Windows or alerts Components.classes on other systems.
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-20 14:31:26 PDT
Making a guess on the assignee here. I'm assuming this was an issue since bug 795268 and as such all versions since Firefox 20 are affected.
Comment 3 Anton Kovalyov (:anton) 2013-05-20 15:09:58 PDT
I have a patch working. Writing a test case now, then will submit for review.
Comment 4 Anton Kovalyov (:anton) 2013-05-20 15:42:48 PDT
Created attachment 751867 [details] [diff] [review]
Use textNodes for escapeHTML function

See attached patch. Do I need to request sec-approval? Is there anyone in particular who needs to review this patch?

The problem was with Cleopatra's implementation of |escapeHTML| which wasn't a big problem within Cleopatra itself since it's a server-less web app. Nevertheless, once this patch is out I will push the change upstream to Cleopatra (its on GitHub so I can't just open a hidden bug there).
Comment 5 Anton Kovalyov (:anton) 2013-05-20 15:43:25 PDT
*** Bug 858507 has been marked as a duplicate of this bug. ***
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-20 16:33:43 PDT
Comment on attachment 751867 [details] [diff] [review]
Use textNodes for escapeHTML function

Yes, once you have review you'll need sec-approval.

Since I don't know anything about this code, can you give a very brief high-level overview of where this code runs (chrome iframe?), and what it's doing? Does it need to run in a privileged context?
Comment 7 Anton Kovalyov (:anton) 2013-05-20 16:44:53 PDT
Once we get data from the profiler, we create a new chrome iframe and load a copy of Cleopatra (UI for the profiler in there) and send it JSON we got from the profiler. Cleopatra then parses that data and renders the UI. All communication is done via the postMessage interface.

This code needs to be able to talk to the outside chrome code via postMessage in order to send events and open files in the debugger/view-source. If it's possible to do so without running it in a privileged context then we don't need it I guess.

The actual function |escapeHTML| is used to filter tree information that profiler sends us and is used within that file (tree.js) and is used only twice.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-20 16:48:23 PDT
AFAIK postMessage between chrome/content windows should work. Can we just make this iframe type="content"?
Comment 9 Anton Kovalyov (:anton) 2013-05-20 16:59:19 PDT
Okay, will do.
Comment 10 Anton Kovalyov (:anton) 2013-05-20 18:05:39 PDT
So, even though when testing manually UI works fine with type="content", all tests start failing. Probably because I use references to iframe's |window| and |document| objects to test Cleopatra.
Comment 11 Anton Kovalyov (:anton) 2013-05-20 19:17:34 PDT
Scratch that. As I told Gavin on IRC, type="content" makes window.parent === window which breaks our chrome <-> content postMessage link.
Comment 12 Benoit Girard (:BenWa) 2013-05-20 19:58:27 PDT
It might be a good time to audit all the uses of innerHTML in cleopatra.
Comment 13 Benoit Girard (:BenWa) 2013-05-20 19:59:35 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #2)
> Making a guess on the assignee here. I'm assuming this was an issue since
> bug 795268 and as such all versions since Firefox 20 are affected.

Release users are only affected if they have flipped the pref for profiler.
Comment 14 Benoit Girard (:BenWa) 2013-05-20 20:04:54 PDT
Comment on attachment 751867 [details] [diff] [review]
Use textNodes for escapeHTML function

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

::: browser/devtools/profiler/cleopatra/js/tree.js
@@ +7,1 @@
>  function escapeHTML(html) {

We also have this problem in js/sourceView.js which doesn't live in Gecko it seems. We should fix it there too when we backport the fix.
Comment 15 Anton Kovalyov (:anton) 2013-05-20 20:16:46 PDT
Comment on attachment 751867 [details] [diff] [review]
Use textNodes for escapeHTML function

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Relatively easy. The patch has a test that checks if |escapeHTML| works correctly so based on this one could deduce the rest. 

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

Tests. (Not sure about bulls-eye tho)

Which older supported branches are affected by this flaw?

Beta, Aurora, Nightly and Stable (disabled by default, only if users flipped the switch to enable Profiler). Other than that, UX Nightly?

If not all supported branches, which bug introduced the flaw?

Bug 795268.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Should be pretty easy to uplift to Aurora and Beta. Gavin said we're not uplifting to Stable.

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

Not likely. The bug is very isolated.
Comment 16 Mariusz Mlynski 2013-05-20 20:17:44 PDT
(In reply to Benoit Girard (:BenWa) from comment #13)
> Release users are only affected if they have flipped the pref for profiler.

The exploit works for me in the current release build (21.0) using default preferences.
Comment 17 Anton Kovalyov (:anton) 2013-05-20 20:19:06 PDT
In this case, I'll let Gavin make a decision.
Comment 18 Al Billings [:abillings] 2013-05-20 21:43:31 PDT
We need to separate tests from the fix in the patch and only check in the tests some time after we release with this issue resolved. In this instance, we wouldn't check in the tests at the same time as the fix (so we don't pwn ourselves).
Comment 19 Anton Kovalyov (:anton) 2013-05-20 22:07:50 PDT
Created attachment 752020 [details] [diff] [review]
Use textNode instead of innerHTML

Extracted tests into a separate patch (will upload shortly). No code changes so carrying over BenWa's r+. New sec-approval below.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Moderately easy. Once the patch that replaces innerHTML goes into all branches one can deduce that something is up.

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

No. (Tests are in a separate patch now)

Which older supported branches are affected by this flaw?

Nightly, Aurora, Beta and Stable.

If not all supported branches, which bug introduced the flaw?

Bug 795268.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Should be pretty easy to uplift to Aurora and Beta. Gavin said we're not uplifting to Stable.

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

Not likely. The bug is very isolated.
Comment 20 Anton Kovalyov (:anton) 2013-05-20 22:08:48 PDT
Created attachment 752021 [details] [diff] [review]
Tests for escapeHTML

Tests for the fix. Carrying over r+ since there were no code changes.
Comment 21 Frederik Braun [:freddyb] 2013-05-21 01:33:35 PDT
A pity, that my bug 858507 from the secreview wasn't addressed earlier. Nice find that it's exploitable with a data URI :/

I want to argue about the sec rating though: Is it really sec-critical if you have to lure somebody on a data: URI and *then* make the victim click through the profile UI to trigger the code execution? It requires heavy user-interaction in both steps.
Comment 22 Frederik Braun [:freddyb] 2013-05-21 01:34:09 PDT
Patch for escapeHTML looks good to me btw..
Comment 23 Mariusz Mlynski 2013-05-21 07:52:27 PDT
(In reply to Frederik Braun [:freddyb] from comment #21)
> I want to argue about the sec rating though: Is it really sec-critical if
> you have to lure somebody on a data: URI and *then* make the victim click
> through the profile UI to trigger the code execution? It requires heavy
> user-interaction in both steps.

The user never sees the data: URI, it's opened in a hidden iframe and can be further concealed. In order to find out which resource is causing heavy load on the CPU, users may need to perform the steps triggering the payload, and the data: document can give them an incentive to do so by slowing down the browser.

I'm not the one to tell about the security rating, but historically, bugs exploiting browser functionalities in the "just use" manner were considered critical (bug 572129, bug 796866), while those that required some copying-and-pasting (eg. bug 771859, bug 800363) were rated sec-high.
Comment 24 Al Billings [:abillings] 2013-05-21 11:05:29 PDT
Comment on attachment 752020 [details] [diff] [review]
Use textNode instead of innerHTML

Sec-approval+ for m-c. Please prepare and nominate branch patches. Let's get this in across the board.
Comment 25 Anton Kovalyov (:anton) 2013-05-21 11:11:03 PDT
I've been told that sometimes we try to hide security patches within other patches. Should I do that here as well?
Comment 26 Al Billings [:abillings] 2013-05-21 11:30:40 PDT
We often check them in with a bunch of other things.
Comment 27 Anton Kovalyov (:anton) 2013-05-21 13:43:18 PDT
Created attachment 752352 [details] [diff] [review]
Use textNode instead of innerHTML (beta)

Carrying over r+ and sec-approval+ since there were no code changes between channels.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 795268
User impact if declined: User exposed to arbitrary code execution attack
Testing completed (on m-c, etc.): Unit/integration tests on m-c and on beta repos.
Risk to taking this patch (and alternatives if risky): Low, patch is very isolated.
String or IDL/UUID changes made by this patch: N/A
Comment 28 Anton Kovalyov (:anton) 2013-05-21 13:44:17 PDT
Created attachment 752353 [details] [diff] [review]
Use textNode instead of innerHTML (aurora)

Carrying over r+ and sec-approval+ since there were no code changes between channels.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 795268
User impact if declined: User exposed to arbitrary code execution attack
Testing completed (on m-c, etc.): Unit/integration tests on m-c and on beta repos.
Risk to taking this patch (and alternatives if risky): Low, patch is very isolated.
String or IDL/UUID changes made by this patch: N/A
Comment 29 Anton Kovalyov (:anton) 2013-05-21 13:45:16 PDT
So I have patches for m-c, aurora (a?) and beta (a?). Didn't do anything for stable cause the tracking flag is still wontfix set by Gavin.
Comment 30 Daniel Veditz [:dveditz] 2013-05-21 14:52:30 PDT
(In reply to Benoit Girard (:BenWa) from comment #12)
> It might be a good time to audit all the uses of innerHTML in cleopatra.

It's always a good time to audit uses of innerHTML anywhere. innerHTML is Satan's candy.
Comment 31 Daniel Veditz [:dveditz] 2013-05-21 15:31:20 PDT
(In reply to Mariusz Mlynski from comment #23)
> historically, bugs exploiting browser functionalities in the "just use"
> manner were considered critical (bug 572129, bug 796866), while those
> that required some copying-and-pasting (eg. bug 771859, bug 800363)
> were rated sec-high.

sec-critical usually represents vulnerabilities that could be turned into a reliable (enough) drive-by malware install for the typical user. Using the profiler is not a common activity (and when you do it's usually on your own code and not an attacker's); sec-high is more appropriate.
Comment 32 Frederik Braun [:freddyb] 2013-05-22 00:29:53 PDT
(In reply to Daniel Veditz [:dveditz] from comment #30)
> (In reply to Benoit Girard (:BenWa) from comment #12)
> > It might be a good time to audit all the uses of innerHTML in cleopatra.
> 
> It's always a good time to audit uses of innerHTML anywhere. innerHTML is
> Satan's candy.

FWIW, that's what was done in the sec review. We found this very same issue in bug 858507 but underestimated its impact. Thus it wasn't timely fixed :/
Comment 33 Anton Kovalyov (:anton) 2013-05-22 10:16:47 PDT
Do I need to ping someone in particular for channel approvals?
Comment 34 Al Billings [:abillings] 2013-05-22 14:42:34 PDT
Anton, if just need to mark the patch (which you did) and they'll come along and either approve or ask questions (which they did). You're golden.
Comment 35 Anton Kovalyov (:anton) 2013-05-22 14:46:36 PDT
Yeah, thanks.

Pushed to Aurora (hidden within another devtools bugfix): https://hg.mozilla.org/releases/mozilla-aurora/rev/bca5f2d86152

About to push to fx-team (also hidden) and then will try to find something for Beta.
Comment 36 Anton Kovalyov (:anton) 2013-05-22 14:52:19 PDT
fx-team: https://hg.mozilla.org/integration/fx-team/rev/03f322ae358f (should get into m-c soon)
Comment 37 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-06-02 17:05:13 PDT
This made it to m-c: http://hg.mozilla.org/mozilla-central/rev/03f322ae358f

Still need to get this on beta soon. I don't think you need to necessarily worry about finding it a ride-along.
Comment 38 Anton Kovalyov (:anton) 2013-06-02 17:45:37 PDT
I'll try to find something tomorrow but if there's nothing—should I just commit it as is?
Comment 39 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-06-02 17:48:40 PDT
Yes. I suppose you could just land with the wrong bug # in the commit message (intentionally), then comment in that (non-security) bug pointing to this one. Or just omit the bug # entirely, I guess.
Comment 41 Anton Kovalyov (:anton) 2013-06-03 18:17:45 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/4e647fb5ac5d
Comment 42 Anton Kovalyov (:anton) 2013-06-27 17:36:48 PDT
Since this fix is in all trees now I landed a test case for it hidden in this big profiler refactor patch: https://hg.mozilla.org/integration/fx-team/rev/addbd7cf1741

Seemed like a good fit.
Comment 43 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-07-11 13:54:28 PDT
Mihaela, please verify this is fixed, thank you.
Comment 44 Mihaela Velimiroviciu (:mihaelav) 2013-07-12 01:54:13 PDT
Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20100101 Firefox/23.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:23.0) Gecko/20100101 Firefox/23.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0

Verified the fix on latest Firefox 23 beta 5, build id 20130711122148: there are no assertions nor Windows calculator launch.

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