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

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

[asanka, 2016-07-08 19:24:16]

Description was changed from

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

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

to

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

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

[asanka, 2016-07-08 19:24:54]

asanka@chromium.org changed reviewers:
+ bbudge@chromium.org

[asanka, 2016-07-08 19:26:35]

bbudge: PTAL?

[bbudge, 2016-07-11 20:08:23]

One comment, otherwise LGTM

https://codereview.chromium.org/2124373002/diff/40001/chrome/test/ppapi/ppapi...
File chrome/test/ppapi/ppapi_filechooser_browsertest.cc (right):

https://codereview.chromium.org/2124373002/diff/40001/chrome/test/ppapi/ppapi...
chrome/test/ppapi/ppapi_filechooser_browsertest.cc:356: #if defined(OS_WIN)
Could you add a comment explaining what this tests, and how zone_identifier
works? I think it would be difficult for anyone who is not familiar with the
issue to figure out just from reading this code.

[asanka, 2016-09-12 16:07:04]

Thanks!

https://codereview.chromium.org/2124373002/diff/40001/chrome/test/ppapi/ppapi...
File chrome/test/ppapi/ppapi_filechooser_browsertest.cc (right):

https://codereview.chromium.org/2124373002/diff/40001/chrome/test/ppapi/ppapi...
chrome/test/ppapi/ppapi_filechooser_browsertest.cc:356: #if defined(OS_WIN)
On 2016/07/11 at 20:08:22, bbudge wrote:
> Could you add a comment explaining what this tests, and how zone_identifier
works? I think it would be difficult for anyone who is not familiar with the
issue to figure out just from reading this code.

Done.

[asanka, 2016-09-12 17:31:02]

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

[commit-bot: I haz the power, 2016-09-12 17:31: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-09-12 17:56:55]

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

[commit-bot: I haz the power, 2016-09-12 17:56:56]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[asanka, 2016-09-21 16:15:31]

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

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

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-21 17:42:03]

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

[commit-bot: I haz the power, 2016-09-21 17:42:04]

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-22 14:28:33]

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

[commit-bot: I haz the power, 2016-09-22 14:28:52]

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-22 15:32:52]

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

[commit-bot: I haz the power, 2016-09-22 15:32: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-09-22 18:15:05]

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

[commit-bot: I haz the power, 2016-09-22 18:18:14]

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-22 19:04:23]

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

[commit-bot: I haz the power, 2016-09-22 19:04:24]

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-11-23 21:44:38]

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

[commit-bot: I haz the power, 2016-11-23 21:45:20]

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-11-23 23:11:07]

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

[commit-bot: I haz the power, 2016-11-23 23:11:08]

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-11-30 15:16:21]

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

[commit-bot: I haz the power, 2016-11-30 15:16:47]

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-11-30 16:33:38]

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

[commit-bot: I haz the power, 2016-11-30 16:33:39]

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-12-02 02:01:29]

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

[commit-bot: I haz the power, 2016-12-02 02:02:09]

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-12-02 02:53:36]

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

[commit-bot: I haz the power, 2016-12-02 02:53:37]

Dry run: This issue passed the CQ dry run.

[asanka, 2016-12-02 14:53:48]

Patchset #10 (id:180001) has been deleted

[asanka, 2016-12-02 14:53:56]

Patchset #10 (id:200001) has been deleted

[asanka, 2016-12-02 14:54:03]

Patchset #10 (id:220001) has been deleted

[asanka, 2016-12-02 14:56:27]

Resurrect with better test support

[asanka, 2016-12-02 14:58:51]

asanka@chromium.org changed reviewers:
+ jam@chromium.org, mark@chromium.org

[asanka, 2016-12-02 15:03:04]

asanka@chromium.org changed reviewers:
+ avi@chromium.org, jochen@chromium.org
- jam@chromium.org, mark@chromium.org

[asanka, 2016-12-02 15:04:11]

asanka@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[asanka, 2016-12-02 17:54:34]

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

[commit-bot: I haz the power, 2016-12-02 17:54:53]

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

[asanka, 2016-12-02 19:11:39]

Description was changed from

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

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

to

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

[commit-bot: I haz the power, 2016-12-02 19:20:53]

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

[commit-bot: I haz the power, 2016-12-02 19:20:54]

Dry run: This issue passed the CQ dry run.

[asanka, 2016-12-02 19:23:00]

asanka@chromium.org changed reviewers:
+ brettw@chromium.org
- phajdan.jr@chromium.org

[asanka, 2016-12-02 19:23:01]

bbudge: Thanks for your review! Resurrecting this old CL.

brettw:
  base/test/test_file_util*
  content/common/quarantine_win.cc
  content/common/quarantine_win_unittest.cc

     I'm moving Windows specific code from //base/test/test_file_util* to
//content/common/quarantine_win*.

avi:  
  content/common/quarantine_mac.mm
  content/common/quarantine_mac_unittest.mm

     Mac stuff. I'm mostly moving code around. But could you make sure we are
doing right by 10.9 vs 10.10+? Perhaps some of the warning suppressions could be
removed?

jochen:
  content/browser/BUILD.gn
  content/common/BUILD.gn
  content/common/quarantine.cc
  content/common/quarantine_constants_linux.h
  content/common/quarantine_linux.cc
  content/common/quarantine_linux_unittest.cc
  content/common/quarantine_unittest.cc
  content/public/common/BUILD.gn
  content/public/common/quarantine.h

     I'm moving the quarantine_ stuff from //content/browser/download to
//content/common and //content/public/common. This is so that both //content and
//chrome can use it.

[Avi (use Gerrit), 2016-12-02 19:58:27]

lgtm

https://codereview.chromium.org/2124373002/diff/280001/chrome/browser/downloa...
File chrome/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/2124373002/diff/280001/chrome/browser/downloa...
chrome/browser/download/download_browsertest.cc:1184: // |browser_tests| aren't
run from a process that has LSFileQuarantineEnabled
The lack of LSFileQuarantineEnabled shouldn't be a problem; the presence of that
bit means that files get quarantined by the OS by default, but if we explicitly
call code in the test process to quarantine a file, that should work.

https://codereview.chromium.org/2124373002/diff/280001/content/common/quarant...
File content/common/quarantine_mac.mm (right):

https://codereview.chromium.org/2124373002/diff/280001/content/common/quarant...
content/common/quarantine_mac.mm:303: // Don't consider it an error if we fail
to add origin metadata.
space before this line

[jochen (gone - plz use gerrit), 2016-12-05 15:31:04]

Can you move the quarantine files into a subdirectory in common (either
common/download as the files are also in browser/download or common/quarantine)

I'll defer to brettw for the content review, as he owns all the files

[asanka, 2016-12-06 20:07:09]

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

[commit-bot: I haz the power, 2016-12-06 20:07:36]

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

[asanka, 2016-12-06 20:34:36]

Thanks!

jochen: I moved quarantine_* files to //content/common/quarantine

brettw: Ping

https://codereview.chromium.org/2124373002/diff/280001/chrome/browser/downloa...
File chrome/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/2124373002/diff/280001/chrome/browser/downloa...
chrome/browser/download/download_browsertest.cc:1184: // |browser_tests| aren't
run from a process that has LSFileQuarantineEnabled
On 2016/12/02 at 19:58:27, Avi (at offsite M-Th) wrote:
> The lack of LSFileQuarantineEnabled shouldn't be a problem; the presence of
that bit means that files get quarantined by the OS by default, but if we
explicitly call code in the test process to quarantine a file, that should work.

Acknowledged. This is what we do for the unit test.

The browser test was a bit tricky since the target file is created and annotated
deep in the guts of the browser and I wanted to keep the test as black-box as
possible. :-/

[commit-bot: I haz the power, 2016-12-06 21:10:21]

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

[commit-bot: I haz the power, 2016-12-06 21:10:22]

Dry run: This issue passed the CQ dry run.

[brettw, 2016-12-06 22:48:42]

I still need to look at pepper_file_io_host but I need more time to page in how
that works.

https://codereview.chromium.org/2124373002/diff/280001/chrome/browser/downloa...
File chrome/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/2124373002/diff/280001/chrome/browser/downloa...
chrome/browser/download/download_browsertest.cc:1204: // restrictions on MOTW
can be applied. While Chrome doesn't directly apply
I don't know what "MOTW" is ("message of the week"?). Can you define it?

https://codereview.chromium.org/2124373002/diff/300001/content/common/quarant...
File content/common/quarantine/quarantine_win.cc (right):

https://codereview.chromium.org/2124373002/diff/300001/content/common/quarant...
content/common/quarantine/quarantine_win.cc:85:
base::SplitString(zone_identifier_contents, "\n", base::TRIM_WHITESPACE,
I think you could use SplitStringPiece here instead since nothing needs to
outlive the input. This will prevent a bunch of string allocs.

https://codereview.chromium.org/2124373002/diff/300001/content/public/common/...
File content/public/common/quarantine.h (right):

https://codereview.chromium.org/2124373002/diff/300001/content/public/common/...
content/public/common/quarantine.h:95: // any quarantine metadata is prsent for
the file.
prsent -> present

[brettw, 2016-12-07 00:11:11]

lgtm

[jochen (gone - plz use gerrit), 2016-12-07 15:34:20]

rubberstamp lgtm now that Brett approved

[asanka, 2016-12-08 15:11:07]

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

[commit-bot: I haz the power, 2016-12-08 15:11:28]

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

[asanka, 2016-12-08 15:13:29]

Thanks everyone!

https://codereview.chromium.org/2124373002/diff/280001/chrome/browser/downloa...
File chrome/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/2124373002/diff/280001/chrome/browser/downloa...
chrome/browser/download/download_browsertest.cc:1204: // restrictions on MOTW
can be applied. While Chrome doesn't directly apply
On 2016/12/06 at 22:48:42, brettw (ping after 24h) wrote:
> I don't know what "MOTW" is ("message of the week"?). Can you define it?

Done :-)

https://codereview.chromium.org/2124373002/diff/300001/content/common/quarant...
File content/common/quarantine/quarantine_win.cc (right):

https://codereview.chromium.org/2124373002/diff/300001/content/common/quarant...
content/common/quarantine/quarantine_win.cc:85:
base::SplitString(zone_identifier_contents, "\n", base::TRIM_WHITESPACE,
On 2016/12/06 at 22:48:42, brettw (ping after 24h) wrote:
> I think you could use SplitStringPiece here instead since nothing needs to
outlive the input. This will prevent a bunch of string allocs.

Done.

https://codereview.chromium.org/2124373002/diff/300001/content/public/common/...
File content/public/common/quarantine.h (right):

https://codereview.chromium.org/2124373002/diff/300001/content/public/common/...
content/public/common/quarantine.h:95: // any quarantine metadata is prsent for
the file.
On 2016/12/06 at 22:48:42, brettw (ping after 24h) wrote:
> prsent -> present

Done.

[asanka, 2016-12-08 15:48:48]

The CQ bit was checked by asanka@chromium.org

[asanka, 2016-12-08 15:48:48]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
avi@chromium.org, brettw@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2124373002/#ps320001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-12-08 15:49:12]

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-12-08 16:37:04]

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

[commit-bot: I haz the power, 2016-12-08 16:37:05]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[asanka, 2016-12-08 16:54:19]

The CQ bit was checked by asanka@chromium.org

[commit-bot: I haz the power, 2016-12-08 16:54:38]

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-12-08 18:21:56]

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

[commit-bot: I haz the power, 2016-12-08 18:21:57]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[asanka, 2016-12-08 18:44:28]

The CQ bit was checked by asanka@chromium.org

[commit-bot: I haz the power, 2016-12-08 18:45:09]

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-12-08 20:53:38]

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

[commit-bot: I haz the power, 2016-12-08 20:53:40]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[asanka, 2016-12-08 21:11:27]

The CQ bit was checked by asanka@chromium.org

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

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-12-08 21:53:37]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1481231487029730,
"parent_rev": "0823ee7af6ab70d0939f3b6268ea234577b81666", "commit_rev":
"0557658d8d05a9cda6f5708d35fb26741ee67d93"}

[commit-bot: I haz the power, 2016-12-08 21:54:07]

Committed patchset #14 (id:320001)

[commit-bot: I haz the power, 2016-12-08 21:56:57]

Description was changed from

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

to

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

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

[commit-bot: I haz the power, 2016-12-08 21:56:59]

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