+benjhayden who implemented chrome.downloads API

There are two main issues here:

1. apks not included in blocked file type list
2. Extensions with debugger permission being able to launch downloaded files without user gesture

Can we make the debugger permission mutually exclusive with downloads permission? I can't imagine an extension with a legitimate use case using these two.

Though, I think a more effective measure is to require a native confirmation dialog for downloads.open. I did the initial security review for this API and was okay with a user gesture, but it looks like it's not enough. Perhaps requiring a native confirmation dialog for the first use is better ("<extension> wants to run <binary>. Allow? ([x] Always allow)")

#2: Is there a use case for an extension being able to debug itself?

In the context of <a download>, we've attempted in the past to distinguish between script initiated events and "trusted" events. Would that have mitigated this attack? Or does the Input.dispatchMouseEvent generate "trusted" events?

That said, I have no objections to requiring an additional native confirmation for download.open.

Oh, by the way: I'd like to stress that xdg-open can, in principle, detect filetypes by MIME type, and this works e.g. for Windows executables. So when the user has wine installed, "xdg-open ./some_windows_executable" will launch the executable using the wine compatibility layer. (Chrome doesn't block *.exe on Linux anyway, but if it did, this would be a bypass for that.)

Related to issue 596346.

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

commit 49d4920a95c5f0796a4bfc807c66311b9134fee6
Author: palmer <palmer@chromium.org>
Date: Sat Nov 19 01:55:28 2016

Treat APKs as dangerous on Linux as well as on Android.

BUG= 666824 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2508803010
Cr-Commit-Position: refs/heads/master@{#433376}

[modify] https://crrev.com/49d4920a95c5f0796a4bfc807c66311b9134fee6/chrome/browser/resources/safe_browsing/download_file_types.asciipb

jannh: Re #4, would you advocate that we apply a similar fix for .exe (and perhaps the other Windows executable extensions) on Linux?

In principle, any platform could have an emulator or other runtime support for the executables of any other platform. Perhaps we should change download_file_types.asciipb so that the executable file name extensions of *any* platform are considered dangerous on *all* platforms?

Or, maybe downloads.open shouldn't exist? (Maybe so few people use extensions that use it that we can get rid of it?) meacer and rdevlin.cronin, thoughts on that?

Adding more OS flags to reflect the broader understanding.

I'm also wondering if there are any other extension → native privilege escalations due specifically to the debugger permission. If there are, were they fixed, or have we decided to accept the risk (and hopefully marked debugger as extra dangerous in the UI, or apply greater scrutiny to them in the Chrome Web Store, or something)?

Or perhaps the debugger API should not be able to generate events that count as user gestures? Is it strictly necessary that it be able to do so?

> jannh: Re #4, would you advocate that we apply a similar fix for .exe (and
> perhaps the other Windows executable extensions) on Linux?

> In principle, any platform could have an emulator or other runtime support
> for the executables of any other platform. Perhaps we should change
> download_file_types.asciipb so that the executable file name extensions of
> *any* platform are considered dangerous on *all* platforms?

Hm. I guess it makes sense. I'm not aware of a lot of executable formats for which emulation works like this, but I think that there is no good reason to not warn a user about binaries from another platform.


> Or, maybe downloads.open shouldn't exist? (Maybe so few people use
> extensions that use it that we can get rid of it?) meacer and rdevlin.cronin,
> thoughts on that?

(Well, I'm always in favor of killing APIs like this.)


> I'm also wondering if there are any other extension → native privilege
> escalations due specifically to the debugger permission.

The following aren't exactly ->native escalations, but still noteworthy IMO:

I haven't tested this yet, but in the browser protocol part of the debugger API, there are things related to shadow DOM access, like a flag `includeUserAgentShadowDOM`, so it seems like the debugger API can leak shadow DOM nodes. See  issue #666246  for why this might be bad.

The debugger permission also permits injecting code into other extensions' content scripts, which may or may not be a big deal, depending on how much the corresponding blessed extension context trusts the content script. IIRC I tested that this works.

I also haven't tested this yet, and my understanding is that there is ongoing work to reduce the impact of issues like this, but I think that it might be possible to set breakpoints in scripts like extensions::binding and leak references to `require` and `requireNative` that way.

Looks like there's more to be done here, but is it worth merging #6 into M56 for now?

Passing this to nparker, owner of the larger questions.

palmer -- Are you referring to the question of warning for all file types on all platforms?  My thoughts:

1) For those types for which the Safe Browsing backend returns a verdict other than "unknown", we still may download without any warning. e.g. "safe" .exe's from a site you've visited before won't show a warning.  Would chrome.downloads.open be able to launch those? If so I'd say we should get rid of it.


2) Chrome's warning lose some credibility if they say thing that don't make sense to the user.  For instance, saying "This .sh may harm your computer" on a Chromebook.  We do still warn on BAD files (like malware .exes or .apks) on all platforms, but the generic warning is tailored to the platform.


awhalley -- I will push #6 right now via the component updater. We could merge too -- that'll reach new users who haven't had an update yet.

#6 is pushed, should reach ~most users in like 6 hrs.

+palmer for #14

Your change meets the bar and is auto-approved for M56 (branch: 2924)

Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

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

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

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

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

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

Re #14: I don't know the answer about chrome.downloads.open. Asanka is more likely to know.

Is it too late to merge #6 into M56? I was out on vacation from the 19th until today, so I wasn't around to do it.

Marking this as Assigned since there are still open questions. (Settle down, sheriffbot!)

palmer@ - Good to merge!

--> asanka for the question in #14/#16

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

commit 243ea2f9d9665b141f54e425db8eae6f0db1e959
Author: Chris Palmer <palmer@chromium.org>
Date: Tue Jan 03 18:58:51 2017

Treat APKs as dangerous on Linux as well as on Android.

BUG= 666824 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2508803010
Cr-Commit-Position: refs/heads/master@{#433376}
(cherry picked from commit 49d4920a95c5f0796a4bfc807c66311b9134fee6)

Review-Url: https://codereview.chromium.org/2608253004 .
Cr-Commit-Position: refs/branch-heads/2924@{#651}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/243ea2f9d9665b141f54e425db8eae6f0db1e959/chrome/browser/resources/safe_browsing/download_file_types.asciipb

Worth closing this and opening a new bug to track the remaining work?

sgtm

The 90-day deadline expires 2017-02-16 (in 9 days). While the missing blacklisting of APK files has been resolved, as far as I know, the gesture requirement bypass using the debugger API has not been addressed. Are you planning to fix that before the deadline?

awhalley -- Are there other bugs opened to track remaining work?

nparker@ - not that I know of, who's best placed to write those?

Skimming this bug, it looks like the remaining work is a native dialog confirmation for the download.open API? meacer or asanka, do you have more context and know if that is accurate? If so, asanka, do you maintain the chrome.downloads API?

Yes, that sounds accurate. There is a user gesture requirement for downloads.open, but clearly it's not enough. We should consider adding a native dialog with an "always allow" option (or something similar).

benjhayden@ was the original owner of the API, but I don't know who owns it at the moment. Extensions team perhaps?

(releasing bug, since I'm no longer on downloads).

+dtrainor for triage.

In addition to chrome.download.open(), the 'open' action in chrome://downloads will require the same treatment.

Assigning to dtrainor to make sure he sees this

+dahlke for FYI on getting the native dialog confirmation for download.open and the chrome://downloads open action.

qinmin@ if you don't have cycles bounce back to me thanks!

+Rachelis as FYI. Requiring a native dialogue SGTM given the potential threat. I assume there an existing UI pattern for this type of blocking behavior. Could someone point me to an example? 

I think the dangerous download prompt is probably a similar type of prompt to what we want to show.  Same kind of intercept check and native dialog in the code.  asanka@ or benjhayden@ let me know if I'm wrong.

qinmin@ I believe for download.open we need to adjust downloads_api DownloadsOpenFunction() to show a native dialog.  For chrome://downloads open it looks like we need to adjust MdDownloadsDOMHandler::HandleOpenFile() to show a dialog as well for these types?

meacer@ what are your thoughts on what file criteria we should show this under?

Can you add a screenshot please David?

nparker has a list of dangerous file types at chrome/common/safe_browsing/file_type_policies.h, can we use that?

nparker: Is that an appropriate use of the list?

Yup, I'd think we'd want to show a dialog in cases where we'd show a warning or prompt if downloaded normally (assuming I understand the issue correctly).

@dahlke, rachelis@, this is how the warning dialog looks like on Chrome today.

We could have a similar dialog with text: "This file may harm your computer." The dialog has ok/cancel button. And also an extra checkbox to "always allow" opening the same file type?

Deleted: Screenshot .png 226 KB

	Screenshot .png 226 KB View Download

The overall UI pattern looks good. For the text, we should use something similar to what we use on Android. I changed the buttons and verb to make more sense in this context (see attached for screenshot).

"This type of file can harm your device. Do you want to open [filename] anyway?"

Buttons: Cancel, Open

Do we really need to have an "always allow" UI. I am not sure that is warranted in this case.

Deleted: Screenshot_20170508-154438.png 388 KB

	Screenshot_20170508-154438.png 388 KB View Download

Ping from security sheriff. Last activity here is 6 months old.

Do you still consider adding the dialog? Should we try to find another owner, if you don't have cycles for that?

Security sheriff ping, and escalating to RBS for M65. Nothing has happened here for a long time, and we really should get the confirmation dialog done asap. Can we at least confirm and commit to a plan of action in the M65/M66 timeframe, thanks.

re #53: I think we are still waiting UI confirmation on the native dialog. #46 is the android infobar, but android doesn't support extensions and chrome://downloads at all. We need UI for desktop platforms.

And here is my UI question:
regarding open action on chrome://downloads, user should have already allowed the dangerous download on a native dialog when downloading the file, do we really need another native dialog when user opening it?

rachelis@, not sure the desktop UI design  is on your plate, but would you please help us triage this?

Bug 595841 also discusses a browser mediated UI for downloads. Should we block this bug on that one?

i think we should

The original Project Zero bug is now public: https://bugs.chromium.org/p/project-zero/issues/detail?id=1000

Does anyone object to opening up this bug as well?

Opening up.

+aboss@ for fyi.

Friendly ping to get an update on this issue as it is marked as stable blocker for M65.

Thanks..!

M65 Stable promotion is coming VERY soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge  into the release branch ASAP. Thank you.

Reassigning to qinmin@.  Min can you work with Rachel directly?  +aboss@ to help facilitate.  Thanks!

M65 already branched, and M66 branch is coming soon
punting to M67 for UI designers to have time to work on this

Is there anything left to do here besides the follow-up work? Could we open a new bug as mentioned in c#27 for that to avoid cluttering this issue?

add some security folks to the bug.

Security team, any idea whether the extra dialog is really needed? 

Currently when downloading a dangerous file, we already show a popup to ask the user to acknowledge the download. And if opening the download requires another popup, feels like too much user interaction involved.

in the bug desciption, .apk is used to trigger the issue. But .apk is already marked as dangerous on desktop, so not sure if there are any other cases that can trigger the problem.

Is this different than Issue 595841?

I believe the idea here is that we want this as a defense-in-depth to deal with scenarios whereby the Download in question circumvented other protections, as we've seen in a number of vulnerability chains. 

Relying upon the "Dangerous" types list alone seems risky; for instance, on Windows there are file types not on the "Dangerous" list that are run with the equivalent of RCE. 

To clarify, is the ask then to have a dialog (or some sort of similar safeguard) that prompts the user every time they open a file from chrome://downloads? If so, I agree with Min that this seems particularly noisy for the user. Or would this apply only for .apk files opened from chrome://downloads? 

Any updates on this?

M67 Stable promotion is coming soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. Thank you.

*** Bulk Edit ***
M67 Stable promotion is coming soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. 

If fix is already merged to M67 and nothing else is pending, pls mark the bug as fixed. Thank you.

*** Bulk Edit ***
M67 Stable promotion is coming VERY soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. 

If fix is already merged to M67 and nothing else is pending, pls mark the bug as fixed. Thank you.

*** Bulk Edit ***
M67 Stable promotion is coming VERY soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. 

If fix is already merged to M67 and nothing else is pending, pls mark the bug as fixed. Thank you.

Sadly punting to 68 :-(

Of course.

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

commit 7b7cbbbf249aaa55b915ed3b316aceeb5c0c0bdb
Author: Min Qin <qinmin@chromium.org>
Date: Mon Jun 04 21:49:31 2018

Prompt user before openning a download from extension

Downloads api allows an extension to open a download with a user gesture.
However, the current user gesture check is passed from the renderer,
and it can be easily compromised.
This CL adds a prompt dialog to notify user before opening a download.

BUG= 666824 

Change-Id: I6ec9709fa183f9784db323603f356dc6aa9ea6e1
Reviewed-on: https://chromium-review.googlesource.com/1069320
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564250}
[modify] https://crrev.com/7b7cbbbf249aaa55b915ed3b316aceeb5c0c0bdb/chrome/app/generated_resources.grd
[modify] https://crrev.com/7b7cbbbf249aaa55b915ed3b316aceeb5c0c0bdb/chrome/browser/BUILD.gn
[add] https://crrev.com/7b7cbbbf249aaa55b915ed3b316aceeb5c0c0bdb/chrome/browser/download/download_open_prompt.cc
[add] https://crrev.com/7b7cbbbf249aaa55b915ed3b316aceeb5c0c0bdb/chrome/browser/download/download_open_prompt.h
[modify] https://crrev.com/7b7cbbbf249aaa55b915ed3b316aceeb5c0c0bdb/chrome/browser/extensions/api/downloads/downloads_api.cc
[modify] https://crrev.com/7b7cbbbf249aaa55b915ed3b316aceeb5c0c0bdb/chrome/browser/extensions/api/downloads/downloads_api.h
[modify] https://crrev.com/7b7cbbbf249aaa55b915ed3b316aceeb5c0c0bdb/chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc
[modify] https://crrev.com/7b7cbbbf249aaa55b915ed3b316aceeb5c0c0bdb/chrome/browser/ui/browser_dialogs.h
[modify] https://crrev.com/7b7cbbbf249aaa55b915ed3b316aceeb5c0c0bdb/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/7b7cbbbf249aaa55b915ed3b316aceeb5c0c0bdb/content/public/browser/web_contents.h

since this just landed today, let's verify this in canary first and then we can merge to m68.

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

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

Is this bug rightly marked as impacting Desktop? Seems like this is impacting mostly android?

android doesn't support extensions, this is desktop only

Approving merge for M68. branch:3440

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

commit 95f31b9f36c02b47c6872520041fedb491ce8693
Author: Min Qin <qinmin@chromium.org>
Date: Tue Jun 12 17:13:49 2018

Prompt user before openning a download from extension

Downloads api allows an extension to open a download with a user gesture.
However, the current user gesture check is passed from the renderer,
and it can be easily compromised.
This CL adds a prompt dialog to notify user before opening a download.

BUG= 666824 

Change-Id: I6ec9709fa183f9784db323603f356dc6aa9ea6e1
Reviewed-on: https://chromium-review.googlesource.com/1069320
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#564250}(cherry picked from commit 7b7cbbbf249aaa55b915ed3b316aceeb5c0c0bdb)
Reviewed-on: https://chromium-review.googlesource.com/1097535
Reviewed-by: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#302}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/95f31b9f36c02b47c6872520041fedb491ce8693/chrome/app/generated_resources.grd
[modify] https://crrev.com/95f31b9f36c02b47c6872520041fedb491ce8693/chrome/browser/BUILD.gn
[add] https://crrev.com/95f31b9f36c02b47c6872520041fedb491ce8693/chrome/browser/download/download_open_prompt.cc
[add] https://crrev.com/95f31b9f36c02b47c6872520041fedb491ce8693/chrome/browser/download/download_open_prompt.h
[modify] https://crrev.com/95f31b9f36c02b47c6872520041fedb491ce8693/chrome/browser/extensions/api/downloads/downloads_api.cc
[modify] https://crrev.com/95f31b9f36c02b47c6872520041fedb491ce8693/chrome/browser/extensions/api/downloads/downloads_api.h
[modify] https://crrev.com/95f31b9f36c02b47c6872520041fedb491ce8693/chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc
[modify] https://crrev.com/95f31b9f36c02b47c6872520041fedb491ce8693/chrome/browser/ui/browser_dialogs.h
[modify] https://crrev.com/95f31b9f36c02b47c6872520041fedb491ce8693/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/95f31b9f36c02b47c6872520041fedb491ce8693/content/public/browser/web_contents.h