[SafeBrowsing] Send pingbacks for additional file types.

[SafeBrowsing] Send pingbacks for additional file types.

This change expands the list of file types supported by Safe Browsing.

Downloads of these files will now behave in a manner similar to .exe
file downloads. I.e. there will be no dangerous file warning at the
start of the download. Instead, the download will run to completion
and the user will be prompted based on the results of the Safe
Browsing query.

BUG=507821
Committed: https://crrev.com/d55999aedb284074bb86700c3e04d2abd5a5023b
Cr-Commit-Position: refs/heads/master@{#337986}

[asanka, 2015-07-08 00:22:02]

asanka@chromium.org changed reviewers:
+ asvitkine@chromium.org, shess@chromium.org

[asanka, 2015-07-08 00:22:02]

asanka@chromium.org changed required reviewers:
+ asvitkine@chromium.org, shess@chromium.org

[asanka, 2015-07-08 00:23:29]

PTAL?

shess:
  chrome/browser/safe_browsing/download_protection_service.cc
  chrome/common/safe_browsing/download_protection_util.cc

asvitkine:
  tools/metrics/histograms/histograms.xml

[Scott Hess - ex-Googler, 2015-07-08 04:27:52]

shess@chromium.org changed reviewers:
+ nparker@chromium.org

[Scott Hess - ex-Googler, 2015-07-08 04:27:52]

I'm trying to avoid further safe-browsing work, so let's loop in Nathan and if
he's happy I'll rubber-stamp as owner.

[Actually, I'm OOT for a week starting Friday, so you have my permission to TBR
me once Nathan is happy.]

[moheeb1, 2015-07-08 10:22:53]

On 2015/07/08 00:23:29, asanka wrote:
> PTAL?
> 
> shess:
>   chrome/browser/safe_browsing/download_protection_service.cc
>   chrome/common/safe_browsing/download_protection_util.cc
> 
> asvitkine:
>   tools/metrics/histograms/histograms.xml


Are planning on adding other file extensions? it wold be great if we can cover
most file extensions that may result code execution. Maybe a good starting point
would be to add UMA histograms covering the larger list of these extensions.
thanks.

[grt (UTC plus 2), 2015-07-08 11:55:15]

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

[grt (UTC plus 2), 2015-07-08 11:55:16]

Are these extensions put on PE images to get them past the download protection
service? Should we run all PE images through the service regardless of the
filename/extension?

[Alexei Svitkine (slow), 2015-07-08 14:56:08]

histograms lgtm, some optional suggestions about the code

https://codereview.chromium.org/1230483005/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/1230483005/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:103: if
(f.MatchesExtension(FILE_PATH_LITERAL(".apk"))) return EXTENSION_APK;
Optional:

Would it make sense to just have a static array of {ext, type} structs that this
loops through instead? I'd imagine it would at least result in smaller machine
code.

https://codereview.chromium.org/1230483005/diff/1/chrome/common/safe_browsing...
File chrome/common/safe_browsing/download_protection_util.cc (right):

https://codereview.chromium.org/1230483005/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/download_protection_util.cc:21:
file.MatchesExtension(FILE_PATH_LITERAL(".bas")) ||
Optional:

I wonder if this function can call GetExtensionType() and use the return type of
that function to do its logic? (e.g. something like != EXTENSION_OTHER)

Or is it more nuanced than that?

[asanka, 2015-07-08 19:43:20]

On 2015/07/08 at 10:22:53, moheeb wrote:
> On 2015/07/08 00:23:29, asanka wrote:
> > PTAL?
> > 
> > shess:
> >   chrome/browser/safe_browsing/download_protection_service.cc
> >   chrome/common/safe_browsing/download_protection_util.cc
> > 
> > asvitkine:
> >   tools/metrics/histograms/histograms.xml
> 
> 
> Are planning on adding other file extensions? it wold be great if we can cover
most file extensions that may result code execution. Maybe a good starting point
would be to add UMA histograms covering the larger list of these extensions.
thanks.

There is UMA for dangerous file types. UMA is logged when the file type triggers
a dangerous or malicious warning. I'll add logging for all cases.

[asanka, 2015-07-08 19:48:37]

On 2015/07/08 at 11:55:16, grt wrote:
> Are these extensions put on PE images to get them past the download protection
service? Should we run all PE images through the service regardless of the
filename/extension?

The .vbe and .jse are encoded scripts. See
https://msdn.microsoft.com/en-us/library/xw61tsx7(v=vs.84).aspx for more info.
Since they are run through the Windows Script Host, they can run with user level
privileges and have access to the file system.

I'll leave the PE question up to the SB folks. In the meantime, I'll make sure
that all PE based file formats are included in the list of reported file types.

[Nathan Parker, 2015-07-08 20:11:08]

Code LGTM.  I'll leave the file-type questions up to SB folks.

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

https://codereview.chromium.org/1230483005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service.cc:104: const int
EXTENSION_OTHER = 18;
Nit: Add comment or make the name indicate that this is an index into the array.

[moheeb1, 2015-07-08 20:25:29]

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

[moheeb1, 2015-07-08 20:25:30]

Thanks for working this Asanka, I have a couple comments otherwise looks good.

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

https://codereview.chromium.org/1230483005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service.cc:101:
FILE_PATH_LITERAL(".mhtml"),
Would be great to expand this list to cover all dangerous file we know about, so
we get a sense of prevalance of these file types.

https://codereview.chromium.org/1230483005/diff/20001/chrome/common/safe_brow...
File chrome/common/safe_browsing/download_protection_util.cc (right):

https://codereview.chromium.org/1230483005/diff/20001/chrome/common/safe_brow...
chrome/common/safe_browsing/download_protection_util.cc:30:
FILE_PATH_LITERAL(".mht"), FILE_PATH_LITERAL(".mhtml"),
could you please also add .wsf  (windows script file format). It would be great
to use this opportunity to expand this list to all possible dangerous
extensions, since the use of these extensions is very dynamic.

[moheeb1, 2015-07-08 20:29:11]

On 2015/07/08 19:48:37, asanka wrote:
> On 2015/07/08 at 11:55:16, grt wrote:
> > Are these extensions put on PE images to get them past the download
protection
> service? Should we run all PE images through the service regardless of the
> filename/extension?
> 
> The .vbe and .jse are encoded scripts. See
> https://msdn.microsoft.com/en-us/library/xw61tsx7(v=vs.84).aspx for more info.
> Since they are run through the Windows Script Host, they can run with user
level
> privileges and have access to the file system.
> 
> I'll leave the PE question up to the SB folks. In the meantime, I'll make sure
> that all PE based file formats are included in the list of reported file
types.


I would agree, let's make sure that we are covering all PE extensions in the
current list. 
Separately we can perhaps think through scenarios that would make it possible to
have a PE that's not covered by the PE extensions list and add the PE image
analysis.

[asanka, 2015-07-08 21:50:01]

OK. Added a bunch more file types. Also tested that the histograms are being
reported correctly.

Another reminder that this changes the behavior for these files and I'm still a
bit concerned that we'd be able to communicate the threat level appropriately to
the user based on the SB response.

Thanks for the reviews so far!

shess: Would you mind stamping?

[Scott Hess - ex-Googler, 2015-07-08 21:50:55]

lgtm

[asanka, 2015-07-09 03:37:20]

The CQ bit was checked by asanka@chromium.org

[asanka, 2015-07-09 03:37:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/1230483005/#ps60001
(title: "Also .dll")

[commit-bot: I haz the power, 2015-07-09 03:38:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230483005/60001

[commit-bot: I haz the power, 2015-07-09 04:43:12]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-09 04:44:02]

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