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

chrome/browser/safe_browsing/download_protection_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/safe_browsing/download_protection_service.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/format_macros.h"
@@ skipping 260 matching lines @@
       const CheckDownloadCallback& callback,
       DownloadProtectionService* service,
       const scoped_refptr<SafeBrowsingDatabaseManager>& database_manager,
       BinaryFeatureExtractor* binary_feature_extractor)
       : item_(item),
         url_chain_(item->GetUrlChain()),
         referrer_url_(item->GetReferrerUrl()),
         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),
         database_manager_(database_manager),
         pingback_enabled_(service_->enabled()),
         finished_(false),
         type_(ClientDownloadRequest::WIN_EXECUTABLE),
         start_time_(base::TimeTicks::Now()),
         weakptr_factory_(this) {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ skipping 187 matching lines @@
 
  private:
   friend struct BrowserThread::DeleteOnThread<BrowserThread::UI>;
   friend class base::DeleteHelper<CheckClientDownloadRequest>;
 
   ~CheckClientDownloadRequest() override {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
     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.
 
     // TODO(noelutz): DownloadInfo should also contain the IP address of
     // every URL in the redirect chain.  We also should check whether the
     // download URL is hosted on the internal network.
     BrowserThread::PostTask(
         BrowserThread::IO,
         FROM_HERE,
         base::Bind(&CheckClientDownloadRequest::CheckWhitelists, this));
@@ skipping 59 matching lines @@
         base::Bind(&CheckClientDownloadRequest::OnZipAnalysisFinished,
                    weakptr_factory_.GetWeakPtr()));
     analyzer_->Start();
   }
 
   void OnZipAnalysisFinished(const zip_analyzer::Results& results) {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
     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_);
     UMA_HISTOGRAM_BOOLEAN("SBClientDownload.ZipFileHasArchiveButNoExecutable",
                           results.has_archive && !archived_executable_);
     UMA_HISTOGRAM_TIMES("SBClientDownload.ExtractZipFeaturesTime",
                         base::TimeTicks::Now() - zip_analysis_start_time_);
     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();
   }
 
 #if defined(OS_MACOSX)
   void StartExtractDmgFeatures() {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
     DCHECK(item_);
     dmg_analyzer_ = new SandboxedDMGAnalyzer(
         item_->GetFullPath(),
         base::Bind(&CheckClientDownloadRequest::OnDmgAnalysisFinished,
                    weakptr_factory_.GetWeakPtr()));
     dmg_analyzer_->Start();
     dmg_analysis_start_time_ = base::TimeTicks::Now();
   }
 
   void OnDmgAnalysisFinished(const zip_analyzer::Results& results) {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
     DCHECK_EQ(ClientDownloadRequest::MAC_EXECUTABLE, type_);
     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",
                           archived_executable_);
     UMA_HISTOGRAM_TIMES("SBClientDownload.ExtractDmgFeaturesTime",
                         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();
   }
 #endif  // defined(OS_MACOSX)
 
   static void RecordCountOfSignedOrWhitelistedDownload() {
     UMA_HISTOGRAM_COUNTS("SBClientDownload.SignedOrWhitelistedDownload", 1);
   }
 
@@ skipping 162 matching lines @@
       if (tab_referrer_url_.is_valid()) {
         resource->set_referrer(SanitizeUrl(tab_referrer_url_));
         DVLOG(2) << "tab referrer " << resource->referrer();
       }
     }
 
     request.set_user_initiated(item_->HasUserGesture());
     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());
     if (archived_executable_)
       request.mutable_archived_binary()->Swap(&archived_binary_);
     if (!request.SerializeToString(&client_download_request_data_)) {
       FinishRequest(UNKNOWN, REASON_INVALID_REQUEST_PROTO);
       return;
     }
     service_->client_download_request_callbacks_.Notify(item_, &request);
@@ skipping 104 matching lines @@
                    << cert->subject().GetDisplayName()
                    << " issuer=" << issuer->subject().GetDisplayName();
           return true;
         }
       }
       cert = issuer;
     }
     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_;
   // Copies of data from |item_| for access on other threads.
   std::vector<GURL> url_chain_;
   GURL referrer_url_;
   // URL chain of redirects leading to (but not including) |tab_url|.
   std::vector<GURL> tab_redirects_;
   // URL and referrer of the window the download was started from.
   GURL tab_url_;
   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>
       archived_binary_;
   CheckDownloadCallback callback_;
   // Will be NULL if the request has been canceled.
   DownloadProtectionService* service_;
   scoped_refptr<BinaryFeatureExtractor> binary_feature_extractor_;
   scoped_refptr<SafeBrowsingDatabaseManager> database_manager_;
   const bool pingback_enabled_;
@@ skipping 241 matching lines @@
 GURL DownloadProtectionService::GetDownloadRequestUrl() {
   GURL url(kDownloadRequestUrl);
   std::string api_key = google_apis::GetAPIKey();
   if (!api_key.empty())
     url = url.Resolve("?key=" + net::EscapeQueryParamValue(api_key, true));
 
   return url;
 }
 
 }  // namespace safe_browsing