From 049218ea45ac2ac8c41499b4b2729992077fabfc Mon Sep 17 00:00:00 2001
From: Greg Thompson <grt@chromium.org>
Date: Thu, 30 Jul 2026 23:28:20 -0700
Subject: [PATCH] [GCPW] Restrict GCPW's data directory to SYSTEM + Local Admin

GCPW uses the directory "C:\ProgramData\Google\Credential Provider" as
a convenient place to store its startup sentinel and other data. Apply
a DACL to this directory to restrict access to only local SYSTEM and
built-in administrators.

Fixed: 511731805
Change-Id: I5623da7fc5f2dc031849633a4c1e5b3cf46711c1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8162382
Commit-Queue: Greg Thompson <grt@chromium.org>
Reviewed-by: Yusuf Sengul <yusufsn@google.com>
Cr-Commit-Position: refs/heads/main@{#1671646}
---

diff --git a/chrome/credential_provider/gaiacp/gcp_utils.cc b/chrome/credential_provider/gaiacp/gcp_utils.cc
index 11385717..fb32ae8 100644
--- a/chrome/credential_provider/gaiacp/gcp_utils.cc
+++ b/chrome/credential_provider/gaiacp/gcp_utils.cc
@@ -45,6 +45,8 @@
 #include "base/win/current_module.h"
 #include "base/win/embedded_i18n/language_selector.h"
 #include "base/win/ntsecapi_shim.h"
+#include "base/win/security_descriptor.h"
+#include "base/win/sid.h"
 #include "base/win/wbemidl_shim.h"
 #include "base/win/win_util.h"
 #include "base/win/wincred_shim.h"
@@ -146,16 +148,11 @@
 };
 
 base::FilePath GetStartupSentinelLocation(const std::wstring& version) {
-  base::FilePath sentinel_path;
-  if (!base::PathService::Get(base::DIR_COMMON_APP_DATA, &sentinel_path)) {
-    HRESULT hr = HRESULT_FROM_WIN32(::GetLastError());
-    LOGFN(ERROR) << "PathService::Get(DIR_COMMON_APP_DATA) hr=" << putHR(hr);
+  base::FilePath sentinel_path = GetDataDirectory();
+  if (sentinel_path.empty()) {
     return base::FilePath();
   }
 
-  sentinel_path = sentinel_path.Append(GetInstallParentDirectoryName())
-                      .Append(kCredentialProviderFolder);
-
   return sentinel_path.Append(version).AppendASCII(kSentinelFilename);
 }
 
@@ -270,17 +267,11 @@
 // and |file_dir|.
 base::FilePath GetDirectoryFilePath(const std::wstring& sid,
                                     const std::wstring& file_dir) {
-  base::FilePath path;
-  if (!base::PathService::Get(base::DIR_COMMON_APP_DATA, &path)) {
-    HRESULT hr = HRESULT_FROM_WIN32(::GetLastError());
-    LOGFN(ERROR) << "PathService::Get(DIR_COMMON_APP_DATA) hr=" << putHR(hr);
+  base::FilePath path = GetDataDirectory();
+  if (path.empty()) {
     return base::FilePath();
   }
-  path = path.Append(GetInstallParentDirectoryName())
-             .Append(kCredentialProviderFolder)
-             .Append(file_dir)
-             .Append(sid);
-  return path;
+  return path.Append(file_dir).Append(sid);
 }
 
 }  // namespace
@@ -921,6 +912,101 @@
          kHoursToDisableGCPW;
 }
 
+bool SecureCreateDirectory(const base::FilePath& path) {
+  if (path.empty()) {
+    return false;
+  }
+
+  // Create the parent directory, allowing ordinary inherited permissions.
+  base::File::Error error = base::File::FILE_OK;
+  if (base::FilePath parent = path.DirName();
+      !parent.empty() && parent != path &&
+      !base::CreateDirectoryAndGetError(parent, &error)) {
+    LOGFN(ERROR) << "Failed to create parent directory for " << path << "; "
+                 << base::File::ErrorToString(error);
+    return false;
+  }
+
+  base::win::SecurityDescriptor sd;
+  sd.set_dacl_protected(true);  // Protects from inheriting parent DACLs
+
+  // Add DACL entries strictly for SYSTEM and BuiltinAdministrators with full
+  // control.
+  if (!sd.SetDaclEntry(base::win::WellKnownSid::kLocalSystem,
+                       base::win::SecurityAccessMode::kGrant,
+                       GENERIC_ALL | STANDARD_RIGHTS_ALL,
+                       SUB_CONTAINERS_AND_OBJECTS_INHERIT) ||
+      !sd.SetDaclEntry(base::win::WellKnownSid::kBuiltinAdministrators,
+                       base::win::SecurityAccessMode::kGrant,
+                       GENERIC_ALL | STANDARD_RIGHTS_ALL,
+                       SUB_CONTAINERS_AND_OBJECTS_INHERIT)) {
+    LOGFN(ERROR) << "Failed to set DACL entries for " << path;
+    return false;
+  }
+
+  // Attempt to create the directory with the security descriptor attached.
+  auto self_relative = sd.ToSelfRelative();
+  if (!self_relative) {
+    LOGFN(ERROR) << "Failed to convert SecurityDescriptor to self-relative for "
+                 << path;
+    return false;
+  }
+  SECURITY_ATTRIBUTES sa{sizeof(SECURITY_ATTRIBUTES), self_relative->get(),
+                         FALSE};
+
+  if (::CreateDirectory(path.value().c_str(), &sa)) {
+    return true;
+  }
+
+  // If CreateDirectory failed (e.g., because the directory already exists),
+  // open a handle to the directory to set the DACL.
+  base::File handle(::CreateFile(
+      path.value().c_str(), WRITE_DAC | READ_CONTROL,
+      FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr,
+      OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT,
+      nullptr));
+  if (!handle.IsValid()) {
+    HRESULT hr = HRESULT_FROM_WIN32(::GetLastError());
+    LOGFN(ERROR) << "Failed to open handle for existing path " << path
+                 << " hr=" << putHR(hr);
+    return false;
+  }
+
+  BY_HANDLE_FILE_INFORMATION file_info = {};
+  if (!::GetFileInformationByHandle(handle.GetPlatformFile(), &file_info)) {
+    HRESULT hr = HRESULT_FROM_WIN32(::GetLastError());
+    LOGFN(ERROR) << "GetFileInformationByHandle failed for " << path
+                 << " hr=" << putHR(hr);
+    return false;
+  }
+
+  // Verify that a directory (not a file) was just opened.
+  if ((file_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) == 0) {
+    LOGFN(ERROR) << "Path exists but is not a directory: " << path;
+    return false;
+  }
+
+  // Verify that the directory does not have a reparse point (e.g., it's not a
+  // symlink or a junction point).
+  if ((file_info.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0) {
+    LOGFN(ERROR) << "Path exists but is a reparse point (junction/symlink): "
+                 << path;
+    return false;
+  }
+
+  // Apply the DACL to the directory.
+  if (!sd.WriteToHandle(handle.GetPlatformFile(),
+                        base::win::SecurityObjectType::kFile,
+                        DACL_SECURITY_INFORMATION)) {
+    HRESULT hr = HRESULT_FROM_WIN32(::GetLastError());
+    LOGFN(ERROR) << "Failed to write SecurityDescriptor to handle for " << path
+                 << " hr=" << putHR(hr);
+    return false;
+  }
+
+  return true;
+}
+
 bool WriteToStartupSentinel() {
   LOGFN(VERBOSE);
   // Always try to write to the startup sentinel file. If writing or opening
@@ -939,14 +1025,11 @@
       GetStartupSentinelLocation(TEXT(CHROME_VERSION_STRING));
   if (!startup_sentinel_path.empty()) {
     base::FilePath startup_sentinel_directory = startup_sentinel_path.DirName();
-    if (!base::DirectoryExists(startup_sentinel_directory)) {
-      base::File::Error error;
-      if (!base::CreateDirectoryAndGetError(startup_sentinel_directory,
-                                            &error)) {
-        LOGFN(ERROR) << "Could not create sentinel directory='"
-                     << startup_sentinel_directory << "' error=" << error;
-        return false;
-      }
+    base::File::Error error;
+    if (!base::CreateDirectoryAndGetError(startup_sentinel_directory, &error)) {
+      LOGFN(ERROR) << "Could not create sentinel directory='"
+                   << startup_sentinel_directory << "' error=" << error;
+      return false;
     }
     base::File startup_sentinel(
         startup_sentinel_path,
@@ -1114,6 +1197,24 @@
   return S_OK;
 }
 
+base::FilePath GetDataDirectory() {
+  base::FilePath path;
+  if (!base::PathService::Get(base::DIR_COMMON_APP_DATA, &path)) {

diff --git a/chrome/credential_provider/gaiacp/gcp_utils_unittest.cc b/chrome/credential_provider/gaiacp/gcp_utils_unittest.cc
index fb018ddd..64269494 100644
--- a/chrome/credential_provider/gaiacp/gcp_utils_unittest.cc
+++ b/chrome/credential_provider/gaiacp/gcp_utils_unittest.cc
@@ -4,6 +4,10 @@
 
 #include "chrome/credential_provider/gaiacp/gcp_utils.h"
 
+#include <windows.h>
+
+#include <shlobj.h>
+
 #include <algorithm>
 #include <array>
 #include <string_view>
@@ -11,22 +15,37 @@
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/containers/span.h"
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
 #include "base/process/launch.h"
 #include "base/strings/strcat.h"
 #include "base/strings/strcat_win.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/gmock_expected_support.h"
+#include "base/test/test_file_util.h"
 #include "base/test/test_reg_util_win.h"
 #include "base/values.h"
+#include "base/win/access_token.h"
 #include "base/win/scoped_handle.h"
+#include "base/win/security_descriptor.h"
+#include "base/win/security_util.h"
+#include "base/win/sid.h"
 #include "build/build_config.h"
 #include "chrome/credential_provider/common/gcp_strings.h"
 #include "chrome/credential_provider/gaiacp/mdm_utils.h"
 #include "chrome/credential_provider/gaiacp/reg_utils.h"
 #include "chrome/credential_provider/test/gcp_fakes.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace credential_provider {
 
+using ::testing::AllOf;
+using ::testing::HasSubstr;
+using ::testing::Not;
+using ::testing::Optional;
+using ::testing::StartsWith;
+
 TEST(GcpPasswordTest, GenerateRandomPassword) {
   wchar_t password[64];
 
@@ -615,4 +634,86 @@
                        ::testing::Values("true"),
                        ::testing::Values("device_resource_id", "")));
 
+class GcpUtilsSecureCreateDirectoryTest : public ::testing::Test {
+ protected:
+  void SetUp() override {
+    if (!::IsUserAnAdmin()) {
+      GTEST_SKIP() << "Test requires administrative privileges.";
+    }
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+  }
+
+  void TearDown() override {
+    if (temp_dir_.IsValid() && ::IsUserAnAdmin()) {
+      if (auto token = base::win::AccessToken::FromCurrentProcess(); token) {
+        std::vector<base::win::Sid> sids;
+        sids.push_back(token->User().Clone());
+        base::win::GrantAccessToPath(
+            temp_dir_.GetPath(), sids, GENERIC_ALL | STANDARD_RIGHTS_ALL,
+            CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE);
+      }
+    }
+  }
+
+  base::ScopedTempDir temp_dir_;
+};
+
+TEST_F(GcpUtilsSecureCreateDirectoryTest, NewDirectory) {
+  base::FilePath new_dir = temp_dir_.GetPath().Append(L"NewSecureDir");
+  EXPECT_FALSE(base::DirectoryExists(new_dir));
+  EXPECT_TRUE(SecureCreateDirectory(new_dir));
+  EXPECT_TRUE(base::DirectoryExists(new_dir));
+  EXPECT_THAT(base::GetFileDacl(new_dir),
+              AllOf(StartsWith(L"D:P"), HasSubstr(L";;;BA)"),
+                    HasSubstr(L";;;SY)"), Not(HasSubstr(L";;;WD)"))));
+}
+
+TEST_F(GcpUtilsSecureCreateDirectoryTest, ExistingDirectory) {
+  base::FilePath existing_dir = temp_dir_.GetPath().Append(L"ExistingDir");
+  EXPECT_TRUE(base::CreateDirectory(existing_dir));
+  EXPECT_TRUE(base::DirectoryExists(existing_dir));
+  EXPECT_TRUE(SecureCreateDirectory(existing_dir));
+  EXPECT_THAT(base::GetFileDacl(existing_dir),
+              AllOf(StartsWith(L"D:P"), HasSubstr(L";;;BA)"),
+                    HasSubstr(L";;;SY)"), Not(HasSubstr(L";;;WD)"))));
+}
+
+TEST_F(GcpUtilsSecureCreateDirectoryTest,
+       IdempotentOnExistingSecuredDirectory) {
+  base::FilePath target_dir = temp_dir_.GetPath().Append(L"SecuredDir");
+  EXPECT_TRUE(SecureCreateDirectory(target_dir));
+  EXPECT_THAT(base::GetFileDacl(target_dir),
+              AllOf(StartsWith(L"D:P"), HasSubstr(L";;;BA)"),
+                    HasSubstr(L";;;SY)"), Not(HasSubstr(L";;;WD)"))));
+
+  // Apply SecureCreateDirectory a second time to an already secured directory.
+  EXPECT_TRUE(SecureCreateDirectory(target_dir));
+  EXPECT_THAT(base::GetFileDacl(target_dir),
+              AllOf(StartsWith(L"D:P"), HasSubstr(L";;;BA)"),
+                    HasSubstr(L";;;SY)"), Not(HasSubstr(L";;;WD)"))));
+}
+
+TEST_F(GcpUtilsSecureCreateDirectoryTest, FailOnExistingFileConflict) {
+  base::FilePath file_path = temp_dir_.GetPath().Append(L"ConflictingFile");
+  ASSERT_TRUE(base::WriteFile(file_path, "dummy data"));
+  EXPECT_TRUE(base::PathExists(file_path));
+  EXPECT_FALSE(base::DirectoryExists(file_path));
+
+  // Should fail because file_path exists as a file, not a directory.
+  EXPECT_FALSE(SecureCreateDirectory(file_path));
+}
+
+TEST_F(GcpUtilsSecureCreateDirectoryTest, FailOnReparsePoint) {
+  base::FilePath target_dir = temp_dir_.GetPath().Append(L"TargetDir");
+  base::FilePath link_dir = temp_dir_.GetPath().Append(L"LinkDir");
+  ASSERT_TRUE(base::CreateDirectory(target_dir));
+  if (::CreateSymbolicLinkW(link_dir.value().c_str(),
+                            target_dir.value().c_str(),
+                            SYMBOLIC_LINK_FLAG_DIRECTORY)) {
+    // If symlink/reparse point creation succeeded, SecureCreateDirectory must
+    // reject it.
+    EXPECT_FALSE(SecureCreateDirectory(link_dir));
+  }
+}
+
 }  // namespace credential_provider
diff --git a/chrome/credential_provider/test/gcp_setup_unittest.cc b/chrome/credential_provider/test/gcp_setup_unittest.cc
index 69a3d834..bba356f 100644
--- a/chrome/credential_provider/test/gcp_setup_unittest.cc
+++ b/chrome/credential_provider/test/gcp_setup_unittest.cc
@@ -6,6 +6,7 @@
 
 #include <datetimeapi.h>
 #include <lmerr.h>
+#include <shlobj.h>
 #include <wrl/client.h>
 
 #include <memory>
@@ -354,6 +355,10 @@
   programdata_override_ = std::make_unique<base::ScopedPathOverride>(
       base::DIR_COMMON_APP_DATA, scoped_temp_progdata_dir_.GetPath());
 
+  if (!::IsUserAnAdmin()) {
+    GTEST_SKIP() << "Test requires administrative privileges.";
+  }
+
   ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
 
   // In non-component builds, base::FILE_MODULE will always return the path
diff --git a/chrome/credential_provider/test/gls_runner_test_base.cc b/chrome/credential_provider/test/gls_runner_test_base.cc
index 72277f4d..98a2352 100644
--- a/chrome/credential_provider/test/gls_runner_test_base.cc
+++ b/chrome/credential_provider/test/gls_runner_test_base.cc
@@ -4,6 +4,8 @@
 
 #include "gls_runner_test_base.h"
 
+#include <shlobj.h>
+
 #include <memory>
 
 #include "base/base_switches.h"
@@ -167,6 +169,10 @@
   ASSERT_TRUE(scoped_temp_progdata_dir_.CreateUniqueTempDir());
   programdata_override_ = std::make_unique<base::ScopedPathOverride>(
       base::DIR_COMMON_APP_DATA, scoped_temp_progdata_dir_.GetPath());
+
+  if (!::IsUserAnAdmin()) {
+    GTEST_SKIP() << "Test requires administrative privileges.";
+  }
 }
 
 void GlsRunnerTestBase::TearDown() {