[SafeBrowsing] Send pingbacks for additional file types.

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/compiler_specific.h"
 #include "base/format_macros.h"
 #include "base/memory/scoped_ptr.h"
@@ skipping 44 matching lines @@
 namespace {
 static const int64 kDownloadRequestTimeoutMs = 7000;
 }  // namespace
 
 namespace safe_browsing {
 
 const char DownloadProtectionService::kDownloadRequestUrl[] =
     "https://sb-ssl.google.com/safebrowsing/clientreport/download";
 
 namespace {
-// List of extensions for which we track some UMA stats.
-enum MaliciousExtensionType {
-  EXTENSION_EXE,
-  EXTENSION_MSI,
-  EXTENSION_CAB,
-  EXTENSION_SYS,
-  EXTENSION_SCR,
-  EXTENSION_DRV,
-  EXTENSION_BAT,
-  EXTENSION_ZIP,
-  EXTENSION_RAR,
-  EXTENSION_DLL,
-  EXTENSION_PIF,
-  EXTENSION_COM,
-  EXTENSION_JAR,
-  EXTENSION_CLASS,
-  EXTENSION_PDF,
-  EXTENSION_VB,
-  EXTENSION_REG,
-  EXTENSION_GRP,
-  EXTENSION_OTHER,  // Groups all other extensions into one bucket.
-  EXTENSION_CRX,
-  EXTENSION_APK,
-  EXTENSION_DMG,
-  EXTENSION_PKG,
-  EXTENSION_TORRENT,
-  EXTENSION_WEBSITE,
-  EXTENSION_URL,
-  EXTENSION_MAX,
+// List of extensions for which we track some UMA stats. The position of the
+// extension in kDangerousFileTypes is considered to be the UMA enumeration
+// value. Naturally, new values should only be added at the end.
+const base::FilePath::CharType* const kDangerousFileTypes[] = {
+    FILE_PATH_LITERAL(".exe"),
+    FILE_PATH_LITERAL(".msi"),
+    FILE_PATH_LITERAL(".cab"),
+    FILE_PATH_LITERAL(".sys"),
+    FILE_PATH_LITERAL(".scr"),
+    FILE_PATH_LITERAL(".drv"),
+    FILE_PATH_LITERAL(".bat"),
+    FILE_PATH_LITERAL(".zip"),
+    FILE_PATH_LITERAL(".rar"),
+    FILE_PATH_LITERAL(".dll"),
+    FILE_PATH_LITERAL(".pif"),
+    FILE_PATH_LITERAL(".com"),
+    FILE_PATH_LITERAL(".jar"),
+    FILE_PATH_LITERAL(".class"),
+    FILE_PATH_LITERAL(".pdf"),
+    FILE_PATH_LITERAL(".vb"),
+    FILE_PATH_LITERAL(".reg"),
+    FILE_PATH_LITERAL(".grp"),
+    nullptr,  // The "Other" bucket. This is in the middle of the array due to
+              // historical reasons.
+    FILE_PATH_LITERAL(".crx"),
+    FILE_PATH_LITERAL(".apk"),
+    FILE_PATH_LITERAL(".dmg"),
+    FILE_PATH_LITERAL(".pkg"),
+    FILE_PATH_LITERAL(".torrent"),
+    FILE_PATH_LITERAL(".website"),
+    FILE_PATH_LITERAL(".url"),
+    FILE_PATH_LITERAL(".vbe"),
+    FILE_PATH_LITERAL(".vbs"),
+    FILE_PATH_LITERAL(".js"),
+    FILE_PATH_LITERAL(".jse"),
+    FILE_PATH_LITERAL(".mht"),
+    FILE_PATH_LITERAL(".mhtml"),

[moheeb1, 2015/07/08 20:25:29]

Would be great to expand this list to cover all dangerous file we know about, so
we get a sense of prevalance of these file types.

 };
 
-MaliciousExtensionType GetExtensionType(const base::FilePath& f) {
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".exe"))) return EXTENSION_EXE;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".msi"))) return EXTENSION_MSI;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".cab"))) return EXTENSION_CAB;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".sys"))) return EXTENSION_SYS;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".scr"))) return EXTENSION_SCR;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".drv"))) return EXTENSION_DRV;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".bat"))) return EXTENSION_BAT;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".zip"))) return EXTENSION_ZIP;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".rar"))) return EXTENSION_RAR;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".dll"))) return EXTENSION_DLL;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".pif"))) return EXTENSION_PIF;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".com"))) return EXTENSION_COM;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".jar"))) return EXTENSION_JAR;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".class"))) return EXTENSION_CLASS;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".pdf"))) return EXTENSION_PDF;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".vb"))) return EXTENSION_VB;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".reg"))) return EXTENSION_REG;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".grp"))) return EXTENSION_GRP;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".crx"))) return EXTENSION_CRX;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".apk"))) return EXTENSION_APK;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".dmg"))) return EXTENSION_DMG;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".pkg"))) return EXTENSION_PKG;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".torrent")))
-    return EXTENSION_TORRENT;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".website")))
-    return EXTENSION_WEBSITE;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".url"))) return EXTENSION_URL;
-  return EXTENSION_OTHER;
-}
+const int EXTENSION_OTHER = 18;

[Nathan Parker, 2015/07/08 20:11:08]

Nit: Add comment or make the name indicate that this is an index into the array.

 
 void RecordFileExtensionType(const base::FilePath& file) {
+  DCHECK_EQ(static_cast<base::FilePath::CharType*>(nullptr),
+            kDangerousFileTypes[EXTENSION_OTHER]);
+
+  int extension_type = EXTENSION_OTHER;
+  for (const auto& extension : kDangerousFileTypes) {
+    if (extension && file.MatchesExtension(extension)) {
+      extension_type = &extension - kDangerousFileTypes;
+      break;
+    }
+  }
+
   UMA_HISTOGRAM_ENUMERATION("SBClientDownload.DownloadExtensions",
-                            GetExtensionType(file),
-                            EXTENSION_MAX);
+                            extension_type, arraysize(kDangerousFileTypes));
 }
 
 // Enumerate for histogramming purposes.
 // DO NOT CHANGE THE ORDERING OF THESE VALUES (different histogram data will
 // be mixed together based on their values).
 enum SBStatsType {
   DOWNLOAD_URL_CHECKS_TOTAL,
   DOWNLOAD_URL_CHECKS_CANCELED,
   DOWNLOAD_URL_CHECKS_MALWARE,
 
@@ skipping 479 matching lines @@
 
   void CheckWhitelists() {
     DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
     if (!database_manager_.get()) {
       PostFinishTask(UNKNOWN, REASON_SB_DISABLED);
       return;
     }
 
     const GURL& url = url_chain_.back();
+    // TODO(asanka): This may acquire a lock on the SB DB on the IO thread.
     if (url.is_valid() && database_manager_->MatchDownloadWhitelistUrl(url)) {
       DVLOG(2) << url << " is on the download whitelist.";
       RecordCountOfSignedOrWhitelistedDownload();
       // TODO(grt): Continue processing without uploading so that
       // ClientDownloadRequest callbacks can be run even for this type of safe
       // download.
       PostFinishTask(SAFE, REASON_WHITELISTED_URL);
       return;
     }
 
@@ skipping 480 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