873966 - (CVE-2013-1688) Arbitrary code execution from Profiler

Status: VERIFIED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Description

[Mariusz Mlynski]

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]

Attachment: Exploit

This opens C:\Windows\System32\calc.exe on Windows or alerts Components.classes on other systems.

Comment 2

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

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

I have a patch working. Writing a test case now, then will submit for review.

Comment 4

[Anton Kovalyov (:anton)]

Attachment: 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 6

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

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

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: [e-mail]]]

AFAIK postMessage between chrome/content windows should work. Can we just make this iframe type="content"?

Comment 9

[Anton Kovalyov (:anton)]

Okay, will do.

Comment 10

[Anton Kovalyov (:anton)]

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

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

It might be a good time to audit all the uses of innerHTML in cleopatra.

Comment 13

[Benoit Girard (:BenWa)]

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

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

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]

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

In this case, I'll let Gavin make a decision.

Comment 18

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

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

Attachment: 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)]

Attachment: Tests for escapeHTML

Tests for the fix. Carrying over r+ since there were no code changes.

Comment 21

[Frederik Braun [:freddy]]

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 [:freddy]]

Patch for escapeHTML looks good to me btw..

Comment 23

[Mariusz Mlynski]

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

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

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 - ex-MoCo]]

We often check them in with a bunch of other things.

Comment 27

[Anton Kovalyov (:anton)]

Attachment: 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)]

Attachment: 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)]

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

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

(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 [:freddy]]

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

Do I need to ping someone in particular for channel approvals?

Comment 34

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

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

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

fx-team: https://hg.mozilla.org/integration/fx-team/rev/03f322ae358f (should get into m-c soon)

Comment 37

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

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

I'll try to find something tomorrow but if there's nothing—should I just commit it as is?

Comment 39

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

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

https://hg.mozilla.org/releases/mozilla-beta/rev/4e647fb5ac5d

Comment 42

[Anton Kovalyov (:anton)]

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

[u279076]

Mihaela, please verify this is fixed, thank you.

Comment 44

[Mihaela Velimiroviciu (:mihaelav)]

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.