Re: Add SafeBrowsing support for checking downloaded zip files that contain executables. (issue 10382113)

[bry...@chromium.org]

Please take another look.


http://codereview.chromium.org/10382113/diff/4003/chrome/browser/safe_browsing/download_protection_service.cc
File chrome/browser/safe_browsing/download_protection_service.cc
(right):

http://codereview.chromium.org/10382113/diff/4003/chrome/browser/safe_browsing/download_protection_service.cc#newcode72
chrome/browser/safe_browsing/download_protection_service.cc:72: return
ClientDownloadRequest::ZIPPED_WIN_EXECUTABLE;
On 2012/05/11 22:42:06, mattm wrote:
> Seems a little weird to say this since we haven't looked in the zip
file yet,
> right? Is the idea that we only ever send the ping if that happens to
be true?

Yep, that's right. Added a comment to clarify.

http://codereview.chromium.org/10382113/diff/4003/chrome/common/safe_browsing/csd.proto
File chrome/common/safe_browsing/csd.proto (right):

http://codereview.chromium.org/10382113/diff/4003/chrome/common/safe_browsing/csd.proto#newcode172
chrome/common/safe_browsing/csd.proto:172: ZIPPED_WIN_EXECUTABLE = 3;
On 2012/05/11 23:12:32, mattm wrote:
> On 2012/05/11 23:09:41, Brian Ryner wrote:
> > On 2012/05/11 22:42:06, mattm wrote:
> > > APK isn't a win executable?
> >
> > It's not, no. Are you suggesting that we don't ping on APK's inside
ZIP
> files,
> > or to update this comment to not say "executable"?

> Oh just that "zipped win executable" contains "one of the above"
types, when one
> of those types isn't a win executable. So, more that it shouldn't say
"win".

Sounds reasonable, renamed the enum.

http://codereview.chromium.org/10382113/

[ma...@chromium.org]

lgtm


http://codereview.chromium.org/10382113/diff/13004/chrome/browser/safe_browsing/download_protection_service.cc
File chrome/browser/safe_browsing/download_protection_service.cc
(right):

http://codereview.chromium.org/10382113/diff/13004/chrome/browser/safe_browsing/download_protection_service.cc#newcode539
chrome/browser/safe_browsing/download_protection_service.cc:539: //
Don't consider an archived archive to be executable.
maybe add a histogram for archived archives too?

http://codereview.chromium.org/10382113/

[bry...@chromium.org]

Hm, as things currently stand, that would not be reliable in the case where
an
archive has both an exe and a zip inside, since I'm breaking out of the zip
traversal as soon as we find an exe. Do you think it's worth changing this?


http://codereview.chromium.org/10382113/

[noe...@google.com]

lgtm



http://codereview.chromium.org/10382113/

[ma...@chromium.org]

I was more concerned about counting the case where there was an archive but
no
executable, so that would be okay as long as it was documented.

http://codereview.chromium.org/10382113/

[bry...@chromium.org]

PTAL

http://codereview.chromium.org/10382113/diff/13004/chrome/browser/safe_browsing/download_protection_service.cc
File chrome/browser/safe_browsing/download_protection_service.cc
(right):

http://codereview.chromium.org/10382113/diff/13004/chrome/browser/safe_browsing/download_protection_service.cc#newcode539
chrome/browser/safe_browsing/download_protection_service.cc:539: //
Don't consider an archived archive to be executable.

On 2012/05/12 00:47:06, mattm wrote:
> maybe add a histogram for archived archives too?

Done.

http://codereview.chromium.org/10382113/

[ma...@chromium.org]

http://codereview.chromium.org/10382113/diff/2008/chrome/browser/safe_browsing/download_protection_service.cc
File chrome/browser/safe_browsing/download_protection_service.cc
(right):

http://codereview.chromium.org/10382113/diff/2008/chrome/browser/safe_browsing/download_protection_service.cc#newcode561
chrome/browser/safe_browsing/download_protection_service.cc:561:
zip_file_has_archive);
should it be: zip_file_has_archive && !info_.zipped_executable ?

http://codereview.chromium.org/10382113/

[bry...@chromium.org]

http://codereview.chromium.org/10382113/diff/2008/chrome/browser/safe_browsing/download_protection_service.cc
File chrome/browser/safe_browsing/download_protection_service.cc
(right):

http://codereview.chromium.org/10382113/diff/2008/chrome/browser/safe_browsing/download_protection_service.cc#newcode561
chrome/browser/safe_browsing/download_protection_service.cc:561:
zip_file_has_archive);

On 2012/05/15 01:02:35, mattm wrote:
> should it be: zip_file_has_archive && !info_.zipped_executable ?

Whoops, yes. Fixed.

http://codereview.chromium.org/10382113/

[ma...@chromium.org]

lgtm



http://codereview.chromium.org/10382113/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bry...@chromium.org/10382113/8006


https://chromiumcodereview.appspot.com/10382113/

[commi...@chromium.org]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is win_rel, revision is 137067, job name
was 10382113-8006 (retry) (previous was lost) (previous was lost) (previous
was
lost).

https://chromiumcodereview.appspot.com/10382113/

[commi...@chromium.org]

[commi...@chromium.org]

The commit queue went berserk retrying too often for a

seemingly flaky test. Builder is android, revision is 137081, job name
was 10382113-8006 (previous was lost) (previous was lost) (previous was

[commi...@chromium.org]

[commi...@chromium.org]

Try job failure for 10382113-8006 (retry) on win_rel for
step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=29287


https://chromiumcodereview.appspot.com/10382113/

[bry...@chromium.org]

Whoops, this is probably a real test failure. Looking into it.

https://chromiumcodereview.appspot.com/10382113/

[bry...@chromium.org]

+randy

This seems to be happening because the test in question uses files with
a .zip
extension, but beyond that I'm having a little trouble figuring out what's
going
on. It looks like something about it using a temporary filename and also
trying to use a readonly folder is causing the DownloadItem to get an
interrupt
reason of FILE_ACCESS_DENIED. Not sure why this is happening now but wasn't
originally. Any ideas?


https://chromiumcodereview.appspot.com/10382113/

[bry...@chromium.org]

Oh, and in case you can't get to the test log for some reason, the failing
test
I'm referring to is DownloadTest.DownloadErrorReadonlyFolder.

[rds...@chromium.org]

Asanka: My apologies, but could you take a look at this? I have two
reasons for
asking: Your work with the file determination logic probably means you're
more
on top of the "failing over" to new directories that the test in question
targets, and you're more comfortable in the windows environment than I and
the
failure is windows specific.

I took a glance over the code and didn't spot anything obvious--it looks
like
the download is failing where it should be succeeding because of the new
safebrowsing check, but there isn't an obvious reason why.

Willing to take a look?


http://codereview.chromium.org/10382113/

[bry...@chromium.org]

I think I may have figured it out, see the chrome_download_manager_delegate
change in the latest patchset. Running trybots on this now to verify.

I also discovered (by trying to build this on Linux) an operator precedence
bug
which I also fixed, as well as adding the zipped_executable field to
DownloadInfo::DebugString().

[asa...@chromium.org]

On 2012/05/16 18:38:50, rdsmith wrote:
> Asanka: My apologies, but could you take a look at this? I have two
> reasons
for
> asking: Your work with the file determination logic probably means you're
> more
> on top of the "failing over" to new directories that the test in question
> targets, and you're more comfortable in the windows environment than I
> and the
> failure is windows specific.

> I took a glance over the code and didn't spot anything obvious--it looks
> like
> the download is failing where it should be succeeding because of the new
> safebrowsing check, but there isn't an obvious reason why.

> Willing to take a look?

This is a bug in CDMD. I think patchset 8 above should fix it.

http://codereview.chromium.org/10382113/

[rds...@chromium.org]

Ah, got it. Yuck. Thanks, Brian and Asanka, for figuring that one out.


http://codereview.chromium.org/10382113/

[bry...@chromium.org]

On 2012/05/16 20:21:48, rdsmith wrote:
> Ah, got it. Yuck. Thanks, Brian and Asanka, for figuring that one out.

Randy or Asanka, I think I'll need one of you to LGTM for
chrome/browser/download.

Thanks,


http://codereview.chromium.org/10382113/

[asa...@chromium.org]

chrome/browser/downloads/ LGTM



http://codereview.chromium.org/10382113/diff/10007/chrome/browser/safe_browsing/download_protection_service.cc
File chrome/browser/safe_browsing/download_protection_service.cc
(right):

http://codereview.chromium.org/10382113/diff/10007/chrome/browser/safe_browsing/download_protection_service.cc#newcode178
chrome/browser/safe_browsing/download_protection_service.cc:178:
"target_file:%s, referrer_url:%s, sha256_hash:%s, total_bytes:%" PRId64
Drive by nit: "%s" doesn't work with FilePath::CharType* on Windows.

http://codereview.chromium.org/10382113/

[bry...@chromium.org]

http://codereview.chromium.org/10382113/diff/10007/chrome/browser/safe_browsing/download_protection_service.cc
File chrome/browser/safe_browsing/download_protection_service.cc
(right):

http://codereview.chromium.org/10382113/diff/10007/chrome/browser/safe_browsing/download_protection_service.cc#newcode178
chrome/browser/safe_browsing/download_protection_service.cc:178:
"target_file:%s, referrer_url:%s, sha256_hash:%s, total_bytes:%" PRId64

On 2012/05/16 20:28:18, asanka wrote:
> Drive by nit: "%s" doesn't work with FilePath::CharType* on Windows.

Fixed by switching to PRFilePath. Thanks for catching this; the
compiler doesn't.

http://codereview.chromium.org/10382113/

[bry...@chromium.org]

Looks like this test passes now on the windows trybot. Noe or Matt, want to
take one more look at the latest changes before I submit?

http://codereview.chromium.org/10382113/

[ma...@chromium.org]

lgtm



http://codereview.chromium.org/10382113/

[commi...@chromium.org]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/bry...@chromium.org/10382113/15010


https://chromiumcodereview.appspot.com/10382113/

[commi...@chromium.org]

Change committed as 137599

https://chromiumcodereview.appspot.com/10382113/