On Windows, we could use BFFM_ENABLEOK to have the dialog start out with a disabled OK button, then start a timer from the BFFM_INITIALIZED message so that it's not enabled for a half-second or whatever timeout we think is reasonable.

I actually meant ignoring the result of the directory listing returned from the dialog if the time passed is too short (making the dialog a no-op), but not allowing the user to accept the dialog before a certain timeout also seems reasonable. We do that for extension install dialogs using a 500ms delay.

jsbell@ flushing input queue is not possible. 

It seems reasonable some timeout to debounce this. Is that possible in the file input control?

I don't think there is anything that I can do in the input stack and it needs to be at the file api/input file control (which is Blink>Forms) to fix this.

Over to Joshua..

This is one of our oldest open security bugs (480 days). We really need to get progress on it.

Note that Medium severity bugs are Pri-1, not 2. However, I think this may be worse than Medium. I would call it High; it works too easily. The 'gesture laundering' is quite problematic, as is the default directory (/home on Linux — ?!, and \Users\palmer on Windows).

fortunately, on Linux, the exploit (even the limited one in #2) simply crashes Chrome after grinding for a while. I take it webkitdirectory just doesn't work on Linux? The PoC works reliably on Windows.

Especially since the feature is non-standard and name-prefixed, I think we should consider removing it until we have a good story for how to make this safe. I don't consider it safe at all right now. I'm debating whether we should call it Critical severity, to be honest.

+estark for Security UX thoughts; +owencm for API design thoughts (we've been talking about this in other contexts)

Can someone please test how this behaves on Android? Thanks.

Re-assigning based on the component. keishi, can you please take a look? Thanks.

Re #59: this isn't very interesting on Android phones at least, as it requires multiple taps in precise places. Perhaps an Android tablet with an attached keyboard would behave a bit more like Desktop (due to default focus) but I expect not. Further, as far as I can tell, on Android and iOS, the picker acts as a single file picker rather than a folder picker. 

#63: It sounds like `webkitdirectory` is not supported on Android or iOS, and is non-functional on Linux. That's a point in favor of un-shipping it (at least until we can make it safe), in my view.

A quick check of our use counters for Blink.UseCounter.Features for PrefixedDirectoryAttribute versus total page visits shows that this appears to be used by a tiny percentage of page visits, which does align with this being non-standard and prefixed (and I think supports un-shipping this for now).

We commit ourselves to a 60 day deadline for fixing for high severity vulnerabilities, and have exceeded it here. If you're unable to look into this soon, could you please find another owner or remove yourself so that this gets back into the security triage queue?

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

webkitdirectory is supported on Linux - not sure how you're determining otherwise. It's also supported by Firefox, Edge and Safari (as of Tech Preview 42 I believe); despite the prefix in the name, it was in high enough demand that other browsers implemented support.

I implemented the suggestion in #55 locally (ignore the result of the dialog if < 500ms between dialog launch and dialog commit). On Linux this seems insufficient; using the focus-switch trick, focus is shifted to to the Input element after the first repeat; on the second repeat the dialog is shown (and the time is recorded); the dialog pops up with focus on the Upload button (which depresses); the dialog then waits until the user releases the Enter key - which could be past the threshold - at which point it commits, with no way for the user to cancel. It may be a better mitigation on other platforms.

Can we re-enumerate the possible mitigations here?

(1) show the picker on key up (so repeat won't be triggered) (#37)
(2) start the picker with focus not on the accept button (platform-specific; may not be possible everywhere)
(3) start the picker with the accept button disabled; enable after some threshold (platform-specific; may not be possible everywhere. what is behavior if the Enter key is held down?) (#54)
(4) ignore the key down if the event is repeating (this only mitigates the specific repro; if focus starts on the input it wouldn't help) (#38)

jsbell do you object to unshipping the webkitdirectory, at least for now? Other than that, it sounds like (1) (#37) is the fastest path to a reliable cross-platform fix?

Unshipping doesn't seem feasible; the usage is relatively high and it is relied upon by Google properties (e.g. Drive) and likely all the other "cloud storage" services (DropBox, etc)

Note that the default UX is so bad that most use of it is via script mapping a click on some more attractive UI to a click() call against the control, so I suspect the "key up" change wouldn't break the web, but I don't know forms/events that well.

Re #67: The PoC in #2 consistently crashes the entire browser on Linux.

Re #71: Is there a use counter for it? We should at least have that.

It sounds like you're saying gesture laundering is the only way that it works in the field?

> Re #67: The PoC in #2 consistently crashes the entire browser on Linux.

That may be Chrome trying to recurse through the entire file hierarchy of your home directory. The *feature* works, even if the naive POC does not. If you run through a modified repro by initially selecting a subdir then the enter key repetition still behaves per POC.

> Re #71: Is there a use counter for it? We should at least have that.

PrefixedDirectoryAttribute is the right one.

https://www.chromestatus.com/metrics/feature/timeline/popularity/47

No idea about the recent spike, but 0.03% of sites sounds about right. It's that it's a critical feature on those sites. (like Drive)

> It sounds like you're saying gesture laundering is the only way that it works in the field?

If "only" means "primary" and "it works" means "the feature is used" then sure. E.g. run through Drive's UI for "Upload Directory". That's pretty typical.

FWIW, I mocked up the "keyup" approach and sent a sample patch to estark@. "Works for me" but I'm unsure if it's sufficient, i.e. are there tweaks to the POC that bypass it.

I suggest approaching this as follows:

(1) Land the keyup mitigation for webkitdirectory. It's not perfect because an attacker could still trick the user into clicking the upload button or pressing Enter again. But it helps a little bit, on all platforms. And there's no harm in committing a partial fix because the bug is already public.

(2) Land the keyup mitigation for <input type=file> as well. I suggest doing this separately because it seems more likely to cause compat controversy.

(3) Land platform-specific mitigations: change the default directory on Windows and/or see if we can get the timer to work as described in #54. Investigate if similar things are possible on Mac and Linux.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/745e1b4545d51a2ad620d590682ca6eadd95f36c

commit 745e1b4545d51a2ad620d590682ca6eadd95f36c
Author: Joshua Bell <jsbell@chromium.org>
Date: Tue Dec 12 19:28:54 2017

<input type=file webkitdirectory>: Launch picker on Enter keyup

Avoid key repeat triggering an unintentional commit of the subsequent
dialog by launching the picker on the Enter keyup event rather than on
keydown.

Bug:  637098 
Change-Id: Ia327a7a9f92c34fd3fcdf971d8c21a5ce744254b
Reviewed-on: https://chromium-review.googlesource.com/814916
Commit-Queue: Joshua Bell <jsbell@chromium.org>
Reviewed-by: Chris Palmer <palmer@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Keishi Hattori <keishi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523507}
[modify] https://crrev.com/745e1b4545d51a2ad620d590682ca6eadd95f36c/third_party/WebKit/LayoutTests/fast/forms/enter-clicks-buttons.html
[add] https://crrev.com/745e1b4545d51a2ad620d590682ca6eadd95f36c/third_party/WebKit/LayoutTests/fast/forms/file/file-input-webkitdirectory-key-enter-prevent-keypress-expected.txt
[add] https://crrev.com/745e1b4545d51a2ad620d590682ca6eadd95f36c/third_party/WebKit/LayoutTests/fast/forms/file/file-input-webkitdirectory-key-enter-prevent-keypress.html
[add] https://crrev.com/745e1b4545d51a2ad620d590682ca6eadd95f36c/third_party/WebKit/LayoutTests/fast/forms/file/file-input-webkitdirectory-key-enter-prevent-keyup-expected.txt
[add] https://crrev.com/745e1b4545d51a2ad620d590682ca6eadd95f36c/third_party/WebKit/LayoutTests/fast/forms/file/file-input-webkitdirectory-key-enter-prevent-keyup.html
[modify] https://crrev.com/745e1b4545d51a2ad620d590682ca6eadd95f36c/third_party/WebKit/Source/core/html/forms/FileInputType.cpp
[modify] https://crrev.com/745e1b4545d51a2ad620d590682ca6eadd95f36c/third_party/WebKit/Source/core/html/forms/FileInputType.h
[modify] https://crrev.com/745e1b4545d51a2ad620d590682ca6eadd95f36c/third_party/WebKit/Source/core/html/forms/KeyboardClickableInputTypeView.cpp
[modify] https://crrev.com/745e1b4545d51a2ad620d590682ca6eadd95f36c/third_party/WebKit/Source/core/html/forms/KeyboardClickableInputTypeView.h

So how well/not well does the PoC work in a ToT build with #76? Are we done, or is there more to do?

Re #77: I'm not sure what happened, but I don't think the fix in #76 was effective. In https://www.bayden.com/test/upload.asp, if you focus the upload button and hold down Enter, the dialog appears and auto-submits.

Similarly, the POC HTML file attached  Issue 782842  still works in 65.0.3298.2. 

Re #78: Ah. I'm not sure why the behavior at upload.asp is flaky, but the mitigation is completely bypassed in the POC for 782842 because the attack page simply calls .click() on the control after verifying that the user has the Enter key depressed (this scenario is alluded to in Comment 75.1).

So I don't think we're out of the woods here yet.

I have a WIP patch at https://chromium-review.googlesource.com/c/chromium/src/+/833399 for Mac+Win that disables the OK button until the user changes the selection. Haven't successfully compiled/tested the windows version yet but the mac version defeats the PoC in this bug and 782842. I *think* this might be what Safari is doing as well.

Does anyone have thoughts about this approach? It could be a little annoying from a usability perspective but at this point I think we should prioritize a reliable fix asap, even if it's a little annoying.

(also have not investigated whether the same approach is possible on Linux)

Re #80: The approach for Windows in the WIP looks like it /should/ work, but unfortunately it doesn't, because the Selection change message is getting sent without a user action after initialization. To workaround, we could perhaps require that 500ms or so has passed between BFFM_INITIALIZED and BFFM_SELCHANGED before enabling the button?

(Additionally, compile fails with 
../../ui/shell_dialogs/select_file_dialog_win.cc(532,43):  error: reinterpret_cast from 'int' to 'LPARAM' (aka 'long') is not allowed
    SendMessage(window, BFFM_ENABLEOK, 0, reinterpret_cast<LPARAM>(1)); )

Re: compile fail, yeah I don't have a windows box so I'm compiling on the bots...

Is there anything to distinguish the BFFM_SELCHANGED messages before user action occurs? What's the value of the parameter in those cases? I was hoping to avoid timing stuff because it doesn't seem like half a second is enough for a user to see and process the dialog and realize that they should stop pressing the key... and the longer we go the more annoying it gets.

Re #83: The lParam on the BFFM_SELCHANGED is a PIDL (basically, a reference to the currently-selected path).

Unfortunately, I don't think there's a good way to use this approach that doesn't involve a timer. We could do something whereby we check (via GetAsyncKeyState or whatever) whether the ENTER key is down while the dialog is initializing and refuse to enable the OK button until it's not? 

Alternatively, we could require an out-of-band confirmation from the user that they do indeed want to upload the files (e.g. show a Chrome UI explaining exactly what the web page is going to receive, with no default button).

Another approach would be to select a non-valid path (e.g. "My Computer") as the default, such that Windows doesn't enable the OK button. Unfortunately, it sounds like this may have broken in Windows 10. https://answers.microsoft.com/en-us/insider/forum/insider_wintp-insider_files-insiderplat_pc/problem-with-shbrowseforfolder/a263da47-36f4-433f-b86f-20ef95d60fb8

Re #83: "it doesn't seem like half a second is enough for a user to see and process the dialog and realize that they should stop pressing the key"

An important point here is that it doesn't really matter if the user stops holding the key; the OK button will be disabled because we disabled it and the user hasn't selected a folder manually. The timer is only needed to prevent non-user-initiated BFFM_SELCHANGED events that occur as Windows expands the tree in the the dialog. 

  struct SelectFolderDialogOptions {
    const wchar_t* default_path;
    bool is_upload;
+   ULONGLONG initialized_tickcount;
  };

Inside if (message == BFFM_INITIALIZED) { if (options->is_upload) { we then disable the OK button and set the initialization tickcount:

      options->initialized_tickcount = GetTickCount64();
      SendMessage(window, BFFM_ENABLEOK, 0, 0);

Then, we reject enabling of the OK button on BFFM_SELCHANGED until a tick count expires:

    SendMessage(window, BFFM_ENABLEOK, 0, 
                static_cast<LPARAM>(
                    (GetTickCount64() - options->initialized_tickcount > 1000) ? 1 : 0 ));


The downside of this approach is that if the user picks a folder in less than a second, they'll have to reselect it to enable the OK button, making it potentially appear flaky.

Oh, I see, that makes sense and sounds okay to me. (I was thinking of the proposal in #53/54.) Is there anyone with a Windows dev environment set up who can build and test elawrence's proposal in #86? I'm trying to get one set up but it will take me a few days at least. (I changed my patch in #80 to be Mac-only.)

Btw meacer noticed that it looks like Edge is doing something different and using some other kind of file upload dialog (?) which doesn't suffer from this problem. Might be https://msdn.microsoft.com/en-us/library/windows/desktop/bb776913(v=vs.85).aspx

RE #87: I've been testing this on Windows for a while. It's effective against the POC's in this bug and  Issue 782842 .

With regard to Edge, yes, I believe it's using the IFileDialog with the FOS_PICKFOLDERS option as suggested by the SHBrowseForFolder documentation. However, using the IFileDialog alone isn't enough (it's what Firefox uses and Firefox is vulnerable to this attack). 

elawrence, are you going to upload your patch for review? I'm hoping someone can submit a fix faster than it takes me to get a Windows build up and running.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/64ef541aad859cc5be38da44863fb1ea24c7c817

commit 64ef541aad859cc5be38da44863fb1ea24c7c817
Author: Emily Stark <estark@google.com>
Date: Tue Dec 19 21:29:51 2017

Mac: do not enable file upload until selection has changed

To avoid files being uploaded without the user realizing, require the
user to change the selection on the file dialog before the OK button
is enabled.

Bug:  637098 
Change-Id: Ic6faffb3f6075fb969ae930992d2553162346d02
Reviewed-on: https://chromium-review.googlesource.com/833399
Reviewed-by: ccameron <ccameron@chromium.org>
Commit-Queue: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525148}
[modify] https://crrev.com/64ef541aad859cc5be38da44863fb1ea24c7c817/ui/shell_dialogs/select_file_dialog_mac.mm

Update: Eric's got a Windows fix up for review, thanks Eric!!!

So that leaves us with linux. +erg and estade: could either of you help us with this high-priority security UX bug? What we'd like to do ideally is disable the OK button on the file upload dialog until the user has changed the file/directory selection. Do you know if that's possible on Linux?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8dad1bd71cda36807d8b53f7f88ceb5d4227cd00

commit 8dad1bd71cda36807d8b53f7f88ceb5d4227cd00
Author: Emily Stark <estark@chromium.org>
Date: Tue Dec 19 22:40:27 2017

Revert "Mac: do not enable file upload until selection has changed"

This reverts commit 64ef541aad859cc5be38da44863fb1ea24c7c817.

Reason for revert:
(1) The CL fixes the webkitdirectory bug but breaks the normal
<input type=file> upload dialog.
(2) Can fix tapted's nits posted after submit.

Original change's description:
> Mac: do not enable file upload until selection has changed
> 
> To avoid files being uploaded without the user realizing, require the
> user to change the selection on the file dialog before the OK button
> is enabled.
> 
> Bug:  637098 
> Change-Id: Ic6faffb3f6075fb969ae930992d2553162346d02
> Reviewed-on: https://chromium-review.googlesource.com/833399
> Reviewed-by: ccameron <ccameron@chromium.org>
> Commit-Queue: Emily Stark <estark@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#525148}

TBR=tapted@chromium.org,ccameron@chromium.org,estark@chromium.org

Change-Id: I885af00fff4f2a20b8a59e8c0dbc8e9f4b4a06fa
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  637098 
Reviewed-on: https://chromium-review.googlesource.com/835208
Reviewed-by: Emily Stark <estark@chromium.org>
Commit-Queue: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525166}
[modify] https://crrev.com/8dad1bd71cda36807d8b53f7f88ceb5d4227cd00/ui/shell_dialogs/select_file_dialog_mac.mm

Windows CL up at https://chromium-review.googlesource.com/c/chromium/src/+/835068.

I have a patch here[1] for gtk. There's also a KDE file chooser which I didn't change.

Since there's no way to start with no selection, it disables the "open" button until the selection changes. My only misgiving is that if the default selection (first file in the list) is the one the user wants to open, it's unintuitive to have to select something else then re-select the first file. It might even mean they have to navigate to a different directory, then go back again if there's no other file there.

[1] https://chromium-review.googlesource.com/#/c/chromium/src/+/838411

estade@:  I think it would be too frustrating (especially for the single file in a directory case) to wonder why the OK button isn't becoming enabled.

Could we instead enable the OK button only when the enter key is released?  Or not open the dialog at all until it's released?

I implemented that idea as per #92, but now that I look at the windows patch what it actually does is delay acceptance for one second.

RE #100: To be precise, the Windows patch has exactly the same limitation as the gtk patch in #97-- the user must select a different item in the picker in order to enable the OK button. (The timer is used to avoid the problem that Windows sends "the user changed the selection" notifications erroneously during construction of the UI).

While we all agree that the "must select a different file once" behavior is a shortcoming, the alternative near-term proposal is to rip out the API entirely, which would definitely be more disruptive.

In an ideal world, we'd probably have something like what Firefox does for other dialogs (like the Save File prompt) whereby the dialog appears with the Acceptance Button disabled. The UI visually counts down for some short period (e.g. 3 seconds) so the user understands that they cannot accept right away. The problem with getting to that ideal world is that it would almost certainly require doing away with all of our platform-provided file picker UIs and rewriting our own in WebUI or Cocoa/Views, which would be a major project.

WRT #99: The patch in #76 was circumvented because it only handled the case where the dialog was shown because the user ENTER on the control itself. In an attack scenario, the attacker instead triggers the dialog by doing control.click() such that the held ENTER key flows through to the subsequent dialog.

We /could/ have code inside FileInputType::HandleDOMActivateEvent that does the equivalent of: 
  
   while GetAsyncKeyState(VK_RETURN) {
     // pump events
   }

...to avoid allowing a held-down ENTER key from flowing through to the platform-specific file picker dialog. The attacker would need to change their social engineering script to something like "Hit ENTER ten times" or something like that, and pop the dialog between the second and third keystroke.

c#102: I think that's better than having to select a different file.  If we're concerned about the "press enter 10 times" case, then we could combine the solution in #102 with a 1s delay to enable the OK button.

I just want to avoid the scenario where users are confused why the OK button isn't working :)

>  with a 1s delay to enable the OK button.

To be clear, I don't think we have the ability to enable the OK button at a specific time on Windows (and possibly other platforms). 

We *can* prevent the user's selection changes from automatically enabling the OK button for a fixed period of time (that's what the patch in #96 does) but the button does not automatically become enabled when that timer expires.

c#104.  I think it's possible, at least on Linux with the GtkFileChooser.

We had the same issue with selection changing during ui construction on Linux, but we're able to get around that without resorting to a (hacky?) timer by instead ignoring all selection changes that come before the ui is first drawn.

If the question is whether we can switch over to just using a timer completely, I think we can --- but would that solve the problem? If you hold Enter for five seconds it will just delay the confirmation of the selection by one second.

I'm not really that opposed to forcing the user to change the selection. We could add a message within the file chooser (GTK gives us that ability) if it's really that confusing; we could add an extra button or checkbox like the "I am not a robot" captcha; etc. If this is urgent I'd just land as is and worry about UI improvements later.

Re #106: On Windows, we unfortunately get non-user-initiated selection change events both before and after the BFFM_INITIALIZED event.

> If the question is whether we can switch over to just using a 
> timer completely, I think we can --- but would that solve the 
> problem? If you hold Enter for five seconds it will just delay 
> the confirmation of the selection by one second.

I think the hope is that the user, upon seeing the unexpected dialog, would release the Enter key. The concern noted in #83 is "How fast can we reasonably expect the user to react to the unexpected?"

From a security stand point what is the trigger we are using to know the user has recognized the dialog and intentionally pressed enter? An enter after the key repeat delay?

Re #109: Perhaps ideally we would not allow Enter alone to submit the dialog, instead requiring that the user first manually select a folder, as this is a user-gesture that is significantly harder to fake. Unfortunately, the platform-specific dialogs do not seem to generally offer a good mechanism for requesting such behavior.

On windows I suspect you could inject a windows subclass of the dialog so that you can see mouse/key events. With that ability you could then reenable the button when some key/mouse combination occurs.

I think we need some more help from platform-specific UI experts to get this fixed.

tapted, would you be able to help with a Mac fix as you discussed in https://chromium-review.googlesource.com/c/chromium/src/+/833399#message-6e16fee88103110901b38fd125c8eb5bbaebf3ab?

elawrence/sky, what's the current state of the Windows fix? Do either of you have cycles to keep working on it?

Re #112: I believe the Windows fix at https://chromium-review.googlesource.com/c/chromium/src/+/835068 to be effective, but it results in the user-experience regression that the user is forced to pick a different folder in order to submit. I believe that's a fine tradeoff in a world where the alternative on the table is to remove the Web API entirely.

If the appropriate UX powers-that-be deem that unshippably confusing/bad/etc, we could take a narrower one liner fix that prevents the dialog from appearing until the user releases the ENTER key. That protects against fewer scenarios but is even less likely to be noticed by a user.

I'm pretty swamped. For reference, the Mac-specific comment from http://crrev.com/c/835068 was

> Patch Set 8:
> 
> > Patch Set 8:
> > 
> > (1 comment)
> > avi@ said:
> > You're already reverting, but here are my thoughts:
> > 
> > You want to enable/disable the OK button. The way you find that button is that you use -[NSView viewWithTag:] to search for the OK button, which is documented to be tagged with NSFileHandlingPanelOKButton. That's what you should be enabling/disabling.

> tapted@ said:
> I played with the POC a bit. I'm not sure disabling the button is right. If someone really wants to pass their desktop folder or some other default `favourite` it's unclear what they need to do to enable the button.
> 
> Mac doesn't key-repeat like other platforms. Also buttons activate on keydown not keyup. I'm not sure this is because of key repeat. I think instead this might be because we are redispatching the event from the webcontents after it didn't preventDefault on it.
> 
> That is, it ends up in command_dispatcher.mm's redispatchKeyEvent where we just do `[NSApp sendEvent:event];`. In most valid use-cases this goes to the mainMenu to activate some menu command like Cmd+L. But Enter gets redispatched too AFAIK.
> 
> Maybe we can check something in redispatchKeyEvent. E.g. if `[owner_ attachedSheet]` is non-nil we probably don't want to redispatch the event.


I haven't done any experiments to see whether this is feasible yet. (if it is, and nothing else breaks, it should be a simple fix)

So that theory was bung. But I found a much simpler fix: we can just refuse to accept the dialog if the current event is a key-repeat -> https://chromium-review.googlesource.com/875522

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/31f50deaacf54de542d01c557cf9f01cab4aa154

commit 31f50deaacf54de542d01c557cf9f01cab4aa154
Author: Trent Apted <tapted@chromium.org>
Date: Mon Jan 22 22:52:10 2018

Mac: Prevent a held-down Enter key from closing the Open File dialog

Bug:  637098 
Change-Id: I18ddab35db1ff3155b847576880de4df4e1290ba
Reviewed-on: https://chromium-review.googlesource.com/875522
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531035}
[modify] https://crrev.com/31f50deaacf54de542d01c557cf9f01cab4aa154/ui/shell_dialogs/select_file_dialog_mac.mm

The CL in #116 lands "Prevent a held-down Enter key from closing the Open File dialog" for Mac. I don't think we have a good way to do exactly that for Windows, but it should be pretty easy to do "Delay the Open File dialog from opening until the user releases the ENTER key" which should provide similar protection.

Would this be preferable to the current Windows CL in #113?

Proposal in #117 sounds good to me. I prefer landing an imperfect fix that helps somewhat rather than taking more time to try to come up with something perfect.

Reminder that this is our oldest High severity security vulnerability (536 days), and is from an external reporter who is probably hoping to get a VRP payout. keishi, can you please make this bug a high priority, or reassign it to someone who can?

And, if that's not possible, I'll reiterate again that the feature is prefixed as well as being unsafe. Unshipping it absolutely should be an option. We can't have High severity bugs in nominally-experimental features for ~ 18 months. +awhalley FYI

Ahh, I had forgotten about #50. That precludes the possibility of a VRP payout, but does mean we should make this bug public too.

+jschuh: Can we get a Windows person to write a fix for Windows that will please the code review gods?

Bruce, could you have someone on the Windows side cover this? The right thing seems to be preventing a default directory selection at the interface to blink. However, it also seems that the other platforms have done some whack-a-mole at OS layer, so maybe we just do that for Windows (still assuming the right solution is to prevent a default selection rather than try to race the keypress).

I talked with @palmer about severity. This feels much more medium to me. High severity is reserved for zero-interaction bypass of critical security boundary (such as silent UXSS, RCE, or sandbox escape). I appreciate that it was under-scored originally as low, but given that the user interaction is significant enough to prevent e.g. mass exploitation via a malicious ad network, I feel that medium is the right call.

Acknowledged. I'll make sure this gets dealt with on Windows.

Assigning to robliao@ for assessment, although implementation will probably fall to pbos@

Actually assign for assessment...

Assigning to me per offline discussion.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2a02a69e3173a85fc6ae4f39c7aec28b2ea0da54

commit 2a02a69e3173a85fc6ae4f39c7aec28b2ea0da54
Author: Peter Boström <pbos@chromium.org>
Date: Thu Feb 22 22:25:46 2018

Add modal confirmation dialog to folder upload

Before this change <input webkitdirectory/> contains a severe security
hole as tricking the user to hold ENTER is sufficient to share a large
swath of their filesystem (for Windows this is usually Desktop).

The exploit works as follows:

1. A site tricks the user into holding ENTER (this is trivial).
2. While detecting that the user is holding ENTER, trigger a hidden
   <input webkitdirectory/> field (input.click()).
3. The folder picker is shown but accepted instantly, which gives the
   site read access to whatever happens to be selected. On my Windows
   machine I've observed this to be the Desktop, but I've seen other
   selections as well (maybe the user folder).

To prevent sharing files without the user's consent this change
introduces a modal interstitial that:

1. Tells the user what's about to be shared (N files, from directory X).
2. Advises the user only to do this for sites they trust.
3. Defaults to Cancel so that the example gesture laundering does not
   work.

This method has a couple of benefits. It provides additional information
to the user which might accidentally have selected the wrong folder and
is about to upload more files than intended. It doesn't rely on
OS-specific behavior, so it provides level protection everywhere. It
also has a lower maintenance cost as it doesn't need to be added for new
platforms or updates to existing OS APIs.

There's opportunity for better laundering protection (such as preventing
the dialog from being accepted too quickly), but the attack surface is
significantly smaller with this mitigation implemented.

According to usage counters <input webkitdirectory/> is used for <0.001%
of page loads (below the consideration-for-deprecation threshold), which
makes the interruption fairly low while providing significant additional
protection against a malicious attacker.

Bug:  chromium:637098 
Change-Id: I8ac43f8a61cd4476f581b9e57b07cdf88e28f85c
Reviewed-on: https://chromium-review.googlesource.com/929809
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538590}
[modify] https://crrev.com/2a02a69e3173a85fc6ae4f39c7aec28b2ea0da54/chrome/app/generated_resources.grd
[modify] https://crrev.com/2a02a69e3173a85fc6ae4f39c7aec28b2ea0da54/chrome/browser/file_select_helper.cc
[modify] https://crrev.com/2a02a69e3173a85fc6ae4f39c7aec28b2ea0da54/chrome/browser/file_select_helper.h
[modify] https://crrev.com/2a02a69e3173a85fc6ae4f39c7aec28b2ea0da54/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/2a02a69e3173a85fc6ae4f39c7aec28b2ea0da54/chrome/browser/ui/browser_dialogs.cc
[modify] https://crrev.com/2a02a69e3173a85fc6ae4f39c7aec28b2ea0da54/chrome/browser/ui/browser_dialogs.h
[add] https://crrev.com/2a02a69e3173a85fc6ae4f39c7aec28b2ea0da54/chrome/browser/ui/views/folder_upload_confirmation_view.cc
[add] https://crrev.com/2a02a69e3173a85fc6ae4f39c7aec28b2ea0da54/chrome/browser/ui/views/folder_upload_confirmation_view.h
[add] https://crrev.com/2a02a69e3173a85fc6ae4f39c7aec28b2ea0da54/chrome/browser/ui/views/folder_upload_confirmation_view_browsertest.cc
[modify] https://crrev.com/2a02a69e3173a85fc6ae4f39c7aec28b2ea0da54/chrome/test/BUILD.gn

#130 adds a confirmation dialog to all browsers building with toolkit_views enabled (this includes Mac). The dialog asks the user to confirm that they wanted to upload N files and which folder was selected. The dialog defaults to Cancel so tricking the user into accepting the dialog without noticing is significantly harder.

There's a few potential additional measures we could do, but given that the API usage is fairly low I'd opt for moving on.

Deleted: folder-confirmation-dialog-update.png 20.7 KB

	folder-confirmation-dialog-update.png 20.7 KB View Download

Since this is quite a nasty vulnerability, would it be possible to have the fix merged? (Just a suggestion, take it or leave it.)

pbos@ - we're right up to the last merges before the final 65 Beta - what's your take on the regression risk of these changes? Cheers!

I would like this change to spin more in Canary/Dev before making that call. The API usage is very low (<0.001% of page loads) so any crash rates or similar will take significantly longer to manifest. I'll do the merge if you want to make that call, but I think we should consider the change virtually untested in Canary until it's spun for a lot longer because of this.

If we have evidence of ongoing abuse that shifts my position here of course (or if you feel strongly about merging this change). I'm kind of shrugging my shoulders here.

Thanks pbos@. Let's reconsider for a 65 merge after it's made it out to Beta, even if that means only picking it up if there's a stable refresh.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/997810155defb24013257791f46dd1c50310cfc2

commit 997810155defb24013257791f46dd1c50310cfc2
Author: Peter Boström <pbos@chromium.org>
Date: Tue Feb 27 00:41:08 2018

Make OK the default folder-upload-dialog button

Removes the default-button override while maintaining that the focus
should initially be on the cancel button. This makes the "Upload" / OK
button the blue button, which is the expected style, even while not
initially focused to prevent abuse.

Bug:  chromium:637098 
Change-Id: I63cbd4f69f6d1033a2437691296087f466afbd3f
Reviewed-on: https://chromium-review.googlesource.com/938000
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539351}
[modify] https://crrev.com/997810155defb24013257791f46dd1c50310cfc2/chrome/browser/ui/views/folder_upload_confirmation_view.cc
[modify] https://crrev.com/997810155defb24013257791f46dd1c50310cfc2/chrome/browser/ui/views/folder_upload_confirmation_view.h

Update in #136 changes the button styling, attaching screenshot for posterity.

Deleted: folder_upload_update.png 6.9 KB

	folder_upload_update.png 6.9 KB View Download

Tested the issue on mac 10.13.3, win-10 and ubuntu 14.04 using chrome version #66.0.3356.0 using the html file from comment #2.

Attached screen casts for reference.

Observations on mac:
====================
On opening the PoC-Read-all-Filesv2.html file in mac 10.13.3 and pressing the enter button for 5 seconds, opened a pop up to upload the files from a directory with "upload" and "cancel" button.

Observations on Win-10:
=======================
On opening the PoC-Read-all-Filesv2.html file in win-10 and pressing the enter button for 5 seconds, intermittently opened a pop up to upload the files from a directory with "upload" and "cancel" button as per the screenshot at comment #137.

Observations on Ubuntu 14.04:
=============================
Fix is working as expected. On opening the PoC-Read-all-Filesv2.html file in ubuntu 14.04 and pressing the enter button for 5 seconds, opened a pop up to upload the files from a directory with "upload" and "cancel" button.

pbos@ - Could you please check the attached screen casts and please let us confirm the fix on OS-Win and OS-Mac.

Thanks...!!

Deleted: 637098@mac.mp4 511 KB

	637098@mac.mp4 511 KB View Download

Deleted: 637098@win10.mp4 616 KB

	637098@win10.mp4 616 KB View Download

That you're not getting the interstitial on Mac is because tapted@ has already added some mitigation for when holding Enter in the MacOS folder picker. This can be defeated by making the user press Enter repeatedly instead of holding it.

Looks like WAI on Windows, the text of the page gets changed when it brings up the (that gets instantly accepted). It takes time before you get to confirm it is because it's counting your 45k files, which is unfortunate. There's an alert in there (alert('I can read ' + this.files.length + ' files from anywhere on your pc!');) that would've been triggered if you inadvertently shared files with the site.

Attaching a more visible version that you can use for exploit testing. Click on the <input> field and only press Enter. If nothing happens by just pressing Enter repeatedly (because you cancel the confirmation dialog), then the mitigation is working. If you hit the alert in there then the dialog got accepted by just pressing Enter which isn't expected.

tapted@ also brought to my attention that Mac will accept the default button when pressing Enter regardless of where focus lies, so this has to be revisited for Mac. One suggestion is to not provide a default button (no blue button). I'll have to check with UX, but at least MacOS have some level of mitigation in place.

Deleted: PoC-Read-all-Files.html 450 bytes

PoC-Read-all-Files.html 450 bytes View Download

To append to #139 I spoke with tapted@, I think we're fine with the current level of mitigation for Mac. This is no longer a silent hold-enter trigger on any platform, any exploit is therefore significantly harder. Given that neither Windows or Mac end up passing files back on to the site by holding Enter, fix lgtm.

I find it a bit odd that the prompt asking you whether or not you want to allow a website to exploit a security vulnerability to read your files has "OK" as the default button, yet the prompt asking you whether or not you really want an extension from the Chrome Web Store has "Cancel" as the default button.

*** Boilerplate reminders! ***
Please do NOT publicly disclose details until a fix has been released to all our users. Early public disclosure may cancel the provisional reward. Also, please be considerate about disclosure when the bug affects a core library that may be used by other products. Please do NOT share this information with third parties who are not directly involved in fixing the bug. Doing so may cancel the provisional reward. Please be honest if you have already disclosed anything publicly or to third parties. Lastly, we understand that some of you are not interested in money. We offer the option to donate your reward to an eligible charity. If you prefer this option, let us know and we will also match your donation - subject to our discretion. Any rewards that are unclaimed after 12 months will be donated to a charity of our choosing.
*********************************

Hello greencardesh@ - the Chrome VRP panel took a look at this, and given circumstances decided to award $2,000 - cheers!

This bug requires manual review: There is .grd file changes and we are only 31 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This should already be in M66 so I don't know what sheriffbot's deal is here.

removed merge-request label