Include attributes of zipped binaries in ClientDownloadRequests.

Include attributes of zipped binaries in ClientDownloadRequests.

This change give the SandboxedZipAnalyzer the power to extract archived
binaries into a temporary file and extract features from them.

BUG=462584
Committed: https://crrev.com/88f4fa3f1874945019604aa1e88af6ebe61c1c1f
Cr-Commit-Position: refs/heads/master@{#321803}

[grt (UTC plus 2), 2015-03-16 02:37:24]

Patchset #2 (id:20001) has been deleted

[grt (UTC plus 2), 2015-03-20 17:56:13]

Patchset #6 (id:120001) has been deleted

[grt (UTC plus 2), 2015-03-20 17:56:50]

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

[grt (UTC plus 2), 2015-03-20 17:59:53]

PTAL. I have the new binary files needed by the new unittest in
https://codereview.chromium.org/1021323002/ so they can land separately, since I
seem to recall that the CQ chokes on binary files. Patch Set 7 includes those
test files for the sake of try runs. Patch Set 6 lacks them. Thanks.

[grt (UTC plus 2), 2015-03-20 20:51:22]

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

https://codereview.chromium.org/999003003/diff/180001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/sandboxed_zip_analyzer_unittest.cc:100:
ASSERT_TRUE(binary.has_image_headers());
oh yeah. this needs #if defined(OS_WIN) around it.

[mattm, 2015-03-23 01:53:40]

lgtm with nit

https://codereview.chromium.org/999003003/diff/180001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/sandboxed_zip_analyzer.cc (right):

https://codereview.chromium.org/999003003/diff/180001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/sandboxed_zip_analyzer.cc:121: void
SandboxedZipAnalyzer::CloseTemporaryFile() {
nit: I'd move this so that StartProcessOnIOThread and OnUtilityProcessStarted
stay next to each other

[grt (UTC plus 2), 2015-03-23 13:26:20]

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

[grt (UTC plus 2), 2015-03-23 13:27:16]

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

[grt (UTC plus 2), 2015-03-23 13:29:16]

Thanks.

+tsepez for chrome/common/chrome_utility_messages.h OWNERS review
+sky TBR for chrome/utility/*

https://codereview.chromium.org/999003003/diff/180001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/sandboxed_zip_analyzer.cc (right):

https://codereview.chromium.org/999003003/diff/180001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/sandboxed_zip_analyzer.cc:121: void
SandboxedZipAnalyzer::CloseTemporaryFile() {
On 2015/03/23 01:53:40, mattm wrote:
> nit: I'd move this so that StartProcessOnIOThread and OnUtilityProcessStarted
> stay next to each other

Done. I also made the order in the .h match that here in the .cc.

[sky, 2015-03-23 16:00:28]

I'm not familiar with chrome/utility. Can you find a better owner?

[grt (UTC plus 2), 2015-03-23 16:49:34]

grt@chromium.org changed reviewers:
+ jhawkins@chromium.org
- sky@chromium.org

[grt (UTC plus 2), 2015-03-23 16:51:06]

TBR jhawkins@chromium.org for chrome/utility/chrome_content_utility_client.*
OWNERShip.

[James Hawkins, 2015-03-23 16:54:55]

lgtm

[James Hawkins, 2015-03-23 16:55:26]

On 2015/03/23 16:54:55, James Hawkins wrote:
> lgtm

Consider just adding me as a reviewer next time; I tend to be pretty prompt with
reviews :-)

[grt (UTC plus 2), 2015-03-23 16:56:16]

On 2015/03/23 16:55:26, James Hawkins wrote:
> On 2015/03/23 16:54:55, James Hawkins wrote:
> > lgtm
> 
> Consider just adding me as a reviewer next time; I tend to be pretty prompt
with
> reviews :-)

I see! Thanks for the prompt review. Much appreciated.

[Tom Sepez, 2015-03-23 17:15:55]

Messages LGTM, one unrelated question.

https://codereview.chromium.org/999003003/diff/240001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/sandboxed_zip_analyzer.cc (right):

https://codereview.chromium.org/999003003/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/sandboxed_zip_analyzer.cc:68:
zip_file_.Initialize(zip_file_name_,
Not part of your CL, but while we're looking at it, can you remind me where
zip_file_name_ comes from, and why we think it's safe?  Just curious, and I
apologize for being a pest.

[grt (UTC plus 2), 2015-03-23 17:16:40]

The CQ bit was checked by grt@chromium.org

[grt (UTC plus 2), 2015-03-23 17:16:40]

The patchset sent to the CQ was uploaded after l-g-t-m from mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/999003003/#ps240001
(title: "fix ordering of SandboxedZipAnalyzer methods")

[commit-bot: I haz the power, 2015-03-23 17:16:54]

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

[grt (UTC plus 2), 2015-03-23 17:22:13]

Thanks.

https://codereview.chromium.org/999003003/diff/240001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/sandboxed_zip_analyzer.cc (right):

https://codereview.chromium.org/999003003/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/sandboxed_zip_analyzer.cc:68:
zip_file_.Initialize(zip_file_name_,
On 2015/03/23 17:15:55, Tom Sepez wrote:
> Not part of your CL, but while we're looking at it, can you remind me where
> zip_file_name_ comes from, and why we think it's safe?  Just curious, and I
> apologize for being a pest.

No worries. zip_file_name_ is the path to the file corresponding to a
DownloadItem that is being processed by the DownloadProtectionService. It is a
freshly-downloaded file whose target filename ends with .zip. I am not certain,
but I believe that at this point zipe_file_name_ (from
DownloadItem::GetFullPath()) is the temporary file created by the
content::DownloadManager machinery while the download is in progress. I think
it's renamed to the desired target path (DownloadItem::GetTargetFilePath())
after the DownloadProtectionService reports that the download is safe for the
user to touch.

Does this answer your question?

[commit-bot: I haz the power, 2015-03-23 17:40:08]

Committed patchset #11 (id:240001)

[commit-bot: I haz the power, 2015-03-23 17:42:12]

Patchset 11 (id:??) landed as
https://crrev.com/88f4fa3f1874945019604aa1e88af6ebe61c1c1f
Cr-Commit-Position: refs/heads/master@{#321803}