Prevent files saved via the "Save as..." page menu item from

being named maliciously.  This is mainly copying some code from
the download manager because it seems like a pretty large task to
refactor the save-as code right now.

Here's a demo page:
http://ponderer.org/tests/title-with-.exe.html

Clean up the naming convention of register prefs for the
safe browsing service to make it more like the other
register methods.

Review URL: http://codereview.chromium.org/16523

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@7595 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 99 insertions, 12 deletions

diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc
index b4bdea9..bb80bf0 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -1121,15 +1121,7 @@ void DownloadManager::GenerateFilename(DownloadCreateInfo* info,
                                 L"download");
   DCHECK(!file_name.empty());
 
-  // Make sure we get the right file extension.
-  std::wstring extension;
-  GenerateExtension(file_name, info->mime_type, &extension);
-  file_util::ReplaceExtension(&file_name, extension);
-
-  // Prepend "_" to the file name if it's a reserved name
-  if (win_util::IsReservedName(file_name))
-    file_name = std::wstring(L"_") + file_name;
-
+  GenerateSafeFilename(info->mime_type, &file_name);
   generated_name->assign(file_name);
 }
 
@@ -1272,6 +1264,26 @@ void DownloadManager::DangerousDownloadValidated(DownloadItem* download) {
                         download->original_name()));
 }
 
+void DownloadManager::GenerateSafeFilename(const std::string& mime_type,
+                                           std::wstring* file_name) {
+  // Make sure we get the right file extension.
+  std::wstring extension;
+  GenerateExtension(*file_name, mime_type, &extension);
+  file_util::ReplaceExtension(file_name, extension);
+
+  // Prepend "_" to the file name if it's a reserved name
+  std::wstring leaf_name = file_util::GetFilenameFromPath(*file_name);
+  DCHECK(!leaf_name.empty());
+  if (win_util::IsReservedName(leaf_name)) {
+    file_util::UpOneDirectoryOrEmpty(file_name);
+    if (file_name->empty()) {
+      file_name->assign(std::wstring(L"_") + leaf_name);
+    } else {
+      file_util::AppendToPath(file_name, std::wstring(L"_") + leaf_name);
+    }
+  }
+}
+
 // Operations posted to us from the history service ----------------------------
 
 // The history service has retrieved all download entries. 'entries' contains
diff --git a/chrome/browser/download/download_manager.h b/chrome/browser/download/download_manager.h
index e2d3b5b..959fe82 100644
--- a/chrome/browser/download/download_manager.h
+++ b/chrome/browser/download/download_manager.h
@@ -407,6 +407,12 @@ class DownloadManager : public base::RefCountedThreadSafe<DownloadManager>,
   // Called when the user has validated the donwload of a dangerous file.
   void DangerousDownloadValidated(DownloadItem* download);
 
+  // Used to make sure we have a safe file extension and filename for a
+  // download.  |file_name| can either be just the file name or it can be a
+  // full path to a file.
+  void GenerateSafeFilename(const std::string& mime_type,
+                            std::wstring* file_name);
+
  private:
   // Shutdown the download manager.  This call is needed only after Init.
   void Shutdown();
diff --git a/chrome/browser/download/download_manager_unittest.cc b/chrome/browser/download/download_manager_unittest.cc
index d7648b9..21190eed 100644
--- a/chrome/browser/download/download_manager_unittest.cc
+++ b/chrome/browser/download/download_manager_unittest.cc
@@ -34,7 +34,9 @@ class DownloadManagerTest : public testing::Test {
   DISALLOW_EVIL_CONSTRUCTORS(DownloadManagerTest);
 };
 
-static const struct {
+namespace {
+
+const struct {
   const char* disposition;
   const wchar_t* url;
   const char* mime_type;
@@ -310,6 +312,8 @@ static const struct {
   // TODO(darin): Add some raw 8-bit Content-Disposition tests.
 };
 
+}  // namespace
+
 // Tests to ensure that the file names we generate from hints from the server
 // (content-disposition, URL name, etc) don't cause security holes.
 TEST_F(DownloadManagerTest, TestDownloadFilename) {
@@ -323,3 +327,56 @@ TEST_F(DownloadManagerTest, TestDownloadFilename) {
   }
 }
 
+namespace {
+
+const struct {
+  const wchar_t* path;
+  const char* mime_type;
+  const wchar_t* expected_path;
+} kSafeFilenameCases[] = {
+  { L"C:\\foo\\bar.htm",
+    "text/html",
+    L"C:\\foo\\bar.htm" },
+  { L"C:\\foo\\bar.html",
+    "text/html",
+    L"C:\\foo\\bar.html" },
+  { L"C:\\foo\\bar",
+    "text/html",
+    L"C:\\foo\\bar.htm" },
+
+  { L"C:\\bar.html",
+    "image/png",
+    L"C:\\bar.png" },
+  { L"C:\\bar",
+    "image/png",
+    L"C:\\bar.png" },
+
+  { L"C:\\foo\\bar.exe",
+    "text/html",
+    L"C:\\foo\\bar.htm" },
+  { L"C:\\foo\\bar.exe",
+    "image/gif",
+    L"C:\\foo\\bar.gif" },
+
+  { L"C:\\foo\\google.com",
+    "text/html",
+    L"C:\\foo\\google.htm" },
+
+  { L"C:\\foo\\con.htm",
+    "text/html",
+    L"C:\\foo\\_con.htm" },
+  { L"C:\\foo\\con",
+    "text/html",
+    L"C:\\foo\\_con.htm" },
+};
+
+}  // namespace
+
+TEST_F(DownloadManagerTest, GetSafeFilename) {
+  for (int i = 0; i < arraysize(kSafeFilenameCases); ++i) {
+    std::wstring path(kSafeFilenameCases[i].path);
+    download_manager_->GenerateSafeFilename(kSafeFilenameCases[i].mime_type,
+        &path);
+    EXPECT_EQ(kSafeFilenameCases[i].expected_path, path);
+  }
+}
diff --git a/chrome/browser/download/save_package.cc b/chrome/browser/download/save_package.cc
index d01d2d7..809ee17 100644
--- a/chrome/browser/download/save_package.cc
+++ b/chrome/browser/download/save_package.cc
@@ -916,7 +916,11 @@ std::wstring SavePackage::GetSuggestNameForSaveAs(PrefService* prefs,
 // Static.
 bool SavePackage::GetSaveInfo(const std::wstring& suggest_name,
                               HWND container_hwnd,
-                              SavePackageParam* param) {
+                              SavePackageParam* param,
+                              DownloadManager* download_manager) {
+  // TODO(tc): It might be nice to move this code into the download
+  // manager.  http://crbug.com/6025
+
   // Use "Web Page, Complete" option as default choice of saving page.
   unsigned index = 2;
 
@@ -944,6 +948,12 @@ bool SavePackage::GetSaveInfo(const std::wstring& suggest_name,
     // saved as complete-HTML.
     index = 1;
   }
+
+  DCHECK(download_manager);
+  // Ensure the filename is safe.
+  download_manager->GenerateSafeFilename(param->current_tab_mime_type,
+      &param->saved_main_file_path);
+
   // The option index is not zero-based.
   DCHECK(index > 0 && index < 3);
   param->dir = file_util::GetDirectoryFromPath(param->saved_main_file_path);
diff --git a/chrome/browser/download/save_package.h b/chrome/browser/download/save_package.h
index d7b76e9..6b53bb8 100644
--- a/chrome/browser/download/save_package.h
+++ b/chrome/browser/download/save_package.h
@@ -22,6 +22,7 @@
 class SaveFileManager;
 class SavePackage;
 class DownloadItem;
+class DownloadManager;
 class GURL;
 class MessageLoop;
 class PrefService;
@@ -158,7 +159,8 @@ class SavePackage : public base::RefCountedThreadSafe<SavePackage>,
   };
   static bool GetSaveInfo(const std::wstring& suggest_name,
                           HWND container_hwnd,
-                          SavePackageParam* param);
+                          SavePackageParam* param,
+                          DownloadManager* download_manager);
 
   // File name is consist of pure file name, dot and file extension name. File
   // name might has no dot and file extension, or has multiple dot inside file