Neutralize dangerous subresource files during Save Page.

chrome/browser/download/chrome_download_manager_delegate.cc

Index: chrome/browser/download/chrome_download_manager_delegate.cc
diff --git a/chrome/browser/download/chrome_download_manager_delegate.cc b/chrome/browser/download/chrome_download_manager_delegate.cc
index 4819efc41ff4c4aad3f8e04a819c1181bbabcbcb..50a79b969ae9d168505037a4829c22fc37c52588 100644
--- a/chrome/browser/download/chrome_download_manager_delegate.cc
+++ b/chrome/browser/download/chrome_download_manager_delegate.cc
@@ -42,6 +42,8 @@
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/features.h"
 #include "chrome/common/pref_names.h"
+#include "chrome/common/safe_browsing/file_type_policies.h"
+#include "chrome/grit/generated_resources.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_member.h"
 #include "components/prefs/pref_service.h"
@@ -51,6 +53,7 @@
 #include "content/public/browser/page_navigator.h"
 #include "net/base/filename_util.h"
 #include "net/base/mime_util.h"
+#include "ui/base/l10n/l10n_util.h"
 
 #if BUILDFLAG(ANDROID_JAVA_UI)
 #include "chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.h"
@@ -432,6 +435,22 @@ void ChromeDownloadManagerDelegate::ChooseSavePath(
       callback);
 }
 
+void ChromeDownloadManagerDelegate::SanitizeSaveItemFilename(
+    base::FilePath* filename) {
+  safe_browsing::FileTypePolicies* file_type_policies =
+      safe_browsing::FileTypePolicies::GetInstance();
+  if (!file_type_policies)

[Nathan Parker, 2016/06/13 21:15:00]

This shouldn't be necessary.  The Singleton guarantees it.

+    return;
+
+  if (file_type_policies->GetFileDangerLevel(*filename) ==
+      safe_browsing::DownloadFileType::NOT_DANGEROUS)

[Nathan Parker, 2016/06/13 21:15:00]

So we're not actually checking with safe browsing here, we're just renaming any
file type that's potentially dangerous, correct?  That does mean that things
like .zips (NOT_DANGEROUS) won't be adjusted or scanned. I suspect that's ok,
since that'd be a lot of friction to overcome.... Except... if they followed
this with a .search-ms file to help the user find it.  I'm leaning towards
saying this is sufficient.

[asanka, 2016/06/14 21:24:07]

Yeah. I was a bit on the fence about what to do with supported-but-not-dangerous
file types.

These are:

[
  "bin",
  "pdf",
  "rtf",
  "001",
  "7z",
  "ace",
  "arc",
  "arj",
  "b64",
  "balz",
  "bhx",
  "bz",
  "bz2",
  "bzip2",
  "cab",
  "cpio",
  "fat",
  "gz",
  "gzip",
  "hfs",
  "hqx",
  "iso",
  "lha",
  "lpaq1",
  "lpaq5",
  "lpaq8",
  "lzh",
  "lzma",
  "mim",
  "ntfs",
  "paq8f",
  "paq8jd",
  "paq8l",
  "paq8o",
  "pea",
  "quad",
  "r00",
  "r01",
  "r02",
  "r03",
  "r04",
  "r05",
  "r06",
  "r07",
  "r08",
  "r09",
  "r10",
  "r11",
  "r12",
  "r13",
  "r14",
  "r15",
  "r16",
  "r17",
  "r18",
  "r19",
  "r20",
  "r21",
  "r22",
  "r23",
  "r24",
  "r25",
  "r26",
  "r27",
  "r28",
  "r29",
  "rar",
  "squashfs",
  "swm",
  "tar",
  "taz",
  "tbz",
  "tbz2",
  "tgz",
  "tpz",
  "txz",
  "tz",
  "udf",
  "uu",
  "uue",
  "vhd",
  "vhdx",
  "vmdk",
  "wim",
  "wrc",
  "xar",
  "xxe",
  "xz",
  "z",
  "zip",
  "zipx",
  "zpaq",
  "torrent",
  "svg",
  "apk"
]

By the way, why aren't the archive file types considered dangerous? If safe
browsing is enabled, then we won't warn since we wait to act on the SB ping. If
safe browsing is disabled shouldn't Chrome prompt for these file types?

[Nathan Parker, 2016/06/15 00:01:21]

While it's true that an archive can contain an executable, I think it would be
confusing to warn the user if we didn't KNOW it had an executable. I suspect
we'd lose credibility if we said "this .zip file can harm your computer" when
it's just full of text files.  Ya?

[asanka, 2016/06/16 18:35:16]

On 2016/06/15 at 00:01:21, Nathan Parker wrote:
[...]
True. The danger classifications for archives predate Chrome's ability to look
inside them. So at the time the only option was to warn the user.

Also, as far as the issue of archive formats as subresources; there's sufficient
friction here that I don't feel special handling is necessary.

+    return;
+
+  base::FilePath::FilePath default_filename = base::FilePath::FromUTF8Unsafe(
+      l10n_util::GetStringUTF8(IDS_DEFAULT_DOWNLOAD_FILENAME));
+  *filename = filename->AddExtension(default_filename.BaseName().value());
+}
+
 void ChromeDownloadManagerDelegate::OpenDownloadUsingPlatformHandler(
     DownloadItem* download) {
   base::FilePath platform_path(