873963 - Arbitrary code execution from Font Inspector

Status: VERIFIED FIXED

(DevTools :: Inspector, defect)

Description

[Mariusz Mlynski]

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

Comment 1

[Mariusz Mlynski]

Attachment: Exploit (launches calc.exe on windows)

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

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Paul is apparently offline for a while, so assigning to jwalker per blame.

Comment 3

[Joe Walker [:jwalker]]

Attachment: v1

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.

Comment 4

[Joe Walker [:jwalker]]

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.

Comment 5

[Joe Walker [:jwalker]]

I'd be interested in some submerge advice; When do we need to submerge security patches?

Comment 6

[Dave Camp (:dcamp)]

Comment on attachment 752119 [details] [diff] [review]
v1

r+ as a peer, but mgoodwin's review should be binding here.

Comment 7

[Mark Goodwin [:mgoodwin]]

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 8

[Mark Goodwin [:mgoodwin]]

Comment on attachment 752119 [details] [diff] [review]
v1

This looks good but I'd like to see the sandbox change too.

Comment 9

[Joe Walker [:jwalker]]

Attachment: v2

Comment 10

[Mark Goodwin [:mgoodwin]]

Comment on attachment 752379 [details] [diff] [review]
v2

Thank you.

Comment 11

[Joe Walker [:jwalker]]

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.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

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 13

[Joe Walker [:jwalker]]

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

Comment 14

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 15

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=dea12c804f30
https://hg.mozilla.org/integration/fx-team/rev/dea12c804f30

Comment 16

[Rob Campbell [:rc] (:robcee)]

don't forget about Aurora and Beta!

Comment 17

[Joe Walker [:jwalker]]

Attachment: v3 (fixed title)

[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

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(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.

Comment 19

[Joe Walker [:jwalker]]

Aurora landing:
https://tbpl.mozilla.org/?tree=Mozilla-Aurora&rev=1702db2af514
https://hg.mozilla.org/releases/mozilla-aurora/rev/1702db2af514

Beta landing:
https://tbpl.mozilla.org/?tree=Mozilla-Beta&rev=60c7f8e6ccd7
https://hg.mozilla.org/releases/mozilla-beta/rev/60c7f8e6ccd7

Comment 20

[Manuela Muntean [Away]]

Verified fixed with Firefox 22 beta 4 (build ID: 20130605070403) on: Windows 7 64bit, Ubuntu 12.10 32bit and Mac OSX 10.8.3

Comment 21

[Manuela Muntean [Away]]

Verified fixed with Firefox 23 beta 1 (build ID: 20130625125232) on: Windows 7 64bit, Ubuntu 12.10 32bit and Mac OSX 10.8.3

Comment 23

[Manuela Muntean [Away]]

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?

Comment 24

[Manuela Muntean [Away]]

Needinfo per comment 23.

Comment 25

[Tracy Walker [:tracy]]

I was able to download the test exploit and verified on 24b10

Comment 27

[Jesse Ruderman]

Can the testcase be made public now? (Does it demonstrate anything that's not publicly known and not fixed?)

Comment 28

[Joe Walker [: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.