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

[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
Committed: https://crrev.com/bd57338ff5d58fff3147982c3f631cbe43e86f9c
Cr-Commit-Position: refs/heads/master@{#420060}

[asanka, 2016-07-06 20:57:06]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-06 20:58:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-06 23:19:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-06 23:19:53]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[asanka, 2016-07-07 18:39:45]

Patchset #1 (id:1) has been deleted

[asanka, 2016-07-07 18:39:51]

Patchset #1 (id:20001) has been deleted

[asanka, 2016-07-07 18:39:58]

Patchset #1 (id:40001) has been deleted

[asanka, 2016-07-07 18:40:07]

Patchset #1 (id:60001) has been deleted

[asanka, 2016-07-08 19:08:31]

Description was changed from

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

BUG=none
==========

to

==========
[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 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. The
QuarantineFile() function is henceforth a platform independent mechanism
for annotating a downloaded file.

BUG=598812
==========

[asanka, 2016-07-08 19:10:34]

Description was changed from

==========
[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 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. The
QuarantineFile() function is henceforth a platform independent mechanism
for annotating a downloaded file.

BUG=598812
==========

to

==========
[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 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. 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
==========

[asanka, 2016-07-08 19:12:10]

asanka@chromium.org changed reviewers:
+ asvitkine@chromium.org, cpu@chromium.org, rsesek@chromium.org,
svaldez@chromium.org

[asanka, 2016-07-08 19:12:34]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-08 19:13:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[asanka, 2016-07-08 19:14:25]

Description was changed from

==========
[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 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. 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
==========

to

==========
[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
==========

[commit-bot: I haz the power, 2016-07-08 19:17:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-08 19:17:14]

Dry run: This issue passed the CQ dry run.

[asanka, 2016-07-08 19:17:52]

cpu:
  content/browser/download/quarantine_win*
  (also for removing content/browser/safe_util_win*)

rsesek:
  content/browser/download/quarantine_mac*
  content/browser/download/quarantine_linux*

svaldez:
  content/browser/download/base_file.cc
  FYI on all

asvitkine:
  tools/metrics/histograms/histograms.xml
  Metrics code in content/browser/download/quarantine_win.cc

[Robert Sesek, 2016-07-08 19:38:35]

mac LGTM

https://codereview.chromium.org/2123023002/diff/140001/content/browser/downlo...
File content/browser/download/quarantine.h (right):

https://codereview.chromium.org/2123023002/diff/140001/content/browser/downlo...
content/browser/download/quarantine.h:5: #ifndef
__CONTENT_BROWSER_DOWNLOAD_QUARANTINE_H_
nit: header guard shouldn't have __ prefix

https://codereview.chromium.org/2123023002/diff/140001/content/browser/downlo...
File content/browser/download/quarantine_mac.mm (right):

https://codereview.chromium.org/2123023002/diff/140001/content/browser/downlo...
content/browser/download/quarantine_mac.mm:189: succeeded =
Use &= instead of |&& succeed| ?

[svaldez, 2016-07-11 14:55:46]

lgtm

https://codereview.chromium.org/2123023002/diff/140001/content/browser/downlo...
File content/browser/download/quarantine_constants_linux.h (right):

https://codereview.chromium.org/2123023002/diff/140001/content/browser/downlo...
content/browser/download/quarantine_constants_linux.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
*2016?

https://codereview.chromium.org/2123023002/diff/140001/content/browser/downlo...
File content/browser/download/quarantine_linux.cc (right):

https://codereview.chromium.org/2123023002/diff/140001/content/browser/downlo...
content/browser/download/quarantine_linux.cc:48: if (source_url.is_valid()) {
Should you be early failing if setting the SourceURL attribute fails?

https://codereview.chromium.org/2123023002/diff/140001/content/browser/downlo...
File content/browser/download/quarantine_mac.mm (right):

https://codereview.chromium.org/2123023002/diff/140001/content/browser/downlo...
content/browser/download/quarantine_mac.mm:189: succeeded =
On 2016/07/08 19:38:35, Robert Sesek wrote:
> Use &= instead of |&& succeed| ?

Similar to the linux version, I think this is to prevent shortcircuiting if the
first metadata add fails. Don't know whether this should actually try setting
the Origin Metadata if the Quarantine Metadata fails.

[svaldez, 2016-07-11 14:55:46]

lgtm

[Alexei Svitkine (slow), 2016-07-11 16:46:20]

lgtm

[asanka, 2016-09-08 18:58:51]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 18:59:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 19:21:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 19:21:21]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[asanka, 2016-09-09 15:38:01]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 15:38:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 15:41:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 15:41:36]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[asanka, 2016-09-09 19:05:03]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 19:05:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 19:17:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 19:17:39]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[asanka, 2016-09-09 20:03:03]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 20:03:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 22:16:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 22:16:51]

Dry run: This issue passed the CQ dry run.

[asanka, 2016-09-12 15:36:38]

Thanks folks! Sorry about the delay. I'm picking this up again from out of the
post-vacation backlog.

cpu: Could you take a look at content/browser/* and content/test/* ?

https://codereview.chromium.org/2123023002/diff/140001/content/browser/downlo...
File content/browser/download/quarantine.h (right):

https://codereview.chromium.org/2123023002/diff/140001/content/browser/downlo...
content/browser/download/quarantine.h:5: #ifndef
__CONTENT_BROWSER_DOWNLOAD_QUARANTINE_H_
On 2016/07/08 at 19:38:35, Robert Sesek wrote:
> nit: header guard shouldn't have __ prefix

Done.

https://codereview.chromium.org/2123023002/diff/140001/content/browser/downlo...
File content/browser/download/quarantine_linux.cc (right):

https://codereview.chromium.org/2123023002/diff/140001/content/browser/downlo...
content/browser/download/quarantine_linux.cc:48: if (source_url.is_valid()) {
On 2016/07/11 at 14:55:45, svaldez wrote:
> Should you be early failing if setting the SourceURL attribute fails?

Currently it's possible for the referrer URL to be invalid/empty, but the source
must always be valid. So I've modified the patch to:

* Attempt to set the source and referrer attributes if the corresponding URLs
are valid. No short circuits.
* The return value would be ANNOTATION_FAILED if any of the following is true:
  * Source URL was invalid.
  * Source URL was valid, but setting the source attribute failed.
  * Referrer URL was valid, but setting the referrer attribute failed.

https://codereview.chromium.org/2123023002/diff/140001/content/browser/downlo...
File content/browser/download/quarantine_mac.mm (right):

https://codereview.chromium.org/2123023002/diff/140001/content/browser/downlo...
content/browser/download/quarantine_mac.mm:189: succeeded =
On 2016/07/11 at 14:55:45, svaldez wrote:
> On 2016/07/08 19:38:35, Robert Sesek wrote:
> > Use &= instead of |&& succeed| ?
> 
> Similar to the linux version, I think this is to prevent shortcircuiting if
the first metadata add fails. Don't know whether this should actually try
setting the Origin Metadata if the Quarantine Metadata fails.

svaldez is correct about the intended effect, and I think Robert also noticed
it. Using &= on bools is defined and correct in this case. But I have a personal
preference to avoid relying on bool->int promotion.

Instead, I've modified to code to explicitly store the results from the
AddOrigin... and AddQuarantine.. calls separately and determine the return value
based on them. This, I think, is more readable.

[Robert Sesek, 2016-09-12 21:09:10]

LGTM, agree that's more readable

[asanka, 2016-09-15 21:06:49]

cpu: Ping?

[cpu_(ooo_6.6-7.5), 2016-09-20 15:38:02]

looking ..

[cpu_(ooo_6.6-7.5), 2016-09-20 15:52:34]

lgtm

https://codereview.chromium.org/2123023002/diff/220001/content/browser/downlo...
File content/browser/download/base_file.cc (right):

https://codereview.chromium.org/2123023002/diff/220001/content/browser/downlo...
content/browser/download/base_file.cc:408: //
On windows the attachment service can delete the file directly. I forgot in what
cases.

https://codereview.chromium.org/2123023002/diff/220001/content/browser/downlo...
File content/browser/download/quarantine_win.cc (right):

https://codereview.chromium.org/2123023002/diff/220001/content/browser/downlo...
content/browser/download/quarantine_win.cc:63: }
interesting function. This only tests the stream exists, not its content which
is also important...

https://codereview.chromium.org/2123023002/diff/220001/content/browser/downlo...
content/browser/download/quarantine_win.cc:137: // deleted. See:
http://msdn.microsoft.com/en-us/bb776299
here ^^, see my previous comment about the file gone missing.

[asanka, 2016-09-20 20:42:20]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 20:42:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 23:18:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 23:18:09]

Dry run: This issue passed the CQ dry run.

[asanka, 2016-09-21 14:57:00]

Thanks everyone!

https://codereview.chromium.org/2123023002/diff/220001/content/browser/downlo...
File content/browser/download/base_file.cc (right):

https://codereview.chromium.org/2123023002/diff/220001/content/browser/downlo...
content/browser/download/base_file.cc:408: //
On 2016/09/20 at 15:52:33, cpu wrote:
> On windows the attachment service can delete the file directly. I forgot in
what cases.

Got it. This status specifically addresses the case where the file was missing
before the Attachment Services check. The cases where the file is missing after
the Attachment Services check is covered under the VIRUS_INFECTED,
SECURITY_CHECK_FAILED, BLOCKED_BY_POLICY and ACCESS_DENIED cases above.

https://codereview.chromium.org/2123023002/diff/220001/content/browser/downlo...
File content/browser/download/quarantine_win.cc (right):

https://codereview.chromium.org/2123023002/diff/220001/content/browser/downlo...
content/browser/download/quarantine_win.cc:63: }
On 2016/09/20 at 15:52:33, cpu wrote:
> interesting function. This only tests the stream exists, not its content which
is also important...

Yup. The assumption being that if Attachment Services creates the stream, the
its contents is likely valid. I didn't want this check to be too sensitive to
the format of the Zone.Identifier stream.

I added a check for the content as well. Now we verify that the stream has a
"[ZoneTransfer]" section.

https://codereview.chromium.org/2123023002/diff/220001/content/browser/downlo...
content/browser/download/quarantine_win.cc:137: // deleted. See:
http://msdn.microsoft.com/en-us/bb776299
On 2016/09/20 at 15:52:33, cpu wrote:
> here ^^, see my previous comment about the file gone missing.

Yup. Thanks!

[asanka, 2016-09-21 14:57:12]

The CQ bit was checked by asanka@chromium.org

[asanka, 2016-09-21 14:57:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
svaldez@chromium.org, asvitkine@chromium.org, rsesek@chromium.org,
cpu@chromium.org
Link to the patchset: https://codereview.chromium.org/2123023002/#ps240001
(title: "[win] Verify that the Zone.Identifier stream has the correct
contents.")

[commit-bot: I haz the power, 2016-09-21 14:57:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 15:04:08]

Committed patchset #9 (id:240001)

[commit-bot: I haz the power, 2016-09-21 15:06:21]

Description was changed from

==========
[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
==========

to

==========
[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

Committed: https://crrev.com/bd57338ff5d58fff3147982c3f631cbe43e86f9c
Cr-Commit-Position: refs/heads/master@{#420060}
==========

[commit-bot: I haz the power, 2016-09-21 15:06:23]

Patchset 9 (id:??) landed as
https://crrev.com/bd57338ff5d58fff3147982c3f631cbe43e86f9c
Cr-Commit-Position: refs/heads/master@{#420060}

[brucedawson, 2016-09-27 17:16:59]

brucedawson@chromium.org changed reviewers:
+ brucedawson@chromium.org

[brucedawson, 2016-09-27 17:17:00]

https://codereview.chromium.org/2123023002/diff/240001/content/browser/downlo...
File content/browser/download/quarantine_win.cc (right):

https://codereview.chromium.org/2123023002/diff/240001/content/browser/downlo...
content/browser/download/quarantine_win.cc:108: case ERROR_ACCESS_DENIED:
This case is incorrect. ERROR_ACCESS_DENIED is equal to 5, and the "if
(SUCCEEDED(hr))" test already filters out all positive numbers.

The E_ACCESSDENIED case should be sufficient - it is the HRESULT equivalent of
ERROR_ACCESS_DENIED.

This was found through the experimental VC++ /analyze static analysis builder
which gave this warning:

src\content\browser\download\quarantine_win.cc(109) : warning C6221: Implicit
cast between semantically different integer types:  comparing HRESULT to an
integer.

It's harmless, but confusing. Please remove?

[asanka, 2016-09-27 17:26:48]

https://codereview.chromium.org/2123023002/diff/240001/content/browser/downlo...
File content/browser/download/quarantine_win.cc (right):

https://codereview.chromium.org/2123023002/diff/240001/content/browser/downlo...
content/browser/download/quarantine_win.cc:108: case ERROR_ACCESS_DENIED:
On 2016/09/27 at 17:17:00, brucedawson wrote:
> This case is incorrect. ERROR_ACCESS_DENIED is equal to 5, and the "if
(SUCCEEDED(hr))" test already filters out all positive numbers.
> 
> The E_ACCESSDENIED case should be sufficient - it is the HRESULT equivalent of
ERROR_ACCESS_DENIED.
> 
> This was found through the experimental VC++ /analyze static analysis builder
which gave this warning:
> 
> src\content\browser\download\quarantine_win.cc(109) : warning C6221: Implicit
cast between semantically different integer types:  comparing HRESULT to an
integer.
> 
> It's harmless, but confusing. Please remove?

Thanks for catching this. I'll rework the code so that we properly catch these
cases. The SUCCEEDED(hr) test isn't quite correct.

Testing for ERROR_ACCESS_DENIED isn't a mistake. While the API is documented to
return HRESULT, it actually passes through the return value of
IOfficeAntivirus::Scan() as returned by any registered MSOfficeAntiVirus
provider. In practice we see a mix of HRESULT and system error codes, of which
ERROR_ACCESS_DENIED is pretty common. This is why the SUCCEEDED(hr) test above
is wrong.

[asanka, 2016-09-27 20:32:23]

https://codereview.chromium.org/2123023002/diff/240001/content/browser/downlo...
File content/browser/download/quarantine_win.cc (right):

https://codereview.chromium.org/2123023002/diff/240001/content/browser/downlo...
content/browser/download/quarantine_win.cc:108: case ERROR_ACCESS_DENIED:
On 2016/09/27 at 17:26:48, asanka wrote:
> On 2016/09/27 at 17:17:00, brucedawson wrote:
> > This case is incorrect. ERROR_ACCESS_DENIED is equal to 5, and the "if
(SUCCEEDED(hr))" test already filters out all positive numbers.
> > 
> > The E_ACCESSDENIED case should be sufficient - it is the HRESULT equivalent
of ERROR_ACCESS_DENIED.
> > 
> > This was found through the experimental VC++ /analyze static analysis
builder which gave this warning:
> > 
> > src\content\browser\download\quarantine_win.cc(109) : warning C6221:
Implicit cast between semantically different integer types:  comparing HRESULT
to an integer.
> > 
> > It's harmless, but confusing. Please remove?
> 
> Thanks for catching this. I'll rework the code so that we properly catch these
cases. The SUCCEEDED(hr) test isn't quite correct.
> 
> Testing for ERROR_ACCESS_DENIED isn't a mistake. While the API is documented
to return HRESULT, it actually passes through the return value of
IOfficeAntivirus::Scan() as returned by any registered MSOfficeAntiVirus
provider. In practice we see a mix of HRESULT and system error codes, of which
ERROR_ACCESS_DENIED is pretty common. This is why the SUCCEEDED(hr) test above
is wrong.

https://codereview.chromium.org/2374793002