Closed
Bug 873966
(CVE-2013-1688)
Opened 12 years ago
Closed 12 years ago
Arbitrary code execution from Profiler
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect)
Tracking
(firefox21 wontfix, firefox22+ fixed, firefox23+ verified, firefox24+ verified, firefox-esr17 unaffected)
VERIFIED
FIXED
Firefox 24
People
(Reporter: marius.mlynski, Assigned: anton)
References
Details
(6 keywords, Whiteboard: [adv-main22+])
Attachments
(5 files, 1 obsolete file)
843 bytes,
text/html
|
Details | |
910 bytes,
patch
|
anton
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
2.66 KB,
patch
|
anton
:
review+
|
Details | Diff | Splinter Review |
200 bytes,
patch
|
anton
:
review+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
200 bytes,
patch
|
anton
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
This opens C:\Windows\System32\calc.exe on Windows or alerts Components.classes on other systems.
Reporter | ||
Updated•12 years ago
|
Attachment #751593 -
Attachment mime type: text/plain → text/html
Comment 2•12 years ago
|
||
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 | ||
Comment 3•12 years ago
|
||
I have a patch working. Writing a test case now, then will submit for review.
Status: NEW → ASSIGNED
Updated•12 years ago
|
Assignee | ||
Comment 4•12 years ago
|
||
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•12 years ago
|
Attachment #751867 -
Flags: review?(bgirard)
Comment 6•12 years ago
|
||
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•12 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.
Comment 8•12 years ago
|
||
AFAIK postMessage between chrome/content windows should work. Can we just make this iframe type="content"?
Assignee | ||
Comment 9•12 years ago
|
||
Okay, will do.
Assignee | ||
Comment 10•12 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•12 years ago
|
||
Scratch that. As I told Gavin on IRC, type="content" makes window.parent === window which breaks our chrome <-> content postMessage link.
Comment 12•12 years ago
|
||
It might be a good time to audit all the uses of innerHTML in cleopatra.
Comment 13•12 years ago
|
||
(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•12 years ago
|
||
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•12 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•12 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•12 years ago
|
||
In this case, I'll let Gavin make a decision.
Comment 18•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Tests for the fix. Carrying over r+ since there were no code changes.
Attachment #752021 -
Flags: review+
Comment 21•12 years ago
|
||
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•12 years ago
|
||
Patch for escapeHTML looks good to me btw..
Reporter | ||
Comment 23•12 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 24•12 years ago
|
||
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.
Updated•12 years ago
|
Attachment #752020 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 25•12 years ago
|
||
I've been told that sometimes we try to hide security patches within other patches. Should I do that here as well?
Comment 26•12 years ago
|
||
We often check them in with a bunch of other things.
Assignee | ||
Comment 27•12 years ago
|
||
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•12 years ago
|
||
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•12 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.
Comment 30•12 years ago
|
||
(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
Comment 31•12 years ago
|
||
(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
Comment 32•12 years ago
|
||
(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•12 years ago
|
||
Do I need to ping someone in particular for channel approvals?
Updated•12 years ago
|
Attachment #752352 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•12 years ago
|
Attachment #752353 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 34•12 years ago
|
||
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•12 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•12 years ago
|
||
fx-team: https://hg.mozilla.org/integration/fx-team/rev/03f322ae358f (should get into m-c soon)
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Updated•12 years ago
|
Comment 37•12 years ago
|
||
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: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Assignee | ||
Comment 38•12 years ago
|
||
I'll try to find something tomorrow but if there's nothing—should I just commit it as is?
Comment 39•12 years ago
|
||
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.
Updated•12 years ago
|
Flags: sec-bounty? → sec-bounty+
Assignee | ||
Comment 41•12 years ago
|
||
Updated•12 years ago
|
Whiteboard: [adv-main22+]
Updated•12 years ago
|
Alias: CVE-2013-1688
Assignee | ||
Comment 42•12 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.
Comment 43•12 years ago
|
||
Mihaela, please verify this is fixed, thank you.
Keywords: verifyme
QA Contact: mihaela.velimiroviciu
Comment 44•12 years ago
|
||
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.
Updated•11 years ago
|
Status: RESOLVED → VERIFIED
Updated•11 years ago
|
Group: core-security
Updated•7 years ago
|
Product: Firefox → DevTools
Updated•3 years ago
|
status-b2g18:
unaffected → ---
Keywords: csectype-sandbox-escape
Updated•8 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•