Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10382113:
Add SafeBrowsing support for checking downloaded zip files that contain executables. (Closed)
|
Created:
8 years, 7 months ago by Brian Ryner Modified:
8 years, 7 months ago CC:
chromium-reviews, Randy Smith (Not in Mondays) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd SafeBrowsing support for checking downloaded zip files that contain executables.
BUG=none
TEST=DownloadProtectionServiceTest.CheckClientDownloadZip
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137599
Patch Set 1 #Patch Set 2 : Fix Windows DownloadType check #Patch Set 3 : Minor cleanup #
Total comments: 10
Patch Set 4 : Address Noe's comments #Patch Set 5 : Address Matt's review comments #
Total comments: 2
Patch Set 6 : Add a histogram for zipped archives #
Total comments: 2
Patch Set 7 : Fix histogram #Patch Set 8 : Merge, fix readonly folder bug and other issues #
Total comments: 2
Patch Set 9 : Fix FilePath printf format on Windows #
Messages
Total messages: 35 (0 generated)
Mostly LG. Just have some minor comments. noe. http://codereview.chromium.org/10382113/diff/4003/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/download_protection_service.cc (right): http://codereview.chromium.org/10382113/diff/4003/chrome/browser/safe_browsin... chrome/browser/safe_browsing/download_protection_service.cc:494: RecordImprovedProtectionStats(REASON_NOT_BINARY_FILE); might be nice to have a separate reason for that. http://codereview.chromium.org/10382113/diff/4003/chrome/browser/safe_browsin... chrome/browser/safe_browsing/download_protection_service.cc:523: void ExtractZipFeatures() { Would you mind adding a histogram for both this method and the signature method to measure how long it takes? We're seeing quite a few requests being cancelled on the client (>5%). It would be nice to see if it's because these checks take too long.
Please take another look. Thanks, http://codereview.chromium.org/10382113/diff/4003/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/download_protection_service.cc (right): http://codereview.chromium.org/10382113/diff/4003/chrome/browser/safe_browsin... chrome/browser/safe_browsing/download_protection_service.cc:494: RecordImprovedProtectionStats(REASON_NOT_BINARY_FILE); On 2012/05/11 17:54:33, noelutz wrote: > might be nice to have a separate reason for that. Done. http://codereview.chromium.org/10382113/diff/4003/chrome/browser/safe_browsin... chrome/browser/safe_browsing/download_protection_service.cc:523: void ExtractZipFeatures() { On 2012/05/11 17:54:33, noelutz wrote: > Would you mind adding a histogram for both this method and the signature method > to measure how long it takes? We're seeing quite a few requests being cancelled > on the client (>5%). It would be nice to see if it's because these checks take > too long. Done.
http://codereview.chromium.org/10382113/diff/4003/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/download_protection_service.cc (right): http://codereview.chromium.org/10382113/diff/4003/chrome/browser/safe_browsin... chrome/browser/safe_browsing/download_protection_service.cc:72: return ClientDownloadRequest::ZIPPED_WIN_EXECUTABLE; 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? http://codereview.chromium.org/10382113/diff/4003/chrome/common/safe_browsing... File chrome/common/safe_browsing/csd.proto (right): http://codereview.chromium.org/10382113/diff/4003/chrome/common/safe_browsing... chrome/common/safe_browsing/csd.proto:172: ZIPPED_WIN_EXECUTABLE = 3; APK isn't a win executable?
http://codereview.chromium.org/10382113/diff/4003/chrome/common/safe_browsing... File chrome/common/safe_browsing/csd.proto (right): http://codereview.chromium.org/10382113/diff/4003/chrome/common/safe_browsing... chrome/common/safe_browsing/csd.proto:172: ZIPPED_WIN_EXECUTABLE = 3; 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"?
http://codereview.chromium.org/10382113/diff/4003/chrome/common/safe_browsing... File chrome/common/safe_browsing/csd.proto (right): http://codereview.chromium.org/10382113/diff/4003/chrome/common/safe_browsing... chrome/common/safe_browsing/csd.proto:172: ZIPPED_WIN_EXECUTABLE = 3; 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".
Please take another look. http://codereview.chromium.org/10382113/diff/4003/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/download_protection_service.cc (right): http://codereview.chromium.org/10382113/diff/4003/chrome/browser/safe_browsin... 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... File chrome/common/safe_browsing/csd.proto (right): http://codereview.chromium.org/10382113/diff/4003/chrome/common/safe_browsing... 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.
lgtm http://codereview.chromium.org/10382113/diff/13004/chrome/browser/safe_browsi... File chrome/browser/safe_browsing/download_protection_service.cc (right): http://codereview.chromium.org/10382113/diff/13004/chrome/browser/safe_browsi... 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?
On 2012/05/12 00:47:06, mattm wrote: > lgtm > > http://codereview.chromium.org/10382113/diff/13004/chrome/browser/safe_browsi... > File chrome/browser/safe_browsing/download_protection_service.cc (right): > > http://codereview.chromium.org/10382113/diff/13004/chrome/browser/safe_browsi... > 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? 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?
lgtm
On 2012/05/13 18:12:44, Brian Ryner wrote: > On 2012/05/12 00:47:06, mattm wrote: > > lgtm > > > > > http://codereview.chromium.org/10382113/diff/13004/chrome/browser/safe_browsi... > > File chrome/browser/safe_browsing/download_protection_service.cc (right): > > > > > http://codereview.chromium.org/10382113/diff/13004/chrome/browser/safe_browsi... > > 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? > > 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? 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.
PTAL http://codereview.chromium.org/10382113/diff/13004/chrome/browser/safe_browsi... File chrome/browser/safe_browsing/download_protection_service.cc (right): http://codereview.chromium.org/10382113/diff/13004/chrome/browser/safe_browsi... 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/diff/2008/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/download_protection_service.cc (right): http://codereview.chromium.org/10382113/diff/2008/chrome/browser/safe_browsin... 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/diff/2008/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/download_protection_service.cc (right): http://codereview.chromium.org/10382113/diff/2008/chrome/browser/safe_browsin... 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.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bryner@chromium.org/10382113/8006
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).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bryner@chromium.org/10382113/8006
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 lost) (previous was lost).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bryner@chromium.org/10382113/8006
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&nu...
Whoops, this is probably a real test failure. Looking into it.
+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?
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. On 2012/05/16 04:55:25, Brian Ryner wrote: > +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?
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?
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(). 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?
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.
Ah, got it. Yuck. Thanks, Brian and Asanka, for figuring that one out.
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,
chrome/browser/downloads/ LGTM http://codereview.chromium.org/10382113/diff/10007/chrome/browser/safe_browsi... File chrome/browser/safe_browsing/download_protection_service.cc (right): http://codereview.chromium.org/10382113/diff/10007/chrome/browser/safe_browsi... 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/diff/10007/chrome/browser/safe_browsi... File chrome/browser/safe_browsing/download_protection_service.cc (right): http://codereview.chromium.org/10382113/diff/10007/chrome/browser/safe_browsi... 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.
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?
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bryner@chromium.org/10382113/15010
Change committed as 137599 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
