Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1441243002:
Be more lenient about inspecting/pinging on invalid downloaded .zip's (Closed)
|
Created:
5 years, 1 month ago by Nathan Parker Modified:
5 years ago Reviewers:
asanka CC:
chromium-reviews, grt+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBe more lenient about inspecting/pinging on invalid downloaded .zip's
For zips that failed to parse completely, send a ping if
we see an exe/zip in it or if the user is in safe browsing extended-reporting.
BUG=553595
Committed: https://crrev.com/74fbd56171b18ef19ad7426018a774b200c0baa8
Cr-Commit-Position: refs/heads/master@{#361410}
Patch Set 1 #Patch Set 2 : Add basic tests #Patch Set 3 : Add UrlFetcherFactory for test #Patch Set 4 : Parameterize tests #Patch Set 5 : nits #Patch Set 6 : mac typo #Patch Set 7 : Fix expected result in test #
Total comments: 6
Patch Set 8 : nits #Patch Set 9 : Adjust proto tag number to match server side. #
Messages
Total messages: 35 (15 generated)
nparker@chromium.org changed reviewers: + asanka@chromium.org
This isn't quite ready for review, but I wanted to get your early take on the idea. ZipAnalyzer appears to parse incrementally, but we were dropping all extracted contents if the last char was bad.
LG. We should probably mention somewhere that we expect some unzip tools to extract the files even if there are errors in the zip. Also we should be able to put together a couple of zip files with errors so that we can write some tests for these.
Description was changed from ========== Record invalid downloaded .zip's and send pings in some additional cases. For invalid zips, send ping if we see an exe/zip in it or user is in safe browsing extended-reporting. BUG=53595 ========== to ========== Be more lenient about inspecting/pinging on invalid downloaded .zip's For zips that failed to parse completely, send a ping if we see an exe/zip in it or if the user is in safe browsing extended-reporting. BUG=53595 ==========
I haven't added example invalid .zips -- I could do that in a separate cl.
Oh, an PTAL. Thanks.
https://codereview.chromium.org/1441243002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/1441243002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:494: const bool in_incognito = item_->GetBrowserContext()->IsOffTheRecord(); Nit: Maybe return early if in_incognito is true. Also why are we just conditionalizing pings for invalid zips? We don't seem to be reporting anything more for a regular zip file than the fact that our zip analyzer failed. https://codereview.chromium.org/1441243002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:588: archive_is_valid_ = By the way, what happens if our utility process crashes while analyzing the zip?
Code LGTM. I just have some questions. I remember at some point that there was at least one zip file that caused the analyzer to crash. I can't find the issue report though.
https://codereview.chromium.org/1441243002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/1441243002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:494: const bool in_incognito = item_->GetBrowserContext()->IsOffTheRecord(); On 2015/11/18 21:37:55, asanka wrote: > Nit: Maybe return early if in_incognito is true. > > Also why are we just conditionalizing pings for invalid zips? We don't seem to > be reporting anything more for a regular zip file than the fact that our zip > analyzer failed. We're sending pings here that weren't sent before, and those pings include URLs. So rather than sending pings for ALL invalid zip (which may include a class of non-threating zips just made with incompatible zippers), we do it only for those users who are willing to share more info. Does that answer your question? Maybe I'm not understanding it. https://codereview.chromium.org/1441243002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:588: archive_is_valid_ = On 2015/11/18 21:37:55, asanka wrote: > By the way, what happens if our utility process crashes while analyzing the zip? I suspect the download would never complete, since OnAnalyzeZipFileFinished() would never get called. I don't recall a case where we saw it crash -- I've asked ihalle on the bug. mbarbella plans to do some fuzztesting on it, so we should discover bugs there.
Still LGTM https://codereview.chromium.org/1441243002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/1441243002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:494: const bool in_incognito = item_->GetBrowserContext()->IsOffTheRecord(); On 2015/11/20 at 06:22:16, nparker wrote: > On 2015/11/18 21:37:55, asanka wrote: > > Nit: Maybe return early if in_incognito is true. > > > > Also why are we just conditionalizing pings for invalid zips? We don't seem to > > be reporting anything more for a regular zip file than the fact that our zip > > analyzer failed. > > We're sending pings here that weren't sent before, and those pings include URLs. So rather than sending pings for ALL invalid zip (which may include a class of non-threating zips just made with incompatible zippers), we do it only for those users who are willing to share more info. Does that answer your question? Maybe I'm not understanding it. It does. I was just curious. https://codereview.chromium.org/1441243002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:588: archive_is_valid_ = On 2015/11/20 at 06:22:16, nparker wrote: > On 2015/11/18 21:37:55, asanka wrote: > > By the way, what happens if our utility process crashes while analyzing the zip? > > I suspect the download would never complete, since OnAnalyzeZipFileFinished() would never get called. I don't recall a case where we saw it crash -- I've asked ihalle on the bug. mbarbella plans to do some fuzztesting on it, so we should discover bugs there. I pointed out one crash bug, though I didn't look deep enough to know what the cause was. I also haven't looked into the utility process logic to see what would happen if the process died. I think we should have some well defined behavior in the event of a crash.
The CQ bit was checked by nparker@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1441243002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1441243002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nparker@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org Link to the patchset: https://codereview.chromium.org/1441243002/#ps160001 (title: "Adjust proto tag number to match server side.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1441243002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1441243002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by nparker@chromium.org
The CQ bit was checked by nparker@chromium.org
The CQ bit was checked by nparker@chromium.org
The CQ bit was unchecked by nparker@chromium.org
The CQ bit was checked by nparker@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1441243002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1441243002/160001
The failing tests should not be generating .zip downloads, so I'm currently baffled by how they are affected by this CL.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/11/23 at 23:47:02, nparker wrote: > The failing tests should not be generating .zip downloads, so I'm currently baffled by how they are affected by this CL. Might be https://code.google.com/p/chromium/issues/detail?id=560329 . Looks like the offending tests are disabled now. You should try again.
Yes, I just found that via another route. Thanks.
The CQ bit was checked by nparker@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1441243002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1441243002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/74fbd56171b18ef19ad7426018a774b200c0baa8 Cr-Commit-Position: refs/heads/master@{#361410}
Message was sent while issue was closed.
Description was changed from ========== Be more lenient about inspecting/pinging on invalid downloaded .zip's For zips that failed to parse completely, send a ping if we see an exe/zip in it or if the user is in safe browsing extended-reporting. BUG=53595 Committed: https://crrev.com/74fbd56171b18ef19ad7426018a774b200c0baa8 Cr-Commit-Position: refs/heads/master@{#361410} ========== to ========== Be more lenient about inspecting/pinging on invalid downloaded .zip's For zips that failed to parse completely, send a ping if we see an exe/zip in it or if the user is in safe browsing extended-reporting. BUG=553595 Committed: https://crrev.com/74fbd56171b18ef19ad7426018a774b200c0baa8 Cr-Commit-Position: refs/heads/master@{#361410} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
