Closed Bug 873963 Opened 8 years ago Closed 8 years ago
Arbitrary code execution from Font Inspector
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.
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.
Update: 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.
I'd be interested in some submerge advice; When do we need to submerge security patches?
Comment on attachment 752119 [details] [diff] [review] v1 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] v1 This looks good but I'd like to see the sandbox change too.
Attachment #752119 - Flags: feedback?(mgoodwin) → feedback-
Comment on attachment 752379 [details] [diff] [review] v2 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? Thanks.
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] v2 [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] v2 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!
[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
(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.
Status: NEW → RESOLVED
Closed: 8 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
Whiteboard: [fixed-in-fx-team] → [fixed-in-fx-team][adv-main22-]
Verified fixed with Firefox 23 beta 1 (build ID: 20130625125232) on: Windows 7 64bit, Ubuntu 12.10 32bit and Mac OSX 10.8.3
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?
Needinfo per comment 23.
I was able to download the test exploit and verified on 24b10
Can the testcase be made public now? (Does it demonstrate anything that's not publicly known and not fixed?)
(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.
You need to log in before you can comment on or make changes to this bug.