Improve desktop shortcut creation:

- remove more comments (which generally only apply to the browser itself)
- add #!/usr/bin/env xdg-open shebang
- make the .desktop file placed on the desktop executable
- add more tests to make sure we're still secure

TEST=Covered by unit_tests.
BUG=22589

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

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

2 files changed, 125 insertions, 29 deletions

diff --git a/chrome/browser/shell_integration_linux.cc b/chrome/browser/shell_integration_linux.cc
index 78c8443..dab372f 100644
--- a/chrome/browser/shell_integration_linux.cc
+++ b/chrome/browser/shell_integration_linux.cc
@@ -14,6 +14,7 @@
 #include <vector>
 
 #include "base/command_line.h"
+#include "base/eintr_wrapper.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/message_loop.h"
@@ -117,16 +118,65 @@ class CreateDesktopShortcutTask : public Task {
     if (!GetDesktopShortcutTemplate(&template_contents))
       return;
 
+    FilePath shortcut_filename =
+        ShellIntegration::GetDesktopShortcutFilename(shortcut_info_.url);
+
     std::string contents = ShellIntegration::GetDesktopFileContents(
         template_contents, shortcut_info_.url, shortcut_info_.title);
 
+    if (shortcut_info_.create_on_desktop)
+      CreateOnDesktop(shortcut_filename, contents);
+
+    if (shortcut_info_.create_in_applications_menu)
+      CreateInApplicationsMenu(shortcut_filename, contents);
+  }
+
+ private:
+  void CreateOnDesktop(const FilePath& shortcut_filename,
+                       const std::string& contents) {
+    // TODO(phajdan.jr): Report errors from this function, possibly as infobars.
+
+    // Make sure that we will later call openat in a secure way.
+    DCHECK_EQ(shortcut_filename.BaseName().value(), shortcut_filename.value());
+
+    FilePath desktop_path;
+    if (!PathService::Get(chrome::DIR_USER_DESKTOP, &desktop_path))
+      return;
+
+    int desktop_fd = open(desktop_path.value().c_str(), O_RDONLY | O_DIRECTORY);
+    if (desktop_fd < 0)
+      return;
+
+    int fd = openat(desktop_fd, shortcut_filename.value().c_str(),
+                    O_CREAT | O_EXCL | O_WRONLY,
+                    S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);
+    if (fd < 0) {
+      HANDLE_EINTR(close(desktop_fd));
+      return;
+    }
+
+    ssize_t bytes_written = file_util::WriteFileDescriptor(fd, contents.data(),
+                                                           contents.length());
+    HANDLE_EINTR(close(fd));
+
+    if (bytes_written != static_cast<ssize_t>(contents.length())) {
+      // Delete the file. No shortuct is better than corrupted one. Use unlinkat
+      // to make sure we're deleting the file in the directory we think we are.
+      // Even if an attacker manager to put something other at
+      // |shortcut_filename| we'll just undo his action.
+      unlinkat(desktop_fd, shortcut_filename.value().c_str(), 0);
+    }
+
+    HANDLE_EINTR(close(desktop_fd));
+  }
+
+  void CreateInApplicationsMenu(const FilePath& shortcut_filename,
+                                const std::string& contents) {
+    // TODO(phajdan.jr): Report errors from this function, possibly as infobars.
     ScopedTempDir temp_dir;
     if (!temp_dir.CreateUniqueTempDir())
       return;
 
-    FilePath shortcut_filename =
-        ShellIntegration::GetDesktopShortcutFilename(shortcut_info_.url);
-
     FilePath temp_file_path = temp_dir.path().Append(shortcut_filename);
 
     int bytes_written = file_util::WriteFile(temp_file_path, contents.data(),
@@ -135,32 +185,19 @@ class CreateDesktopShortcutTask : public Task {
     if (bytes_written != static_cast<int>(contents.length()))
         return;
 
-    if (shortcut_info_.create_on_desktop) {
-      FilePath desktop_path;
-      if (!PathService::Get(chrome::DIR_USER_DESKTOP, &desktop_path))
-        return;
-      desktop_path = desktop_path.Append(shortcut_filename);
-
-      if (!file_util::PathExists(desktop_path))
-        file_util::CopyFile(temp_file_path, desktop_path);
-    }
-
-    if (shortcut_info_.create_in_applications_menu) {
-      std::vector<std::string> argv;
-      argv.push_back("xdg-desktop-menu");
-      argv.push_back("install");
+    std::vector<std::string> argv;
+    argv.push_back("xdg-desktop-menu");
+    argv.push_back("install");
 
-      // Always install in user mode, even if someone runs the browser as root
-      // (people do that).
-      argv.push_back("--mode");
-      argv.push_back("user");
+    // Always install in user mode, even if someone runs the browser as root
+    // (people do that).
+    argv.push_back("--mode");
+    argv.push_back("user");
 
-      argv.push_back(temp_file_path.value());
-      LaunchXdgUtility(argv);
-    }
+    argv.push_back(temp_file_path.value());
+    LaunchXdgUtility(argv);
   }
 
- private:
   const ShellIntegration::ShortcutInfo shortcut_info_;
 
   DISALLOW_COPY_AND_ASSIGN(CreateDesktopShortcutTask);
@@ -226,7 +263,9 @@ std::string ShellIntegration::GetDesktopFileContents(
     const std::string& template_contents, const GURL& url,
     const string16& title) {
   // See http://standards.freedesktop.org/desktop-entry-spec/latest/
-  std::string output_buffer;
+  // Although not required by the spec, Nautilus on Ubuntu Karmic creates its
+  // launchers with an xdg-open shebang. Follow that convention.
+  std::string output_buffer("#!/usr/bin/env xdg-open\n");
   StringTokenizer tokenizer(template_contents, "\n");
   while (tokenizer.GetNext()) {
     // TODO(phajdan.jr): Add the icon.
@@ -243,7 +282,10 @@ std::string ShellIntegration::GetDesktopFileContents(
       std::string app_switch(StringPrintf("\"--%s=%s\"",
                                           WideToUTF8(app_switch_wide).c_str(),
                                           url.spec().c_str()));
+      // Sanitize the command line string.
       ReplaceSubstringsAfterOffset(&app_switch, 0, "%", "%%");
+      ReplaceSubstringsAfterOffset(&app_switch, 0, ";", "");
+      ReplaceSubstringsAfterOffset(&app_switch, 0, "$", "");
       output_buffer += std::string("Exec=") + final_path + app_switch + "\n";
     } else if (tokenizer.token().substr(0, 5) == "Name=") {
       std::string final_title = UTF16ToUTF8(title);
@@ -255,8 +297,10 @@ std::string ShellIntegration::GetDesktopFileContents(
           final_title.find("\r") != std::string::npos)
         final_title = url.spec();
       output_buffer += StringPrintf("Name=%s\n", final_title.c_str());
-    } else if (tokenizer.token().substr(0, 8) == "Comment=") {
-      // Skip the line.
+    } else if (tokenizer.token().substr(0, 11) == "GenericName" ||
+               tokenizer.token().substr(0, 7) == "Comment" ||
+               tokenizer.token().substr(0, 1) == "#") {
+      // Skip comment lines.
     } else {
       output_buffer += tokenizer.token() + "\n";
     }
diff --git a/chrome/browser/shell_integration_unittest.cc b/chrome/browser/shell_integration_unittest.cc
index 6f0d71d..3b01c30 100644
--- a/chrome/browser/shell_integration_unittest.cc
+++ b/chrome/browser/shell_integration_unittest.cc
@@ -44,7 +44,7 @@ TEST(ShellIntegrationTest, GetDesktopFileContents) {
     const char* expected_output;
   } test_cases[] = {
     // Dumb case.
-    { "ignored", "ignored", "", "" },
+    { "ignored", "ignored", "", "#!/usr/bin/env xdg-open\n" },
 
     // Real-world case.
     { "http://gmail.com",
@@ -62,6 +62,7 @@ TEST(ShellIntegrationTest, GetDesktopFileContents) {
       "Categories=Application;Network;WebBrowser;\n"
       "MimeType=text/html;text/xml;application/xhtml_xml;\n",
 
+      "#!/usr/bin/env xdg-open\n"
       "[Desktop Entry]\n"
       "Version=1.0\n"
       "Encoding=UTF-8\n"
@@ -74,6 +75,32 @@ TEST(ShellIntegrationTest, GetDesktopFileContents) {
       "MimeType=text/html;text/xml;application/xhtml_xml;\n"
     },
 
+    // Make sure we don't insert duplicate shebangs.
+    { "http://gmail.com",
+      "GMail",
+
+      "#!/some/shebang\n"
+      "Name=Google Chrome\n"
+      "Exec=/opt/google/chrome/google-chrome %U\n",
+
+      "#!/usr/bin/env xdg-open\n"
+      "Name=GMail\n"
+      "Exec=/opt/google/chrome/google-chrome \"--app=http://gmail.com/\"\n"
+    },
+
+    // Make sure i18n-ed comments are removed.
+    { "http://gmail.com",
+      "GMail",
+
+      "Name=Google Chrome\n"
+      "Exec=/opt/google/chrome/google-chrome %U\n"
+      "Comment[pl]=Jakis komentarz.\n",
+
+      "#!/usr/bin/env xdg-open\n"
+      "Name=GMail\n"
+      "Exec=/opt/google/chrome/google-chrome \"--app=http://gmail.com/\"\n"
+    },
+
     // Now we're starting to be more evil...
     { "http://evil.com/evil --join-the-b0tnet",
       "Ownz0red\nExec=rm -rf /",
@@ -81,10 +108,35 @@ TEST(ShellIntegrationTest, GetDesktopFileContents) {
       "Name=Google Chrome\n"
       "Exec=/opt/google/chrome/google-chrome %U\n",
 
+      "#!/usr/bin/env xdg-open\n"
       "Name=http://evil.com/evil%20--join-the-b0tnet\n"
       "Exec=/opt/google/chrome/google-chrome "
       "\"--app=http://evil.com/evil%%20--join-the-b0tnet\"\n"
     },
+    { "http://evil.com/evil; rm -rf /; \"; rm -rf $HOME >ownz0red",
+      "Innocent Title",
+
+      "Name=Google Chrome\n"
+      "Exec=/opt/google/chrome/google-chrome %U\n",
+
+      "#!/usr/bin/env xdg-open\n"
+      "Name=Innocent Title\n"
+      "Exec=/opt/google/chrome/google-chrome "
+      "\"--app=http://evil.com/evil%%20rm%%20-rf%%20/%%20%%22%%20rm%%20"
+      "-rf%%20HOME%%20%%3Eownz0red\"\n"
+    },
+    { "http://evil.com/evil | cat `echo ownz0red` >/dev/null\\",
+      "Innocent Title",
+
+      "Name=Google Chrome\n"
+      "Exec=/opt/google/chrome/google-chrome %U\n",
+
+      "#!/usr/bin/env xdg-open\n"
+      "Name=Innocent Title\n"
+      "Exec=/opt/google/chrome/google-chrome "
+      "\"--app=http://evil.com/evil%%20%%7C%%20cat%%20%%60echo%%20ownz0red"
+      "%%60%%20%%3E/dev/null/\"\n"
+    },
   };
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(test_cases); i++) {
     EXPECT_EQ(test_cases[i].expected_output,