1116385 - (CVE-2016-1941) Download "open file" dialog delay is too quick when it is under a popup which closes, doesn't prevent clickjacking

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Jordi Chancel]

Attachment: TESTCASE1.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20141222200458

Steps to reproduce:

This new Mozilla Firefox Vulnerability is similar than a last moderate vulnerability that i have reported (Fixed in Mozilla Firefox 27)  which is the "MFSA 2014-03" : Bug 916726 .

Steps :
1 : Firstly , Click on the "CLICKME" link on this page (a download "open file" dialog will be loaded with a popup which will cover this one).
2 : Into the popup loaded (which cover the download dialog) , you should double-click on the "DOUBLECLICK" button.

Note : 
- If the testcase do not works the 1st time , please retry , and normally it will work.
- If the testcase do not works the 2nd time , please retry again until it works , but normally it works directly the first time.



Actual results:

- With the 1st click of the double click , the popup will be closed , using window.close(). 
- The 2nd click goes download and execute the .dmg file into the download dialog by pressing to the "open file" button which hasn't a delay of security. 

The method used in this vulnerability is that the "DOUBLECLICK" button is at same place into the popup loaded than the "open file" button of the download dialog which is under this malicious popup , and the when a download dialog is loaded and is cover by a popup, if you close this popup , this dialog don't load a security delay to protect user of clickjacking ).

More information :
When a malicious .dmg is executed , it can appear like Mozilla Firefox containing a fake webpage with a malicious .pkg file (or others) which can be named "double click me" , "Open me to update firefox" or what a malicious hacker want for mislead Firefox lambda user to execute this dangerous file and take the control of its computer. 
( I will make a screen video capture for show you the severity of this new vulnerability ).


Expected results:

If the testcase do not works the 1st time , please retry , and normally it will work.

If the testcase do not works the 2nd time , please retry again until it works , but normally it works directly the first time.

Comment 1

[Jordi Chancel]

With the TESTCASE1.HTML , you should allow the popup to be open for the vulnerability working , but it's possible to use other method for sidestep this security.

I will code a better testecase later for show you this.

Comment 2

[Jordi Chancel]

Attachment: testcase2(test for myself).html

Comment 3

[Jordi Chancel]

Attachment: TESTCASE2 for MAC OS X.html

I think that with the Testcase1 uploaded you must allow the popup because the setTimeout to open the popup was too long/prolonged.

Now, use the Testcase2 please and tell me if the popup is directly loaded or not.

And tell me too if with the testcase on the URL , the popup is directly loaded or not also.

Thank you for your next answers.

Comment 4

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

Hi Jordy, we have spent some time reviewing your tests. However, none of us can see a problem. The file download dialog appears above the popup window, so there is no chance for the user to interact with the doubleclick button, as far as we can see. Sometimes the popup window appears first for a split second, but it does not appear to render in time for the user to see the button inside of it.

Do you have any other information that we can use? Thank you.

Comment 5

[Jordi Chancel]

Attachment: Video Example MAC OS X.html

I think than the testcase works only on MAC OS X. 

You must use Mac Os X for test TESTCASE2.HTML or the testcase in the URL.

I think that the testcase don't works on WINDOWS or linux.


I have tested this testcase on 2 different MAC OS X Computer with Mozilla Firefox Beta and Mozilla Firefox Stable update and it works very well.

Please use Mac Os X for test the TESTCASE and it will work like it works in this Video.

Comment 6

[Jordi Chancel]

I have tested this testcase on 2 different MAC OS X Computer with Mozilla Firefox Beta and Mozilla Firefox Stable update and it works very well.

Please use Mac Os X for test the TESTCASE and it will work like it works in the Video.

And if it doesn't works the first time it's because the download dialog is too long to appear because it's probably the first time of this file tries to be downloaded ( the first time of you go on an URL , the loading is more long than the others times) or it's because the browsing/download speed is too low , 

so if the download 'open file' dialog is loaded before the popup , the testcase works perfectly.

Comment 7

[Daniel Veditz [:dveditz]]

I'm not entirely convinced a real user would fall for this, but I do know some folks with slow enough double-click speeds that if they weren't suspicious about the double popups they might click the OK button. (On my own mac system the button placement was off just enough to break the PoC, but I assume a real attacker would customize the placement taking screen resolution, fonts, whatever, into account.) While I'm not too worried about large numbers of people being spoofed, the consequences for anyone who fell into the trap is potentially high so I'll go with sec-moderate.

The problem is the dialog's onfocus has a fixed delay of 250ms.
http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js#631

That's too short as we discussed in bug 916726. The onfocus needs to use the same delay as the initial delay (pref-based, adjustable). Opening the target dialog to exhaust the delay, then covering and unhiding at an opportune time is a known variant of the basic bug 162020 attack.

Comment 8

[Daniel Veditz [:dveditz]]

CommonDialog.js has the same issue; we should resolve these consistently
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/src/CommonDialog.jsm#252

CC Gavin because he may object to dialog suckification
http://lcamtuf.blogspot.com/2010/08/on-designing-uis-for-non-robots.html?showComment=1282275549014#c3475138460455690208

The shorter security.notification_enable_delay might be OK for a dialog re-focus if we think the user will remember that the dialog is there.

The other two places where the delay is used, the install confirmation dialog and Notifications (doorhangers) are OK. The install dialog restarts the countdown each time it regains focus, and doorhangers close if they lose focus.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Attachment: part 1 - CommonDialog

Summary: there is already code supposed to prevent this, but the problem (as described in comment 7), is that the refocus delay is too short, at 250ms. We should make it follow the pref security.dialog_enable_delay, which is 1000ms.

Both CommonDialog.jsm and nsHelperAppDlg.js has rolled its custom implementation of this feature, but in order to make it consistent and easier to reuse, this patch refactors the CommonDialog.jsm one (which is the best impl), and raises the refocus delay to 1000ms.


Next patch will make nsHelperAppDlg use this same code.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Attachment: part 2 - nsHelperAppDlg

Now fix nsHelperAppDlg, which fixes the testcase attached to this bug

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 8671164 [details] [diff] [review]
part 1 - CommonDialog

Review of attachment 8671164 [details] [diff] [review]:
-----------------------------------------------------------------

I know testing stuff that relies on timing is tricky but we could really use some tests here.

::: toolkit/components/prompts/src/SharedPromptUtils.jsm
@@ +62,5 @@
> +
> +    this.tryCall(this.disableDialog);
> +
> +    this.focusTarget.addEventListener("blur", this, false);
> +    this.focusTarget.addEventListener("focus", this, false);

To try and avoid unnecessary calls can you add an unload listener on focusTarget.ownerDocument and remove these listeners there and clear the timer.

@@ +93,5 @@
> +        // which makes it likely that the given fn might throw.
> +        try {
> +            fn();
> +        } catch (e) { }
> +    },

This doesn't need to be on the prototype it could just be a top-level function which saves a bit of typing. An alternative that can make things safer (meaning you don't have to remember to do this for every call of the functions) is instead to wrap the functions in the constructor using something like:

function makeSafe(fn) {
  return function() {
    try {
      fn();
    } catch(e) { }
  }
}

...

this.enableDialog = makeSafe(enableDialog)

Given that we're unlikely to be making much changes to this code in the future I'll leave it up to you whether you want to make that change or not.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Attachment: CommonDialog v2

Yeah I like the makeSafe function suggestion. Also added the unload handler. Re-requesting review in case you want to take a look at the changes

Comment 13

[:Felipe Gomes (needinfo for replies!)]

(In reply to Dave Townsend [:mossop] from comment #11)
> Comment on attachment 8671164 [details] [diff] [review]
> part 1 - CommonDialog
> 
> Review of attachment 8671164 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I know testing stuff that relies on timing is tricky but we could really use
> some tests here.

I wrote a test that uses the unknownContentType dialog to test this code. I pushed everything to tryserver (together with bug 724353) and it is green.

The test is straightforward so I'm not gonna bother with requesting review. The test can be seen here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cdaacd2e657

I sent an email to Dveditz to check if I can land these now or if some coordination would be preferred.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/integration/fx-team/rev/dca478776c91

Comment 15

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/9fce08491c53 for the Linux-only https://treeherder.mozilla.org/logviewer.html#?job_id=5091855&repo=fx-team

Comment 16

[:Felipe Gomes (needinfo for replies!)]

Test fix worked well: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4790bfc4a9fc
so, relanded:
https://hg.mozilla.org/integration/fx-team/rev/52917169ff8d

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/52917169ff8d

Comment 18

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

Fixed, Jordi can you verify?

Comment 19

[Jordi Chancel]

Yes it's fixed. Nice job !