931768 - Arbitrary code execution using openDialog

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mariusz Mlynski]

Attachment: Exploit

This is a regression from bug 918345. The new implementation of nsGlobalWindow::OpenDialog doesn't check if it's called from chrome, which allows web content to open privileged windows.

The attached example launches C:\Windows\System32\calc.exe (Windows) or alerts Components.classes (other systems) in Nightly 27.0a1 (2013-10-28).

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[(no longer active)]

Comment on attachment 823355 [details] [diff] [review]
Patch (v1)

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  The patch makes it clear that this is a chrome permission check, and hence exposes the bug, but coming up with the exploit in the test case here probably takes a bit more effort.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  See above.

Which older supported branches are affected by this flaw? Firefox 27, where bug 918345 landed.

If not all supported branches, which bug introduced the flaw?  Bug 918345.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? N/A.

How likely is this patch to cause regressions; how much testing does it need?  It should bring us back to the old behavior, so the risk for regressions should be minimal.

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 823355 [details] [diff] [review]
Patch (v1)

Did you try the patch? I don't see how this could fix it, this is a non-scriptable method.

Comment 4

[Peter Van der Beken [:peterv]]

We probably just need to mark openDialog ChromeOnly in the Window.webidl.

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1

Working on an automated testcase, pretty sad that we didn't have one already.

Comment 6

[(no longer active)]

Marking this as [ChromeOnly] will hide the method from regular content, which changes our existing behavior.  Is that OK?

Comment 7

[Peter Van der Beken [:peterv]]

Calling it has always thrown so you'll just get a different kind of exception (ReferenceError vs a security exception). The only difference in behaviour is if it's used for browser detection ("openDialog in window"), but I think we should try it anyway.

Comment 8

[Peter Van der Beken [:peterv]]

And actually, until we switch completely to WebIDL for the Window object there won't be a difference at all, since we'll just fall back to XPConnect if the WebIDL property isn't installed.

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: v1 with testcase

[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?

Yeah, we're specifically marking an accidentally exposed API as ChromeOnly.

Which older supported branches are affected by this flaw?

Firefox 27.

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

Bug 918345.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The patch will be identical.

How likely is this patch to cause regressions; how much testing does it need?

Very low risk, we're basically reverting to the previous behaviour.

Comment 12

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

If we haven't branched to make 27 Aurora and Central into 28 (effectively) yet, you can check this in without sec-approval. I thought the branching happened later today.

Otherwise, I'll give it sec-approval but we'll have to put it on Aurora if we go post-branching.

Comment 13

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

Comment on attachment 823577 [details] [diff] [review]
v1 with testcase

Looks like we just missed the branching. I'm giving sec-approval+ and approving for Aurora to get this in.

Comment 14

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bcda05da19cd

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3f97ca730409

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bcda05da19cd

Comment 17

[Tracy Walker [:tracy]]

setting status-firefox28 to fixed per comment #16

Comment 18

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed issue on FF27, 2013-10-28.
Verified fixed, FF27 2013-11-20.
Verified fixed, FF28 2013-11-26.