[SafeBrowsing] Send pings for Zip files that contain other archives.

[SafeBrowsing] Send pings for Zip files that contain other archives.

BUG=515216
Committed: https://crrev.com/04a4eb914ba76b0a1ee8b776bcf53a429ba45fe2
Cr-Commit-Position: refs/heads/master@{#341475}

[asanka, 2015-07-29 19:24:19]

asanka@chromium.org changed reviewers:
+ mattm@chromium.org, moheeb@chromium.org

[asanka, 2015-07-29 19:24:20]

asanka@chromium.org changed required reviewers:
+ mattm@chromium.org

[asanka, 2015-07-29 21:41:43]

PTAL?

[mattm, 2015-07-29 22:53:26]

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

https://codereview.chromium.org/1262753002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service.cc:789: if
(zipped_executable_)
should archived_binary be set for zipped_archive too? Maybe instead of checking
zipped_executable_ here should just checked !archived_binary_.empty() ?    (Then
you could get rid of the zipped_executable_ and zipped_archive_ member variables
too.)

https://codereview.chromium.org/1262753002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/download_protection_service_unittest.cc
(right):

https://codereview.chromium.org/1262753002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service_unittest.cc:1081:
EXPECT_EQ(ClientDownloadRequest_DownloadType_WIN_EXECUTABLE,
Seems like this case should also be checking request->download_type() ==
ZIPPED_EXECUTABLE.

https://codereview.chromium.org/1262753002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service_unittest.cc:1122:
ASSERT_TRUE(zip::Zip(zip_source_dir.path(), a_tmp, false));
Isn't this creating a zip with file.txt, file.exe, and file.rar? Shouldn't the
result below still be ZIPPED_EXECUTABLE since it contains both an exe and a rar
and the exe takes precedence?

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

https://codereview.chromium.org/1262753002/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/download_protection_util.cc:95: // change to
ZIPPED_ARCHIVE based on the zip analyzer.
still a little confusing.. maybe "we initially set the ZIPPED_EXECUTABLE type,
although the type may change to ZIPPED_ARCHIVE before sending based on the zip
analyzer

[asanka, 2015-07-31 01:04:42]

PTAL?

Of the archive types, we only consider .zip to be a supported type. However if a
zip file contains any of the other archive types we consider it a ZIPPED_ARCHIVE
(except when they also contain binaries).

I expanded the list of archive types, though I'm a bit iffy about the inclusion
of .gz files because of the potential false positive rate.

https://codereview.chromium.org/1262753002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/download_protection_service_unittest.cc
(right):

https://codereview.chromium.org/1262753002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service_unittest.cc:1122:
ASSERT_TRUE(zip::Zip(zip_source_dir.path(), a_tmp, false));
On 2015/07/29 at 22:53:25, mattm wrote:
> Isn't this creating a zip with file.txt, file.exe, and file.rar? Shouldn't the
result below still be ZIPPED_EXECUTABLE since it contains both an exe and a rar
and the exe takes precedence?

Indeed. Fixed by testing both with file.exe and without.

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

https://codereview.chromium.org/1262753002/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/download_protection_util.cc:95: // change to
ZIPPED_ARCHIVE based on the zip analyzer.
On 2015/07/29 at 22:53:25, mattm wrote:
> still a little confusing.. maybe "we initially set the ZIPPED_EXECUTABLE type,
although the type may change to ZIPPED_ARCHIVE before sending based on the zip
analyzer

Reworded.

[asanka, 2015-07-31 01:06:34]

https://codereview.chromium.org/1262753002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/download_protection_service_unittest.cc
(right):

https://codereview.chromium.org/1262753002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service_unittest.cc:1081:
EXPECT_EQ(ClientDownloadRequest_DownloadType_WIN_EXECUTABLE,
On 2015/07/29 at 22:53:25, mattm wrote:
> Seems like this case should also be checking request->download_type() ==
ZIPPED_EXECUTABLE.

This is looking at the download type of the binary inside the archive. That's
going to be WIN_EXECUTABLE. The download type for the outer zip file is
ZIPPED_EXECUTABLE.

[mattm, 2015-07-31 18:40:10]

lgtm

[asanka, 2015-07-31 20:34:50]

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

[asanka, 2015-07-31 20:34:50]

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

[asanka, 2015-07-31 20:35:25]

mattm: Thanks!

Could you take a look?

+palmer:
  chrome/common/chrome_utility_messages.h
+asvitkine:
  tools/metrics/histograms/histograms.xml

[Alexei Svitkine (slow), 2015-07-31 20:49:36]

lgtm

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

https://codereview.chromium.org/1262753002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/sandboxed_zip_analyzer_unittest.cc:171: {
Nit: Not sure if this is the correct indent format for this scenario. Maybe wrap
line the ones above?

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

https://codereview.chromium.org/1262753002/diff/80001/chrome/common/safe_brow...
chrome/common/safe_browsing/download_protection_util.cc:36: for (const auto&
extension : kArchiveFileTypes)
Nit: {}

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

https://codereview.chromium.org/1262753002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:36800: +    downloaded ZIP file.Each
archive file type in a single ZIP file is recorded
Nit: Space after .

[asanka, 2015-07-31 21:13:03]

asvitkine: Thanks!

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

https://codereview.chromium.org/1262753002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/sandboxed_zip_analyzer_unittest.cc:171: {
On 2015/07/31 at 20:49:36, Alexei Svitkine wrote:
> Nit: Not sure if this is the correct indent format for this scenario. Maybe
wrap line the ones above?

It's a bit odd, but that's what clang-format gives us. :-/

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

https://codereview.chromium.org/1262753002/diff/80001/chrome/common/safe_brow...
chrome/common/safe_browsing/download_protection_util.cc:36: for (const auto&
extension : kArchiveFileTypes)
On 2015/07/31 at 20:49:36, Alexei Svitkine wrote:
> Nit: {}

Done.

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

https://codereview.chromium.org/1262753002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:36800: +    downloaded ZIP file.Each
archive file type in a single ZIP file is recorded
On 2015/07/31 at 20:49:36, Alexei Svitkine wrote:
> Nit: Space after .

Done.

[palmer, 2015-07-31 22:48:18]

IPC security LGTM.

https://codereview.chromium.org/1262753002/diff/100001/chrome/common/safe_bro...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/1262753002/diff/100001/chrome/common/safe_bro...
chrome/common/safe_browsing/csd.proto:249: ZIPPED_ARCHIVE = 5;  // .zip file
containing another archive.
|NESTED_ARCHIVE| is more explanatory, IMO.

https://codereview.chromium.org/1262753002/diff/100001/chrome/common/safe_bro...
File chrome/common/safe_browsing/download_protection_util.cc (right):

https://codereview.chromium.org/1262753002/diff/100001/chrome/common/safe_bro...
chrome/common/safe_browsing/download_protection_util.cc:40: // TODO(mattm):
should .dmg be checked here instead of IsSupportedBinaryFile?
Yes, I think so. rsesek@ wrote a handler for it; you might want to talk to him.

[asanka, 2015-08-02 03:43:05]

The CQ bit was checked by asanka@chromium.org

[asanka, 2015-08-02 03:43:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, mattm@chromium.org, palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1262753002/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-08-02 03:43:11]

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

[asanka, 2015-08-02 03:43:32]

https://codereview.chromium.org/1262753002/diff/100001/chrome/common/safe_bro...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/1262753002/diff/100001/chrome/common/safe_bro...
chrome/common/safe_browsing/csd.proto:249: ZIPPED_ARCHIVE = 5;  // .zip file
containing another archive.
On 2015/07/31 at 22:48:18, palmer wrote:
> |NESTED_ARCHIVE| is more explanatory, IMO.

We are currently looking just at zip files, but I concede that even then
|NESTED_ARCHIVE| would've been a better name. I'm going to leave it as is
because it reflects the meanings and names used internally by the SafeBrowsing
service. However, in the future we should consider generalizing these to make
them less .zip specific.

https://codereview.chromium.org/1262753002/diff/100001/chrome/common/safe_bro...
File chrome/common/safe_browsing/download_protection_util.cc (right):

https://codereview.chromium.org/1262753002/diff/100001/chrome/common/safe_bro...
chrome/common/safe_browsing/download_protection_util.cc:40: // TODO(mattm):
should .dmg be checked here instead of IsSupportedBinaryFile?
On 2015/07/31 at 22:48:18, palmer wrote:
> Yes, I think so. rsesek@ wrote a handler for it; you might want to talk to
him.

Cool. Thanks. Noted and I'll send a note to the SB folks to follow up.

[commit-bot: I haz the power, 2015-08-02 04:38:07]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-08-02 04:38:59]

Patchset 8 (id:??) landed as
https://crrev.com/04a4eb914ba76b0a1ee8b776bcf53a429ba45fe2
Cr-Commit-Position: refs/heads/master@{#341475}