Closed Bug 1116385 (CVE-2016-1941) Opened 9 years ago Closed 9 years ago

Download "open file" dialog delay is too quick when it is under a popup which closes, doesn't prevent clickjacking

Categories

(Toolkit :: Downloads API, defect)

39 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: jordi.chancel, Assigned: Felipe)

References

()

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main44+])

Attachments

(4 files, 3 obsolete files)

Attached file TESTCASE1.html (obsolete) —
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.
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.
Attached file testcase2(test for myself).html (obsolete) —
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.
Attachment #8542410 - Attachment is obsolete: true
Attachment #8542428 - Attachment is obsolete: true
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.
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.
Whiteboard: WORKS ONLY ON MAC OS X
Attachment #8542446 - Attachment description: TESTCASE2.html → TESTCASE2 for MAC OS X.html
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.
Component: General → Download Manager
Product: Core → Toolkit
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.
Assignee: nobody → felipc
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: sec-moderate
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.
Whiteboard: WORKS ONLY ON MAC OS X → sec-moderate/sec-high? (Comment 7 by Daniel Veditz: the trap is potentially high)
Summary: Download "open file" dialog delay is too quick when it is under a popup which closes, doesn't prevent clickjacking → Mozilla Firefox for Mac OS X : Download "open file" dialog delay is too quick when it is under a popup which closes, doesn't prevent clickjacking
Version: 34 Branch → 39 Branch
Whiteboard: sec-moderate/sec-high? (Comment 7 by Daniel Veditz: the trap is potentially high) → (Comment 7 by Daniel Veditz: the trap is potentially high)
Group: core-security → toolkit-core-security
Flags: sec-bounty?
Whiteboard: (Comment 7 by Daniel Veditz: the trap is potentially high)
Attached patch part 1 - CommonDialog (obsolete) — Splinter Review
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.
Attachment #8671164 - Flags: review?(dtownsend)
Now fix nsHelperAppDlg, which fixes the testcase attached to this bug
Attachment #8671165 - Flags: review?(dtownsend)
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.
Attachment #8671164 - Flags: review?(dtownsend) → review+
Attachment #8671165 - Flags: review?(dtownsend) → review+
Attached patch CommonDialog v2Splinter Review
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
Attachment #8671164 - Attachment is obsolete: true
Attachment #8671680 - Flags: review?(dtownsend)
Attachment #8671680 - Flags: review?(dtownsend) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/52917169ff8d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Group: toolkit-core-security → core-security-release
Flags: sec-bounty? → sec-bounty+
Fixed, Jordi can you verify?
Flags: needinfo?(jordi.chancel)
Whiteboard: [post-critsmash-triage]
Yes it's fixed. Nice job !
Flags: needinfo?(jordi.chancel)
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main44+]
Alias: CVE-2016-1941
Summary: Mozilla Firefox for Mac OS X : Download "open file" dialog delay is too quick when it is under a popup which closes, doesn't prevent clickjacking → Download "open file" dialog delay is too quick when it is under a popup which closes, doesn't prevent clickjacking
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.