Be more lenient about inspecting/pinging on invalid downloaded .zip's

chrome/browser/safe_browsing/download_protection_service.cc

Index: chrome/browser/safe_browsing/download_protection_service.cc
diff --git a/chrome/browser/safe_browsing/download_protection_service.cc b/chrome/browser/safe_browsing/download_protection_service.cc
index f9c62c3c0a1fa2b4131c2ad8e107b999fae9e3a3..7d24f33a954fe7b926411bcdcb136cdec83ed7b0 100644
--- a/chrome/browser/safe_browsing/download_protection_service.cc
+++ b/chrome/browser/safe_browsing/download_protection_service.cc
@@ -278,6 +278,7 @@ class DownloadProtectionService::CheckClientDownloadRequest
         tab_url_(item->GetTabUrl()),
         tab_referrer_url_(item->GetTabReferrerUrl()),
         archived_executable_(false),
+        archive_is_valid_(ArchiveValid::UNSET),
         callback_(callback),
         service_(service),
         binary_feature_extractor_(binary_feature_extractor),
@@ -485,6 +486,22 @@ class DownloadProtectionService::CheckClientDownloadRequest
     DCHECK(item_ == NULL);
   }
 
+  // .zip files that look invalid to Chrome can often be successfully unpacked
+  // by other archive tools, so they may be a real threat.  For that reason,
+  // we send pings for them for some users.
+  bool CanReportInvalidArchives() {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+    const bool in_incognito = item_->GetBrowserContext()->IsOffTheRecord();

[asanka, 2015/11/18 21:37:55]

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.

[Nathan Parker, 2015/11/20 06:22:16]

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.

[asanka, 2015/11/20 15:55:40]

It does. I was just curious.

+
+    Profile* profile = Profile::FromBrowserContext(item_->GetBrowserContext());
+    const bool is_extended_reporting =
+        profile &&
+        profile->GetPrefs()->GetBoolean(
+            prefs::kSafeBrowsingExtendedReportingEnabled);
+
+    return !in_incognito && is_extended_reporting;
+  }
+
   void OnFileFeatureExtractionDone() {
     // This can run in any thread, since it just posts more messages.
 
@@ -564,15 +581,19 @@ class DownloadProtectionService::CheckClientDownloadRequest
     DCHECK_EQ(ClientDownloadRequest::ZIPPED_EXECUTABLE, type_);
     if (!service_)
       return;
-    if (results.success) {
-      archived_executable_ = results.has_executable;
-      archived_binary_.CopyFrom(results.archived_binary);
-      DVLOG(1) << "Zip analysis finished for " << item_->GetFullPath().value()
-               << ", has_executable=" << results.has_executable
-               << " has_archive=" << results.has_archive;
-    } else {
-      DVLOG(1) << "Zip analysis failed for " << item_->GetFullPath().value();
-    }
+
+    // Even if !results.success, some of the zip may have been parsed.
+    // Some unzippers will successfully unpack archives that we cannot,
+    // so we're lenient here.
+    archive_is_valid_ =

[asanka, 2015/11/18 21:37:55]

By the way, what happens if our utility process crashes while analyzing the zip?

[Nathan Parker, 2015/11/20 06:22:16]

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.

[asanka, 2015/11/20 15:55:40]

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.

+        (results.success ? ArchiveValid::VALID : ArchiveValid::INVALID);
+    archived_executable_ = results.has_executable;
+    archived_binary_.CopyFrom(results.archived_binary);
+    DVLOG(1) << "Zip analysis finished for " << item_->GetFullPath().value()
+             << ", has_executable=" << results.has_executable
+             << ", has_archive=" << results.has_archive
+             << ", success=" << results.success;
+
     UMA_HISTOGRAM_BOOLEAN("SBClientDownload.ZipFileSuccess", results.success);
     UMA_HISTOGRAM_BOOLEAN("SBClientDownload.ZipFileHasExecutable",
                           archived_executable_);
@@ -583,13 +604,18 @@ class DownloadProtectionService::CheckClientDownloadRequest
     for (const auto& file_name : results.archived_archive_filenames)
       RecordArchivedArchiveFileExtensionType(file_name);
 
-    if (!archived_executable_ && !results.has_archive) {
-      PostFinishTask(UNKNOWN, REASON_ARCHIVE_WITHOUT_BINARIES);
-      return;
+    if (!archived_executable_) {
+      if (results.has_archive) {
+        type_ = ClientDownloadRequest::ZIPPED_ARCHIVE;
+      } else if (!results.success && CanReportInvalidArchives()) {
+        type_ = ClientDownloadRequest::INVALID_ZIP;
+      } else {
+        // Normal zip w/o EXEs, or invalid zip and not extended-reporting.
+        PostFinishTask(UNKNOWN, REASON_ARCHIVE_WITHOUT_BINARIES);
+        return;
+      }
     }
 
-    if (!archived_executable_ && results.has_archive)
-      type_ = ClientDownloadRequest::ZIPPED_ARCHIVE;
     OnFileFeatureExtractionDone();
   }
 
@@ -611,15 +637,14 @@ class DownloadProtectionService::CheckClientDownloadRequest
     if (!service_)
       return;
 
-    if (results.success) {
-      archived_executable_ = results.has_executable;
-      archived_binary_.CopyFrom(results.archived_binary);
-      DVLOG(1) << "DMG analysis has finished for "
-               << item_->GetFullPath().value() << ", has_executable="
-               << results.has_executable;
-    } else {
-      DVLOG(1) << "DMG analysis failed for "<< item_->GetFullPath().value();
-    }
+    // Even if !results.success, some of the DMG may have been parsed.
+    archive_is_valid_ =
+        (results.success ? ArchiveValid::VALID : ArchiveValid::INVALID);
+    archived_executable_ = results.has_executable;
+    archived_binary_.CopyFrom(results.archived_binary);
+    DVLOG(1) << "DMG analysis has finished for " << item_->GetFullPath().value()
+             << ", has_executable=" << results.has_executable
+             << ", success=" << results.success;
 
     UMA_HISTOGRAM_BOOLEAN("SBClientDownload.DmgFileSuccess", results.success);
     UMA_HISTOGRAM_BOOLEAN("SBClientDownload.DmgFileHasExecutable",
@@ -628,8 +653,12 @@ class DownloadProtectionService::CheckClientDownloadRequest
                         base::TimeTicks::Now() - dmg_analysis_start_time_);
 
     if (!archived_executable_) {
-      PostFinishTask(UNKNOWN, REASON_ARCHIVE_WITHOUT_BINARIES);
-      return;
+      if (!results.success && CanReportInvalidArchives()) {
+        type_ = ClientDownloadRequest::INVALID_MAC_ARCHIVE;
+      } else {
+        PostFinishTask(UNKNOWN, REASON_ARCHIVE_WITHOUT_BINARIES);
+        return;
+      }
     }
 
     OnFileFeatureExtractionDone();
@@ -812,6 +841,8 @@ class DownloadProtectionService::CheckClientDownloadRequest
     request.set_file_basename(
         item_->GetTargetFilePath().BaseName().AsUTF8Unsafe());
     request.set_download_type(type_);
+    if (archive_is_valid_ != ArchiveValid::UNSET)
+      request.set_archive_valid(archive_is_valid_ == ArchiveValid::VALID);
     request.mutable_signature()->CopyFrom(signature_info_);
     if (image_headers_)
       request.set_allocated_image_headers(image_headers_.release());
@@ -936,6 +967,8 @@ class DownloadProtectionService::CheckClientDownloadRequest
     return false;
   }
 
+  enum class ArchiveValid { UNSET, VALID, INVALID };
+
   // The DownloadItem we are checking. Will be NULL if the request has been
   // canceled. Must be accessed only on UI thread.
   content::DownloadItem* item_;
@@ -949,6 +982,8 @@ class DownloadProtectionService::CheckClientDownloadRequest
   GURL tab_referrer_url_;
 
   bool archived_executable_;
+  ArchiveValid archive_is_valid_;
+
   ClientDownloadRequest_SignatureInfo signature_info_;
   scoped_ptr<ClientDownloadRequest_ImageHeaders> image_headers_;
   google::protobuf::RepeatedPtrField<ClientDownloadRequest_ArchivedBinary>