[Downloads] Prevent dangerous files from being opened automatically.

chrome/browser/download/download_extensions.cc

Index: chrome/browser/download/download_extensions.cc
diff --git a/chrome/browser/download/download_extensions.cc b/chrome/browser/download/download_extensions.cc
index 623bdf48d855f651fcc7ef343522d450bd9a5fba..85fad633a85ad8fe5996c83683cd7420c62cec84 100644
--- a/chrome/browser/download/download_extensions.cc
+++ b/chrome/browser/download/download_extensions.cc
@@ -57,157 +57,184 @@ namespace download_util {
  *
  * ***** END LICENSE BLOCK ***** */
 
-static const struct Executables {
-    const char* extension;
-    DownloadDangerLevel level;
-} g_executables[] = {
-  // Some files are dangerous on all platforms.
+namespace {
+
+enum DownloadDangerFlags {

[Randy Smith (Not in Mondays), 2015/06/09 19:40:04]

Jamming these together with DownloadDangerLevel feels very weird to me.  Is
there any reason, given that g_executables is a static, that we couldn't just
add another entry to the Executable structure?

[asanka, 2015/06/12 20:12:52]

Yeah. I see what you mean. I split it out into a separate field.

+  // The file type should not be allowed to open automatically.
+  //
+  // Includes:
+  //   * Executables that are likely to start and execute arbitrary privileged
+  //     code without requiring user interaction.
+  //   * Files that aren't traditionally executables, but are handled by
+  //     applications that have a less than stellar safety track record. Such
+  //     application may allow arbitrary code execution upon opening a file.
+  //   * Files that don't have executable code, but may modify system
+  //     configuration in ways that are unsafe.
   //
-  // Flash files downloaded locally can sometimes access the local filesystem.
-  { "swf", DANGEROUS },
-  { "spl", DANGEROUS },
-  // Chrome extensions should be obtained through the web store.
-  { "crx", ALLOW_ON_USER_GESTURE },
+  // Doesn't include:
+  //   * Files containing executable code if opening said file is mediated via
+  //     a trusted application that obtains user consent.
+  EXCLUDE_FROM_AUTO_OPEN = 1 << 4,
+
+  // Special flag that indicates that the file path didn't have an extension.
+  NO_EXTENSION = 1 << 5,
+
+  // Mask for extracting DownloadDangerLevel from DownloadDangerFlags.
+  DOWNLOAD_DANGER_LEVEL_MASK = 0xf
+};
+
+static const struct Executable {
+  const char* extension;   // Extension sans leading extension separator.
+  int danger_level_flags;  // Bit combination of DownloadDangerLevel and
+                           // DownloadDangerFlags.
+} g_executables[] = {
+    // Some files are dangerous on all platforms.
+    //
+    // Flash files downloaded locally can sometimes access the local filesystem.
+    {"swf", DANGEROUS | EXCLUDE_FROM_AUTO_OPEN},
+    {"spl", DANGEROUS | EXCLUDE_FROM_AUTO_OPEN},
+    // Chrome extensions should be obtained through the web store.
+    {"crx", ALLOW_ON_USER_GESTURE},
 
   // Windows, all file categories.
 #if defined(OS_WIN)
-  { "ad", ALLOW_ON_USER_GESTURE },
-  { "ade", ALLOW_ON_USER_GESTURE },
-  { "adp", ALLOW_ON_USER_GESTURE },
-  { "app", ALLOW_ON_USER_GESTURE },
-  { "application", ALLOW_ON_USER_GESTURE },
-  { "asp", ALLOW_ON_USER_GESTURE },
-  { "asx", ALLOW_ON_USER_GESTURE },
-  { "bas", ALLOW_ON_USER_GESTURE },
-  { "bat", ALLOW_ON_USER_GESTURE },
-  { "cfg", DANGEROUS },
-  { "chi", ALLOW_ON_USER_GESTURE },
-  { "chm", ALLOW_ON_USER_GESTURE },
-  { "cmd", ALLOW_ON_USER_GESTURE },
-  { "com", ALLOW_ON_USER_GESTURE },
-  { "cpl", ALLOW_ON_USER_GESTURE },
-  { "crt", ALLOW_ON_USER_GESTURE },
-  { "dll", DANGEROUS },
-  { "drv", DANGEROUS },
-  { "exe", ALLOW_ON_USER_GESTURE },
-  { "fxp", ALLOW_ON_USER_GESTURE },
-  { "grp", DANGEROUS },
-  { "hlp", ALLOW_ON_USER_GESTURE },
-  { "hta", ALLOW_ON_USER_GESTURE },
-  { "htt", ALLOW_ON_USER_GESTURE },
-  { "inf", ALLOW_ON_USER_GESTURE },
-  { "ini", DANGEROUS },
-  { "ins", ALLOW_ON_USER_GESTURE },
-  { "isp", ALLOW_ON_USER_GESTURE },
-  { "js", ALLOW_ON_USER_GESTURE },
-  { "jse", ALLOW_ON_USER_GESTURE },
-  { "lnk", ALLOW_ON_USER_GESTURE },
-  { "local", DANGEROUS },
-  { "mad", ALLOW_ON_USER_GESTURE },
-  { "maf", ALLOW_ON_USER_GESTURE },
-  { "mag", ALLOW_ON_USER_GESTURE },
-  { "mam", ALLOW_ON_USER_GESTURE },
-  { "manifest", DANGEROUS },
-  { "maq", ALLOW_ON_USER_GESTURE },
-  { "mar", ALLOW_ON_USER_GESTURE },
-  { "mas", ALLOW_ON_USER_GESTURE },
-  { "mat", ALLOW_ON_USER_GESTURE },
-  { "mau", ALLOW_ON_USER_GESTURE },
-  { "mav", ALLOW_ON_USER_GESTURE },
-  { "maw", ALLOW_ON_USER_GESTURE },
-  { "mda", ALLOW_ON_USER_GESTURE },
-  { "mdb", ALLOW_ON_USER_GESTURE },
-  { "mde", ALLOW_ON_USER_GESTURE },
-  { "mdt", ALLOW_ON_USER_GESTURE },
-  { "mdw", ALLOW_ON_USER_GESTURE },
-  { "mdz", ALLOW_ON_USER_GESTURE },
-  { "mht", ALLOW_ON_USER_GESTURE },
-  { "mhtml", ALLOW_ON_USER_GESTURE },
-  { "mmc", ALLOW_ON_USER_GESTURE },
-  { "mof", DANGEROUS },
-  { "msc", ALLOW_ON_USER_GESTURE },
-  { "msh", ALLOW_ON_USER_GESTURE },
-  { "mshxml", ALLOW_ON_USER_GESTURE },
-  { "msi", ALLOW_ON_USER_GESTURE },
-  { "msp", ALLOW_ON_USER_GESTURE },
-  { "mst", ALLOW_ON_USER_GESTURE },
-  { "ocx", DANGEROUS },
-  { "ops", ALLOW_ON_USER_GESTURE },
-  { "pcd", ALLOW_ON_USER_GESTURE },
-  { "pif", ALLOW_ON_USER_GESTURE },
-  { "plg", ALLOW_ON_USER_GESTURE },
-  { "prf", ALLOW_ON_USER_GESTURE },
-  { "prg", ALLOW_ON_USER_GESTURE },
-  { "pst", ALLOW_ON_USER_GESTURE },
-  { "reg", ALLOW_ON_USER_GESTURE },
-  { "scf", ALLOW_ON_USER_GESTURE },
-  { "scr", ALLOW_ON_USER_GESTURE },
-  { "sct", ALLOW_ON_USER_GESTURE },
-  { "shb", ALLOW_ON_USER_GESTURE },
-  { "shs", ALLOW_ON_USER_GESTURE },
-  { "sys", DANGEROUS },
-  { "url", DANGEROUS },
-  { "vb", ALLOW_ON_USER_GESTURE },
-  { "vbe", ALLOW_ON_USER_GESTURE },
-  { "vbs", ALLOW_ON_USER_GESTURE },
-  { "vsd", ALLOW_ON_USER_GESTURE },
-  { "vsmacros", ALLOW_ON_USER_GESTURE },
-  { "vss", ALLOW_ON_USER_GESTURE },
-  { "vst", ALLOW_ON_USER_GESTURE },
-  { "vsw", ALLOW_ON_USER_GESTURE },
-  { "ws", ALLOW_ON_USER_GESTURE },
-  { "wsc", ALLOW_ON_USER_GESTURE },
-  { "wsf", ALLOW_ON_USER_GESTURE },
-  { "wsh", ALLOW_ON_USER_GESTURE },
-  { "xbap", DANGEROUS },
+    {"ad", ALLOW_ON_USER_GESTURE},
+    {"ade", ALLOW_ON_USER_GESTURE},
+    {"adp", ALLOW_ON_USER_GESTURE},
+    {"app", ALLOW_ON_USER_GESTURE},
+    {"application", ALLOW_ON_USER_GESTURE},

[Randy Smith (Not in Mondays), 2015/06/09 19:40:04]

Naively, I would have thought that something named .application would be an
executable that shouldn't be auto-opened; do you know something I don't?

[asanka, 2015/06/12 20:12:52]

I added some details in the form of comments. Let me know if it helps.

+    {"asp", ALLOW_ON_USER_GESTURE},
+    {"asx", ALLOW_ON_USER_GESTURE},
+    {"bas", ALLOW_ON_USER_GESTURE},
+    {"bat", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"cfg", DANGEROUS | EXCLUDE_FROM_AUTO_OPEN},
+    {"chi", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"chm", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"cmd", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"com", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"cpl", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"crt", ALLOW_ON_USER_GESTURE},
+    {"dll", DANGEROUS | EXCLUDE_FROM_AUTO_OPEN},
+    {"drv", DANGEROUS | EXCLUDE_FROM_AUTO_OPEN},
+    {"exe", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"fxp", ALLOW_ON_USER_GESTURE},
+    {"grp", DANGEROUS},
+    {"hlp", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},

[Randy Smith (Not in Mondays), 2015/06/09 19:40:04]

This is something I could easily imagine users wanting to auto-open, and I don't
see the danger; why block it?

[asanka, 2015/06/12 20:12:52]

This was due to the known default file handler having multiple vulnerabilities
in the past. However, I did remove the flag for this round.

+    {"hta", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"htt", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"inf", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"ini", DANGEROUS | EXCLUDE_FROM_AUTO_OPEN},
+    {"ins", ALLOW_ON_USER_GESTURE},
+    {"isp", ALLOW_ON_USER_GESTURE},
+    {"js", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"jse", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"lnk", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"local", DANGEROUS},
+    {"mad", ALLOW_ON_USER_GESTURE},
+    {"maf", ALLOW_ON_USER_GESTURE},
+    {"mag", ALLOW_ON_USER_GESTURE},
+    {"mam", ALLOW_ON_USER_GESTURE},
+    {"manifest", DANGEROUS},
+    {"maq", ALLOW_ON_USER_GESTURE},
+    {"mar", ALLOW_ON_USER_GESTURE},
+    {"mas", ALLOW_ON_USER_GESTURE},
+    {"mat", ALLOW_ON_USER_GESTURE},
+    {"mau", ALLOW_ON_USER_GESTURE},
+    {"mav", ALLOW_ON_USER_GESTURE},
+    {"maw", ALLOW_ON_USER_GESTURE},
+    {"mda", ALLOW_ON_USER_GESTURE},
+    {"mdb", ALLOW_ON_USER_GESTURE},
+    {"mde", ALLOW_ON_USER_GESTURE},
+    {"mdt", ALLOW_ON_USER_GESTURE},
+    {"mdw", ALLOW_ON_USER_GESTURE},
+    {"mdz", ALLOW_ON_USER_GESTURE},
+    {"mht", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"mhtml", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},

[Randy Smith (Not in Mondays), 2015/06/09 19:40:04]

Does opening a web page on the disk give it some special privileges?  This is
another case in which I could imagine users wanting auto-open.  (Applies to
other archive formats too.)

[asanka, 2015/06/12 20:12:52]

Removed the flag.

+    {"mmc", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"mof", DANGEROUS | EXCLUDE_FROM_AUTO_OPEN},
+    {"msc", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"msh", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"mshxml", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"msi", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"msp", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"mst", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"ocx", DANGEROUS | EXCLUDE_FROM_AUTO_OPEN},
+    {"ops", ALLOW_ON_USER_GESTURE},
+    {"pcd", ALLOW_ON_USER_GESTURE},
+    {"pif", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"plg", ALLOW_ON_USER_GESTURE},
+    {"prf", ALLOW_ON_USER_GESTURE},
+    {"prg", ALLOW_ON_USER_GESTURE},
+    {"pst", ALLOW_ON_USER_GESTURE},
+    {"reg", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"scf", ALLOW_ON_USER_GESTURE},
+    {"scr", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"sct", ALLOW_ON_USER_GESTURE},
+    {"shb", ALLOW_ON_USER_GESTURE},
+    {"shs", ALLOW_ON_USER_GESTURE},
+    {"sys", DANGEROUS | EXCLUDE_FROM_AUTO_OPEN},
+    {"url", DANGEROUS | EXCLUDE_FROM_AUTO_OPEN},

[Randy Smith (Not in Mondays), 2015/06/09 19:40:04]

Why?  I'm not sure why people would want to download a URL, but I'd think that
if a web page had nefarious intent, it could just do a "document.location =". 
What cases am I missing?

[asanka, 2015/06/12 20:12:52]

A .url file is an OLE persisted container that can point at a lot more things
than what 'document.location'.

+    {"vb", ALLOW_ON_USER_GESTURE},

[Randy Smith (Not in Mondays), 2015/06/09 19:40:04]

Why isn't open a .vb file dangerous?  Does it always open in the IDE?

[asanka, 2015/06/12 20:12:52]

.vb (not to be confused with .vbs) isn't associated with the Windows Script
Host. It's usually associated with an Visual Studio.

+    {"vbe", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"vbs", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"vsd", ALLOW_ON_USER_GESTURE},
+    {"vsmacros", ALLOW_ON_USER_GESTURE},

[Randy Smith (Not in Mondays), 2015/06/09 19:40:04]

You know Windows (much) better than I, but I'd think there's executable
nefariousness to be had in almost any macro language.

[asanka, 2015/06/12 20:12:52]

As far as I could discover, opening this type of file doesn't automatically
execute code until the macro is invoked.

+    {"vss", ALLOW_ON_USER_GESTURE},
+    {"vst", ALLOW_ON_USER_GESTURE},
+    {"vsw", ALLOW_ON_USER_GESTURE},
+    {"ws", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"wsc", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"wsf", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"wsh", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"xbap", DANGEROUS | EXCLUDE_FROM_AUTO_OPEN},
 #endif  // OS_WIN
 
   // Java.
 #if !defined(OS_CHROMEOS)
-  { "class", DANGEROUS },
-  { "jar", DANGEROUS },
-  { "jnlp", DANGEROUS },
+    {"class", DANGEROUS | EXCLUDE_FROM_AUTO_OPEN},
+    {"jar", DANGEROUS | EXCLUDE_FROM_AUTO_OPEN},
+    {"jnlp", DANGEROUS | EXCLUDE_FROM_AUTO_OPEN},
 #endif
 
   // Scripting languages. (Shells are handled below.)
 #if !defined(OS_CHROMEOS) && !defined(OS_ANDROID)
-  { "pl", ALLOW_ON_USER_GESTURE },
-  { "py", ALLOW_ON_USER_GESTURE },
-  { "pyc", ALLOW_ON_USER_GESTURE },
-  { "pyw", ALLOW_ON_USER_GESTURE },
-  { "rb", ALLOW_ON_USER_GESTURE },
+    {"pl", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"py", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"pyc", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"pyw", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"rb", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
 #endif
 
   // Shell languages. (OS_ANDROID is OS_POSIX.) OS_WIN shells are handled above.
 #if defined(OS_POSIX)
-  { "bash", ALLOW_ON_USER_GESTURE },
-  { "csh", ALLOW_ON_USER_GESTURE },
-  { "ksh", ALLOW_ON_USER_GESTURE },
-  { "sh", ALLOW_ON_USER_GESTURE },
-  { "shar", ALLOW_ON_USER_GESTURE },
-  { "tcsh", ALLOW_ON_USER_GESTURE },
+    {"bash", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"csh", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"ksh", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"sh", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"shar", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
+    {"tcsh", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
 #endif
 #if defined(OS_MACOSX)
-  { "command", ALLOW_ON_USER_GESTURE },
+    {"command", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
 #endif
 
   // Package management formats. OS_WIN package formats are handled above.
 #if defined(OS_MACOSX) || defined(OS_LINUX)
-  { "pkg", ALLOW_ON_USER_GESTURE },
+    {"pkg", ALLOW_ON_USER_GESTURE},

[Randy Smith (Not in Mondays), 2015/06/09 19:40:04]

Hmmm.  This is right at the boundary for me--I can definitely see the user
wanting to auto-execute, but it can do almost arbitrary bad things, right?  

[asanka, 2015/06/12 20:12:52]

Added.

 #endif
 #if defined(OS_LINUX)
-  { "deb", ALLOW_ON_USER_GESTURE },
-  { "rpm", ALLOW_ON_USER_GESTURE },
+    {"deb", ALLOW_ON_USER_GESTURE},
+    {"rpm", ALLOW_ON_USER_GESTURE},

[Randy Smith (Not in Mondays), 2015/06/09 19:40:04]

I think that "opening" a package on Linux installs it, which I believe includes
running a script, so I think these are executable equivalents.

[asanka, 2015/06/12 20:12:52]

Added.

 #endif
 #if defined(OS_ANDROID)
-  { "dex", ALLOW_ON_USER_GESTURE },  // Really an executable format.
+    {"dex", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},

[palmer, 2015/06/08 19:05:07]

I'm actually not sure if Android even would directly run a dex, or what would
happen. Obviously it's maximally safe to specify EXCLUDE_FROM_AUTO_OPEN here.

[asanka, 2015/06/09 00:38:10]

Yeah. I added the flag for a couple of file types that don't make sense to open
directly. This should make no difference if the files can't be opened directly.
If they can (by, say, some custom editor), then users won't be able to auto open
them.

 #endif
 };
 
-DownloadDangerLevel GetFileDangerLevel(const base::FilePath& path) {
+int GetFileDangerFlags(const base::FilePath& path) {
   base::FilePath::StringType extension(path.FinalExtension());
   if (extension.empty())
-    return NOT_DANGEROUS;
+    return NO_EXTENSION;
   if (!base::IsStringASCII(extension))
     return NOT_DANGEROUS;
 #if defined(OS_WIN)
@@ -220,13 +247,26 @@ DownloadDangerLevel GetFileDangerLevel(const base::FilePath& path) {
   if (ascii_extension[0] == base::FilePath::kExtensionSeparator)
     ascii_extension.erase(0, 1);
 
-  for (size_t i = 0; i < arraysize(g_executables); ++i) {
-    if (LowerCaseEqualsASCII(ascii_extension, g_executables[i].extension))
-      return g_executables[i].level;
+  for (const auto& file : g_executables) {
+    if (LowerCaseEqualsASCII(ascii_extension, file.extension))
+      return file.danger_level_flags;
   }
+
   return NOT_DANGEROUS;
 }
 
+}  // namespace
+
+DownloadDangerLevel GetFileDangerLevel(const base::FilePath& path) {
+  int flags = GetFileDangerFlags(path);
+  return static_cast<DownloadDangerLevel>(flags & DOWNLOAD_DANGER_LEVEL_MASK);
+}
+
+bool IsAllowedToOpenAutomatically(const base::FilePath& path) {
+  int flags = GetFileDangerFlags(path);
+  return !(flags == NO_EXTENSION || (flags & EXCLUDE_FROM_AUTO_OPEN));
+}
+
 static const char* kExecutableWhiteList[] = {
   // JavaScript is just as powerful as EXE.
   "text/javascript",
@@ -260,5 +300,4 @@ bool IsExecutableMimeType(const std::string& mime_type) {
   return net::MatchesMimeType("application/*", mime_type);
 }
 
-
 }  // namespace download_util