1440079 - (CVE-2019-11697) Tricking user into accepting PopupNotification prompts through holding down accessKey

Status: VERIFIED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect, P1)

Description

[Abdulrahman Alqabandi]

Attachment: installExt.xpi

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.167 Safari/537.36

Steps to reproduce:

Install attached PoC extension and follow instructions.


Actual results:

I am mainly using the behavior where if a XPI file is visited then Firefox will automatically attempt to install and prompt user for confirmation.

I noticed that you press ALT+A you can agree to installing the webextension. And so I ask a user to hold down ALT+A and I then attempt to install addon only to trick user into agreeing to install.


Expected results:

Block from ALT+A being activated as soon as the install prompt appears.

A version of this can be done from normal web. See comment 1 and comment 2

Comment 1

[Abdulrahman Alqabandi]

Attachment: new.html

Host the attached HTML and open it with Firefox. Hold down ALT+A and you will automatically install extension.

Comment 2

[Abdulrahman Alqabandi]

Attachment: new2.html

This one is different in that all I do is open a popup to an XPI with noopener tag, this way we initiate install prompt automatically. Bypassing the usual "We prevented this site from installing..." initial prompt as it appears in the PoC for comment 1.

I also redirect original page to a legitimate extension that is more popular. 


Here is a video of all three instances: https://www.youtube.com/watch?v=_UsdiuH9yHc

Comment 3

[Abdulrahman Alqabandi]

Attachment: new3.html

Using fullscreen we can perform a little more convincing attack.

Video: https://www.youtube.com/watch?v=9hK_8DjfrC8

Comment 4

[Abdulrahman Alqabandi]

Attachment: new4.html

Using bug 1294413, we can cover the webextension install prompt with our own message.

Comment 5

[Abdulrahman Alqabandi]

Attachment: mozerrext.png

Screenshot of covering xpinstall prompt using bug 1294413

Comment 6

[Kris Maglione [:kmag]]

We used to have a mitigation for this in the old XPInstall dialog. It disabled the install button until a timeout had elapsed without any input or the dialog losing focus. I'm not sure why that was removed in the new flow.

Comment 7

[Daniel Veditz [:dveditz]]

So this is essentially bug 162020 (from 2002!) or one of the many variants come back to bite us :-(

Andy: do you remember who implemented these prompts?

Comment 8

[Kris Maglione [:kmag]]

(In reply to Daniel Veditz [:dveditz] from comment #7)
> So this is essentially bug 162020 (from 2002!) or one of the many variants
> come back to bite us :-(
> 
> Andy: do you remember who implemented these prompts?

As far as I can tell, this protection disappeared with some install flow changes that Mossop implemented years ago. The revision history is pretty messy, though, so it's hard to track down the exact bug.

But the last time we significantly refactored the install flow was in bug 1308295, by aswan.

Comment 9

[Andrew Swan [:aswan]]

My understanding was that the delay is supposed to be handled inside PopupNotifications.jsm
eg: 
https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/toolkit/modules/PopupNotifications.jsm#1496-1501

where buttonDelay comes from:
https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/modules/libpref/init/all.js#2628

Comment 10

[Andrew Swan [:aswan]]

I did a little more archaeology, the old install dialog is actually still in the tree at toolkit/mozapps/extensions/content/xpinstallConfirm.{xul,js}  There are some differences:

PopupNotifications:
  - Use the preference security.notification_enable_delay which defaults to 500ms
  - just ignores clicks before the delay has elapsed
xpInstallConfirm:
  - Uses the preference security.dialog_enable_delay which defaults to 1 second
  - Disables the button and displays a counter that counts down during the delay, enabling the button after the delay is over

The real functional difference is 1 second vs 500ms, the rest appears to be cosmetic (but probably more useful to avoid confusing/frustrating users the longer the lockout lasts)

I didn't actually verify that the PopupNotifications delays are working, but they appear to be covered by a test:
https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/browser/base/content/test/popupNotifications/browser_popupNotification_3.js#128-151

Comment 11

[Abdulrahman Alqabandi]

Even if the old fix is implemented (timing out confirm button for X number of seconds) can be bypassed by loading a window in the background of the main window and then focusing on it after >X number of seconds. 

That can only be done using WebExtensions (a version can be done using normal web but requires much higher user interaction).
I think it's worth thinking about dealing with the prompt onblur. Either reset the timer onfocus or send a cancel event onblur (so if the user blurs a tab/window with a XPinstall is the same as if he pressed "cancel")

Comment 12

[Kris Maglione [:kmag]]

(In reply to Abdulrahman Alqabandi from comment #11)
> Even if the old fix is implemented (timing out confirm button for X number
> of seconds) can be bypassed by loading a window in the background of the
> main window and then focusing on it after >X number of seconds. 

 No it can't. The delay in the old xpinstall dialog requires that the dialog be focused for the entire length of the delay. The counter resets whenever the dialog blurs.

Comment 13

[Abdulrahman Alqabandi]

Bug described in comment 2 no longer works on latest Nightly.

Comment 14

[Andrew Swan [:aswan]]

Finally getting back to this, I'd like to get a second opinion on what we should do here.
I think the main functional difference we need is the blur/focus handling that xpinstallConfirm had (to be explicit, that is the notification must be focused for the entire delay, if it loses focus we start the delay timer over again).
There's also the difference in the timing of the delay (500 vs 1000 milliseconds, see comment 10).
Finally, disabling the primary button is cosmetic but as we expand the number of situations where the button doesn't work, it gets more and more important).

This will go into PopupNotifications, I'm not sure how much of it to make default and how much to make opt-in for users of PopupNotifications.  Johann, any thoughts?

Comment 15

[Andrew Swan [:aswan]]

And upon reading more carefully, PopupNotifications does handle the case where a different top level window is focused:
https://searchfox.org/mozilla-central/rev/795575287259a370d00518098472eaa5b362bfa7/toolkit/modules/PopupNotifications.jsm#622-627

There are still a few different "attacks" described in this bug:

Neither of the attacks from the new2.html or new3.html attachments appear to work on a recent Nightly

The attack in new4.html takes advantage of bug 1294413.  I think the last one should be handled separately, but bug 1294413 is public.  It looks like :jkt started working on it a while back.  Jonathan, do you think you can pick that one back up?

Finally, the attack in new.html entails requires tricking the user into holding down the access key that accepts the permission prompt, that works due to standard keyboard repeat behavior where holding down a key (or combination of keys) generates a series of keypress events.  I think it would make sense for doorhangers to ignore key events for repeated keypresses, but fixing that is going to entail messing with a pile of XBL bindings :(  I'll see if I can come up with something though.

Comment 16

[Andrew Swan [:aswan]]

And follow-up to the last part of the previous comment, accessKey handling doesn't happen from an XBL binding, its at a much lower level, so I'm not sure how practical its going to be to ignore events that come from keyboard repeat...

Comment 17

[Johann Hofmann [:johannh]]

Hm I have to say that the "holding down alt or ctrl + a" exploit worries me. It effectively renders the 500ms delay completely useless. This affects all PopupNotifications (also permission prompts) and we should probably investigate fixing it despite the parts about that being pretty hard :)

Comment 18

[Abdulrahman Alqabandi]

Hrmm. For some reason on a recent Nightly build holding down ALT+A doesn't trigger the event listener (for the PoC in comment 1). I re-tested on an older build and it still works there. Odd.

If you are having trouble reproducing the bug described in comment 1 just replace 'onkeypress' with 'onkeydown' in the original PoC attached.

Comment 19

[Andrew Swan [:aswan]]

I chatted with Johann a bit and we're both unsure about the implementation here.
A few questions -- :smaug, if you're not the right person to answer these can you redirect?

The quick background is that the attack we want to fix here involves tricking a user into holding down the key combination that corresponds to the access key for the accept button on a permission dialog, which causes one of the later repeated keypress events to accept the permission before the user gets a chance to see it.

So I think the desired behavior is these notifications ignore keypress events if the "repeat" property is true.  These notifications are a pile of xul and xbl but underneath it all is a xul button with an accesskey attribute.

So first question: can we make this change from javascript or are events that match an accesskey handled at some lower layer and not observable from javascript?  If its the latter, how feasible is this to address?  Would something like adding an attribute to xul elements that would cause repeated keypress events to be ignored be reasonable?  Regardless, pointers to the existing code that handles accesskeys would be helpful...

Comment 20

[Matthew N. [:MattN]]

Did you try a capturing addEventListener on the button with preventDefault or stopImmediatePropagation if .repeat is true?

Comment 21

[Abdulrahman Alqabandi]

Attachment: xpi.html

I'm doubtful that involving the repeat var would suffice in preventing a similar attack. Hypothetically if repeat was checked, wouldn't we just be able to show the notification as soon as 'alt' was pressed once (and 'a' pressed very shortly after)? Or would the 500ms be enough for the user to notice something happening visually and release the keys?

Attached is a PoC without using repeat. Worth testing it when fix is applied.

Comment 22

[Johann Hofmann [:johannh]]

(In reply to Abdulrahman Alqabandi from comment #21)
> Created attachment 8972955 [details]
> xpi.html
> 
> Or would the 500ms be enough for the user to
> notice something happening visually and release the keys?

Yes, that's what this delay assumes. The idea for a fix is to always reset the counter when a repeat key event is registered, so the only exploit left would have to time this so that the user presses "a" over 500ms after the dialog was spawned. And even then they would only have managed to skip the first doorhanger, whereas add-on installs have several levels of confirmation (unlike permission requests, but I think that's an okay trade-off).

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #20)
> Did you try a capturing addEventListener on the button with preventDefault
> or stopImmediatePropagation if .repeat is true?

I was planning to look into that soon, but so far we just talked about it. My suspicion is that EventStateManager handles the shortcut before we get a chance to, but I might be wrong.

Comment 23

[Andrew Swan [:aswan]]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #20)
> Did you try a capturing addEventListener on the button with preventDefault
> or stopImmediatePropagation if .repeat is true?

We already have a capturing listener:
https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/toolkit/modules/PopupNotifications.jsm#1146

That listener doesn't currently try to handle the accessKey but empricially, it is not invoked when the accessKey is pressed)

Comment 24

[Andrew Swan [:aswan]]

I'm not actively working on this, if whoever handles Notifications and Alerts is completely overwhelmed, I can try to help out but we need an answer to comment 19 first.

Comment 25

[Matthew N. [:MattN]]

(In reply to Andrew Swan [:aswan] from comment #24)
> I'm not actively working on this, if whoever handles Notifications and
> Alerts is completely overwhelmed, 

The component is a dumping ground for like 4-5 different "notification" things so it doesn't really map to resourcing. I would say doorhanger stuff should fall under the Privacy team since they did a lot of work on it in the last few years.

Comment 26

[Johann Hofmann [:johannh]]

I can take a look :)

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Access keys are handled in https://searchfox.org/mozilla-central/rev/00dd116638001772fe354b081353b73f1cad405d/dom/events/EventStateManager.cpp#708-737 which is before DOM event dispatch.

Comment 28

[Johann Hofmann [:johannh]]

Attachment: Add a way to prevent accepting repeat key events for accesskeys

So, this is a pretty simple first patch that considers a magic accesskey-prevent-repeat attribute on elements to avoid performing the accesskey in that case.

Smaug, do you think that's the right approach?

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8976278 [details] [diff] [review]
Add a way to prevent accepting repeat key events for accesskeys

Do we ever need to handle access keys for repeated keys?
a11y folks may have an opinion here, but I hope not.
Ask surkov?

accesskey-prevent-repeat shouldn't of course be supported on HTML elements.

Comment 30

[Johann Hofmann [:johannh]]

Hm, yeah. I agree that repeat key events for access keys doesn't sound useful. OTOH when testing this for HTML elements, Chrome and Safari have the same behavior (allowing repeated access keys). I'm not sure we should start breaking compatibility in content land here, since this attack pretty specifically targets browser chrome.

So, we could either globally disallow repeat for XUL elements or in chrome windows (with the expectation that our chrome windows will contain HTML at some point).

surkov, do you have an opinion on this?

Comment 31

[alexander :surkov (:asurkov)]

(In reply to Johann Hofmann [:johannh] from comment #30)
> Hm, yeah. I agree that repeat key events for access keys doesn't sound
> useful. OTOH when testing this for HTML elements, Chrome and Safari have the
> same behavior (allowing repeated access keys). I'm not sure we should start
> breaking compatibility in content land here, since this attack pretty
> specifically targets browser chrome.

I'm not aware of a reason why we would have to handle access keys for repeated keys, but agreed it better stay compatible with other browsers. Curious, whether this should be addressed by HTML standard to bring everyone on the same page?

> So, we could either globally disallow repeat for XUL elements or in chrome
> windows (with the expectation that our chrome windows will contain HTML at
> some point).

It seems reasonable.

Comment 32

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 24 weeks.

Comment 33

[David Durst [:ddurst]]

Moving back to P1 because it's blocking another P1. Johann: can you finish off this bug and unblock bug 1435616?

Comment 34

[Johann Hofmann [:johannh]]

Yup, sorry, this is overdue...

(Clearing NI for jkt as well, it's been a while)

Comment 35

[Johann Hofmann [:johannh]]

Attachment: Bug 1440079 - Don't allow repeated access keys in chrome documents. r=smaug

Comment 36

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/9ee0c8bc5a2f2d30d3a5e8170d01d98449bc07e8
https://hg.mozilla.org/mozilla-central/rev/9ee0c8bc5a2f

Comment 37

[Ryan VanderMeulen [:RyanVM]]

I'm inclined to say that this can ride the trains. Do you agree, Johann?

Comment 38

[Johann Hofmann [:johannh]]

Yeah, I agree, we shouldn't rush this. :)

Comment 39

[Timea Cernea [:tbabos][inactive]]

Verifying this issue on latest Beta 67.0b3 on Windows 10 x64:

• installed the temporary PoC attached (replaced with keydown) and the prevent notification is not bypassed
• the xpi.html file provided in Comment 21 no longer bypasses the prevent notification
• new2.html file is also prevented from installing
• new3.html is prevented from installing
• new4.html which should cover the web extension install prompt may also be fixed as it no longer displays the message as in the attached screenshot. Now it will look like this https://imgur.com/a/bIetYDK, is this the intended fix?

Hopefully, I covered all the testcases that were provided by the reporter. Abdulrahman, could you please take a look again? Just to be sure... Will verify the rest of the OS afterward. Here is the link for the latest Beta that has this patch: http://archive.mozilla.org/pub/firefox/candidates/67.0b3-candidates/build1/

Comment 40

[Abdulrahman Alqabandi]

It does appear that all original PoC's are fixed on Beta 67.0b3, tested on windows 10 x64.

A few notes:

• 'new3.html' PoC, although the trick doesn't work anymore I did notice that the popup can still block the 'You are now in fullscreen' text. This could be another bug worth a seperate file.

• PoC in Comment 21, 'xpi.html' - Although the original bug is fixed, there is still some way to trick a user into installing by having the user instead of holding down 'ALT+A' they would hold down 'ALT' and at the same time repeatedly press 'A', granted this is much more user interaction but worth noting I think.

I will do more testing and see if all is well and will make a new comment only if I find something.

Comment 41

[Timea Cernea [:tbabos][inactive]]

Thanks Abdulrahman!
If you find any other scenario worth looking into, like the one from Comment 40, please file a new bug for it.

Verified - Fixed on Windows 10, Ubuntu 16.04 and Mac OSx (only the scenarios without the alt+a combination in respect for the missing key combination)

Comment 42

[Frederik Braun [:freddy]]

Requesting bounty consideration on behalf of the reporter.