The API we expose via PPAPI doesn't give us the luxury of knowing when the plugin is done writing to the file until the plugin is unloaded. Until then the plugin is allowed to open/write/close the file to its little cold heart's content. So we don't have a good point during this workflow where we can assert all of the following:

 1. The plugin is no longer going to modify the file.
 2. The user definitely hasn't touched the file and hasn't had an opportunity to open the file.
 3. Another application definitely hasn't touched the file and hasn't had an opportunity to open the file.

If we did, then that's the point where we could submit the file to AV. Submitting it without the final content isn't useful. 2 and 3 are especially tricky since the file is created using its final filename in a user visible directory.

See also go/download-code-path-audit

TL;DR: This isn't a simple fix. I don't think it's feasible within a single milestone. It's definitely fixable even without breaking existing plugins, but requires some extra juggling.

To be clear, we don't care when the plugin is done modifying the file, and we don't need to submit the file to AV ourselves. By tagging a file with a Mark-of-the-Web, we signal to *other* clients the file's web origins, and let those other clients do what they will. (Windows ShellExecute will AV, Signature check, and SmartScreen check the file; Office applications will sandbox the file in the Protected View reader and disable macros, etc).

The MOTW tag is stored in the Zone.Identifier NTFS Alternate Data Stream on the file, and I'd assume the proper fix here is to write the MOTW at the point that the file is created/opened by the broker code. It doesn't matter what subsequent modifications are made to the file by the plugin, so long as those modifications don't touch the Alternate Stream. (I'm not familiar with the PPAPI here, but I doubt it provides access to Alternate Streams, and if it did, there are other interesting attacks possible.)

If adding the MOTW is considered sufficient at this point, should we bypass AttachmentServices entirely? The latter has caused us (and continues to cause)  grief (go/case-of-the-missing-downloads). I wanted to bring up the issue again and have us reconsider its use.

The current method for dealing with MOTW is to submit the download to AttachmentServices and have it scan the file and apply the MOTW as it deems fit to do so. For this, the scan needed to happen at the end of the download when all the data was available. If just applying the MOTW manually without going through AttachmentSerivces is sufficient, then we can apply the MOTW when the file is created and leave it be. PPAPI doesn't have access to alternate streams.

SafeBrowsing wanted the entire contents of the download for malware analysis which is partly where my assessment came from. SB can (and will shortly) make a somewhat less accurate determination based on the document URL of the Flash app and the requested file type. This, once again, wouldn't require the full contents.

I'm not sure who gets to make the determination of whether writing a MOTW is sufficient?

I do know that we have code that tries to write MOTW directly (see SetInternetZoneIdentifierDirectly) if we weren't able to have the IAttachmentExecute API do it. It seems reasonable to me to have the Flash broker do this for any file it writes.

#5: Sure. I was trying to follow up on your #3 where you claim that we don't need to submit the file to AV ourselves, which is what the AttachmentServices invocation is doing. Why is setting the MOTW sufficient here and not for regular downloads?

> Why is setting the MOTW sufficient here

Perhaps I shouldn't have said "we don't need to submit the file" and instead should have said "to block many attacks, we don't need to submit the file"

I do not know if there exists an antivirus package which does not perform a virus scan on file writes and only scans when explicitly called inside IAttachmentExecute::Save(). Maybe one exists.

Scanning the file at write-time is a nice thing to have for several reasons:
1. it's an opportune time to warn the user that the file is malicious
2. the download context URL is available to send back to SmartScreen or the AV-engine's web service,
3. it reduces the possibility of writing latent malware on the local disk which could have its MOTW stripped out accidentally as would happen when copied to a FAT32-formatted SD card, for instance). 
4. it mitigates scenarios like DLL hijacking, where the MOTW/signature is not checked except on the entry module

However, at least in the case of this attack, simply writing the MOTW alone serves as a strong mitigation because the Windows Shell will tag the ZIP file as Internet-originating and perform security checks before *execution*. 

"Check-on-execute" behavior will also block malware that would go undetected in the "check-on-download" codepath-- for instance, consider the scenario where the file written is an encrypted ZIP file and the password is known to the user but not the scanner.

Upgrading to Medium after elawrence@ asking about this offline.

There appears to some confusion in this bug, hopefully will try and clear it up.

The correct behavior documented by MS for a user agent is to use IAttachmentExecute::Save API to ask the OS to write the file to disk. The OS, in conjunction with any installed AV and/or SmartScreen, is then responsible for passing the file through it's scanners and writing the file to disk, along with the correct ADS to label the file appropriately.

If the AttachmentServices API is not available then the user agent *can opt* apply it's own MOTW and that's what we do in SetInternetZoneIdentifierDirectly if the COM call fails.

elawrence@ it's not up to the user agent to write the file and manually set the ADS, unless the IAttachmentExecute::Save API is unavailable.

This is what the normal download flow does. I agree this is not what the Flash download flow does, but I don't agree that we should be needlessly applying the ADS ourselves - we should be trying to find a way to wire up the Flash download flow to the same download flow via IAttachmentExecute I describe above.

I have been informed that "IAttachmentExecute:Save is broken in numerous ways" so it sounds like we should not be using that API and instead setting the ADS ourselves, all the time.  I can raise a new bug to address this issue, and then we can just change Flash to use the same method, writing the ADS at file creation.

I'm in favor of applying MOTW ourselves as mentioned in #4.

asanka: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

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

asanka: Uh oh! This issue still open and hasn't been updated in the last 29 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

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

@asanka: we're triaging the status of security-related bugs this week. Wondering if there is work in progress on this, or if it's blocked on wider refactoring work in 601187?

I think for now we'll apply the MOTW manually for these files. AES can't do much for these since we need to make the file consumer ready right from the get-go, at which point there's no content to scan.

Also note that due to the recent changes due to  issue 533579 , these downloads can now be handled by SafeBrowsing.

What is the ETA for this fix. We are in Security FixIt and your fast response is really appreciated.

It's been a month. Friendly ping. :)

In progress. :-/

Adjusting visibility for code reviewers.

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

commit bd57338ff5d58fff3147982c3f631cbe43e86f9c
Author: asanka <asanka@chromium.org>
Date: Wed Sep 21 15:03:44 2016

[Downloads] Consolidate MOTW annotation APIs into a single API.

Desktop platforms support various mechanisms for safely handling
untrusted files downloaded from the internet. These are summarized
below:

  * Windows:

    * Windows Attachment Services can submit newly downloaded content to
      registered AV programs. In addition, it will also annotate the
      file with the security zone of the source URL if necessary. Logic
      for invoking Attachment Services was in
      content/browser/safe_util_win.{h,cc} even though only
      content/browser/download used it.

    * Zone information could be added manually bypassing Attachment
      Services. This is useful if Attachment Services isn't available
      (doesn't really happen in any supported platform currently), or if
      the download content isn't available yet. This logic was also in
      content/browser/safe_util_win.cc.

  * Mac

    * Files created by Chrome/Chromium will automatically be quarantined
      due to the LSFileQuarantineEnabled entry. In addition, the
      quarantine type (whether the file was downloaded from the web or
      not), referrer (kLSQuarantineOriginURLKey), and source URL
      (kLSQuarantineDataURLKey) can be specified so that they are
      displayed in any UI presented to the user. Chrome also sets the
      "where from" metadata for the file based on the source and
      referrer URLs. This logic lived in
      content/browser/download/file_metadata_mac.{h,mm}.

  * Linux

    * While not mandatory to be used for any quarantine purpose, Chrome
      sets the `user.xdg.origin.url` and the non-standard
      `user.xdg.referrer.url` extended attributes. Logic for this lived
      in content/browser/download/file_metadata_linux.{h,cc}.

This CL introduces a common API in content/browser/download/quarantine.h
that invokes the correct platform specific implementation in
quarantine_*. The QuarantineFile() function is henceforth a platform
independent mechanism for annotating a downloaded file. This new API
will make it easier to annotate files that are downloaded using other
mechanisms (PPAPI, for example).

BUG= 598812 

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

[modify] https://crrev.com/bd57338ff5d58fff3147982c3f631cbe43e86f9c/content/browser/BUILD.gn
[modify] https://crrev.com/bd57338ff5d58fff3147982c3f631cbe43e86f9c/content/browser/download/base_file.cc
[delete] https://crrev.com/ced9605aea022ef7b68ef9d2a241ccb269ff6c38/content/browser/download/base_file_linux.cc
[delete] https://crrev.com/ced9605aea022ef7b68ef9d2a241ccb269ff6c38/content/browser/download/base_file_mac.cc
[modify] https://crrev.com/bd57338ff5d58fff3147982c3f631cbe43e86f9c/content/browser/download/base_file_win.cc
[modify] https://crrev.com/bd57338ff5d58fff3147982c3f631cbe43e86f9c/content/browser/download/download_stats.h
[delete] https://crrev.com/ced9605aea022ef7b68ef9d2a241ccb269ff6c38/content/browser/download/file_metadata_linux.cc
[delete] https://crrev.com/ced9605aea022ef7b68ef9d2a241ccb269ff6c38/content/browser/download/file_metadata_linux.h
[delete] https://crrev.com/ced9605aea022ef7b68ef9d2a241ccb269ff6c38/content/browser/download/file_metadata_mac.h
[delete] https://crrev.com/ced9605aea022ef7b68ef9d2a241ccb269ff6c38/content/browser/download/file_metadata_unittest_linux.cc
[add] https://crrev.com/bd57338ff5d58fff3147982c3f631cbe43e86f9c/content/browser/download/quarantine.cc
[add] https://crrev.com/bd57338ff5d58fff3147982c3f631cbe43e86f9c/content/browser/download/quarantine.h
[add] https://crrev.com/bd57338ff5d58fff3147982c3f631cbe43e86f9c/content/browser/download/quarantine_constants_linux.h
[add] https://crrev.com/bd57338ff5d58fff3147982c3f631cbe43e86f9c/content/browser/download/quarantine_linux.cc
[add] https://crrev.com/bd57338ff5d58fff3147982c3f631cbe43e86f9c/content/browser/download/quarantine_linux_unittest.cc
[rename] https://crrev.com/bd57338ff5d58fff3147982c3f631cbe43e86f9c/content/browser/download/quarantine_mac.mm
[add] https://crrev.com/bd57338ff5d58fff3147982c3f631cbe43e86f9c/content/browser/download/quarantine_win.cc
[add] https://crrev.com/bd57338ff5d58fff3147982c3f631cbe43e86f9c/content/browser/download/quarantine_win_unittest.cc
[delete] https://crrev.com/ced9605aea022ef7b68ef9d2a241ccb269ff6c38/content/browser/safe_util_win.cc
[delete] https://crrev.com/ced9605aea022ef7b68ef9d2a241ccb269ff6c38/content/browser/safe_util_win.h
[modify] https://crrev.com/bd57338ff5d58fff3147982c3f631cbe43e86f9c/content/test/BUILD.gn
[modify] https://crrev.com/bd57338ff5d58fff3147982c3f631cbe43e86f9c/tools/metrics/histograms/histograms.xml

Asanka -- Is some of that functionality described in the last CL new, or is it refactored? I didn't know about Mac quarantining, and I'm curious about the UX. (We could consider dialing back some cases of generic warnings if the OS is going to do it for us.)

It's refactored so that it can be used by the PPAPI code via the quarantine.h API. Some of the expected behavior is described in https://support.apple.com/en-us/HT201940

Thanks Asanka.  Hmm, I can't repro that when downloading a .dmg on Mac w/ M53. What might I be missing? 

Yeah, I downloaded an unsigned .dmg from a test server and can't get a warning to appear either. Tried with both Safari and Chrome.

Here's the file metadata added by Safari and Chrome:

Safari:

Foo-safari.dmg: com.apple.metadata:kMDItemDownloadedDate:
00000000  62 70 6C 69 73 74 30 30 A1 01 33 41 BD 94 1D A3  |bplist00..3A....|
00000010  AF AE 79 08 0A 00 00 00 00 00 00 01 01 00 00 00  |..y.............|
00000020  00 00 00 00 02 00 00 00 00 00 00 00 00 00 00 00  |................|
00000030  00 00 00 00 13                                   |.....|
00000035
Foo-safari.dmg: com.apple.metadata:kMDItemWhereFroms:
00000000  62 70 6C 69 73 74 30 30 A2 01 02 5F 10 21 68 74  |bplist00..._.!ht|
00000010  74 70 73 3A 2F 2F 63 72 2E 6B 75 6E 67 66 6F 6F  |tps://cr.kungfoo|
00000020  2E 6E 65 74 2F 64 6C 2F 46 6F 6F 2E 64 6D 67 5F  |.net/dl/Foo.dmg_|
00000030  10 26 68 74 74 70 73 3A 2F 2F 63 72 2E 6B 75 6E  |.&https://cr.kun|
00000040  67 66 6F 6F 2E 6E 65 74 2F 64 6C 2F 6D 61 63 2D  |gfoo.net/dl/mac-|
00000050  64 6D 67 2E 68 74 6D 6C 08 0B 2F 00 00 00 00 00  |dmg.html../.....|
00000060  00 01 01 00 00 00 00 00 00 00 03 00 00 00 00 00  |................|
00000070  00 00 00 00 00 00 00 00 00 00 58                 |..........X|
0000007b
Foo-safari.dmg: com.apple.quarantine: 0002;57e3e623;Safari;C3959BC9-F244-48D9-983F-DD6136541616

Chrome:

Foo-chrome.dmg: com.apple.metadata:kMDItemWhereFroms:
00000000  62 70 6C 69 73 74 30 30 A2 01 02 5F 10 21 68 74  |bplist00..._.!ht|
00000010  74 70 73 3A 2F 2F 63 72 2E 6B 75 6E 67 66 6F 6F  |tps://cr.kungfoo|
00000020  2E 6E 65 74 2F 64 6C 2F 46 6F 6F 2E 64 6D 67 5F  |.net/dl/Foo.dmg_|
00000030  10 26 68 74 74 70 73 3A 2F 2F 63 72 2E 6B 75 6E  |.&https://cr.kun|
00000040  67 66 6F 6F 2E 6E 65 74 2F 64 6C 2F 6D 61 63 2D  |gfoo.net/dl/mac-|
00000050  64 6D 67 2E 68 74 6D 6C 08 0B 2F 00 00 00 00 00  |dmg.html../.....|
00000060  00 01 01 00 00 00 00 00 00 00 03 00 00 00 00 00  |................|
00000070  00 00 00 00 00 00 00 00 00 00 58                 |..........X|
0000007b
Foo-chrome.dmg: com.apple.quarantine: 0001;57e3e5ff;Google Chrome;E00ED1B8-7D14-4114-A109-FD2BC3F8F95A

Mac folks?

The quarantine bit only brings up a UI when launching an .app with the flag set for the first time. The quarantine flag transitively gets applied to the contents of downloaded archives (dmg/zip/etc).

Thanks Robert! Armed with that information I can verify that the warning is displayed for files downloaded with Chrome.

Deleted: Screen Shot 2016-09-22 at 10.47.16 AM.png 45.3 KB

	Screen Shot 2016-09-22 at 10.47.16 AM.png 45.3 KB View Download

Re #30: 

> The quarantine flag transitively gets applied to the 
> contents of downloaded archives (dmg/zip/etc).

Cool! Does OS X somehow do this itself such that archive-handling applications need not do it themselves? Or do they do it in their native handlers and leave 3rd-party applications to fend for themselves? The latter is the approach on Windows, and it's a big hole: https://textslashplain.com/2016/04/04/downloads-and-the-mark-of-the-web/

Archive-handling programs need to handle this themselves. But macOS's has built-in handlers for most archive types, and both the GUI and CLI handlers copy forward the xattrs.

Friendly ping from Security Sheriff: any updates here?

Sorry about the delay. The Windows side of the PPAPI annotation logic is simple, but I ran into a speed bump on Mac where the PPAPI sometimes fails to re-open the file after it has been quarantined. The API that we expose via PPAPI allows the plugins to close and re-open the file and I'm guessing we don't want to break that.

I can land the Windows-side changes and gate the Mac changes on a larger reworking on how we deal with file writes via PPAPI. One option is to create a temporary file with a harmless name, allow the plugin to go crazy with it. Then once the plugin process goes away, we can rename the file to its intended name and immediately quarantine it. This could also break plugins since the files written by them aren't "visible" until the plugin process goes away (i.e. until the tab is closed or navigates to a different origin or..).

The plugin process lifetime tie-in is to deal with the fact that the API allows the resulting FileRef to be exercised by the plugin at any point after it acquires the permission to write to it.

I think I'll land https://codereview.chromium.org/2124373002 as a non-Mac fix as a start without blocking on possibly breaking change to PPAPI to address the Mac side of things.

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

commit c1ab0290967226e37abb9537f0f53f1616f5fb70
Author: asanka <asanka@chromium.org>
Date: Thu Dec 08 21:53:33 2016

[PPAPI] Quarantine files that are writeable by a Pepper plugin.

Henceforth Chrome will treat files that were writeable by a Pepper
plugin the same way it treats files that were downloaded from the origin
of the plugin.

This new behavior is limited to Windows and Linux. On Mac OS X,
quarantining a file interferes with subsequent opening by a plugin.
PPAPI's file write support currently relies on being able to do so.

This CL makes the following changes:

  * Move quarantine* files into //content/public/common and
    //content/common. This logic now needs to be accessed from both
    //content and //chrome.

  * When a PPAPI plug-in attempts to open a file for writing, //content
    quarantines the file prior to allowing the plug-in to write to it.

BUG= 598812 

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

[modify] https://crrev.com/c1ab0290967226e37abb9537f0f53f1616f5fb70/base/test/test_file_util.h
[modify] https://crrev.com/c1ab0290967226e37abb9537f0f53f1616f5fb70/base/test/test_file_util_win.cc
[modify] https://crrev.com/c1ab0290967226e37abb9537f0f53f1616f5fb70/chrome/browser/download/download_browsertest.cc
[modify] https://crrev.com/c1ab0290967226e37abb9537f0f53f1616f5fb70/chrome/test/ppapi/ppapi_filechooser_browsertest.cc
[modify] https://crrev.com/c1ab0290967226e37abb9537f0f53f1616f5fb70/content/browser/BUILD.gn
[modify] https://crrev.com/c1ab0290967226e37abb9537f0f53f1616f5fb70/content/browser/download/base_file.cc
[delete] https://crrev.com/9d858647530db2c22e1267e443034d23f2e602d9/content/browser/download/quarantine_mac_unittest.mm
[modify] https://crrev.com/c1ab0290967226e37abb9537f0f53f1616f5fb70/content/browser/renderer_host/pepper/pepper_file_io_host.cc
[modify] https://crrev.com/c1ab0290967226e37abb9537f0f53f1616f5fb70/content/browser/renderer_host/pepper/pepper_file_io_host.h
[modify] https://crrev.com/c1ab0290967226e37abb9537f0f53f1616f5fb70/content/common/BUILD.gn
[rename] https://crrev.com/c1ab0290967226e37abb9537f0f53f1616f5fb70/content/common/quarantine/quarantine.cc
[rename] https://crrev.com/c1ab0290967226e37abb9537f0f53f1616f5fb70/content/common/quarantine/quarantine_constants_linux.h
[rename] https://crrev.com/c1ab0290967226e37abb9537f0f53f1616f5fb70/content/common/quarantine/quarantine_linux.cc
[rename] https://crrev.com/c1ab0290967226e37abb9537f0f53f1616f5fb70/content/common/quarantine/quarantine_linux_unittest.cc
[rename] https://crrev.com/c1ab0290967226e37abb9537f0f53f1616f5fb70/content/common/quarantine/quarantine_mac.mm
[add] https://crrev.com/c1ab0290967226e37abb9537f0f53f1616f5fb70/content/common/quarantine/quarantine_mac_unittest.mm
[add] https://crrev.com/c1ab0290967226e37abb9537f0f53f1616f5fb70/content/common/quarantine/quarantine_unittest.cc
[rename] https://crrev.com/c1ab0290967226e37abb9537f0f53f1616f5fb70/content/common/quarantine/quarantine_win.cc
[rename] https://crrev.com/c1ab0290967226e37abb9537f0f53f1616f5fb70/content/common/quarantine/quarantine_win_unittest.cc
[modify] https://crrev.com/c1ab0290967226e37abb9537f0f53f1616f5fb70/content/public/common/BUILD.gn
[rename] https://crrev.com/c1ab0290967226e37abb9537f0f53f1616f5fb70/content/public/common/quarantine.h
[modify] https://crrev.com/c1ab0290967226e37abb9537f0f53f1616f5fb70/content/test/BUILD.gn

This bug has been closed for more than 14 weeks. Removing security view restrictions.

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