From 933b8e77ec40258df410a4495d0c574833331aa1 Mon Sep 17 00:00:00 2001
From: Marwan Tammam <[email protected]>
Date: Tue, 22 Sep 2026 00:16:41 -0700
Subject: [PATCH] [InstallComponent] Stage the CRX under SystemTemp

setup.exe --install-component stages a copy of the caller-supplied CRX
before verifying it. On a system-level install that staging directory
sits under the Chrome install directory and inherits its ACL, which
grants BUILTIN\Users read access. Stage the directory under SystemTemp
instead so that only admins have access and users don't inherit ACL.

Fixed: 561334760
Change-Id: I33d4d21b9682ed3689ee9de58f755c3f0120fd67
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8407915
Reviewed-by: Greg Thompson <[email protected]>
Auto-Submit: Marwan Tammam <[email protected]>
Commit-Queue: Greg Thompson <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1702400}
---
diff --git a/chrome/installer/setup/install_component.cc b/chrome/installer/setup/install_component.cc
index d2062e1..5375b2f 100644
--- a/chrome/installer/setup/install_component.cc
+++ b/chrome/installer/setup/install_component.cc
@@ -10,16 +10,17 @@
 #include <memory>
 #include <optional>
 #include <string>
-#include <string_view>
 #include <utility>
 #include <vector>
 
 #include "base/base64.h"
 #include "base/check.h"
 #include "base/containers/span.h"
+#include "base/files/file.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
 #include "base/json/json_file_value_serializer.h"
 #include "base/logging.h"
 #include "base/values.h"
@@ -60,33 +61,30 @@
     const InstallerState& installer_state,
     base::span<const ComponentConfig> supported_components,
     crx_file::VerifierFormat verifier_format) {
-  // Validate source file: must not be empty and must not reference parent dirs.
-  CHECK(!source_file.empty() && !source_file.ReferencesParent());
+  // Source file must be absolute, non-empty and must not reference parent dirs.
+  CHECK(source_file.IsAbsolute() && !source_file.ReferencesParent());
 
-  // TOCTOU-Safe Staged copy into admin-protected temp directory.
-  SelfCleaningTempDir temp_path;
-  if (!temp_path.Initialize(installer_state.target_path().DirName(),
-                            kInstallTempDir)) {
-    PLOG(ERROR) << "Failed to initialize temporary directory";
+  // Reject anything that is not a regular non-empty file.
+  base::File::Info source_info;
+  if (base::GetFileInfo(source_file, &source_info)) {
+    CHECK(!source_info.is_directory && source_info.size > 0);
+  }
+
+  // The staged CRX is a copy of a caller-chosen file made before the file has
+  // been validated. We stage it in a secure temporary directory (SystemTemp on
+  // Windows when running as SYSTEM or default admin, or the users's private
+  // Temp directory) to ensure unprivileged users cannot read back files they
+  // cannot open themselves.
+  base::ScopedTempDir staged_crx_dir;
+  if (!staged_crx_dir.CreateUniqueTempDir(FILE_PATH_LITERAL("CrxStaging"))) {
+    PLOG(ERROR) << "Failed to create CRX staging directory";
     return installer::INSTALL_COMPONENT_FAILED_INTERNAL;
   }
 
-  FileConductor file_conductor(temp_path.path());
-  absl::Cleanup undo_on_failure = [&file_conductor] {
-    VLOG(1) << "Failure occurred, calling FileConductor::Undo()";
-    file_conductor.Undo();
-  };
-
-  base::FilePath staged_crx_dir;
-  if (!base::CreateTemporaryDirInDir(temp_path.path(), L"CrxStaging",
-                                     &staged_crx_dir)) {
-    PLOG(ERROR) << "Failed to create CRX staging subdirectory in "
-                << temp_path.path();
-    return installer::INSTALL_COMPONENT_FAILED_INTERNAL;
-  }
-
-  base::FilePath staged_file = staged_crx_dir.Append(source_file.BaseName());
-  if (!file_conductor.CopyEntry(source_file, staged_file)) {
+  base::FilePath staged_file =
+      staged_crx_dir.GetPath().Append(source_file.BaseName());
+  if (!base::CopyFile(source_file, staged_file)) {
+    PLOG(ERROR) << "Failed to copy CRX to staging directory";
     return installer::INSTALL_COMPONENT_FAILED_INTERNAL;
   }
 
@@ -127,8 +125,20 @@
     return installer::INSTALL_COMPONENT_FAILED_SIGNATURE;
   }
 
-  // Fresh directory for unpacking the CRX payload to prevent filename
-  // collisions with the staged CRX file.
+  // Temp directory for unpacking the CRX payload, under the installer's dir.
+  SelfCleaningTempDir temp_path;
+  if (!temp_path.Initialize(installer_state.target_path().DirName(),
+                            kInstallTempDir)) {
+    PLOG(ERROR) << "Failed to initialize temporary directory";
+    return installer::INSTALL_COMPONENT_FAILED_INTERNAL;
+  }
+
+  FileConductor file_conductor(temp_path.path());
+  absl::Cleanup undo_on_failure = [&file_conductor] {
+    VLOG(1) << "Failure occurred, calling FileConductor::Undo()";
+    file_conductor.Undo();
+  };
+
   base::FilePath unpack_dir;
   if (!base::CreateTemporaryDirInDir(temp_path.path(), L"Unpacked",
                                      &unpack_dir)) {
@@ -143,6 +153,11 @@
     return installer::INSTALL_COMPONENT_INVALID_INPUT;
   }
 
+  // The staged copy is no longer needed once its contents have been extracted.
+  if (!staged_crx_dir.Delete()) {
+    VLOG(1) << "Failed to delete staged CRX directory";
+  }
+
   // Read manifest.json to get version and validate name.
   base::FilePath manifest_path = unpack_dir.Append(kManifestFilename);
   if (!base::PathExists(manifest_path)) {
diff --git a/chrome/installer/setup/install_component_unittest.cc b/chrome/installer/setup/install_component_unittest.cc
index 98891f15c..85175c3 100644
--- a/chrome/installer/setup/install_component_unittest.cc
+++ b/chrome/installer/setup/install_component_unittest.cc
@@ -4,9 +4,10 @@
 
 #include "chrome/installer/setup/install_component.h"
 
+#include <windows.h>
+
 #include <stdint.h>
 
-#include <memory>
 #include <optional>
 #include <string>
 
@@ -16,6 +17,7 @@
 #include "base/files/scoped_temp_dir.h"
 #include "base/path_service.h"
 #include "base/version.h"
+#include "base/win/security_descriptor.h"
 #include "chrome/installer/setup/installer_state.h"
 #include "chrome/installer/util/util_constants.h"
 #include "components/crx_file/crx_verifier.h"
@@ -509,4 +511,31 @@
   EXPECT_FALSE(base::PathExists(v394_incomplete.AppendASCII("stale.dll")));
 }
 
+// Verifies that the installed component directory inherits its ACL from the
+// parent application directory. This ensures that components installed by an
+// elevated process remain accessible to standard unprivileged user processes.
+TEST(InstallComponentTest, InstalledComponentDirectoryInheritsAcl) {
+  InstallerState installer_state(InstallerState::SYSTEM_LEVEL);
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  installer_state.set_target_path_for_testing(temp_dir.GetPath());
+
+  base::FilePath src_file = GetTestCrxPath();
+  ASSERT_TRUE(base::PathExists(src_file));
+  ASSERT_EQ(INSTALL_COMPONENT_SUCCESS,
+            InstallComponentForTesting(
+                src_file, installer_state, kTestComponents,
+                crx_file::VerifierFormat::CRX3_WITH_TEST_PUBLISHER_PROOF));
+
+  base::FilePath installed_dir =
+      temp_dir.GetPath().AppendASCII(kTestDeveloperCrxId).AppendASCII("394");
+  ASSERT_TRUE(base::PathExists(installed_dir));
+
+  std::optional<base::win::SecurityDescriptor> descriptor =
+      base::win::SecurityDescriptor::FromFile(installed_dir,
+                                              DACL_SECURITY_INFORMATION);
+  ASSERT_TRUE(descriptor.has_value());
+  EXPECT_FALSE(descriptor->dacl_protected());
+}
+
 }  // namespace installer

diff --git a/chrome/installer/setup/install_component_unittest.cc b/chrome/installer/setup/install_component_unittest.cc
index 98891f15c..85175c3 100644
--- a/chrome/installer/setup/install_component_unittest.cc
+++ b/chrome/installer/setup/install_component_unittest.cc
@@ -4,9 +4,10 @@
 
 #include "chrome/installer/setup/install_component.h"
 
+#include <windows.h>
+
 #include <stdint.h>
 
-#include <memory>
 #include <optional>
 #include <string>
 
@@ -16,6 +17,7 @@
 #include "base/files/scoped_temp_dir.h"
 #include "base/path_service.h"
 #include "base/version.h"
+#include "base/win/security_descriptor.h"
 #include "chrome/installer/setup/installer_state.h"
 #include "chrome/installer/util/util_constants.h"
 #include "components/crx_file/crx_verifier.h"
@@ -509,4 +511,31 @@
   EXPECT_FALSE(base::PathExists(v394_incomplete.AppendASCII("stale.dll")));
 }
 
+// Verifies that the installed component directory inherits its ACL from the
+// parent application directory. This ensures that components installed by an
+// elevated process remain accessible to standard unprivileged user processes.
+TEST(InstallComponentTest, InstalledComponentDirectoryInheritsAcl) {
+  InstallerState installer_state(InstallerState::SYSTEM_LEVEL);
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  installer_state.set_target_path_for_testing(temp_dir.GetPath());
+
+  base::FilePath src_file = GetTestCrxPath();
+  ASSERT_TRUE(base::PathExists(src_file));
+  ASSERT_EQ(INSTALL_COMPONENT_SUCCESS,
+            InstallComponentForTesting(
+                src_file, installer_state, kTestComponents,
+                crx_file::VerifierFormat::CRX3_WITH_TEST_PUBLISHER_PROOF));
+
+  base::FilePath installed_dir =
+      temp_dir.GetPath().AppendASCII(kTestDeveloperCrxId).AppendASCII("394");
+  ASSERT_TRUE(base::PathExists(installed_dir));
+
+  std::optional<base::win::SecurityDescriptor> descriptor =
+      base::win::SecurityDescriptor::FromFile(installed_dir,
+                                              DACL_SECURITY_INFORMATION);
+  ASSERT_TRUE(descriptor.has_value());
+  EXPECT_FALSE(descriptor->dacl_protected());
+}
+
 }  // namespace installer