[SafeBrowsing] Block dangerous unchecked downloads based on a Finch trial.

[SafeBrowsing] Block dangerous unchecked downloads based on a Finch trial.

BUG=533579
Committed: https://crrev.com/ee261f306c3c66e96339aa1026d62a6d953302fe
Cr-Commit-Position: refs/heads/master@{#356394}

[asanka, 2015-10-19 23:54:26]

Description was changed from

==========
Block dangerous unchecked downloads based on a Finch trial.

BUG=
==========

to

==========
Block dangerous unchecked downloads based on a Finch trial.

BUG=533579
==========

[asanka, 2015-10-20 01:11:00]

Patchset #1 (id:1) has been deleted

[asanka, 2015-10-20 01:11:11]

Patchset #1 (id:20001) has been deleted

[asanka, 2015-10-20 01:11:17]

Patchset #1 (id:40001) has been deleted

[asanka, 2015-10-20 01:11:26]

Patchset #1 (id:60001) has been deleted

[asanka, 2015-10-20 01:12:26]

asanka@chromium.org changed reviewers:
+ bbudge@chromium.org, isherman@chromium.org, mattm@chromium.org,
sky@chromium.org

[asanka, 2015-10-20 01:12:40]

asanka@chromium.org changed reviewers:
- isherman@chromium.org

[asanka, 2015-10-20 01:25:58]

bbudge:
  ppapi/**
  chrome/test/ppapi/ppapi_browsertest.cc

    I'm a bit on the fence about the newly added tests. Let me know if you
prefer that they be moved out to a separate browsertest instead of being added
to ppapi_browsertest.cc.

sky:
  chrome/browser/file_select_helper.cc
  chrome/test/ppapi/ppapi_browsertest.cc (OWNERS)

    I'm using a custom ui::SelectFileDialogFactory for the browser test. The
SelectFileDialog::SetFactory() method has a comment that's discouraging its use.
However, I didn't find a good alternative that would exercise the code path
without requiring manual interaction.

mattm:
  chrome/browser/safe_browsing/**

The PPAPI FileChooser_Trusted API allows a plugin to download a file without
being subject to SafeBrowsing checks. Nor does the API adequately sanitize the
plugin supplied filename.

This CL is a temporary measure while full SafeBrowsing support is plumbed into
this API. This CL adds basic filename sanitization, and a Finch trial based
throttle that can be used to curb abuse of this API. It also adds UMA to measure
how this API is used in the wild. The intention is for the changes in this CL to
be viable for merging into current release branches.

[sky, 2015-10-21 00:02:52]

What is an 'unchecked' download and how is it triggered?

[mattm, 2015-10-21 00:26:05]

lgtm

https://codereview.chromium.org/1409003002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/unverified_download_field_trial_unittest.cc
(right):

https://codereview.chromium.org/1409003002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/unverified_download_field_trial_unittest.cc:28:
base::FilePath(kHandledFilename)));
also verify the kSafeFilename is not a supported binary file?

https://codereview.chromium.org/1409003002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/unverified_download_field_trial_unittest.cc:88:
param_map[kUnverifiedDownloadFieldTrialWhitelistParam] = ".xyz";
Also test that you can override a SB type by whitelisting it

[asanka, 2015-10-21 04:17:40]

On 2015/10/21 at 00:02:52, sky wrote:
> What is an 'unchecked' download and how is it triggered?

A plugin using PPAPI and which the right permissions can request a writeable
file via a file chooser. The plugin can then write to the file without having
the written contents be verified via Safe Browsing.

Pepper Flash is such a plugin and in turn this API is available to Flash
applets.

[bbudge, 2015-10-21 21:15:54]

On 2015/10/20 01:25:58, asanka wrote:
> bbudge:
>   ppapi/**
>   chrome/test/ppapi/ppapi_browsertest.cc
> 
>     I'm a bit on the fence about the newly added tests. Let me know if you
> prefer that they be moved out to a separate browsertest instead of being added
> to ppapi_browsertest.cc.
> 
> sky:
>   chrome/browser/file_select_helper.cc
>   chrome/test/ppapi/ppapi_browsertest.cc (OWNERS)
> 
>     I'm using a custom ui::SelectFileDialogFactory for the browser test. The
> SelectFileDialog::SetFactory() method has a comment that's discouraging its
use.
> However, I didn't find a good alternative that would exercise the code path
> without requiring manual interaction.
> 
> mattm:
>   chrome/browser/safe_browsing/**
> 
> The PPAPI FileChooser_Trusted API allows a plugin to download a file without
> being subject to SafeBrowsing checks. Nor does the API adequately sanitize the
> plugin supplied filename.
> 
> This CL is a temporary measure while full SafeBrowsing support is plumbed into
> this API. This CL adds basic filename sanitization, and a Finch trial based
> throttle that can be used to curb abuse of this API. It also adds UMA to
measure
> how this API is used in the wild. The intention is for the changes in this CL
to
> be viable for merging into current release branches.

All that FIle Chooser test code does seem out of place in ppapi_browsertest.cc.
Are the tests also a "temporary measure"? If so it might be acceptable for a
while, with a TODO to remove or redo them. Otherwise, it seems better to move
these somewhere else.

[bbudge, 2015-10-21 21:16:04]

https://codereview.chromium.org/1409003002/diff/80001/ppapi/tests/test_file_c...
File ppapi/tests/test_file_chooser.cc (right):

https://codereview.chromium.org/1409003002/diff/80001/ppapi/tests/test_file_c...
ppapi/tests/test_file_chooser.cc:17: return true;
The other PPB_File* tests Init like this:

bool TestFileRef::Init() {
  return CheckTestingInterface() && EnsureRunningOverHTTP();
}

https://codereview.chromium.org/1409003002/diff/80001/ppapi/tests/test_file_c...
ppapi/tests/test_file_chooser.cc:20: bool
TestFileChooser::WriteDefaultContentsToFile(const pp::FileRef& file_ref) {
nit: Move this below RunTests method to match header.

[sky, 2015-10-21 22:46:20]

https://codereview.chromium.org/1409003002/diff/80001/chrome/browser/file_sel...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/1409003002/diff/80001/chrome/browser/file_sel...
chrome/browser/file_select_helper.cc:442:
DCHECK_IMPLIES(!params.default_file_name.empty(),
Please don't use DCHECK_IMPLIES. Yes, it is in base/logging, but it hurts
readability. I hadn't a clue what it means, which likely goes for most of the
team. Worse yet, looking at the definition I have to map it to:

DCHECK_IMPLIES(!!params.default_file_name.empty()

If you had instead used:

DCHECK(params.default_file_name.empty() || ...)

I would immediately understand what you're checking.

https://codereview.chromium.org/1409003002/diff/80001/chrome/browser/file_sel...
chrome/browser/file_select_helper.cc:456: // The parameters contain potentially
unsafe filenames provided by the
I think you should move this to a standalone function that is easy to test
without having to go through a browser test.

[asanka, 2015-10-22 21:55:12]

Thanks for the reviews!

bbudge: I moved the browser tests to ppapi_filechooser_browsertest.cc since the
tests involve special setup and fixtures. The tests that specifically mention
Safe Browsing are the only ones that are temporary and will be replaced when SB
is integrated into this code path. I didn't see general end-to-end tests for the
FileChooser_Trusted API which is why I added the other tests (mostly to act as
smoke tests to catch me if I break something). Is there a better place to keep
those or are they redundant with any end-to-end tests that I'm unaware of?

https://codereview.chromium.org/1409003002/diff/80001/chrome/browser/file_sel...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/1409003002/diff/80001/chrome/browser/file_sel...
chrome/browser/file_select_helper.cc:442:
DCHECK_IMPLIES(!params.default_file_name.empty(),
On 2015/10/21 at 22:46:19, sky wrote:
> Please don't use DCHECK_IMPLIES. Yes, it is in base/logging, but it hurts
readability. I hadn't a clue what it means, which likely goes for most of the
team. Worse yet, looking at the definition I have to map it to:
> 
> DCHECK_IMPLIES(!!params.default_file_name.empty()
> 
> If you had instead used:
> 
> DCHECK(params.default_file_name.empty() || ...)
> 
> I would immediately understand what you're checking.

Done :), and point taken.

https://codereview.chromium.org/1409003002/diff/80001/chrome/browser/file_sel...
chrome/browser/file_select_helper.cc:456: // The parameters contain potentially
unsafe filenames provided by the
On 2015/10/21 at 22:46:20, sky wrote:
> I think you should move this to a standalone function that is easy to test
without having to go through a browser test.

Done and added a test to FileSelectHelpTest unit test to verify the aspects of
sanitization that's relevant to this class. However, I left one browser_test in
place that has an invalid filename because I wanted something to verify that the
sanitization method is called appropriately.

https://codereview.chromium.org/1409003002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/unverified_download_field_trial_unittest.cc
(right):

https://codereview.chromium.org/1409003002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/unverified_download_field_trial_unittest.cc:28:
base::FilePath(kHandledFilename)));
On 2015/10/21 at 00:26:05, mattm wrote:
> also verify the kSafeFilename is not a supported binary file?

Done.

https://codereview.chromium.org/1409003002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/unverified_download_field_trial_unittest.cc:88:
param_map[kUnverifiedDownloadFieldTrialWhitelistParam] = ".xyz";
On 2015/10/21 at 00:26:05, mattm wrote:
> Also test that you can override a SB type by whitelisting it

Done and good point.

https://codereview.chromium.org/1409003002/diff/80001/ppapi/tests/test_file_c...
File ppapi/tests/test_file_chooser.cc (right):

https://codereview.chromium.org/1409003002/diff/80001/ppapi/tests/test_file_c...
ppapi/tests/test_file_chooser.cc:17: return true;
On 2015/10/21 at 21:16:04, bbudge wrote:
> The other PPB_File* tests Init like this:
> 
> bool TestFileRef::Init() {
>   return CheckTestingInterface() && EnsureRunningOverHTTP();
> }

Done.

https://codereview.chromium.org/1409003002/diff/80001/ppapi/tests/test_file_c...
ppapi/tests/test_file_chooser.cc:20: bool
TestFileChooser::WriteDefaultContentsToFile(const pp::FileRef& file_ref) {
On 2015/10/21 at 21:16:04, bbudge wrote:
> nit: Move this below RunTests method to match header.

Done.

[bbudge, 2015-10-22 22:35:51]

ppapi and chrome/test/ppapi LGTM.
Thanks for adding these tests!

https://codereview.chromium.org/1409003002/diff/120001/chrome/test/ppapi/ppap...
File chrome/test/ppapi/ppapi_filechooser_browsertest.cc (right):

https://codereview.chromium.org/1409003002/diff/120001/chrome/test/ppapi/ppap...
chrome/test/ppapi/ppapi_filechooser_browsertest.cc:17: // FileChooser tests.
nit: comment is superfluous

https://codereview.chromium.org/1409003002/diff/120001/chrome/test/ppapi/ppap...
chrome/test/ppapi/ppapi_filechooser_browsertest.cc:216: // that files written
via the FileChooser_Trusted API are properly threaded
nit: the word 'threaded' seems slightly confusing here. How about 'passed' or
'filtered'?

https://codereview.chromium.org/1409003002/diff/120001/ppapi/tests/test_file_...
File ppapi/tests/test_file_chooser.cc (right):

https://codereview.chromium.org/1409003002/diff/120001/ppapi/tests/test_file_...
ppapi/tests/test_file_chooser.cc:79: // for simulating the cancellation can be
found at ppapi_browsertest.cc
nit: update file name.

https://codereview.chromium.org/1409003002/diff/120001/ppapi/tests/test_file_...
File ppapi/tests/test_file_chooser.h (right):

https://codereview.chromium.org/1409003002/diff/120001/ppapi/tests/test_file_...
ppapi/tests/test_file_chooser.h:24: static const size_t
kTestFileContentsSizeBytes;
Is there a reason these are declared in the header rather than in an anonymous
namespace in the .cc file?

[sky, 2015-10-22 23:17:28]

LGTM - I did not review chrome/test/ppapi/ppapi_filechooser_browsertest.cc as
Bill is an owner of that directory, and likely actually understands the code:)

[asanka, 2015-10-23 19:32:58]

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

[asanka, 2015-10-23 19:35:03]

Thanks everyone!

isherman: Could you take a look at histograms.xml?

https://codereview.chromium.org/1409003002/diff/120001/chrome/test/ppapi/ppap...
File chrome/test/ppapi/ppapi_filechooser_browsertest.cc (right):

https://codereview.chromium.org/1409003002/diff/120001/chrome/test/ppapi/ppap...
chrome/test/ppapi/ppapi_filechooser_browsertest.cc:17: // FileChooser tests.
On 2015/10/22 at 22:35:51, bbudge wrote:
> nit: comment is superfluous

Ha. yes :) removed

https://codereview.chromium.org/1409003002/diff/120001/chrome/test/ppapi/ppap...
chrome/test/ppapi/ppapi_filechooser_browsertest.cc:216: // that files written
via the FileChooser_Trusted API are properly threaded
On 2015/10/22 at 22:35:51, bbudge wrote:
> nit: the word 'threaded' seems slightly confusing here. How about 'passed' or
'filtered'?

Went with "passed"

https://codereview.chromium.org/1409003002/diff/120001/ppapi/tests/test_file_...
File ppapi/tests/test_file_chooser.cc (right):

https://codereview.chromium.org/1409003002/diff/120001/ppapi/tests/test_file_...
ppapi/tests/test_file_chooser.cc:79: // for simulating the cancellation can be
found at ppapi_browsertest.cc
On 2015/10/22 at 22:35:51, bbudge wrote:
> nit: update file name.

Done.

https://codereview.chromium.org/1409003002/diff/120001/ppapi/tests/test_file_...
File ppapi/tests/test_file_chooser.h (right):

https://codereview.chromium.org/1409003002/diff/120001/ppapi/tests/test_file_...
ppapi/tests/test_file_chooser.h:24: static const size_t
kTestFileContentsSizeBytes;
On 2015/10/22 at 22:35:51, bbudge wrote:
> Is there a reason these are declared in the header rather than in an anonymous
namespace in the .cc file?

None. Moved to anonymous namespace in .cc file.

[Ilya Sherman, 2015-10-23 20:04:42]

https://codereview.chromium.org/1409003002/diff/140001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/unverified_download_policy.cc (right):

https://codereview.chromium.org/1409003002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/unverified_download_policy.cc:26:
download_protection_util::kSBClientDownloadExtensionsMax);
Could you use UMA_HISTOGRAM_SPARSE_SLOWLY for these histograms, rather than a
dense histogram representation?  I'm guessing that most of the 100+ buckets are
not hit in any given run of Chrome.

(For bonus points, it would be awesome if you followed up with a CL that updates
any existing macro invocations, where safe to do so.)

[asanka, 2015-10-26 14:50:52]

https://codereview.chromium.org/1409003002/diff/140001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/unverified_download_policy.cc (right):

https://codereview.chromium.org/1409003002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/unverified_download_policy.cc:26:
download_protection_util::kSBClientDownloadExtensionsMax);
On 2015/10/23 at 20:04:42, Ilya Sherman wrote:
> Could you use UMA_HISTOGRAM_SPARSE_SLOWLY for these histograms, rather than a
dense histogram representation?  I'm guessing that most of the 100+ buckets are
not hit in any given run of Chrome.
> 
> (For bonus points, it would be awesome if you followed up with a CL that
updates any existing macro invocations, where safe to do so.)

Done. You're correct that these enumerations are not hit very frequently and it
makes sense to use a sparse histogram.

TBH, I didn't fully realize the difference between LinearHistogram and
SparseHistogram until just now when I looked through them. I'll upload a CL
shortly to switch over the user histograms that use the
SBClientDownloadExtensions.

[Ilya Sherman, 2015-10-26 18:43:51]

LGTM, thanks.

[asanka, 2015-10-27 14:06:42]

Description was changed from

==========
Block dangerous unchecked downloads based on a Finch trial.

BUG=533579
==========

to

==========
[SafeBrowsing] Block dangerous unchecked downloads based on a Finch trial.

BUG=533579
==========

[asanka, 2015-10-27 14:07:06]

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

[commit-bot: I haz the power, 2015-10-27 14:07:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1409003002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1409003002/180001

[commit-bot: I haz the power, 2015-10-27 14:14:05]

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

[commit-bot: I haz the power, 2015-10-27 14:14:06]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[asanka, 2015-10-27 18:37:49]

The CQ bit was checked by asanka@chromium.org

[asanka, 2015-10-27 18:37:49]

The patchset sent to the CQ was uploaded after l-g-t-m from mattm@chromium.org,
bbudge@chromium.org, sky@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1409003002/#ps240001
(title: " ")

[commit-bot: I haz the power, 2015-10-27 18:38:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1409003002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1409003002/240001

[commit-bot: I haz the power, 2015-10-27 19:35:04]

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

[commit-bot: I haz the power, 2015-10-27 19:35:05]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[asanka, 2015-10-27 19:47:08]

The CQ bit was checked by asanka@chromium.org

[commit-bot: I haz the power, 2015-10-27 19:47:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1409003002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1409003002/240001

[commit-bot: I haz the power, 2015-10-27 21:04:36]

Committed patchset #9 (id:240001)

[commit-bot: I haz the power, 2015-10-27 21:06:11]

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