Add a security delay to the main action of PopupNotifications

RESOLVED FIXED in Firefox 22

Status

()

defect
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: lcamtuf, Assigned: MattN)

Tracking

({csectype-spoof, privacy, sec-low})

3.6 Branch
Firefox 21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox21 disabled, firefox22 verified, blocking2.0 -, status1.9.2 wanted)

Details

(Whiteboard: [sg:low], )

Attachments

(1 attachment, 4 obsolete attachments)

...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.
Keywords: privacy
Whiteboard: [sg:low]
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.
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.
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.
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.
Currently there is no delay.
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.
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/
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.
blocking2.0: --- → ?
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).
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.
Group: core-security
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.
blocking2.0: ? → -
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.
Also, this doesn't really seem to apply to Firefox 4 anymore.
See comment 4 though.
OS: Windows XP → All
Hardware: x86 → All
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.
Assignee: nobody → mnoorenberghe+bmo
Status: NEW → ASSIGNED
Attachment #565430 - Flags: feedback?(gavin.sharp)
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/mozilla@noorenberghe.ca-4f5e7a800483
Did anyone get a chance to try these builds before they were deleted?

Gavin, what do you think of this approach?
New builds are here:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mozilla@noorenberghe.ca-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.
Attachment #565430 - Flags: feedback?(margaret.leibovic)
This patch still applies cleanly.

Gavin/Margaret: Can you please provide MattN with some feedback here on the approach?
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.
Attachment #565430 - Flags: feedback?(margaret.leibovic) → feedback+
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.
Attachment #565430 - Flags: feedback?(gavin.sharp) → feedback+
Duplicate of this bug: 805395
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!
I'll update the patch today.
Posted patch v.1 750ms delay + tests (obsolete) — Splinter Review
(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/mozilla@noorenberghe.ca-0f4d576fcd87/
Attachment #565430 - Attachment is obsolete: true
Attachment #676850 - Flags: review?(gavin.sharp)
Attachment #676850 - Flags: review?(dveditz)
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.
Attachment #676850 - Attachment is obsolete: true
Attachment #676850 - Flags: review?(gavin.sharp)
Attachment #676850 - Flags: review?(dveditz)
Attachment #676883 - Flags: review?(gavin.sharp)
Attachment #676883 - Flags: review?(dveditz)
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/mozilla@noorenberghe.ca-119b84c82bd4
Posted patch v.2 remove pref observer (obsolete) — Splinter Review
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.
Attachment #676883 - Attachment is obsolete: true
Attachment #676883 - Flags: review?(gavin.sharp)
Attachment #676883 - Flags: review?(dveditz)
Attachment #677167 - Flags: review?(gavin.sharp)
Attachment #677167 - Flags: review?(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 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?
Attachment #677167 - Flags: ui-review?(ux-review)
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.
Attachment #677167 - Flags: review?(gavin.sharp)
Attachment #677167 - Flags: review+
Attachment #677167 - Flags: feedback?(felipc)
Comment on attachment 677167 [details] [diff] [review]
v.2 remove pref observer

Telemetry changes are fine
Attachment #677167 - Flags: feedback?(felipc) → feedback+
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/mozilla@noorenberghe.ca-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.
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. (._.)
Summary: Geolocation prompts need security delays → Add a security delay to the main action of PopupNotifications
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.
Attachment #677167 - Attachment is obsolete: true
Attachment #677167 - Flags: ui-review?(ux-review)
Attachment #677167 - Flags: review?(dveditz)
Attachment #703232 - Flags: ui-review?(shorlander)
Attachment #703232 - Flags: review?(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]:
-----------------------------------------------------------------

> 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.
Attachment #703232 - Flags: ui-review?(shorlander) → ui-review+
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.
Attachment #703232 - Flags: review?(dolske) → review+
https://hg.mozilla.org/mozilla-central/rev/da6fdc53ba9c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Keywords: csec-spoof
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.
You need to log in before you can comment on or make changes to this bug.