Closed Bug 873966 (CVE-2013-1688) Opened 7 years ago Closed 7 years ago

Arbitrary code execution from Profiler

Categories

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

21 Branch
defect
Not set
critical

Tracking

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

VERIFIED FIXED
Firefox 24
Tracking Status
firefox21 --- wontfix
firefox22 + fixed
firefox23 + verified
firefox24 + verified
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: marius.mlynski, Assigned: anton)

References

Details

(4 keywords, Whiteboard: [adv-main22+])

Attachments

(5 files, 1 obsolete file)

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.
Attached file Exploit
This opens C:\Windows\System32\calc.exe on Windows or alerts Components.classes on other systems.
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
Keywords: sec-critical
Depends on: 858507
I have a patch working. Writing a test case now, then will submit for review.
Status: NEW → ASSIGNED
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)
Duplicate of this bug: 858507
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?
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"?
Okay, will do.
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.
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+
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?
(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.
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).
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+
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..
(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+
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.
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?
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?
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
Flags: sec-bounty?
(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-criticalsec-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 :/
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.
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.
fx-team: https://hg.mozilla.org/integration/fx-team/rev/03f322ae358f (should get into m-c soon)
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
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+
Whiteboard: [adv-main22+]
Alias: CVE-2013-1688
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: RESOLVED → VERIFIED
Group: core-security
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.