The default bug view has changed. See this FAQ.
Bug 873966 (CVE-2013-1688)

Arbitrary code execution from Profiler

VERIFIED FIXED in Firefox 22

Status

()

Firefox
Developer Tools: Performance Tools (Profiler/Timeline)
--
critical
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: Mariusz Mlynski, Assigned: anton)

Tracking

(4 keywords)

21 Branch
Firefox 24
csectype-priv-escalation, regression, sec-high, verifyme
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox21 wontfix, firefox22+ fixed, firefox23+ verified, firefox24+ verified, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [adv-main22+])

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
Created attachment 751593 [details]
Exploit

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

Updated

4 years ago
Attachment #751593 - Attachment mime type: text/plain → text/html
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.
Assignee: nobody → anton
status-firefox21: --- → wontfix
status-firefox22: --- → affected
status-firefox23: --- → affected
status-firefox24: --- → affected
tracking-firefox22: --- → ?
tracking-firefox23: --- → ?
tracking-firefox24: --- → ?
Keywords: sec-critical
(Assignee)

Updated

4 years ago
Depends on: 858507
(Assignee)

Comment 3

4 years ago
I have a patch working. Writing a test case now, then will submit for review.
Status: NEW → ASSIGNED

Updated

4 years ago
tracking-firefox22: ? → +
tracking-firefox23: ? → +
tracking-firefox24: ? → +
(Assignee)

Comment 4

4 years ago
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).
Attachment #751867 - Flags: review?(gavin.sharp)
Attachment #751867 - Flags: review?(fbraun)
(Assignee)

Updated

4 years ago
Duplicate of this bug: 858507
(Assignee)

Updated

4 years ago
Attachment #751867 - Flags: review?(bgirard)
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?
(Assignee)

Comment 7

4 years ago
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.
AFAIK postMessage between chrome/content windows should work. Can we just make this iframe type="content"?
(Assignee)

Comment 9

4 years ago
Okay, will do.
(Assignee)

Comment 10

4 years ago
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.
(Assignee)

Comment 11

4 years ago
Scratch that. As I told Gavin on IRC, type="content" makes window.parent === window which breaks our chrome <-> content postMessage link.
It might be a good time to audit all the uses of innerHTML in cleopatra.
(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 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.
Attachment #751867 - Flags: review?(bgirard) → review+
(Assignee)

Comment 15

4 years ago
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.
Attachment #751867 - Flags: sec-approval?
(Reporter)

Comment 16

4 years ago
(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.
(Assignee)

Comment 17

4 years ago
In this case, I'll let Gavin make a decision.
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).
(Assignee)

Comment 19

4 years ago
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.
Attachment #751867 - Attachment is obsolete: true
Attachment #751867 - Flags: sec-approval?
Attachment #751867 - Flags: review?(gavin.sharp)
Attachment #751867 - Flags: review?(fbraun)
Attachment #752020 - Flags: sec-approval?
Attachment #752020 - Flags: review+
(Assignee)

Comment 20

4 years ago
Created attachment 752021 [details] [diff] [review]
Tests for escapeHTML

Tests for the fix. Carrying over r+ since there were no code changes.
Attachment #752021 - Flags: review+
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.
Patch for escapeHTML looks good to me btw..
(Reporter)

Comment 23

4 years ago
(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 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.
Attachment #752020 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 25

4 years ago
I've been told that sometimes we try to hide security patches within other patches. Should I do that here as well?
We often check them in with a bunch of other things.
(Assignee)

Comment 27

4 years ago
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
Attachment #752352 - Flags: review+
Attachment #752352 - Flags: approval-mozilla-beta?
(Assignee)

Comment 28

4 years ago
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
Attachment #752353 - Flags: review+
Attachment #752353 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 29

4 years ago
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.
(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.
Blocks: 795268
status-b2g18: --- → unaffected
status-firefox-esr17: --- → unaffected
Flags: sec-bounty?
Keywords: csec-priv-escalation, regression
(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.
Keywords: sec-critical → sec-high
(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 :/
(Assignee)

Comment 33

4 years ago
Do I need to ping someone in particular for channel approvals?
Attachment #752352 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #752353 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
(Assignee)

Comment 35

4 years ago
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.
(Assignee)

Comment 36

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

Updated

4 years ago
status-firefox23: affected → fixed
(Assignee)

Updated

4 years ago
status-firefox24: affected → fixed
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
(Assignee)

Comment 38

4 years ago
I'll try to find something tomorrow but if there's nothing—should I just commit it as is?
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.
Flags: sec-bounty? → sec-bounty+
(Assignee)

Comment 41

4 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/4e647fb5ac5d
status-firefox22: affected → fixed
Whiteboard: [adv-main22+]
Alias: CVE-2013-1688
(Assignee)

Comment 42

4 years ago
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.
Mihaela, please verify this is fixed, thank you.
Keywords: verifyme
QA Contact: mihaela.velimiroviciu
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.
status-firefox23: fixed → verified

Updated

4 years ago
Status: RESOLVED → VERIFIED
status-firefox24: fixed → verified
Group: core-security
You need to log in before you can comment on or make changes to this bug.