Use the chrome.exe path instead of the directory.

Currently the the sandboxed shortcut parser compares
the Chrome executable path with the shortcut target
path before allowing it to be reset. However, the
shortcut target path normally includes "chrome.exe"
in its target path. Thus, it should be added into
the path set to be checked.

This CL also updates the working-directory to be
the chrome exe directory.

Bug: 1174512
Change-Id: Ie742146fa347f47ed41b478a84e3a6c2276f6da2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2691247
Commit-Queue: Bettina Dea <bdea@chromium.org>
Reviewed-by: Joe Mason <joenotcharles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#859084}

chrome/chrome_cleaner/chrome_utils/chrome_util.cc

@@ -79,7 +79,7 @@ bool RetrieveChromeExePathFromCommandLine(base::FilePath* chrome_exe_path) {
   return true;
 }
 
-void ListChromeExePaths(std::set<base::FilePath>* paths) {
+void ListChromeExeDirectories(std::set<base::FilePath>* paths) {
   DCHECK(paths);
 
   static const unsigned int install_paths[] = {
@@ -106,10 +106,10 @@ void ListChromeExePaths(std::set<base::FilePath>* paths) {
 void ListChromeInstallationPaths(std::set<base::FilePath>* paths) {
   DCHECK(paths);
 
-  std::set<base::FilePath> exe_paths;
-  ListChromeExePaths(&exe_paths);
+  std::set<base::FilePath> chrome_exe_directories;
+  ListChromeExeDirectories(&chrome_exe_directories);
 
-  for (const base::FilePath& exe_path : exe_paths) {
+  for (const base::FilePath& exe_path : chrome_exe_directories) {
     base::FilePath pattern = exe_path.Append(L"??.*.*.*");
     std::vector<base::FilePath> matches;
     CollectMatchingPaths(pattern, &matches);

chrome/chrome_cleaner/chrome_utils/chrome_util.h

@@ -31,9 +31,9 @@ bool RetrieveChromeVersionAndInstalledDomain(std::wstring* chrome_version,
 // path exists.
 bool RetrieveChromeExePathFromCommandLine(base::FilePath* chrome_exe_path);
 
-// Search for all Chrome executable paths, for example
+// Search for all Chrome executable path directories, for example
 // "C:\Program Files\Google\Chrome\Application".
-void ListChromeExePaths(std::set<base::FilePath>* paths);
+void ListChromeExeDirectories(std::set<base::FilePath>* paths);
 
 // Search for all Chrome versioned installation paths, for example
 // "C:\Program Files\Google\Chrome\Application\68.0.3440.84".

chrome/chrome_cleaner/chrome_utils/extensions_util.cc

@@ -334,11 +334,11 @@ void GetExtensionSettingsForceInstalledExtensions(
 void GetMasterPreferencesExtensions(JsonParserAPI* json_parser,
                                     std::vector<ExtensionPolicyFile>* policies,
                                     base::WaitableEvent* done) {
-  std::set<base::FilePath> exe_paths;
-  ListChromeExePaths(&exe_paths);
+  std::set<base::FilePath> chrome_exe_directories;
+  ListChromeExeDirectories(&chrome_exe_directories);
 
   std::map<base::FilePath, std::string> files_read;
-  for (const base::FilePath& path : exe_paths) {
+  for (const base::FilePath& path : chrome_exe_directories) {
     const base::FilePath& master_preferences(
         path.Append(kMasterPreferencesFileName));
     if (!base::PathExists(master_preferences))

chrome/chrome_cleaner/components/reset_shortcuts_component.cc

@@ -73,6 +73,8 @@ void ResetShortcuts(std::vector<ShortcutInformation> shortcuts,
                  << SanitizePath(target_chrome_exe);
   }
 
+  const base::FilePath& chrome_exe_working_dir = target_chrome_exe.DirName();
+
   for (const ShortcutInformation& shortcut : shortcuts) {
     base::FilePath shortcut_path(shortcut.lnk_path);
     base::ScopedBlockingCall scoped_blocking_call(
@@ -82,6 +84,7 @@ void ResetShortcuts(std::vector<ShortcutInformation> shortcuts,
     base::win::ShortcutProperties updated_properties;
     // Use the first chrome.exe path in the set.
     updated_properties.set_target(target_chrome_exe);
+    updated_properties.set_working_dir(chrome_exe_working_dir);
     // Additional Chrome profiles may have custom icons so the icon location
     // should be preserved.
     base::FilePath icon_location(shortcut.icon_location);
@@ -112,10 +115,10 @@ ResetShortcutsComponent::ResetShortcutsComponent(
     : shortcut_parser_(shortcut_parser) {
   shortcut_paths_to_explore_ = GetPathsToExplore();
 
-  std::set<base::FilePath> chrome_exe_paths;
-  ListChromeExePaths(&chrome_exe_paths);
-  for (const auto& path : chrome_exe_paths) {
-    chrome_exe_file_path_set_.Insert(path);
+  std::set<base::FilePath> chrome_exe_directories;
+  ListChromeExeDirectories(&chrome_exe_directories);
+  for (const auto& path : chrome_exe_directories) {
+    chrome_exe_file_path_set_.Insert(path.Append(L"chrome.exe"));
   }
 }
 

chrome/chrome_cleaner/components/reset_shortcuts_component.h

@@ -36,7 +36,7 @@ class ResetShortcutsComponent : public ComponentAPI {
       const std::vector<base::FilePath>& fake_shortcut_location_paths_);
 
   void SetChromeExeFilePathSetForTesting(
-      const FilePathSet& fake_chrome_exe_file_path_set);
+      const FilePathSet& fake_chrome_exe_working_dirs);
 
  private:
   ShortcutParserAPI* shortcut_parser_;

chrome/chrome_cleaner/components/reset_shortcuts_component_unittest.cc

@@ -100,9 +100,11 @@ class ResetShortcutsComponentTest : public base::MultiProcessTest {
   FilePathSet fake_chrome_exe_file_path_set_;
 
   base::ScopedTempDir temp_dir_with_chrome_lnk_;
+  base::ScopedTempDir temp_dir_with_other_chrome_lnk_;
   std::vector<base::FilePath> temp_dirs_paths_;
   base::ScopedTempDir temp_dir_without_chrome_lnk_;
   base::FilePath fake_chrome_path_;
+  base::FilePath fake_other_chrome_path_;
 
   base::test::TaskEnvironment task_environment_;
 
@@ -143,6 +145,8 @@ TEST_F(ResetShortcutsComponentTest,
             L"--app=app --app-id=appId --profile-directory=tmp/directory");
   ASSERT_TRUE(PathEqual(base::FilePath(found_shortcuts[0].target_path),
                         fake_chrome_path_));
+  ASSERT_TRUE(PathEqual(base::FilePath(found_shortcuts[0].working_dir),
+                        fake_chrome_path_.DirName()));
 }
 
 TEST_F(ResetShortcutsComponentTest, ResetShortcutNoArguments) {
@@ -162,6 +166,48 @@ TEST_F(ResetShortcutsComponentTest, ResetShortcutNoArguments) {
   EXPECT_EQ(found_shortcuts[0].command_line_arguments, L"");
   ASSERT_TRUE(PathEqual(base::FilePath(found_shortcuts[0].target_path),
                         fake_chrome_path_));
+  ASSERT_TRUE(PathEqual(base::FilePath(found_shortcuts[0].working_dir),
+                        fake_chrome_path_.DirName()));
+}
+
+TEST_F(ResetShortcutsComponentTest, ResetShortcutMultipleChromeExe) {
+  base::win::ShortcutProperties properties;
+  properties.set_target(fake_chrome_path_);
+  ASSERT_TRUE(temp_dir_with_chrome_lnk_.CreateUniqueTempDir());
+  base::win::ScopedHandle unused_lnk_handle = CreateAndOpenShortcutInTempDir(
+      "Google Chrome.lnk", properties, &temp_dir_with_chrome_lnk_);
+  ASSERT_TRUE(unused_lnk_handle.IsValid());
+  temp_dirs_paths_.push_back(temp_dir_with_chrome_lnk_.GetPath());
+  fake_chrome_exe_file_path_set_.Insert(fake_chrome_path_);
+
+  base::win::ShortcutProperties other_properties;
+  other_properties.set_target(fake_chrome_path_);
+  ASSERT_TRUE(temp_dir_with_other_chrome_lnk_.CreateUniqueTempDir());
+  ASSERT_TRUE(base::CreateTemporaryFileInDir(
+      temp_dir_with_other_chrome_lnk_.GetPath(), &fake_other_chrome_path_));
+  base::win::ScopedHandle unused_other_lnk_handle =
+      CreateAndOpenShortcutInTempDir("Google Chrome.lnk", other_properties,
+                                     &temp_dir_with_other_chrome_lnk_);
+  ASSERT_TRUE(unused_other_lnk_handle.IsValid());
+  temp_dirs_paths_.push_back(temp_dir_with_other_chrome_lnk_.GetPath());
+  fake_chrome_exe_file_path_set_.Insert(fake_other_chrome_path_);
+
+  ResetShortcuts();
+  std::vector<ShortcutInformation> found_shortcuts = component_->GetShortcuts();
+
+  // It should find shortcuts pointing to the first path. The working directory
+  // should be the first path's directory.
+  ASSERT_EQ(found_shortcuts.size(), 2u);
+  EXPECT_EQ(found_shortcuts[0].command_line_arguments, L"");
+  ASSERT_TRUE(PathEqual(base::FilePath(found_shortcuts[0].target_path),
+                        base::FilePath(found_shortcuts[1].target_path)));
+  ASSERT_TRUE(
+      PathEqual(base::FilePath(found_shortcuts[0].working_dir),
+                base::FilePath(found_shortcuts[0].target_path).DirName()));
+  EXPECT_EQ(found_shortcuts[1].command_line_arguments, L"");
+  ASSERT_TRUE(
+      PathEqual(base::FilePath(found_shortcuts[1].working_dir),
+                base::FilePath(found_shortcuts[1].target_path).DirName()));
 }
 
 TEST_F(ResetShortcutsComponentTest, ResetShortcutDifferentName) {
@@ -183,6 +229,8 @@ TEST_F(ResetShortcutsComponentTest, ResetShortcutDifferentName) {
   EXPECT_EQ(found_shortcuts[0].command_line_arguments, L"");
   ASSERT_TRUE(PathEqual(base::FilePath(found_shortcuts[0].target_path),
                         fake_chrome_path_));
+  ASSERT_TRUE(PathEqual(base::FilePath(found_shortcuts[0].working_dir),
+                        fake_chrome_path_.DirName()));
 }
 
 TEST_F(ResetShortcutsComponentTest, ResetShortcutFlagsNoParameters) {
@@ -207,6 +255,8 @@ TEST_F(ResetShortcutsComponentTest, ResetShortcutFlagsNoParameters) {
             L"--app --app-id --profile-directory");
   ASSERT_TRUE(PathEqual(base::FilePath(found_shortcuts[0].target_path),
                         fake_chrome_path_));
+  ASSERT_TRUE(PathEqual(base::FilePath(found_shortcuts[0].working_dir),
+                        fake_chrome_path_.DirName()));
 }
 
 TEST_F(ResetShortcutsComponentTest, ResetShortcutBadBinary) {
@@ -233,6 +283,8 @@ TEST_F(ResetShortcutsComponentTest, ResetShortcutBadBinary) {
   EXPECT_EQ(found_shortcuts[0].command_line_arguments, L"");
   ASSERT_TRUE(PathEqual(base::FilePath(found_shortcuts[0].target_path),
                         fake_chrome_path_));
+  ASSERT_TRUE(PathEqual(base::FilePath(found_shortcuts[0].working_dir),
+                        fake_chrome_path_.DirName()));
 }
 
 TEST_F(ResetShortcutsComponentTest, NoResetNonChromeShortcut) {
@@ -265,6 +317,8 @@ TEST_F(ResetShortcutsComponentTest, NoResetNonChromeShortcut) {
   EXPECT_EQ(found_shortcuts[0].command_line_arguments, L"");
   ASSERT_TRUE(PathEqual(base::FilePath(found_shortcuts[0].target_path),
                         fake_chrome_path_));
+  ASSERT_TRUE(PathEqual(base::FilePath(found_shortcuts[0].working_dir),
+                        fake_chrome_path_.DirName()));
 }
 
 TEST_F(ResetShortcutsComponentTest, ResetShortcutPreserveIconLocation) {
@@ -293,6 +347,8 @@ TEST_F(ResetShortcutsComponentTest, ResetShortcutPreserveIconLocation) {
   EXPECT_EQ(found_shortcuts[0].command_line_arguments, L"");
   ASSERT_TRUE(PathEqual(base::FilePath(found_shortcuts[0].target_path),
                         fake_chrome_path_));
+  ASSERT_TRUE(PathEqual(base::FilePath(found_shortcuts[0].working_dir),
+                        fake_chrome_path_.DirName()));
   EXPECT_EQ(found_shortcuts[0].icon_location, icon_path.value());
   EXPECT_EQ(found_shortcuts[0].icon_index, 2);
 }

chrome/chrome_cleaner/components/system_report_component.cc

@@ -773,12 +773,13 @@ void ReportShortcutModifications(ShortcutParserAPI* shortcut_parser) {
   base::WaitableEvent event(WaitableEvent::ResetPolicy::MANUAL,
                             WaitableEvent::InitialState::NOT_SIGNALED);
 
-  std::set<base::FilePath> chrome_exe_paths;
-  ListChromeExePaths(&chrome_exe_paths);
+  std::set<base::FilePath> chrome_exe_directories;
+  ListChromeExeDirectories(&chrome_exe_directories);
 
+  const std::wstring kChromeExecutableName = L"chrome.exe";
   FilePathSet chrome_exe_file_path_set;
-  for (const auto& path : chrome_exe_paths)
-    chrome_exe_file_path_set.Insert(path);
+  for (const auto& path : chrome_exe_directories)
+    chrome_exe_file_path_set.Insert(path.Append(kChromeExecutableName));
 
   shortcut_parser->FindAndParseChromeShortcutsInFoldersAsync(
       paths_to_explore, chrome_exe_file_path_set,
@@ -794,7 +795,6 @@ void ReportShortcutModifications(ShortcutParserAPI* shortcut_parser) {
 
   InitializeFilePathSanitization();
 
-  const std::wstring kChromeExecutableName = L"chrome.exe";
   for (const ShortcutInformation& shortcut : shortcuts_found) {
     base::FilePath target_path(shortcut.target_path);
 

chrome/chrome_cleaner/mojom/parser_interface.mojom

@@ -26,6 +26,7 @@ interface Parser {
   // command line arguments.
   ParseShortcut(handle<platform> lkn_file_handle)
    => (LnkParsingResult parsing_result, WString? target_path,
+       WString? working_dir,
        WString? command_line_arguments,
        WString? icon_location,
        int32 icon_index);

chrome/chrome_cleaner/parsers/broker/lnk_parser_sandbox_setup_unittest.cc

@@ -78,6 +78,7 @@ TEST_F(LnkParserSandboxSetupTest, ParseCorrectShortcutSandboxedTest) {
 
   base::win::ShortcutProperties shortcut_properties;
   shortcut_properties.set_target(not_lnk_file_path_);
+  shortcut_properties.set_working_dir(temp_dir_.GetPath());
   const int32_t icon_index = 0;
   shortcut_properties.set_icon(not_lnk_file_path_, icon_index);
   const std::wstring lnk_arguments = L"argument1 -f -t -a -o";
@@ -98,8 +99,8 @@ TEST_F(LnkParserSandboxSetupTest, ParseCorrectShortcutSandboxedTest) {
 
   ASSERT_EQ(test_result_code_, mojom::LnkParsingResult::SUCCESS);
   EXPECT_TRUE(CheckParsedShortcut(test_parsed_shortcut_, not_lnk_file_path_,
-                                  lnk_arguments, not_lnk_file_path_,
-                                  icon_index));
+                                  temp_dir_.GetPath(), lnk_arguments,
+                                  not_lnk_file_path_, icon_index));
 }
 
 TEST_F(LnkParserSandboxSetupTest, ParseIncorrectShortcutSandboxedTest) {
@@ -121,7 +122,7 @@ TEST_F(LnkParserSandboxSetupTest, ParseIncorrectShortcutSandboxedTest) {
 
   ASSERT_NE(test_result_code_, mojom::LnkParsingResult::SUCCESS);
   EXPECT_TRUE(CheckParsedShortcut(test_parsed_shortcut_, base::FilePath(L""),
-                                  L"", base::FilePath(L""),
+                                  base::FilePath(L""), L"", base::FilePath(L""),
                                   /*icon_index=*/-1));
 }
 

chrome/chrome_cleaner/parsers/shortcut_parser/broker/sandboxed_shortcut_parser.cc

@@ -109,6 +109,7 @@ void SandboxedShortcutParser::OnShortcutsParsingDone(
     std::vector<ShortcutInformation>* found_shortcuts,
     mojom::LnkParsingResult parsing_result,
     const base::Optional<std::wstring>& optional_file_path,
+    const base::Optional<std::wstring>& optional_working_dir,
     const base::Optional<std::wstring>& optional_command_line_arguments,
     const base::Optional<std::wstring>& optional_icon_location,
     int32_t icon_index) {
@@ -118,6 +119,9 @@ void SandboxedShortcutParser::OnShortcutsParsingDone(
     if (optional_file_path.has_value())
       parsed_shortcut.target_path = optional_file_path.value();
 
+    if (optional_working_dir.has_value())
+      parsed_shortcut.working_dir = optional_working_dir.value();
+
     if (optional_command_line_arguments.has_value()) {
       parsed_shortcut.command_line_arguments =
           optional_command_line_arguments.value();

chrome/chrome_cleaner/parsers/shortcut_parser/broker/sandboxed_shortcut_parser.h

@@ -44,6 +44,7 @@ class SandboxedShortcutParser : public ShortcutParserAPI {
       std::vector<ShortcutInformation>* found_shortcuts,
       mojom::LnkParsingResult parsing_result,
       const base::Optional<std::wstring>& optional_file_path,
+      const base::Optional<std::wstring>& optional_working_dir,
       const base::Optional<std::wstring>& optional_command_line_arguments,
       const base::Optional<std::wstring>& optional_icon_location,
       int32_t icon_index);

chrome/chrome_cleaner/parsers/shortcut_parser/broker/shortcut_parser_api.h

@@ -24,6 +24,7 @@ struct ShortcutInformation {
 
   base::FilePath lnk_path;
   std::wstring target_path;
+  std::wstring working_dir;
   std::wstring command_line_arguments;
   std::wstring icon_location;
   int32_t icon_index;

chrome/chrome_cleaner/parsers/shortcut_parser/sandboxed_lnk_parser_test_util.cc

@@ -37,13 +37,16 @@ base::win::ScopedHandle CreateAndOpenShortcutInTempDir(
 
 bool CheckParsedShortcut(const ParsedLnkFile& parsed_shortcut,
                          base::FilePath expected_target_path,
+                         base::FilePath expected_working_dir,
                          std::wstring expected_arguments,
                          base::FilePath expected_icon_location,
                          const int32_t expected_icon_index) {
-  base::FilePath parsed_file_path(parsed_shortcut.target_path);
-  return PathEqual(parsed_file_path, expected_target_path) &&
+  base::FilePath parsed_target_path(parsed_shortcut.target_path);
+  base::FilePath parsed_working_dir_path(parsed_shortcut.working_dir);
+  return PathEqual(parsed_target_path, expected_target_path) &&
+         PathEqual(parsed_working_dir_path, expected_working_dir) &&
          (parsed_shortcut.command_line_arguments == expected_arguments) &&
-         PathEqual(parsed_file_path, expected_icon_location) &&
+         PathEqual(expected_target_path, expected_icon_location) &&
          (parsed_shortcut.icon_index == expected_icon_index);
 }
 
@@ -52,13 +55,17 @@ void OnLnkParseDone(
     mojom::LnkParsingResult* out_result_code,
     base::OnceClosure callback,
     mojom::LnkParsingResult result_code,
-    const base::Optional<std::wstring>& optional_file_path,
+    const base::Optional<std::wstring>& optional_target_path,
+    const base::Optional<std::wstring>& optional_working_dir,
     const base::Optional<std::wstring>& optional_command_line_arguments,
     const base::Optional<std::wstring>& optional_icon_location,
     int32_t icon_index) {
   *out_result_code = result_code;
-  if (optional_file_path.has_value())
-    out_parsed_shortcut->target_path = optional_file_path.value();
+  if (optional_target_path.has_value())
+    out_parsed_shortcut->target_path = optional_target_path.value();
+
+  if (optional_working_dir.has_value())
+    out_parsed_shortcut->working_dir = optional_working_dir.value();
 
   if (optional_command_line_arguments.has_value()) {
     out_parsed_shortcut->command_line_arguments =

chrome/chrome_cleaner/parsers/shortcut_parser/sandboxed_lnk_parser_test_util.h

@@ -25,6 +25,7 @@ base::win::ScopedHandle CreateAndOpenShortcutInTempDir(
 
 bool CheckParsedShortcut(const ParsedLnkFile& parsed_shortcut,
                          base::FilePath expected_target_path,
+                         base::FilePath expected_working_dir,
                          std::wstring expected_arguments,
                          base::FilePath expected_icon_location,
                          const int32_t icon_index);
@@ -33,7 +34,8 @@ void OnLnkParseDone(
     mojom::LnkParsingResult* out_result_code,
     base::OnceClosure callback,
     mojom::LnkParsingResult result_code,
-    const base::Optional<std::wstring>& optional_file_path,
+    const base::Optional<std::wstring>& optional_target_path,
+    const base::Optional<std::wstring>& optional_working_dir,
     const base::Optional<std::wstring>& optional_command_line_arguments,
     const base::Optional<std::wstring>& optional_icon_location,
     int32_t optional_icon_index);

chrome/chrome_cleaner/parsers/shortcut_parser/target/lnk_parser.cc

@@ -327,7 +327,8 @@ mojom::LnkParsingResult internal::ParseLnkBytes(
   }
 
   if (has_working_dir &&
-      !SkipUtf16StringStructure(file_buffer, &current_byte)) {
+      !ReadUtf16StringStructure(file_buffer, &current_byte,
+                                &parsed_shortcut->working_dir)) {
     return mojom::LnkParsingResult::BAD_FORMAT;
   }
 
@@ -354,6 +355,7 @@ mojom::LnkParsingResult internal::ParseLnkBytes(
 }
 
 ParsedLnkFile::ParsedLnkFile() {}
+ParsedLnkFile::~ParsedLnkFile() {}
 
 // Please note that the documentation used to write this parser was obtained
 // from the following link:

chrome/chrome_cleaner/parsers/shortcut_parser/target/lnk_parser.h

@@ -69,7 +69,9 @@ mojom::LnkParsingResult ParseLnkBytes(std::vector<BYTE> file_buffer,
 
 struct ParsedLnkFile {
   ParsedLnkFile();
+  ~ParsedLnkFile();
   std::wstring target_path;
+  std::wstring working_dir;
   std::wstring command_line_arguments;
   std::wstring icon_location;
   int32_t icon_index = -1;