Closed Bug 873963 Opened 10 years ago Closed 10 years ago

Arbitrary code execution from Font Inspector


(DevTools :: Inspector, defect)

Not set


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

Tracking Status
firefox21 --- unaffected
firefox22 + verified
firefox23 + verified
firefox24 + verified
firefox-esr17 --- unaffected
firefox-esr24 --- fixed
b2g18 --- unaffected


(Reporter: marius.mlynski, Assigned: jwalker)



(Keywords: regression, sec-critical, verifyme, Whiteboard: [fixed-in-fx-team][adv-main22-])


(1 file, 2 obsolete files)

It is possible to execute arbitrary code when a user opens the Font Inspector on a malicious website containing a specially crafted font definition.

The problem lies in the function |FI_buildPreview|, which appends user-controlled, unsanitized font-family names to a string containing CSS code. This allows code injection in the context of a document inheriting chrome privileges of chrome://browser/content/devtools/fontinspector/font-inspector.xhtml
This opens C:\Windows\System32\calc.exe on Windows or alerts Components.classes on other systems.
Paul is apparently offline for a while, so assigning to jwalker per blame.
Assignee: nobody → jwalker
Attached patch v1 (obsolete) — Splinter Review
First stab at a fix. At least the exploit as presented doesn't work for me in my testing with this fix in place.

Outstanding: Check this doesn't break anything, and check that there isn't an additional vector via the 'cssCode' parameter.
This fix doesn't break any fontinspector unit tests.
It looks like the cssCode argument is either "" (no risk) or it comes from cssText already parsed by the CSS engine (line 144), so at first pass attachment 752119 [details] [diff] [review] looks like it might work.
Attachment #752119 - Flags: review?(dcamp)
Attachment #752119 - Flags: feedback?(mgoodwin)
I'd be interested in some submerge advice; When do we need to submerge security patches?
Comment on attachment 752119 [details] [diff] [review]

r+ as a peer, but mgoodwin's review should be binding here.
Attachment #752119 - Flags: review?(dcamp) → review+
I think we should not have allow-same-origin on that iframe sandbox attribute in font-inspector.xhtml - can we fix that too, please?
Comment on attachment 752119 [details] [diff] [review]

This looks good but I'd like to see the sandbox change too.
Attachment #752119 - Flags: feedback?(mgoodwin) → feedback-
Attached patch v2 (obsolete) — Splinter Review
Attachment #752119 - Attachment is obsolete: true
Attachment #752379 - Flags: feedback?(mgoodwin)
Comment on attachment 752379 [details] [diff] [review]

Thank you.
Attachment #752379 - Flags: feedback?(mgoodwin) → feedback+
Dan - can you give me some advice on landing security patches?

The exploit for this bug involves a web page, and coercing the victim to open developer tools, change to the inspector, and open the font inspector.

When should we submerge them with another patch?
Do we need to take care over try too?

You'll want to request sec-approval (and answer the auto-filled questions).

I don't think you need to worry about "masking" this landing by merging with some other change (current release isn't affected), but we will want to coordinate landing on beta/aurora (the sec-approval process does that).
Comment on attachment 752379 [details] [diff] [review]

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

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

Which older supported branches are affected by this flaw?
* aurora and beta, not release

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
* Unsure, but think there are no backports. Should not be too different or hard.

How likely is this patch to cause regressions; how much testing does it need?
* Unlikely
Attachment #752379 - Flags: sec-approval?
Comment on attachment 752379 [details] [diff] [review]

sec-approval for m-c. Please don't make the commit message glaringly obvious. :-)

Also, as soon as this gets into m-c, please prepare and nominate patches for Aurora and Beta so we can avoid shipping this issue to the public.
Attachment #752379 - Flags: sec-approval? → sec-approval+
don't forget about Aurora and Beta!
Attached patch v3 (fixed title)Splinter Review
[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 697983 (font inspector)
User impact if declined: Security risk
Testing completed (on m-c, etc.): baked on on m-c for a week
Risk to taking this patch (and alternatives if risky): low, that we cause regressions
String or IDL/UUID changes made by this patch: none
Attachment #752379 - Attachment is obsolete: true
Attachment #755410 - Flags: approval-mozilla-beta?
Attachment #755410 - Flags: approval-mozilla-aurora?
Attachment #755410 - Flags: approval-mozilla-beta?
Attachment #755410 - Flags: approval-mozilla-beta+
Attachment #755410 - Flags: approval-mozilla-aurora?
Attachment #755410 - Flags: approval-mozilla-aurora+
(In reply to Joe Walker [:jwalker] from comment #17)
> Risk to taking this patch (and alternatives if risky): low, that we cause
> regressions

Not a very useful answer :) "low, only has the potential to break the font-inspector tool, no alternatives" is probably a better one.
Closed: 10 years ago
Resolution: --- → FIXED
Verified fixed with Firefox 22 beta 4 (build ID: 20130605070403) on: Windows 7 64bit, Ubuntu 12.10 32bit and Mac OSX 10.8.3
QA Contact: manuela.muntean
Whiteboard: [fixed-in-fx-team] → [fixed-in-fx-team][adv-main22-]
Keywords: verifyme
Verified fixed with Firefox 23 beta 1 (build ID: 20130625125232) on: Windows 7 64bit, Ubuntu 12.10 32bit and Mac OSX 10.8.3
Keywords: verifyme
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
I wanted to verify this bug with Firefox 24 beta 6, but the exploit attached in comment 1 isn't available anymore, so I wasn't able to test the fix.

Does anyone have any thoughts/suggestions?
Flags: needinfo?
Flags: needinfo?
Keywords: verifyme
Needinfo per comment 23.
Flags: needinfo?
I was able to download the test exploit and verified on 24b10
Flags: needinfo?
Blocks: 697983
Group: core-security
Can the testcase be made public now? (Does it demonstrate anything that's not publicly known and not fixed?)
Flags: needinfo?(jwalker)
(In reply to Jesse Ruderman from comment #27)
> Can the testcase be made public now? (Does it demonstrate anything that's
> not publicly known and not fixed?)

Hard for me to say for sure because I can't read attachment 751590 [details] any more. But, can't think of a reason to keep it non-public.
Flags: needinfo?(jwalker)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.