Don't allow plugins to change file extensions arbitrarily

Currently extensions are allowed to use the download API to override
downloaded file extensions. This causes some security issues
as file with dangerous extensions can be override to safe extensions
and bypassing dangerous prompt. And there are also other possible
attacks using this feature.
This CL blocks plugings from changing the file extensions to an
arbitrary one. The mime type will be used to determine the final
extension. However, it is possible for plugins to remove the
extensions from the final file name.

BUG=989078

Change-Id: Idd28510d3db191f40bbe24256d64449856e4644f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1740048Reviewed-by: David Trainor <dtrainor@chromium.org>
Reviewed-by: Ben Hayden <benjhayden@chromium.org>
Commit-Queue: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#686498}

chrome/browser/download/download_target_determiner.cc

@@ -373,10 +373,14 @@ void DownloadTargetDeterminer::NotifyExtensionsDone(
     // Downloads/music/music/music/bar.mp3.
     base::FilePath new_path(download_prefs_->DownloadPath().Append(
         suggested_path).NormalizePathSeparators());
-    // Do not pass a mime type to GenerateSafeFileName so that it does not force
-    // the filename to have an extension if the (Chrome) extension does not
-    // suggest it.
-    net::GenerateSafeFileName(std::string(), false, &new_path);
+    // If the (Chrome) extension does not suggest an file extension, do not pass
+    // a mime type to GenerateSafeFileName so that it does not force the
+    // filename to have an extension. Otherwise, correct the file extension in
+    // case it is wrongly given.
+    if (new_path.Extension().empty())
+      net::GenerateSafeFileName(std::string(), false, &new_path);
+    else
+      net::GenerateSafeFileName(download_->GetMimeType(), true, &new_path);
     virtual_path_ = new_path;
     create_target_directory_ = true;
   }

chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc

@@ -2329,14 +2329,14 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
                           "  \"paused\": false,"
                           "  \"url\": \"%s\"}]",
                           download_url.c_str())));
-  ASSERT_TRUE(WaitFor(downloads::OnChanged::kEventName,
-                      base::StringPrintf(
-                          "[{\"id\": %d,"
-                          "  \"filename\": {"
-                          "    \"previous\": \"\","
-                          "    \"current\": \"%s\"}}]",
-                          result_id,
-                          GetFilename("file.txt").c_str())));
+  // File will be renamed to file.html due to its mime type.
+  ASSERT_TRUE(
+      WaitFor(downloads::OnChanged::kEventName,
+              base::StringPrintf("[{\"id\": %d,"
+                                 "  \"filename\": {"
+                                 "    \"previous\": \"\","
+                                 "    \"current\": \"%s\"}}]",
+                                 result_id, GetFilename("file.html").c_str())));
   ASSERT_TRUE(WaitFor(downloads::OnChanged::kEventName,
                       base::StringPrintf(
                           "[{\"id\": %d,"
@@ -3008,6 +3008,8 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
   EXPECT_FALSE(determine_result.get());  // No return value.
 }
 
+// Tests that overriding a safe file extension to a dangerous extension will not
+// trigger the dangerous prompt and will not change the extension.
 IN_PROC_BROWSER_TEST_F(
     DownloadExtensionTest,
     DownloadExtensionTest_OnDeterminingFilename_DangerousOverride) {
@@ -3047,7 +3049,7 @@ IN_PROC_BROWSER_TEST_F(
   ASSERT_TRUE(item->GetTargetFilePath().empty());
   ASSERT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
 
-  // Respond to the onDeterminingFilename.
+  // Respond to the onDeterminingFilename with a dangerous extension.
   std::string error;
   ASSERT_TRUE(ExtensionDownloadsEventRouter::DetermineFilename(
       current_browser()->profile(), false, GetExtensionId(), result_id,
@@ -3056,12 +3058,68 @@ IN_PROC_BROWSER_TEST_F(
   EXPECT_EQ("", error);
 
   ASSERT_TRUE(WaitFor(downloads::OnChanged::kEventName,
-                      base::StringPrintf(
-                          "[{\"id\": %d,"
-                          "  \"danger\": {"
-                          "    \"previous\":\"safe\","
-                          "    \"current\":\"file\"}}]",
-                          result_id)));
+                      base::StringPrintf("[{\"id\": %d,"
+                                         "  \"state\": {"
+                                         "    \"previous\": \"in_progress\","
+                                         "    \"current\": \"complete\"}}]",
+                                         result_id)));
+  EXPECT_EQ(downloads_directory().AppendASCII("overridden.txt"),
+            item->GetTargetFilePath());
+}
+
+// Tests that overriding a dangerous file extension to a safe extension will
+// trigger the dangerous prompt and will not change the extension.
+IN_PROC_BROWSER_TEST_F(
+    DownloadExtensionTest,
+    DownloadExtensionTest_OnDeterminingFilename_SafeOverride) {
+  GoOnTheRecord();
+  LoadExtension("downloads_split");
+  AddFilenameDeterminer();
+
+  std::string download_url = "data:application/x-shockwave-flash,";
+  // Start downloading a file.
+  std::unique_ptr<base::Value> result(RunFunctionAndReturnResult(
+      new DownloadsDownloadFunction(),
+      base::StringPrintf("[{\"url\": \"%s\"}]", download_url.c_str())));
+  ASSERT_TRUE(result.get());
+  int result_id = -1;
+  ASSERT_TRUE(result->GetAsInteger(&result_id));
+  DownloadItem* item = GetCurrentManager()->GetDownload(result_id);
+  ASSERT_TRUE(item);
+  ScopedCancellingItem canceller(item);
+  ASSERT_EQ(download_url, item->GetOriginalUrl().spec());
+
+  ASSERT_TRUE(WaitFor(
+      downloads::OnCreated::kEventName,
+      base::StringPrintf("[{\"danger\": \"safe\","
+                         "  \"incognito\": false,"
+                         "  \"id\": %d,"
+                         "  \"mime\": \"application/x-shockwave-flash\","
+                         "  \"paused\": false,"
+                         "  \"url\": \"%s\"}]",
+                         result_id, download_url.c_str())));
+  ASSERT_TRUE(WaitFor(downloads::OnDeterminingFilename::kEventName,
+                      base::StringPrintf("[{\"id\": %d,"
+                                         "  \"filename\":\"download.swf\"}]",
+                                         result_id)));
+  ASSERT_TRUE(item->GetTargetFilePath().empty());
+  ASSERT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
+
+  // Respond to the onDeterminingFilename with a safe extension.
+  std::string error;
+  ASSERT_TRUE(ExtensionDownloadsEventRouter::DetermineFilename(
+      current_browser()->profile(), false, GetExtensionId(), result_id,
+      base::FilePath(FILE_PATH_LITERAL("overridden.txt")),
+      downloads::FILENAME_CONFLICT_ACTION_UNIQUIFY, &error));
+  EXPECT_EQ("", error);
+
+  // Dangerous download prompt will be shown.
+  ASSERT_TRUE(WaitFor(downloads::OnChanged::kEventName,
+                      base::StringPrintf("[{\"id\": %d, "
+                                         "  \"danger\": {"
+                                         "    \"previous\": \"safe\","
+                                         "    \"current\": \"file\"}}]",
+                                         result_id)));
 
   item->ValidateDangerousDownload();
   ASSERT_TRUE(WaitFor(downloads::OnChanged::kEventName,
@@ -3071,6 +3129,7 @@ IN_PROC_BROWSER_TEST_F(
                           "    \"previous\":\"file\","
                           "    \"current\":\"accepted\"}}]",
                           result_id)));
+
   ASSERT_TRUE(WaitFor(downloads::OnChanged::kEventName,
                       base::StringPrintf(
                           "[{\"id\": %d,"
@@ -4342,7 +4401,8 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
   LoadExtension("downloads_split");
   std::unique_ptr<base::Value> result(RunFunctionAndReturnResult(
       new DownloadsDownloadFunction(),
-      "[{\"url\": \"data:,\", \"filename\": \"dangerous.swf\"}]"));
+      "[{\"url\": \"data:application/x-shockwave-flash,\", \"filename\": "
+      "\"dangerous.swf\"}]"));
   ASSERT_TRUE(result.get());
   int result_id = -1;
   ASSERT_TRUE(result->GetAsInteger(&result_id));