1743226 - Hold Down Enter Key will Automatically Launch Downloaded Executable File

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect, P2)

Description

[Irvan Kurniawan (:sourc7)]

Attachment: testcase.html

I noticed in the newer download panel once the download is complete the download panel will get focus. Interestingly when holding down enter key the downloaded file will be automatically opened.

Attacker can abuse this by serving malicious executable, once user hold down the enter key, the malicious executable will be launched. On Linux OS the .exe file will be launched on Wine installed system.

On older version of Firefox the "OK" button is disabled while holding down enter key. On Chrome browser holding down "Enter" key will not launch the executable.

Steps to Reproduce:

1. Visit attached testcase.htnl
2. Hold down the "Enter" key
3. Command prompt (cmd.exe) will be launched

Comment 1

[Irvan Kurniawan (:sourc7)]

Attachment: Firefox - Hold Down Enter Automatically Launch Executable.mp4

Comment 2

[:Gijs (he/him)]

Thanks for the report! This is nightly-specific at the moment, and I'll add it to our tracking bug for shipping the feature.

Comment 3

[Daniel Veditz [:dveditz]]

Impact is bad for people who are fooled, but "hold the enter key" is always suspicious and would limit the attack's reach

Comment 4

[kpatenio]

Update: made some mistakes explaining how we focus on the download panel the first time I made this comment. Edited to correct them.

This what I believe happens after downloading a file:

1. The download panel gains focus via downloads.js. The download panel (particularly via the download item) gains focus via downloads.js.
2. Afterwards, we focus on a download item using rishlistbox.js. This leads into richlistbox.js, where we read the download items in the download panel.
3. Now that we have full focus, we press the Enter key. This then is detected in PanelMultiView.jsm. However, since we are technically not focusing on a button (i.e. it is not "show in finder"), this actually doesn't do anything because button is undefined.
4. That said, we immediately propagate through several eventListeners until we end up in DownloadsView.onDownloadKeyPress.
5. Here, we actually detect the Enter key again and, as a result, proceed with launching the download.

I have some ideas on how we could proceed, although I don't know if they're the best approach accessibility-wise:

• We could stop or modify focus in #2, such that we no longer focus on a download item immediately after a download; we will still end up focusing on the Downloads Panel, and thus we can use keyboard navigation. Admittedly, I have yet to see how this will turn out once navigation is fixed in Bug 1742530. Made some wrong assumptions initially about how we focus on the downloads panel in steps #1 and #2. After realizing this, removing the focus entirely isn't good at all for screen reader use, and this will result in the screen reader never detecting the downloads panel. So definitely not a good approach.
• Or, alternatively, if we disable the condition for the Enter key in #5, this will prevent us from downloading a file with the Enter key; the Enter key will continue to work for the "Show in finder" buttons. I find this approach to be restrictive though for accessibility reasons.

(Gijs suggested what I believe to be the better approach below. Will comment more on that in another comment)

@jamie, do you have any thoughts/suggestions on these approaches?

Comment 5

[:Gijs (he/him)]

FWIW, what we do in other cases like this is disable the enter functionality for some time period (security.dialog_enable_delay pref value, default 1 second). I also at some point fixed some dialogs to reset that timer if there are keypresses (so keeping the enter key pressed or pressing it very quickly would extend the 1s timeout). However, in the dialog case we expose the button's state as disabled for that duration. I don't know how we'd do that here, and/or if there's some other way for this to be "obvious" for users of assistive technology.

Comment 6

[James Teh [:Jamie]]

We definitely shouldn't focus the Downloads panel without focusing an item inside it. That interrupts the user without providing a screen reader user with any context as to what's going on.

Avoiding auto focus of the Downloads panel is still an option, though that's somewhat of a UX question. In doing that, we're saying that this isn't intended to interrupt users, in the same way that doorhangers aren't meant to interrupt users (and thus don't gain focus). If we do that, we need to provide a message to screen reader users telling them how to interact; e.g. press f6 until they reach the Downloads panel, at which point the active download item should get focus.

I think disabling the enter key for some time period like we do elsewhere is reasonable and probably my preferred approach. It's true that it's difficult to communicate this to screen reader users, but most users probably won't try to interact that quickly anyway and it's reasonable that they might try again if it didn't work the first time. I think the security benefit here outweighs the potential user confusion.

How would this be communicated visually? I assume there's a launch button which would get disabled?

Comment 7

[:Gijs (he/him)]

(In reply to James Teh [:Jamie] from comment #6)

How would this be communicated visually? I assume there's a launch button which would get disabled?

No, they are richlistitems and they can be interacted with "as a whole", or with the secondary button inside it. Clicking the overall list item vs. clicking the secondary button has different behaviour, but there is no button equivalent to the entire item - the entire item has a focus/hover and hover:active style when used, and it's not entirely clear how we would indicate disabling the item as a whole. The best option might dimming the contents of the entire thing, but we don't currently have a disabled state for these items.

Comment 8

[kpatenio]

Attachment: Temporarily disabling menu items (voiceOver)

I tried playing around with disabling the menu items for 1 second. Once 1 second has passed, the menu items are renabled, and we then regain focus on the downloads panel. While the menu items are disabled, we won't be able to navigate to them.

Will put up a WIP patch on phabricator soon. (Update: the patch is ready for testing and review now!)

@jteh do let us know if you find any issues with the patch, especially with screen readers.

Comment 9

[kpatenio]

Attachment: Bug 1743226 - improve downloads user experience

Comment 10

[kpatenio]

In a separate discussion while fixing issues with tests for this bug, we found that there are multiple attempts to show the downloads panel. (Noticeable after adding a log statement in _openPopupIfDataReady and running the test case from the patch, for example). Just setting a needinfo (as requested on Slack) to remind us of this behaviour.

gijs, please remove this needinfo if it's no longer required/needed!

Comment 11

[kpatenio]

jteh has already provided feedback on the patch (and I believe the patch is already landed). Removing needinfo.

Comment 12

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

improve downloads user experience r=mhowell,Gijs
https://hg.mozilla.org/integration/autoland/rev/df9b9b555b16da62c828c38ceff229d76b720627
https://hg.mozilla.org/mozilla-central/rev/df9b9b555b16

Comment 13

[:Gijs (he/him)]

(In reply to kpatenio from comment #10)

In a separate discussion while fixing issues with tests for this bug, we found that there are multiple attempts to show the downloads panel. (Noticeable after adding a log statement in _openPopupIfDataReady and running the test case from the patch, for example). Just setting a needinfo (as requested on Slack) to remind us of this behaviour.

gijs, please remove this needinfo if it's no longer required/needed!

Thanks, I filed bug 1747465 for this bit.

Also copying in QA folks given they filed bug 1747447

Comment 14

[Daniel Veditz [:dveditz]]

I don't believe this is entirely fixed. There is a short delay before the dialog is active, but the dangerous "Open" action is still the selected default and activated if you miss seeing the dialog. As I did on a wide monitor while I was looking at my car accellerating on the opposite side of the screen.

Visually, there's nothing showing any focus so there's no user clue what action will be selected by keystroke. When I hit the tab button then there are very clear focus highlights. I suppose that's a separate bug, but it plays into the next bit:

The delay is good, but most useful to defeat "surprise popups" for mouse clicking games. When we have a known dangerous action (in this case we know it's an "executable" type) we should also not have a default focused button. The user should need to tab or mouse-click to get it.

Comment 15

[:Gijs (he/him)]

(In reply to Daniel Veditz [:dveditz] from comment #14)

I don't believe this is entirely fixed. There is a short delay before the dialog is active, but the dangerous "Open" action is still the selected default and activated if you miss seeing the dialog. As I did on a wide monitor while I was looking at my car accellerating on the opposite side of the screen.

Are you testing without the pref enabled? This bug is about the new behaviour in the downloads panel - there's no dialog, only the downloads panel, so the rest of this comment is difficult to understand - there is no "default button" as such...

Comment 16

[:Gijs (he/him)]

(In reply to Daniel Veditz [:dveditz] from comment #14)

The delay is good, but most useful to defeat "surprise popups" for mouse clicking games. When we have a known dangerous action (in this case we know it's an "executable" type)

Esp. in the non-Windows case, I'm not sure the "we know it's executable" bit is true for .exe files generally.

we should also not have a default focused button. The user should need to tab or mouse-click to get it.

For context, there was a bunch of discussion around a11y in/about bug 1742530 and friends, and AIUI we need focus to be in the downloads panel when it opens (or soon after). It would also be confusing for users if the focus was somewhere else in the panel depending on whether the download we started was deemed "dangerous". Normally speaking, we focus items, and hitting enter when an item is focused will open/launch it, which is why this is happening.

We could potentially do something about holding down enter, using similar logic as in EnableDelayHelper (from bug 1552627), but then the trick here would still work for repeatedly pressing enter, and based on your comments that sounds like you also don't think it's sufficient?

Basically, I'm not sure we can easily do what you're suggesting without significant other downsides. We could try always focusing the inner button (to open in the file explorer or similar) but that also seems unhelpful in the majority, non-malicious case (and will look weird to mouse users if we show that focus visually). So I'm struggling to see a way forward that makes this materially safer, without significantly impacting either a11y or "normal" download uses, or both.

Comment 17

[Daniel Veditz [:dveditz]]

Are you testing without the pref enabled?

Which pref? I'm testing in Nightly (where the patch landed) and haven't changed any prefs related to downloads. If it's browser.download.improvements_to_download_panel that one appears to default to true. browser.download.enable_spam_prevention is false

there is no "default button" as such...

a metaphorical button -- a thing that has focus and can be pressed/activated

The new design removes all the impediments to launching an executable from the web, all the opportunities to stop and ask yourself if that's what you want to do. That's nice when that's what you intended, I guess, but it also makes it easier to accidentally launch evil executables because there are no speedbumps slowing down the victim.

I don't suppose you could reset the dialog delay timer each keystroke that comes in during the delay period? That would stop this attack, and the similar "press return repeatedly as fast as you can". (It also might annoy impatient people who have to wait longer and longer the more they jump the gun on the time. Speaking of which, without a focus ring or an actual button, there's no visual clue (that I've noticed) that tells the user that the dialog is disabled initially. I don't know if that's much of a problem.

Another thing that might work would be if you detect keystrokes during the delay period, then remove the focus from the item/launch-"button" and make people do some keyboard navigation (tab probably? arrow?) to get that behavior. I guess that means the panel as a whole still needs to have focus so "tab" would do the right thing, so maybe move focus to the last "show all downloads" item so the default is non-dangerous and a tab keypress puts you on the first item.

Comment 18

[:Gijs (he/him)]

(In reply to Daniel Veditz [:dveditz] from comment #17)

Are you testing without the pref enabled?

Which pref? I'm testing in Nightly (where the patch landed) and haven't changed any prefs related to downloads. If it's browser.download.improvements_to_download_panel that one appears to default to true. browser.download.enable_spam_prevention is false

there is no "default button" as such...

a metaphorical button -- a thing that has focus and can be pressed/activated

The new design removes all the impediments to launching an executable from the web, all the opportunities to stop and ask yourself if that's what you want to do. That's nice when that's what you intended, I guess, but it also makes it easier to accidentally launch evil executables because there are no speedbumps slowing down the victim.

I don't suppose you could reset the dialog delay timer each keystroke that comes in during the delay period?

We could, yes. We could potentially limit that only to files that we think are executables?

That would stop this attack, and the similar "press return repeatedly as fast as you can". (It also might annoy impatient people who have to wait longer and longer the more they jump the gun on the time.

Yeah, I don't think there's any way to avoid that though - fundamentally there's no difference between "being impatient" and the kind of "trick" that the poc here uses, apart from intent, which is impossible to determine from software.

Speaking of which, without a focus ring or an actual button, there's no visual clue (that I've noticed) that tells the user that the dialog is disabled initially. I don't know if that's much of a problem.

The contents of the panel are greyed out (see also bug 1747447). If they're not for you, please file a separate bug with more details.

Another thing that might work would be if you detect keystrokes during the delay period, then remove the focus from the item/launch-"button" and make people do some keyboard navigation (tab probably? arrow?) to get that behavior. I guess that means the panel as a whole still needs to have focus so "tab" would do the right thing, so maybe move focus to the last "show all downloads" item so the default is non-dangerous and a tab keypress puts you on the first item.

Yeah, I guess that could work.

Comment 19

[:Gijs (he/him)]

OK, I'm going to re-close this because given that Monday is merge day we're unlikely to be able to have something ready and landed by then, and if that doesn't happen we'll need to track uplift, which is tricky to do given a patch has already landed in this bug that we won't want to back out. I'll clone this bug to work on the fixes Dan has suggested, and hopefully we'll be able to uplift that into 97.

Comment 20

[Hani Yacoub, Desktop QA]

I confirm that there is a delay before the dialog is active comparing with a build without a fix.
I'll mark the bug as verified as fixed on Windows 10 x64, macOS 11.6 and on Ubuntu 20.04 x64 on Firefox Nightly 97.0a1 (2022-01-07) and on Firefox Nightly 98.0a1 (2022-01-12) .

Gijs, is it possible to have the number of the cloned bug so we could have an eye on and verify it once it will be landed?
Thanks.

Comment 21

[:Gijs (he/him)]

(In reply to Hani Yacoub from comment #20)

I confirm that there is a delay before the dialog is active comparing with a build without a fix.
I'll mark the bug as verified as fixed on Windows 10 x64, macOS 11.6 and on Ubuntu 20.04 x64 on Firefox Nightly 97.0a1 (2022-01-07) and on Firefox Nightly 98.0a1 (2022-01-12) .

Gijs, is it possible to have the number of the cloned bug so we could have an eye on and verify it once it will be landed?
Thanks.

It's bug 1749028, I'll CC you.