Add sampling of unknown filetypes in download protection.

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 cdb87558e9eab975404b2e6fc9823c969d58471d..e3986f974fe8822202879b71aeac82f1079d689a 100644
--- a/chrome/browser/safe_browsing/download_protection_service.cc
+++ b/chrome/browser/safe_browsing/download_protection_service.cc
@@ -114,21 +114,6 @@ enum SBStatsType {
   DOWNLOAD_CHECKS_MAX
 };
 
-// Prepares URLs to be put into a ping message. Currently this just shortens
-// data: URIs, other URLs are included verbatim.
-std::string SanitizeUrl(const GURL& url) {
-  std::string spec = url.spec();
-  if (url.SchemeIs(url::kDataScheme)) {
-    size_t comma_pos = spec.find(',');
-    if (comma_pos != std::string::npos && comma_pos != spec.size() - 1) {
-      std::string hash_value = crypto::SHA256HashString(spec);
-      spec.erase(comma_pos + 1);
-      spec += base::HexEncode(hash_value.data(), hash_value.size());
-    }
-  }
-  return spec;
-}
-
 }  // namespace
 
 // Parent SafeBrowsing::Client class used to lookup the bad binary
@@ -309,6 +294,17 @@ class DownloadProtectionService::CheckClientDownloadRequest
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
     item_->AddObserver(this);
   }
+  bool ShouldSampleUnsupportedFile(const base::FilePath filename) {

[asanka, 2016/06/20 18:15:46]

Nit: Use 'const base::FilePath&'. We aren't benefiting from copy elision here
since the return value from DownloadItem::GetTargetFilePath() is a const
reference.

[Nathan Parker, 2016/06/20 22:47:31]

Done.

+    // If this extension is specifically marked as SAMPLED_PING (as are
+    // all "unknown" extensions), we may want to sample it. Sampling it means
+    // we'll send a "light ping" with private info removed, and we won't
+    // use the verdict.
+    const FileTypePolicies* policies = FileTypePolicies::GetInstance();
+    return service_ && is_extended_reporting_ && !is_incognito_ &&
+           base::RandDouble() < policies->SampledPingProbability() &&
+           policies->PingSettingForFile(filename) ==
+               DownloadFileType::SAMPLED_PING;
+  }
 
   void Start() {
     DVLOG(2) << "Starting SafeBrowsing download check for: "
@@ -322,9 +318,7 @@ class DownloadProtectionService::CheckClientDownloadRequest
                  prefs::kSafeBrowsingExtendedReportingEnabled);
       is_incognito_ = item_->GetBrowserContext()->IsOffTheRecord();
     }
-    // TODO(noelutz): implement some cache to make sure we don't issue the same
-    // request over and over again if a user downloads the same binary multiple
-    // times.
+
     DownloadCheckResultReason reason = REASON_MAX;
     if (!IsSupportedDownload(
         *item_, item_->GetTargetFilePath(), &reason, &type_)) {
@@ -336,9 +330,15 @@ class DownloadProtectionService::CheckClientDownloadRequest
           return;
 
         case REASON_NOT_BINARY_FILE:
-          RecordFileExtensionType(item_->GetTargetFilePath());
-          PostFinishTask(UNKNOWN, reason);
-          return;
+          if (ShouldSampleUnsupportedFile(item_->GetTargetFilePath())) {
+            // Send a "light ping" and don't use the verdict.
+            type_ = ClientDownloadRequest::SAMPLED_UNSUPPORTED_FILE;
+            break;
+          } else {

[asanka, 2016/06/20 18:15:46]

Nit: else { unnecessary.

[Nathan Parker, 2016/06/20 22:47:31]

Done.

+            RecordFileExtensionType(item_->GetTargetFilePath());
+            PostFinishTask(UNKNOWN, reason);
+            return;
+          }
 
         default:
           // We only expect the reasons explicitly handled above.
@@ -464,16 +464,16 @@ class DownloadProtectionService::CheckClientDownloadRequest
       if (!response.ParseFromString(data)) {
         reason = REASON_INVALID_RESPONSE_PROTO;
         result = UNKNOWN;
+      } else if (type_ == ClientDownloadRequest::SAMPLED_UNSUPPORTED_FILE) {
+        // Ignore the verdict because we were just reporting a sampled file.
+        reason = REASON_SAMPLED_UNSUPPORTED_FILE;
+        result = UNKNOWN;
       } else if (response.verdict() == ClientDownloadResponse::SAFE) {
         reason = REASON_DOWNLOAD_SAFE;
         result = SAFE;
       } else if (service_ && !service_->IsSupportedDownload(
           *item_, item_->GetTargetFilePath())) {
-        // The client of the download protection service assumes that we don't
-        // support this download so we cannot return any other verdict than
-        // UNKNOWN even if the server says it's dangerous to download this file.
-        // Note: if service_ is NULL we already cancelled the request and
-        // returned UNKNOWN.
+        // TODO(nparker): Remove this check since it should be impossible.
         reason = REASON_DOWNLOAD_NOT_SUPPORTED;
         result = UNKNOWN;
       } else if (response.verdict() == ClientDownloadResponse::DANGEROUS) {
@@ -528,16 +528,17 @@ class DownloadProtectionService::CheckClientDownloadRequest
       *reason = REASON_INVALID_URL;
       return false;
     }
-    if (!FileTypePolicies::GetInstance()->IsCheckedBinaryFile(target_path)) {
-      *reason = REASON_NOT_BINARY_FILE;
-      return false;
-    }
     if ((!final_url.IsStandard() && !final_url.SchemeIsBlob() &&
          !final_url.SchemeIs(url::kDataScheme)) ||
         final_url.SchemeIsFile()) {
       *reason = REASON_UNSUPPORTED_URL_SCHEME;
       return false;
     }
+    // This check should be last, so we know the earlier checks passed.
+    if (!FileTypePolicies::GetInstance()->IsCheckedBinaryFile(target_path)) {
+      *reason = REASON_NOT_BINARY_FILE;
+      return false;
+    }
     *type = download_protection_util::GetDownloadType(target_path);
     return true;
   }
@@ -879,6 +880,26 @@ class DownloadProtectionService::CheckClientDownloadRequest
     return false;
   }
 
+  // Prepares URLs to be put into a ping message. Currently this just shortens
+  // data: URIs, other URLs are included verbatim. If this is a sampled binary,
+  // we'll send a lite-ping which strips all PII.
+  std::string SanitizeUrl(const GURL& url) const {
+    if (type_ == ClientDownloadRequest::SAMPLED_UNSUPPORTED_FILE)
+      return url.GetOrigin().spec();
+
+    std::string spec = url.spec();
+    if (url.SchemeIs(url::kDataScheme)) {
+      size_t comma_pos = spec.find(',');
+      if (comma_pos != std::string::npos && comma_pos != spec.size() - 1) {
+        std::string hash_value = crypto::SHA256HashString(spec);
+        spec.erase(comma_pos + 1);
+        spec += base::HexEncode(hash_value.data(), hash_value.size());
+      }
+    }
+    return spec;
+  }
+
+

[asanka, 2016/06/20 18:15:46]

Nit: Extra blank line.

[Nathan Parker, 2016/06/20 22:47:31]

Done.

   void SendRequest() {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
@@ -895,6 +916,7 @@ class DownloadProtectionService::CheckClientDownloadRequest
       request.mutable_population()->set_user_population(
           ChromeUserPopulation::SAFE_BROWSING);
     }
+
     request.set_url(SanitizeUrl(item_->GetUrlChain().back()));
     request.mutable_digests()->set_sha256(item_->GetHash());
     request.set_length(item_->GetReceivedBytes());
@@ -938,8 +960,14 @@ class DownloadProtectionService::CheckClientDownloadRequest
     }
 
     request.set_user_initiated(item_->HasUserGesture());
-    request.set_file_basename(
+    if (type_ == ClientDownloadRequest::SAMPLED_UNSUPPORTED_FILE) {
+      request.set_file_basename(
+          base::FilePath(item_->GetTargetFilePath().Extension())
+              .AsUTF8Unsafe());
+    } else {
+      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);
@@ -1020,8 +1048,7 @@ class DownloadProtectionService::CheckClientDownloadRequest
       DVLOG(2) << "SafeBrowsing download verdict for: "
                << item_->DebugString(true) << " verdict:" << reason
                << " result:" << result;
-      UMA_HISTOGRAM_ENUMERATION("SBClientDownload.CheckDownloadStats",
-                                reason,
+      UMA_HISTOGRAM_ENUMERATION("SBClientDownload.CheckDownloadStats", reason,
                                 REASON_MAX);
       callback_.Run(result);
       item_->RemoveObserver(this);
@@ -1368,8 +1395,8 @@ class DownloadProtectionService::PPAPIDownloadRequest
 
   // Given a |default_file_path| and a list of |alternate_extensions|,
   // constructs a FilePath with each possible extension and returns one that
-  // satisfies download_protection_util::IsSupportedBinaryFile(). If none are
-  // supported, returns an empty FilePath.
+  // satisfies IsCheckedBinaryFile(). If none are supported, returns an
+  // empty FilePath.
   static base::FilePath GetSupportedFilePath(
       const base::FilePath& default_file_path,
       const std::vector<base::FilePath::StringType>& alternate_extensions) {
@@ -1412,7 +1439,7 @@ class DownloadProtectionService::PPAPIDownloadRequest
 
   // A download path that is supported by SafeBrowsing. This is determined by
   // invoking GetSupportedFilePath(). If non-empty,
-  // safe_browsing::IsSupportedBinaryFile(supported_path_) is always true. This
+  // IsCheckedBinaryFile(supported_path_) is always true. This
   // path is therefore used as the download target when sending the SafeBrowsing
   // ping.
   const base::FilePath supported_path_;
@@ -1511,6 +1538,8 @@ bool DownloadProtectionService::IsSupportedDownload(
   DownloadCheckResultReason reason = REASON_MAX;
   ClientDownloadRequest::DownloadType type =
       ClientDownloadRequest::WIN_EXECUTABLE;
+  // TODO(nparker): Remove the CRX check here once can support
+  // UNKNOWN types properly.  http://crbug.com/581044
   return (CheckClientDownloadRequest::IsSupportedDownload(
               item, target_path, &reason, &type) &&
           (ClientDownloadRequest::CHROME_EXTENSION != type));