Secure temp directory creation for Windows

This CL modifies `base::CreateNewTempDirectory` to create directories in
a secure location for Windows if the caller is admin, so that low
privilege users cannot attack these directories and cause a privilege
escalation.

The default %TEMP% directory for Windows is insecure, since low
privilege users can get the path of these folders after their creation
and are able to create subfolders and files within these folders which
can lead to privilege escalation.

Bug: 1374377
Change-Id: I1ea53f5b67665f85149d7f4fe108b4df886c6db7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3989615
Auto-Submit: S Ganesh <ganesh@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Austin Sullivan <asully@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Commit-Queue: S Ganesh <ganesh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1067808}

base/files/file_util.h

@@ -367,6 +367,12 @@ BASE_EXPORT ScopedFILE CreateAndOpenTemporaryStreamInDir(const FilePath& dir,
 // the format of prefixyyyy.
 // NOTE: prefix is ignored in the POSIX implementation.
 // If success, return true and output the full path of the directory created.
+//
+// For Windows, this directory is usually created in a secure location under
+// %ProgramFiles% if the caller is admin. This is because the default %TEMP%
+// folder for Windows is insecure, since low privilege users can get the path of
+// folders under %TEMP% after creation and are able to create subfolders and
+// files within these folders which can lead to privilege escalation.
 BASE_EXPORT bool CreateNewTempDirectory(const FilePath::StringType& prefix,
                                         FilePath* new_temp_path);
 

base/files/file_util_unittest.cc

@@ -2718,6 +2718,15 @@ TEST_F(FileUtilTest, CreateNewTempDirectoryTest) {
   FilePath temp_dir;
   ASSERT_TRUE(CreateNewTempDirectory(FilePath::StringType(), &temp_dir));
   EXPECT_TRUE(PathExists(temp_dir));
+
+#if BUILDFLAG(IS_WIN)
+  FilePath expected_parent_dir;
+  EXPECT_TRUE(PathService::Get(
+      ::IsUserAnAdmin() ? int{DIR_PROGRAM_FILES} : int{DIR_TEMP},
+      &expected_parent_dir));
+  EXPECT_TRUE(expected_parent_dir.IsParent(temp_dir));
+#endif  // BUILDFLAG(IS_WIN)
+
   EXPECT_TRUE(DeleteFile(temp_dir));
 }
 

base/files/file_util_win.cc

@@ -30,6 +30,7 @@
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/numerics/safe_conversions.h"
+#include "base/path_service.h"
 #include "base/process/process_handle.h"
 #include "base/rand_util.h"
 #include "base/strings/strcat.h"
@@ -55,6 +56,7 @@ namespace {
 
 const DWORD kFileShareAll =
     FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE;
+const wchar_t kDefaultTempDirPrefix[] = L"ChromiumTemp";
 
 // Returns the Win32 last error code or ERROR_SUCCESS if the last error code is
 // ERROR_FILE_NOT_FOUND or ERROR_PATH_NOT_FOUND. This is useful in cases where
@@ -620,15 +622,33 @@ bool CreateTemporaryDirInDir(const FilePath& base_dir,
   return false;
 }
 
+// The directory is created under %ProgramFiles% for security reasons if the
+// caller is admin. Since only admin can write to %ProgramFiles%, this avoids
+// attacks from lower privilege processes.
+//
+// If unable to create a dir under %ProgramFiles%, the dir is created under
+// %TEMP%. The reasons for not being able to create a dir under %ProgramFiles%
+// could be because we are unable to resolve `DIR_PROGRAM_FILES`, say due to
+// registry redirection, or unable to create a directory due to %ProgramFiles%
+// being read-only or having atypical ACLs.
 bool CreateNewTempDirectory(const FilePath::StringType& prefix,
                             FilePath* new_temp_path) {
   ScopedBlockingCall scoped_blocking_call(FROM_HERE, BlockingType::MAY_BLOCK);
 
-  FilePath system_temp_dir;
-  if (!GetTempDir(&system_temp_dir))
+  DCHECK(new_temp_path);
+
+  FilePath parent_dir;
+  if (::IsUserAnAdmin() && PathService::Get(DIR_PROGRAM_FILES, &parent_dir) &&
+      CreateTemporaryDirInDir(parent_dir,
+                              prefix.empty() ? kDefaultTempDirPrefix : prefix,
+                              new_temp_path)) {
+    return true;
+  }
+
+  if (!GetTempDir(&parent_dir))
     return false;
 
-  return CreateTemporaryDirInDir(system_temp_dir, prefix, new_temp_path);
+  return CreateTemporaryDirInDir(parent_dir, prefix, new_temp_path);
 }
 
 bool CreateDirectoryAndGetError(const FilePath& full_path,

base/files/scoped_temp_dir_unittest.cc

@@ -7,9 +7,14 @@
 #include "base/files/file.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/path_service.h"
 #include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+#if BUILDFLAG(IS_WIN)
+#include <shlobj.h>
+#endif
+
 namespace base {
 
 TEST(ScopedTempDir, FullPath) {
@@ -54,9 +59,18 @@ TEST(ScopedTempDir, TempDir) {
     EXPECT_TRUE(dir.CreateUniqueTempDir());
     test_path = dir.GetPath();
     EXPECT_TRUE(DirectoryExists(test_path));
+
+#if BUILDFLAG(IS_WIN)
+    FilePath expected_parent_dir;
+    EXPECT_TRUE(PathService::Get(
+        ::IsUserAnAdmin() ? int{DIR_PROGRAM_FILES} : int{DIR_TEMP},
+        &expected_parent_dir));
+    EXPECT_TRUE(expected_parent_dir.IsParent(test_path));
+#else   // BUILDFLAG(IS_WIN)
     FilePath tmp_dir;
     EXPECT_TRUE(GetTempDir(&tmp_dir));
     EXPECT_TRUE(test_path.value().find(tmp_dir.value()) != std::string::npos);
+#endif  // BUILDFLAG(IS_WIN)
   }
   EXPECT_FALSE(DirectoryExists(test_path));
 }

base/test/launcher/test_launcher.cc

@@ -54,6 +54,7 @@
 #include "base/test/launcher/test_launcher_tracer.h"
 #include "base/test/launcher/test_results_tracker.h"
 #include "base/test/scoped_logging_settings.h"
+#include "base/test/test_file_util.h"
 #include "base/test/test_switches.h"
 #include "base/test/test_timeouts.h"
 #include "base/threading/thread_restrictions.h"
@@ -876,7 +877,7 @@ void TestRunner::LaunchNextTask(scoped_refptr<TaskRunner> task_runner,
   // this directory will also contain one subdirectory per child for that
   // child's process-wide temp dir.
   base::FilePath task_temp_dir;
-  CHECK(CreateNewTempDirectory(FilePath::StringType(), &task_temp_dir));
+  CHECK(CreateTemporaryDirInDir(GetTempDirForTesting(), {}, &task_temp_dir));
   bool post_to_current_runner = true;
   size_t batch_size = (batch_size_ == 0) ? tests_to_run_.size() : batch_size_;
 

base/test/test_file_util.cc

@@ -81,6 +81,12 @@ bool EvictFileFromSystemCacheWithRetry(const FilePath& path) {
   return false;
 }
 
+FilePath GetTempDirForTesting() {
+  FilePath path;
+  CHECK(GetTempDir(&path));
+  return path;
+}
+
 FilePath CreateUniqueTempDirectoryScopedToTest() {
   ScopedAllowBlockingForTesting allow_blocking;
   FilePath path;

base/test/test_file_util.h

@@ -34,6 +34,10 @@ bool EvictFileFromSystemCacheWithRetry(const FilePath& file);
 // success.
 bool DieFileDie(const FilePath& file, bool recurse);
 
+// Convenience wrapper for `base::GetTempDir()` that returns the temp dir as a
+// `base::FilePath`.
+FilePath GetTempDirForTesting();
+
 // Creates a a new unique directory and returns the generated path. The
 // directory will be automatically deleted when the test completes. Failure
 // upon creation or deletion will cause a test failure.

base/test/test_file_util_win_unittest.cc

@@ -86,4 +86,8 @@ TEST(TestFileUtil, EvictFileWithLongName) {
   ASSERT_TRUE(EvictFileFromSystemCache(temp_file));
 }
 
+TEST(TestFileUtil, GetTempDirForTesting) {
+  ASSERT_FALSE(GetTempDirForTesting().value().empty());
+}
+
 }  // namespace base

chrome/browser/file_system_access/chrome_file_system_access_permission_context_browsertest.cc

@@ -6,8 +6,10 @@
 
 #include <tuple>
 
+#include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/test/test_file_util.h"
 #include "chrome/browser/file_system_access/file_system_access_permission_request_manager.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
@@ -131,7 +133,14 @@ class ChromeFileSystemAccessPermissionContextPrerenderingBrowserTest
       default;
 
   void SetUp() override {
-    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    // Create a scoped directory under %TEMP% instead of using
+    // `base::ScopedTempDir::CreateUniqueTempDir`.
+    // `base::ScopedTempDir::CreateUniqueTempDir` creates a path under
+    // %ProgramFiles% on Windows when running as Admin, which is a blocked path
+    // (`kBlockedPaths`). This can fail some of the tests.
+    ASSERT_TRUE(
+        temp_dir_.CreateUniqueTempDirUnderPath(base::GetTempDirForTesting()));
+
     prerender_test_helper_.SetUp(embedded_test_server());
     InProcessBrowserTest::SetUp();
   }

chrome/browser/file_system_access/chrome_file_system_access_permission_context_unittest.cc

@@ -17,6 +17,7 @@
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_path_override.h"
 #include "base/test/simple_test_clock.h"
+#include "base/test/test_file_util.h"
 #include "base/test/test_future.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
@@ -90,7 +91,13 @@ class ChromeFileSystemAccessPermissionContextTest : public testing::Test {
         features::kFileSystemAccessPersistentPermissions);
   }
   void SetUp() override {
-    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    // Create a scoped directory under %TEMP% instead of using
+    // `base::ScopedTempDir::CreateUniqueTempDir`.
+    // `base::ScopedTempDir::CreateUniqueTempDir` creates a path under
+    // %ProgramFiles% on Windows when running as Admin, which is a blocked path
+    // (`kBlockedPaths`). This can fail some of the tests.
+    ASSERT_TRUE(
+        temp_dir_.CreateUniqueTempDirUnderPath(base::GetTempDirForTesting()));
 
     DownloadCoreServiceFactory::GetForBrowserContext(profile())
         ->SetDownloadManagerDelegateForTesting(

chrome/browser/ui/views/file_system_access/file_system_access_browsertest.cc

@@ -4,12 +4,14 @@
 
 #include <set>
 
+#include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/path_service.h"
 #include "base/strings/strcat.h"
 #include "base/test/bind.h"
 #include "base/test/scoped_path_override.h"
+#include "base/test/test_file_util.h"
 #include "base/test/test_future.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
@@ -54,7 +56,13 @@ using safe_browsing::ClientDownloadRequest;
 class FileSystemAccessBrowserTest : public InProcessBrowserTest {
  public:
   void SetUp() override {
-    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    // Create a scoped directory under %TEMP% instead of using
+    // `base::ScopedTempDir::CreateUniqueTempDir`.
+    // `base::ScopedTempDir::CreateUniqueTempDir` creates a path under
+    // %ProgramFiles% on Windows when running as Admin, which is a blocked path
+    // (`kBlockedPaths`). This can fail some of the tests.
+    ASSERT_TRUE(
+        temp_dir_.CreateUniqueTempDirUnderPath(base::GetTempDirForTesting()));
     InProcessBrowserTest::SetUp();
   }
 
@@ -1159,7 +1167,13 @@ class FileSystemAccessBrowserTestForWebUI : public InProcessBrowserTest {
  public:
   FileSystemAccessBrowserTestForWebUI() {
     base::ScopedAllowBlockingForTesting allow_blocking;
-    CHECK(temp_dir_.CreateUniqueTempDir());
+
+    // Create a scoped directory under %TEMP% instead of using
+    // `base::ScopedTempDir::CreateUniqueTempDir`.
+    // `base::ScopedTempDir::CreateUniqueTempDir` creates a path under
+    // %ProgramFiles% on Windows when running as Admin, which is a blocked path
+    // (`kBlockedPaths`). This can fail some of the tests.
+    CHECK(temp_dir_.CreateUniqueTempDirUnderPath(base::GetTempDirForTesting()));
   }
 
   // Return the evaluated value of a JavaScript |statement| as a std::string.

chrome/test/data/webui/mojo/mojo_file_system_access_browsertest.cc

@@ -2,11 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/path_service.h"
 #include "base/run_loop.h"
 #include "base/test/bind.h"
+#include "base/test/test_file_util.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "chrome/browser/bad_message.h"
 #include "chrome/browser/chrome_content_browser_client.h"
@@ -225,7 +227,14 @@ IN_PROC_BROWSER_TEST_F(MojoFileSystemAccessBrowserTest, CanResolveFilePath) {
   // Create a test file.
   base::ScopedAllowBlockingForTesting allow_blocking;
   base::ScopedTempDir temp_directory;
-  ASSERT_TRUE(temp_directory.CreateUniqueTempDir());
+
+  // Create a scoped directory under %TEMP% instead of using
+  // `base::ScopedTempDir::CreateUniqueTempDir`.
+  // `base::ScopedTempDir::CreateUniqueTempDir` creates a path under
+  // %ProgramFiles% on Windows when running as Admin, which is a blocked path
+  // (`kBlockedPaths`). This can fail some of the tests.
+  ASSERT_TRUE(temp_directory.CreateUniqueTempDirUnderPath(
+      base::GetTempDirForTesting()));
   base::FilePath temp_file;
   ASSERT_TRUE(
       base::CreateTemporaryFileInDir(temp_directory.GetPath(), &temp_file));

chrome/updater/win/win_util.cc

@@ -909,8 +909,8 @@ bool EnableProcessHeapMetadataProtection() {
 
 absl::optional<base::ScopedTempDir> CreateSecureTempDir() {
   base::FilePath temp_dir;
-  if (!update_client::CreateSecureTempDirectory(
-          FILE_PATH_LITERAL(COMPANY_SHORTNAME_STRING), &temp_dir)) {
+  if (!base::CreateNewTempDirectory(FILE_PATH_LITERAL(COMPANY_SHORTNAME_STRING),
+                                    &temp_dir)) {
     return absl::nullopt;
   }
 

components/update_client/background_downloader_win.cc

@@ -739,7 +739,8 @@ HRESULT BackgroundDownloader::InitializeNewJob(
     const ComPtr<IBackgroundCopyJob>& job,
     const GURL& url) {
   base::FilePath tempdir;
-  if (!CreateSecureTempDirectory(FILE_PATH_LITERAL("chrome_BITS_"), &tempdir))
+  if (!base::CreateNewTempDirectory(FILE_PATH_LITERAL("chrome_BITS_"),
+                                    &tempdir))
     return E_FAIL;
 
   const std::wstring filename(base::SysUTF8ToWide(url.ExtractFileName()));

components/update_client/component_unpacker.cc

@@ -80,7 +80,7 @@ bool ComponentUnpacker::Verify() {
 bool ComponentUnpacker::BeginUnzipping() {
   // Mind the reference to non-const type, passed as an argument below.
   base::FilePath& destination = is_delta_ ? unpack_diff_path_ : unpack_path_;
-  if (!CreateSecureTempDirectory(
+  if (!base::CreateNewTempDirectory(
           FILE_PATH_LITERAL("chrome_ComponentUnpacker_BeginUnzipping"),
           &destination)) {
     VLOG(1) << "Unable to create temporary directory for unpacking.";
@@ -107,7 +107,7 @@ void ComponentUnpacker::EndUnzipping(bool result) {
 void ComponentUnpacker::BeginPatching() {
   if (is_delta_) {  // Package is a diff package.
     // Use a different temp directory for the patch output files.
-    if (!CreateSecureTempDirectory(
+    if (!base::CreateNewTempDirectory(
             FILE_PATH_LITERAL("chrome_ComponentUnpacker_BeginPatching"),
             &unpack_path_)) {
       base::SequencedTaskRunnerHandle::Get()->PostTask(

components/update_client/test_installer.cc

@@ -83,7 +83,7 @@ bool ReadOnlyTestInstaller::GetInstalledFile(const std::string& file,
 }
 
 VersionedTestInstaller::VersionedTestInstaller() {
-  CreateSecureTempDirectory(FILE_PATH_LITERAL("TEST_"), &install_directory_);
+  base::CreateNewTempDirectory(FILE_PATH_LITERAL("TEST_"), &install_directory_);
 }
 
 VersionedTestInstaller::~VersionedTestInstaller() {

components/update_client/update_client_unittest.cc

@@ -64,8 +64,8 @@ using base::FilePath;
 // there was an error creating the copy.
 bool MakeTestFile(const FilePath& from_path, FilePath* to_path) {
   FilePath temp_dir;
-  bool result =
-      CreateSecureTempDirectory(FILE_PATH_LITERAL("update_client"), &temp_dir);
+  bool result = base::CreateNewTempDirectory(FILE_PATH_LITERAL("update_client"),
+                                             &temp_dir);
   if (!result)
     return false;
 

components/update_client/url_fetcher_downloader.cc

@@ -37,8 +37,8 @@ void UrlFetcherDownloader::DoStartDownload(const GURL& url) {
 }
 
 void UrlFetcherDownloader::CreateDownloadDir() {
-  CreateSecureTempDirectory(FILE_PATH_LITERAL("chrome_url_fetcher_"),
-                            &download_dir_);
+  base::CreateNewTempDirectory(FILE_PATH_LITERAL("chrome_url_fetcher_"),
+                               &download_dir_);
 }
 
 void UrlFetcherDownloader::StartURLFetch(const GURL& url) {

components/update_client/utils.cc

@@ -173,23 +173,6 @@ base::Value ReadManifest(const base::FilePath& unpack_path) {
   return base::Value::FromUniquePtrValue(std::move(root));
 }
 
-bool CreateSecureTempDirectory(const base::FilePath::StringType& prefix,
-                               base::FilePath* temp_dir) {
-  DCHECK(temp_dir);
-
-#if BUILDFLAG(IS_WIN)
-  const int path_key =
-      ::IsUserAnAdmin() ? int{base::DIR_PROGRAM_FILES} : int{base::DIR_TEMP};
-#else   // BUILDFLAG(IS_WIN)
-  const int path_key = base::DIR_TEMP;
-#endif  // BUILDFLAG(IS_WIN)
-
-  base::FilePath parent_dir;
-  return base::PathService::Get(path_key, &parent_dir)
-             ? base::CreateTemporaryDirInDir(parent_dir, prefix, temp_dir)
-             : false;
-}
-
 std::string GetArchitecture() {
 #if BUILDFLAG(IS_WIN)
   const base::win::OSInfo* os_info = base::win::OSInfo::GetInstance();

components/update_client/utils.h

@@ -97,11 +97,6 @@ CrxInstaller::Result ToInstallerResult(const T& error, int extended_error = 0) {
       extended_error);
 }
 
-// Creates a unique temporary directory. For Windows, the directory is created
-// under %ProgramFiles% if the caller is admin, so it is secure.
-bool CreateSecureTempDirectory(const base::FilePath::StringType& prefix,
-                               base::FilePath* temp_dir);
-
 // Returns a string representation of the processor architecture. Uses
 // `base::win::OSInfo::IsWowX86OnARM64` and
 // `base::win::OSInfo::IsWowAMD64OnARM64` if available on Windows (more