583175 - Add a security delay to the main action of PopupNotifications

Status: RESOLVED FIXED

(Firefox :: Security, defect)

Description

[Michal Zalewski]

...otherwise, it's pretty easy to steal sensitive information - crude PoC here:

http://lcamtuf.coredump.cx/ffgeo/

The PoC does not account for screen resolution / dpi much, so it may be off on your system, but it wouldn't be hard to write a more robust one.

Comment 1

[Johnathan Nightingale [:johnath]]

Michal - have you played with this at all on the FF4 betas? We've changed the way we prompt for geo. I think it's more resistant to the attack you're playing with here, but I'd be interested in your read.

Comment 2

[Michal Zalewski]

Nope, but I asked around just now. From what a friend tells me, you just remove the infobar when the window loses focus, and do not restore it when it is on top again (unless a second call is being made?).

This seems a bit disruptive if true, but again, not sure this is an accurate picture.

Comment 3

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

It's not an "infobar", it's a "popup", anchored near the location bar. Otherwise, what you describe is the current behavior in the released betas. Future betas will have the patch for bug 572967, which will make the notification retrievable from an icon after the focus switch.

Comment 4

[Michal Zalewski]

Is there some sort of a fade-in delay to prevent the attacker from calling getCurrentPosition() for the first time (or again) immediately after regaining focus?

If yes, then yup, it sounds like a good approach. If there is no substantial delay, it could probably still be circumvented, I'm guessing.

Comment 5

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

Currently there is no delay.

Comment 6

[Michal Zalewski]

Just to confirm - do you guys have any particular plan to address this in 3.6?

If this is not going to happen any time soon, would you have an issue with me discussing this class of problems externally? This is not specific to Firefox.

Comment 7

[Michal Zalewski]

FWIW, here's a single-click PoC that gets a pretty good success rate for me; I'm pretty sure with some more research, this could be tweaked to be close to 100%:

http://lcamtuf.coredump.cx/ffgeo2/

Comment 8

[Jesse Ruderman]

Please do discuss this class of problems externally.  I have done so:

http://www.squarefree.com/2004/07/01/race-conditions-in-security-dialogs/
http://www.squarefree.com/2005/12/22/microsoft-patches-ui-race-condition-holes-in-ie/

If you decide to use Firefox's geolocation prompt as an example, let me know and I'll make this bug report public.

You might also be interested in the discussion in bug 561177.  The outcome of that bug might be to lower the extension-install-dialog delay to 1s, and turn that dialog into a panel similar to the geolocation prompt, which could inform what happens to the geolocation prompt.

Comment 9

[Daniel Veditz [:dveditz]]

As mentioned in bug 561177 our delays can be inconsistent across the product. Please use the security.dialog_enable_delay so we can be both consistent and tunable (on 3.6... a retrievable door-hanger is another way to go).

Comment 10

[Michal Zalewski]

Ok, I posted a note at:
http://lcamtuf.blogspot.com/2010/08/on-designing-uis-for-non-robots.html

So please feel free to unlock.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

I don't see us blocking on this, but if we can come up with a design that doesn't hurt the user experience, I'd accept the patch.

Comment 12

[Alexandra (retired)]

I set geo.enabled to false in about:config. Additionally, I set the geo.wifi.uri to localhost, since I never liked this feature. Maybe it's an idea to disable it by default with the option to enable the functionality in the privacy dialog? or the other way around, because novice users might not understand the existence of about:config.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Also, this doesn't really seem to apply to Firefox 4 anymore.

Comment 14

[Michal Zalewski]

See comment 4 though.

Comment 15

[Matthew N. [:MattN]]

Attachment: WIP to test a delay on popup notification actions

This is a WIP patch that adds a security delay on all doorhangers using the pref security.notification_enable_delay defaulting to 500ms.

Changes to the pref only take effect after a restart.  The time taken to click on the button will also be dumped to the console starting with the prefix "PopupNotifications_onButtonCommand interval:". The pref and dump output are for testing purposes.

I have try server builds coming soon.

Comment 16

[Mozilla RelEng Bot]

Try run for 4f5e7a800483 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4f5e7a800483
Results (out of 8 total builds):
    success: 8
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4f5e7a800483

Comment 17

[Matthew N. [:MattN]]

Did anyone get a chance to try these builds before they were deleted?

Gavin, what do you think of this approach?

Comment 18

[Matthew N. [:MattN]]

New builds are here:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-90288c4584fe/

Please try these and let me know if this approach is acceptable.  You can tune security.notification_enable_delay to play with the delay.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

This patch still applies cleanly.

Gavin/Margaret: Can you please provide MattN with some feedback here on the approach?

Comment 20

[:Margaret Leibovic]

Comment on attachment 565430 [details] [diff] [review]
WIP to test a delay on popup notification actions

This approach looks alright to me, although I'm wary of the possible UX impact. I think reviewing this patch is more about answering the "Do we want to do this?" question, and that's more Gavin's call than mine.

Comment 21

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

Comment on attachment 565430 [details] [diff] [review]
WIP to test a delay on popup notification actions

We'll need to make sure the number's right, but this approach makes sense in general.

Comment 23

[Daniel Veditz [:dveditz]]

Please unstick this change and get it in. Personally I'd have preferred re-using the existing pref for everything and fixing the number -- human reaction time is human reaction time. The current 2 seconds is too long, I'd prefer 1 second, but I can live with 500ms (can I get 750ms?).

Time quibbles aside, anything is better than nothing!

Comment 24

[Matthew N. [:MattN]]

I'll update the patch today.

Comment 25

[Matthew N. [:MattN]]

Attachment: v.1 750ms delay + tests

(In reply to Daniel Veditz [:dveditz] from comment #23)
> Please unstick this change and get it in. Personally I'd have preferred
> re-using the existing pref for everything and fixing the number -- human
> reaction time is human reaction time. The current 2 seconds is too long, I'd
> prefer 1 second, but I can live with 500ms (can I get 750ms?).

This patch still uses a new pref (with value 750ms) since 1ms feels too long for doorhangers.  I see that the XPI dialog code[1] rounds to the nearest half-second so 750ms would be rounded to 1s for the countdown. Therefore it would countdown like 2=>1=>Install whereas the current code value of 2000ms does 4=>3=>2=>1=>Install.  The countdown interval is fixed at 500ms in the code.

There are other dialog helper functions which can make use of the pref but I don't see any callers using the functionality. A few extensions are using it https://mxr.mozilla.org/addons/search?string=BUTTON_DELAY_ENABLE

I'd be fine with merging the prefs at 750ms if that's fine with people who know more about the dialog delays.  Test out the try build[2] for linux when it's ready and play with the two prefs. (It may have been affected by the network bustage)

[1] https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/xpinstallConfirm.js?rev=2a7e4bc714fe#26
[2] ftp://ftp.mozilla.org/pub/firefox/try-builds/[e-mail]-0f4d576fcd87/

Comment 26

[Matthew N. [:MattN]]

Attachment: v.1.1 move pref observer to member function

It seems cleaner to move the pref observer to a member function.

Using the same pref would also mean changing existing tests to work with the delay.

Comment 27

[Mozilla RelEng Bot]

Try run for 119b84c82bd4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=119b84c82bd4
Results (out of 32 total builds):
    success: 27
    warnings: 3
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-119b84c82bd4

Comment 28

[Matthew N. [:MattN]]

Attachment: v.2 remove pref observer

I had the cleanup of the pref observer wrong and I don't think it's worth doing the un-init on browser close just for testing purposes so I just set the buttonDelay property directly from the test.

Comment 29

[Daniel Veditz [:dveditz]]

(In reply to Matthew N. [:MattN] from comment #25)
> I see that the XPI dialog code[1] rounds to the nearest half-second so 750ms
> would be rounded to 1s for the countdown. Therefore it would countdown like
> 2=>1=>Install whereas the current code value of 2000ms does 4=>3=>2=>1=>Install.
> The countdown interval is fixed at 500ms in the code.

Personally I'm fine with that -- the complaints about the 2 second delay were specifically about the install dialog.

> I'd be fine with merging the prefs at 750ms if that's fine with people who
> know more about the dialog delays.  Test out the try build[2] for linux when
> it's ready and play with the two prefs. (It may have been affected by the
> network bustage)

A sanity check from the UX team would be a good idea.

Comment 30

[Matthew N. [:MattN]]

Comment on attachment 677167 [details] [diff] [review]
v.2 remove pref observer

(In reply to Daniel Veditz [:dveditz] from comment #29)
> (In reply to Matthew N. [:MattN] from comment #25)
> > I see that the XPI dialog code[1] rounds to the nearest half-second so 750ms
> > would be rounded to 1s for the countdown. Therefore it would countdown like
> > 2=>1=>Install whereas the current code value of 2000ms does 4=>3=>2=>1=>Install.
> > The countdown interval is fixed at 500ms in the code.
> 
> Personally I'm fine with that -- the complaints about the 2 second delay
> were specifically about the install dialog.

Are you saying you're fine with changing the existing pref to 750ms and using that for doorhangers as well?

Comment 31

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

Comment on attachment 677167 [details] [diff] [review]
v.2 remove pref observer

Sorry for the delay on this one :(

The 3000ms timeout in test #23 seems like overkill (and makes the whole test run longer unnecessarily). I would probably just use an "executeSoon" - triggerMainCommand() occurs synchronously, so checking that mainActionClicked was not set in onHidden should be sufficient.

Similarly, it would be nice to avoid the timeout in test #24 - could you add an observer notification that fires when the button is re-enabled after a timeout, and use that to check that the button was initially delayed, and then enabled after roughly the timeout that you set?

The behavior of the button being "enabled" but not working has the potential to be confusing, but I guess the delay is small enough that that isn't a big deal (some input from UX on the number would be good - let's poke them more directly and get them a try build to play with, maybe after the holidays?).

Felipe should probably sign off on the telemetry changes, I'm not so familiar with how those work.

Comment 32

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 677167 [details] [diff] [review]
v.2 remove pref observer

Telemetry changes are fine

Comment 33

[Matthew N. [:MattN]]

Thanks Felipe and Gavin!

Gavin, I changed test 23 like you asked but for test 24 I just lowered the delay and the timeout to make the test run faster since there isn't currently a timer that changes the state of the button as the timeout is only checked when clicked.

UX team: Could I get approval on having a 750ms delay before button clicks work on doorhangers? This is to make it harder for an attacker to trick someone into clicking the doorhanger button as soon as it appears. There is no visual difference for the button during this 750ms as it may be distracting.  The theory is that users wouldn't normally click on the button in that time period unless they were being tricked.

http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/[e-mail]-37a6473a64a2/

Testing instructions:
1) Load http://jsfiddle.net/wrCs8/15/embedded/result/
2) Open the error console (Control/Command-Shift-J) and clear it so you will see a message if you click too soon on the doorhanger's button.
3) Click the button on the page twice and see if you would have accidentally triggered the doorhanger's button.
4) Move the page's button to a more common location away from the doorhanger and test the delay.

You can adjust the pref security.notification_enable_delay and restart to change the security delay.

Comment 34

[Justin Dolske [:Dolske]]

While we're all apologizing for late comments... me too. :)

Adding timeout delays that punish the common (non-malicious case) make me sad, and lead to user-annoyance. We've lived with them in a few cases because they were the easiest fix to avoid a problem, but I'd really really really like to avoid adding more.

I wrote a few comments about a possible alternative over in bug 806076... The nutshell idea is to only add a delay (or a _large_ delay?) when the panel/button opens underneath the mouse pointer. There might also be some interesting experimentation/overengineering possibilities in the vein of having a delay inversely proportional to the distance from the last mouse-click location.

A conservative approach like that would also allow us to just add this protection to _all_ doorhanger prompts -- even if they don't have any apparent security impact. It wouldn't be annoying in the common case, and is a useful guard against "oops I didn't mean to click that".

That said, if MattN is rolling his eyes and just wants to land this 2-year-old bug, I am probably ok with deferring my stonewalling until the 3rd time this issue comes up. (._.)

Comment 35

[Matthew N. [:MattN]]

Attachment: v.3 More conservative delay and telemetry to measure clicks sooner than 2s

I discussed this with dolske and we think that a short delay is better than no delay.  I've change the telemetry probe to measure the time it takes users to click the mainAction button in 25ms buckets up to 2s so we can tune the number in a follow-up.

shorlander: The delay was reduced from 750ms to 500ms in this patch.  Do you think this is a reasonable starting point based on your experience with the try build?

dolske: gavin already gave r+ on v.2 but I changed the telemetry since then.

We can handle other approaches such as only adding a delay if the cursor is near a doorhanger in new bugs.

Comment 36

[Stephen Horlander [:shorlander]]

Comment on attachment 703232 [details] [diff] [review]
v.3 More conservative delay and telemetry to measure clicks sooner than 2s

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

> shorlander: The delay was reduced from 750ms to 500ms in this patch.  Do you
> think this is a reasonable starting point based on your experience with the
> try build?

Yes, I think 500ms is probably short enough that it won't negatively impact UX.

Comment 37

[Justin Dolske [:Dolske]]

Comment on attachment 703232 [details] [diff] [review]
v.3 More conservative delay and telemetry to measure clicks sooner than 2s

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

Bah, I didn't notice that gavin had already review this. So I mostly just tested if the delay felt onerous, which it doesn't.

::: toolkit/content/PopupNotifications.jsm
@@ +34,5 @@
>    this.secondaryActions = secondaryActions || [];
>    this.browser = browser;
>    this.owner = owner;
>    this.options = options || {};
> +  this.timeShown = null;

No real need to have this in the constructor, I'd suggest just putting it into the first block of properties in the prototype.

Comment 38

[Matthew N. [:MattN]]

Thanks.

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/da6fdc53ba9c

Comment 39

[Ryan VanderMeulen [:RyanVM]]

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

Comment 40

[Bogdan Maris, Desktop QA]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:22.0) Gecko/20100101 Firefox/22.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0

Verified as fixed in Firefox 22 beta 5 (buildID: 20130612084701), latest Aurora (buildID: 20130612004016) and latest Nightly (buildID: 20130612031138).

"PopupNotifications_onButtonCommand: Button click happened before the security delay" messages appear every time I double click Click Me.