[SafeBrowsing] Alternate extensions should also be subject to block list.

[SafeBrowsing] Alternate extensions should also be subject to block list.

FileSelectHelper subjects suggested filenames to the safe browsing
white/black lists prior to showing the file picker. However, if the list
of accepted file types passed in to the file picker is not empty, and
the suggested filename doesn't match the accepted file types, the file
picker may append an accpeted file extension to the suggested filename.

In order to prevent this from being abused, this CL causes
FileSelectHelper to also verify that the list of accepted file types is
also safe.

R=sky,nparker,bbudge,holte
BUG=575849
Committed: https://crrev.com/78ff0a5433b1c9f6fd2e3cddfc7e9cb28d809853
Cr-Commit-Position: refs/heads/master@{#371997}

[asanka, 2016-01-21 01:17:11]

Description was changed from

==========
[SafeBrowsing] Alternate extensions should also be subject to block list.

BUG=575849
==========

to

==========
[SafeBrowsing] Alternate extensions should also be subject to block list.

BUG=575849
==========

[asanka, 2016-01-21 01:17:11]

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

[asanka, 2016-01-21 01:21:11]

Description was changed from

==========
[SafeBrowsing] Alternate extensions should also be subject to block list.

BUG=575849
==========

to

==========
[SafeBrowsing] Alternate extensions should also be subject to block list.

FileSelectHelper subjects suggested filenames to the safe browsing
white/black lists prior to showing the file picker. However, if the list
of accepted file types passed in to the file picker is not empty, and
the suggested filename doesn't match the accepted file types, the file
picker may append an accpeted file extension to the suggested filename.

In order to prevent this from being abused, this CL causes
FileSelectHelper to also verify that the list of accepted file types is
also safe.

R=sky,nparker,bbudge
BUG=575849
==========

[asanka, 2016-01-21 01:22:19]

PTAL?

nparker:
  chrome/browser/safe_browsing/unverified_download_field_trial.cc
  chrome/browser/safe_browsing/unverified_download_policy.cc
  chrome/browser/safe_browsing/unverified_download_policy.h
  chrome/browser/safe_browsing/unverified_download_policy_unittest.cc

bbudge:
  chrome/test/ppapi/ppapi_filechooser_browsertest.cc
  ppapi/tests/test_file_chooser.cc
  ppapi/tests/test_file_chooser.h

sky:
  chrome/browser/file_select_helper.cc

[Nathan Parker, 2016-01-21 01:32:27]

lgtm

for safe_browsing/

https://codereview.chromium.org/1613483003/diff/1/chrome/browser/file_select_...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/1613483003/diff/1/chrome/browser/file_select_...
chrome/browser/file_select_helper.cc:445: select_file_types_ =
GetFileTypesFromAcceptType(params->accept_types);
Do we know how often this contains multiple items?  It might be good to have an
UMA metric (could be in CheckUnverifiedDownloadPolicy) to know how often we're
expanding the list.  I'm thinking, if it often tacks on .xml or something, and
we end up blocking every file type.

[asanka, 2016-01-21 01:37:10]

https://codereview.chromium.org/1613483003/diff/1/chrome/browser/file_select_...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/1613483003/diff/1/chrome/browser/file_select_...
chrome/browser/file_select_helper.cc:445: select_file_types_ =
GetFileTypesFromAcceptType(params->accept_types);
On 2016/01/21 01:32:26, nparker wrote:
> Do we know how often this contains multiple items?  It might be good to have
an
> UMA metric (could be in CheckUnverifiedDownloadPolicy) to know how often we're
> expanding the list.  I'm thinking, if it often tacks on .xml or something, and
> we end up blocking every file type.

We don't have data for how often we get multiple file types. I've modified the
code in CheckUnverifiedDownloadPolicy to report the file type that caused the
request to be disallowed instead of the file type of the suggested file name. I
can also add a measurement for the number of alternate extensions we see if you
think this isn't enough.

[Nathan Parker, 2016-01-21 17:11:15]

https://codereview.chromium.org/1613483003/diff/1/chrome/browser/file_select_...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/1613483003/diff/1/chrome/browser/file_select_...
chrome/browser/file_select_helper.cc:445: select_file_types_ =
GetFileTypesFromAcceptType(params->accept_types);
On 2016/01/21 01:37:10, asanka wrote:
> On 2016/01/21 01:32:26, nparker wrote:
> > Do we know how often this contains multiple items?  It might be good to have
> an
> > UMA metric (could be in CheckUnverifiedDownloadPolicy) to know how often
we're
> > expanding the list.  I'm thinking, if it often tacks on .xml or something,
and
> > we end up blocking every file type.
> 
> We don't have data for how often we get multiple file types. I've modified the
> code in CheckUnverifiedDownloadPolicy to report the file type that caused the
> request to be disallowed instead of the file type of the suggested file name.
I
> can also add a measurement for the number of alternate extensions we see if
you
> think this isn't enough.

Yea to gauge the order of magnitude of how often this fires, I think we should
log a histogram of number of entries in the select_file_types_.  I think the
logging of _blocked_ filetype is the right thing.

[bbudge, 2016-01-21 17:58:35]

Pepper LGTM

[sky, 2016-01-21 20:13:15]

https://codereview.chromium.org/1613483003/diff/1/chrome/browser/file_select_...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/1613483003/diff/1/chrome/browser/file_select_...
chrome/browser/file_select_helper.cc:462: for (const auto& extensions :
select_file_types_->extensions) {
Isn't this the same as alternate_extensions = select_file_types_->extensions ?

[asanka, 2016-01-21 20:25:35]

https://codereview.chromium.org/1613483003/diff/1/chrome/browser/file_select_...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/1613483003/diff/1/chrome/browser/file_select_...
chrome/browser/file_select_helper.cc:462: for (const auto& extensions :
select_file_types_->extensions) {
On 2016/01/21 20:13:15, sky wrote:
> Isn't this the same as alternate_extensions = select_file_types_->extensions ?

select_file_types_->extensions is a vector<vector<FilePath::StringType >>.

This loop is flattening the list into a vector<FilePath::StringType> by
concatenating all the vector<FilePath::StringType> elements in
select_file_type_->extensions.

[sky, 2016-01-21 20:40:25]

My mistake, LGTM

[asanka, 2016-01-21 21:39:26]

Description was changed from

==========
[SafeBrowsing] Alternate extensions should also be subject to block list.

FileSelectHelper subjects suggested filenames to the safe browsing
white/black lists prior to showing the file picker. However, if the list
of accepted file types passed in to the file picker is not empty, and
the suggested filename doesn't match the accepted file types, the file
picker may append an accpeted file extension to the suggested filename.

In order to prevent this from being abused, this CL causes
FileSelectHelper to also verify that the list of accepted file types is
also safe.

R=sky,nparker,bbudge
BUG=575849
==========

to

==========
[SafeBrowsing] Alternate extensions should also be subject to block list.

FileSelectHelper subjects suggested filenames to the safe browsing
white/black lists prior to showing the file picker. However, if the list
of accepted file types passed in to the file picker is not empty, and
the suggested filename doesn't match the accepted file types, the file
picker may append an accpeted file extension to the suggested filename.

In order to prevent this from being abused, this CL causes
FileSelectHelper to also verify that the list of accepted file types is
also safe.

R=sky,nparker,bbudge,holte
BUG=575849
==========

[asanka, 2016-01-21 21:39:58]

Description was changed from

==========
[SafeBrowsing] Alternate extensions should also be subject to block list.

FileSelectHelper subjects suggested filenames to the safe browsing
white/black lists prior to showing the file picker. However, if the list
of accepted file types passed in to the file picker is not empty, and
the suggested filename doesn't match the accepted file types, the file
picker may append an accpeted file extension to the suggested filename.

In order to prevent this from being abused, this CL causes
FileSelectHelper to also verify that the list of accepted file types is
also safe.

R=sky,nparker,bbudge,holte
BUG=575849
==========

to

==========
[SafeBrowsing] Alternate extensions should also be subject to block list.

FileSelectHelper subjects suggested filenames to the safe browsing
white/black lists prior to showing the file picker. However, if the list
of accepted file types passed in to the file picker is not empty, and
the suggested filename doesn't match the accepted file types, the file
picker may append an accpeted file extension to the suggested filename.

In order to prevent this from being abused, this CL causes
FileSelectHelper to also verify that the list of accepted file types is
also safe.

R=sky,nparker,bbudge,holte
BUG=575849
==========

[asanka, 2016-01-21 21:39:58]

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

[asanka, 2016-01-21 21:44:10]

sky, bbudge: Thanks!

nparker: I added a measurement for the size of the alternate extension list.
It's measuring size + 1 over the range 1..30. I picked the number 30 arbitrarily
based on the belief that anything around and over that limit is in the
"excessive" category and we don't really care about the exact number.

holte: Could you take a look at histograms.xml and the histogram code in
unverified_download_policy.cc?

[Nathan Parker, 2016-01-21 21:47:43]

https://codereview.chromium.org/1613483003/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/unverified_download_policy.cc (right):

https://codereview.chromium.org/1613483003/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/unverified_download_policy.cc:135:
alternate_extensions.size() + 1, 1, 31, 32);
Thanks.
I'm curious: Why +1?  Why not 0-32?

[asanka, 2016-01-21 22:08:47]

https://codereview.chromium.org/1613483003/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/unverified_download_policy.cc (right):

https://codereview.chromium.org/1613483003/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/unverified_download_policy.cc:135:
alternate_extensions.size() + 1, 1, 31, 32);
On 2016/01/21 21:47:43, nparker wrote:
> Thanks.
> I'm curious: Why +1?  Why not 0-32?

0 is the underflow bucket. If the minimum is 0, it's silently rounded up to 1.

I've decided to go with a smaller range with fewer bucket and with a linear
bucket distribution.

[Steven Holte, 2016-01-22 23:50:49]

https://codereview.chromium.org/1613483003/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/unverified_download_policy.cc (right):

https://codereview.chromium.org/1613483003/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/unverified_download_policy.cc:135:
alternate_extensions.size() + 1, 1, 31, 32);
On 2016/01/21 22:08:47, asanka wrote:
> On 2016/01/21 21:47:43, nparker wrote:
> > Thanks.
> > I'm curious: Why +1?  Why not 0-32?
> 
> 0 is the underflow bucket. If the minimum is 0, it's silently rounded up to 1.
> 
> I've decided to go with a smaller range with fewer bucket and with a linear
> bucket distribution.

You don't need the +1.  This histogram will get a 0-1 bucket, and 0 will record
into correctly.  The fact that it's the "underflow" bucket for this histogram
won't cause issues.

[asanka, 2016-01-23 04:42:59]

https://codereview.chromium.org/1613483003/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/unverified_download_policy.cc (right):

https://codereview.chromium.org/1613483003/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/unverified_download_policy.cc:135:
alternate_extensions.size() + 1, 1, 31, 32);
On 2016/01/22 23:50:49, Steven Holte wrote:
> On 2016/01/21 22:08:47, asanka wrote:
> > On 2016/01/21 21:47:43, nparker wrote:
> > > Thanks.
> > > I'm curious: Why +1?  Why not 0-32?
> > 
> > 0 is the underflow bucket. If the minimum is 0, it's silently rounded up to
1.
> > 
> > I've decided to go with a smaller range with fewer bucket and with a linear
> > bucket distribution.
> 
> You don't need the +1.  This histogram will get a 0-1 bucket, and 0 will
record
> into correctly.  The fact that it's the "underflow" bucket for this histogram
> won't cause issues.

Ah. Got it.

I actually switched to a sparse histogram since having a vector of buckets isn't
really necessary given how infrequent these downloads are, and also because
we'll only be recording this once per PPAPI initiated download.

[Steven Holte, 2016-01-25 18:57:29]

https://codereview.chromium.org/1613483003/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/unverified_download_policy.cc (right):

https://codereview.chromium.org/1613483003/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/unverified_download_policy.cc:134:
alternate_extensions.size());
Is there a hard limit somewhere on how large this can be?  The data will be
harder to process if we get many distinct values, so it would be good to add
std::min(some_limit, alternate_extensions.size()) before recording.

[asanka, 2016-01-25 19:53:42]

https://codereview.chromium.org/1613483003/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/unverified_download_policy.cc (right):

https://codereview.chromium.org/1613483003/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/unverified_download_policy.cc:134:
alternate_extensions.size());
On 2016/01/25 18:57:29, Steven Holte wrote:
> Is there a hard limit somewhere on how large this can be?  The data will be
> harder to process if we get many distinct values, so it would be good to add
> std::min(some_limit, alternate_extensions.size()) before recording.

Done. I added a limit of 16.

[Steven Holte, 2016-01-26 22:27:44]

lgtm with summary update

https://codereview.chromium.org/1613483003/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1613483003/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:40641: +    Count of alternate
extensions + 1 that were supplied when attempting to
Remove +1 here

[asanka, 2016-01-28 02:29:06]

The CQ bit was checked by asanka@chromium.org

[asanka, 2016-01-28 02:29:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, bbudge@chromium.org, sky@chromium.org, holte@chromium.org
Link to the patchset: https://codereview.chromium.org/1613483003/#ps100001
(title: "Fix comment in histograms.xml")

[asanka, 2016-01-28 02:29:22]

Thanks!

https://codereview.chromium.org/1613483003/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1613483003/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:40641: +    Count of alternate
extensions + 1 that were supplied when attempting to
On 2016/01/26 22:27:44, Steven Holte wrote:
> Remove +1 here

Done.

[commit-bot: I haz the power, 2016-01-28 02:29:26]

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

[commit-bot: I haz the power, 2016-01-28 03:48:46]

Description was changed from

==========
[SafeBrowsing] Alternate extensions should also be subject to block list.

FileSelectHelper subjects suggested filenames to the safe browsing
white/black lists prior to showing the file picker. However, if the list
of accepted file types passed in to the file picker is not empty, and
the suggested filename doesn't match the accepted file types, the file
picker may append an accpeted file extension to the suggested filename.

In order to prevent this from being abused, this CL causes
FileSelectHelper to also verify that the list of accepted file types is
also safe.

R=sky,nparker,bbudge,holte
BUG=575849
==========

to

==========
[SafeBrowsing] Alternate extensions should also be subject to block list.

FileSelectHelper subjects suggested filenames to the safe browsing
white/black lists prior to showing the file picker. However, if the list
of accepted file types passed in to the file picker is not empty, and
the suggested filename doesn't match the accepted file types, the file
picker may append an accpeted file extension to the suggested filename.

In order to prevent this from being abused, this CL causes
FileSelectHelper to also verify that the list of accepted file types is
also safe.

R=sky,nparker,bbudge,holte
BUG=575849
==========

[commit-bot: I haz the power, 2016-01-28 03:48:47]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-01-28 03:49:48]

Description was changed from

==========
[SafeBrowsing] Alternate extensions should also be subject to block list.

FileSelectHelper subjects suggested filenames to the safe browsing
white/black lists prior to showing the file picker. However, if the list
of accepted file types passed in to the file picker is not empty, and
the suggested filename doesn't match the accepted file types, the file
picker may append an accpeted file extension to the suggested filename.

In order to prevent this from being abused, this CL causes
FileSelectHelper to also verify that the list of accepted file types is
also safe.

R=sky,nparker,bbudge,holte
BUG=575849
==========

to

==========
[SafeBrowsing] Alternate extensions should also be subject to block list.

FileSelectHelper subjects suggested filenames to the safe browsing
white/black lists prior to showing the file picker. However, if the list
of accepted file types passed in to the file picker is not empty, and
the suggested filename doesn't match the accepted file types, the file
picker may append an accpeted file extension to the suggested filename.

In order to prevent this from being abused, this CL causes
FileSelectHelper to also verify that the list of accepted file types is
also safe.

R=sky,nparker,bbudge,holte
BUG=575849

Committed: https://crrev.com/78ff0a5433b1c9f6fd2e3cddfc7e9cb28d809853
Cr-Commit-Position: refs/heads/master@{#371997}
==========

[commit-bot: I haz the power, 2016-01-28 03:49:49]

Patchset 6 (id:??) landed as
https://crrev.com/78ff0a5433b1c9f6fd2e3cddfc7e9cb28d809853
Cr-Commit-Position: refs/heads/master@{#371997}