Use distinct methods for policy rules in sandbox

The sandbox library on Windows includes a complex rules engine to
allow proxied functions to evaluate their arguments. Details of
this engine leak out of the sandbox interface via the AddRule()
method which allows specification of a subsystem and semantics.

In practice only four actions are currently supported: allowing
access to files or pipes, allowing Dlls to load under CIG and
setting up GDI hooks for Win32 lockdown.

This CL removes the generic AddRule method from sandbox_policy.h
and replaces it with several more-specific functions, allowing
deletion of the SubSystem enum. This should allow for simplification
of the internals of the rules-engine without exposing these
changes to the rest of Chromium.

No functional changes.

Bug: 1023583
Change-Id: Ia14fdd4913345ac00711a44d5efb3e753d07260f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4981986
Reviewed-by: Will Harris <wfh@chromium.org>
Commit-Queue: Alex Gough <ajgo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1216336}

chrome/browser/chrome_content_browser_client.cc

@@ -4884,9 +4884,7 @@ bool ChromeContentBrowserClient::PreSpawnChild(
 
   // Allow loading Chrome's DLLs.
   for (const auto* dll : {chrome::kBrowserResourcesDll, chrome::kElfDll}) {
-    result = config->AddRule(sandbox::SubSystem::kSignedBinary,
-                             sandbox::Semantics::kSignedAllowLoad,
-                             GetModulePath(dll).value().c_str());
+    result = config->AllowExtraDlls(GetModulePath(dll).value().c_str());
     if (result != sandbox::SBOX_ALL_OK)
       return false;
   }

content/browser/utility_sandbox_delegate_win.cc

@@ -66,9 +66,7 @@ bool AudioInitializeConfig(sandbox::TargetConfig* config) {
 
   // The Audio Service process uses a base::SyncSocket for transmitting audio
   // data.
-  result = config->AddRule(sandbox::SubSystem::kNamedPipes,
-                           sandbox::Semantics::kNamedPipesAllowAny,
-                           L"\\\\.\\pipe\\chrome.sync.*");
+  result = config->AllowNamedPipes(L"\\\\.\\pipe\\chrome.sync.*");
   if (result != sandbox::SBOX_ALL_OK) {
     return false;
   }
@@ -152,19 +150,16 @@ bool IconReaderInitializeConfig(sandbox::TargetConfig* config) {
     return false;
 
   // Allow file read. These should match IconLoader::GroupForFilepath().
-  result =
-      config->AddRule(sandbox::SubSystem::kFiles,
-                      sandbox::Semantics::kFilesAllowReadonly, L"\\??\\*.exe");
+  result = config->AllowFileAccess(sandbox::FileSemantics::kAllowReadonly,
+                                   L"\\??\\*.exe");
   if (result != sandbox::SBOX_ALL_OK)
     return false;
-  result =
-      config->AddRule(sandbox::SubSystem::kFiles,
-                      sandbox::Semantics::kFilesAllowReadonly, L"\\??\\*.dll");
+  result = config->AllowFileAccess(sandbox::FileSemantics::kAllowReadonly,
+                                   L"\\??\\*.dll");
   if (result != sandbox::SBOX_ALL_OK)
     return false;
-  result =
-      config->AddRule(sandbox::SubSystem::kFiles,
-                      sandbox::Semantics::kFilesAllowReadonly, L"\\??\\*.ico");
+  result = config->AllowFileAccess(sandbox::FileSemantics::kAllowReadonly,
+                                   L"\\??\\*.ico");
   if (result != sandbox::SBOX_ALL_OK)
     return false;
   return true;

sandbox/policy/win/sandbox_win.cc

@@ -181,15 +181,14 @@ bool AddWindowsFontsDir(TargetConfig* config) {
     return false;
   }
 
-  ResultCode result =
-      config->AddRule(SubSystem::kFiles, Semantics::kFilesAllowReadonly,
-                      directory.value().c_str());
+  ResultCode result = config->AllowFileAccess(FileSemantics::kAllowReadonly,
+                                              directory.value().c_str());
   if (result != SBOX_ALL_OK)
     return false;
 
   std::wstring directory_str = directory.value() + L"\\*";
-  result = config->AddRule(SubSystem::kFiles, Semantics::kFilesAllowReadonly,
-                           directory_str.c_str());
+  result = config->AllowFileAccess(FileSemantics::kAllowReadonly,
+                                   directory_str.c_str());
   if (result != SBOX_ALL_OK)
     return false;
 
@@ -271,9 +270,8 @@ ResultCode AddGenericConfig(sandbox::TargetConfig* config) {
     return SBOX_ERROR_GENERIC;
   base::FilePath pdb_path = exe.DirName().Append(L"*.pdb");
   {
-    ResultCode result =
-        config->AddRule(SubSystem::kFiles, Semantics::kFilesAllowReadonly,
-                        pdb_path.value().c_str());
+    ResultCode result = config->AllowFileAccess(FileSemantics::kAllowReadonly,
+                                                pdb_path.value().c_str());
     if (result != SBOX_ALL_OK) {
       return result;
     }
@@ -295,9 +293,8 @@ ResultCode AddGenericConfig(sandbox::TargetConfig* config) {
     base::FilePath sancov_path =
         base::FilePath(coverage_dir).Append(L"*.sancov");
     {
-      ResultCode result =
-          config->AddRule(SubSystem::kFiles, Semantics::kFilesAllowAny,
-                          sancov_path.value().c_str());
+      ResultCode result = config->AllowFileAccess(FileSemantics::kAllowAny,
+                                                  sancov_path.value().c_str());
       if (result != SBOX_ALL_OK) {
         return result;
       }
@@ -686,8 +683,8 @@ ResultCode GenerateConfigForSandboxedProcess(const base::CommandLine& cmd_line,
     if (logging::IsLoggingToFileEnabled()) {
       auto log_path = logging::GetLogFileFullPath();
       DCHECK(base::FilePath(log_path).IsAbsolute());
-      result = config->AddRule(SubSystem::kFiles, Semantics::kFilesAllowAny,
-                               log_path.c_str());
+      result =
+          config->AllowFileAccess(FileSemantics::kAllowAny, log_path.c_str());
       if (result != SBOX_ALL_OK) {
         return result;
       }
@@ -846,8 +843,7 @@ ResultCode SandboxWin::AddWin32kLockdownPolicy(TargetConfig* config) {
   if (result != SBOX_ALL_OK)
     return result;
 
-  return config->AddRule(SubSystem::kWin32kLockdown, Semantics::kFakeGdiInit,
-                         nullptr);
+  return config->SetFakeGdiInit();
 #else  // !defined(NACL_WIN64)
   return SBOX_ALL_OK;
 #endif

sandbox/policy/win/sandbox_win_unittest.cc

@@ -62,11 +62,17 @@ class TestTargetConfig : public TargetConfig {
   }
   JobLevel GetJobLevel() const override { return sandbox::JobLevel{}; }
   void SetJobMemoryLimit(size_t memory_limit) override {}
-  ResultCode AddRule(SubSystem subsystem,
-                     Semantics semantics,
-                     const wchar_t* pattern) override {
+  ResultCode AllowFileAccess(FileSemantics semantics,
+                             const wchar_t* pattern) override {
     return SBOX_ALL_OK;
   }
+  ResultCode AllowNamedPipes(const wchar_t* pattern) override {
+    return SBOX_ALL_OK;
+  }
+  ResultCode AllowExtraDlls(const wchar_t* pattern) override {
+    return SBOX_ALL_OK;
+  }
+  ResultCode SetFakeGdiInit() override { return SBOX_ALL_OK; }
   void AddDllToUnload(const wchar_t* dll_name) override {
     blocklisted_dlls_.push_back(dll_name);
   }

sandbox/win/fuzzer/sandbox_policy_rule_fuzzer.cc

@@ -18,32 +18,26 @@
 // We only use the first two params so don't need more.
 constexpr size_t maxParams = 2;
 
-// This fills policies with rules based on the current
-// renderer sandbox in Chrome.
+// This fills policies with rules based on the current renderer sandbox in
+// Chrome - the point isn't to test the /sandbox/ but to fuzz the rule matching.
 std::unique_ptr<sandbox::PolicyBase> InitPolicy() {
   auto policy = std::make_unique<sandbox::PolicyBase>("");
   auto* config = policy->GetConfig();
 
-  auto result = config->AddRule(sandbox::SubSystem::kWin32kLockdown,
-                                sandbox::Semantics::kFakeGdiInit, nullptr);
+  auto result = config->SetFakeGdiInit();
   if (result != sandbox::SBOX_ALL_OK)
     return nullptr;
 
-  result = config->AddRule(sandbox::SubSystem::kFiles,
-                           sandbox::Semantics::kFilesAllowAny,
-                           L"\\??\\pipe\\chrome.*");
+  result = config->AllowFileAccess(sandbox::FileSemantics::kAllowAny,
+                                   L"\\??\\pipe\\chrome.*");
   if (result != sandbox::SBOX_ALL_OK)
     return nullptr;
 
-  result = config->AddRule(sandbox::SubSystem::kNamedPipes,
-                           sandbox::Semantics::kNamedPipesAllowAny,
-                           L"\\\\.\\pipe\\chrome.nacl.*");
+  result = config->AllowNamedPipes(L"\\\\.\\pipe\\chrome.unused.*");
   if (result != sandbox::SBOX_ALL_OK)
     return nullptr;
 
-  result = config->AddRule(sandbox::SubSystem::kNamedPipes,
-                           sandbox::Semantics::kNamedPipesAllowAny,
-                           L"\\\\.\\pipe\\chrome.sync.*");
+  result = config->AllowNamedPipes(L"\\\\.\\pipe\\chrome.sync.*");
   if (result != sandbox::SBOX_ALL_OK)
     return nullptr;
 

sandbox/win/src/address_sanitizer_test.cc

@@ -80,8 +80,8 @@ TEST_F(AddressSanitizerTests, TestAddressSanitizer) {
   base::FilePath exe;
   ASSERT_TRUE(base::PathService::Get(base::FILE_EXE, &exe));
   base::FilePath pdb_path = exe.DirName().Append(L"*.pdb");
-  ASSERT_TRUE(runner.AddFsRule(Semantics::kFilesAllowReadonly,
-                               pdb_path.value().c_str()));
+  ASSERT_TRUE(runner.AllowFileAccess(FileSemantics::kAllowReadonly,
+                                     pdb_path.value().c_str()));
 
   env_->SetVar("ASAN_OPTIONS", "exitcode=123");
   if (asan_build) {

sandbox/win/src/file_policy_test.cc

@@ -274,25 +274,25 @@ SBOX_TESTS_COMMAND int File_CopyFile(int argc, wchar_t** argv) {
 
 TEST(FilePolicyTest, DenyNtCreateCalc) {
   TestRunner runner;
-  EXPECT_TRUE(runner.AddRuleSys32(Semantics::kFilesAllowAny, L"calc.txt"));
+  EXPECT_TRUE(runner.AddRuleSys32(FileSemantics::kAllowAny, L"calc.txt"));
   EXPECT_EQ(SBOX_TEST_DENIED, runner.RunTest(L"File_CreateSys32 calc.exe"));
 
   TestRunner before_revert;
   EXPECT_TRUE(
-      before_revert.AddRuleSys32(Semantics::kFilesAllowAny, L"calc.txt"));
+      before_revert.AddRuleSys32(FileSemantics::kAllowAny, L"calc.txt"));
   before_revert.SetTestState(BEFORE_REVERT);
   EXPECT_EQ(SBOX_TEST_SUCCEEDED,
             before_revert.RunTest(L"File_CreateSys32 calc.exe"));
 }
 
 TEST(FilePolicyTest, AllowNtCreateCalc) {
   TestRunner runner;
-  EXPECT_TRUE(runner.AddRuleSys32(Semantics::kFilesAllowAny, L"calc.exe"));
+  EXPECT_TRUE(runner.AddRuleSys32(FileSemantics::kAllowAny, L"calc.exe"));
   EXPECT_EQ(SBOX_TEST_SUCCEEDED, runner.RunTest(L"File_CreateSys32 calc.exe"));
 
   TestRunner before_revert;
   EXPECT_TRUE(
-      before_revert.AddRuleSys32(Semantics::kFilesAllowAny, L"calc.exe"));
+      before_revert.AddRuleSys32(FileSemantics::kAllowAny, L"calc.exe"));
   before_revert.SetTestState(BEFORE_REVERT);
   EXPECT_EQ(SBOX_TEST_SUCCEEDED,
             before_revert.RunTest(L"File_CreateSys32 calc.exe"));
@@ -305,13 +305,13 @@ TEST(FilePolicyTest, AllowNtCreateWithNativePath) {
   std::wstring nt_path = opt_nt_path.value();
 
   TestRunner runner;
-  runner.AddFsRule(Semantics::kFilesAllowReadonly, nt_path.c_str());
+  runner.AllowFileAccess(FileSemantics::kAllowReadonly, nt_path.c_str());
   wchar_t buff[MAX_PATH];
   ::wsprintfW(buff, L"File_CreateSys32 %s", nt_path.c_str());
   EXPECT_EQ(SBOX_TEST_SUCCEEDED, runner.RunTest(buff));
 
   TestRunner runner2;
-  runner2.AddFsRule(Semantics::kFilesAllowReadonly, nt_path.c_str());
+  runner2.AllowFileAccess(FileSemantics::kAllowReadonly, nt_path.c_str());
   nt_path = base::ToLowerASCII(nt_path);
   ::wsprintfW(buff, L"File_CreateSys32 %s", nt_path.c_str());
   EXPECT_EQ(SBOX_TEST_SUCCEEDED, runner2.RunTest(buff));
@@ -320,7 +320,7 @@ TEST(FilePolicyTest, AllowNtCreateWithNativePath) {
 std::unique_ptr<TestRunner> AllowReadOnlyRunner(wchar_t* temp_file_name) {
   auto runner = std::make_unique<TestRunner>();
   EXPECT_TRUE(
-      runner->AddFsRule(Semantics::kFilesAllowReadonly, temp_file_name));
+      runner->AllowFileAccess(FileSemantics::kAllowReadonly, temp_file_name));
   return runner;
 }
 
@@ -381,7 +381,7 @@ TEST(FilePolicyTest, AllowImplicitDeviceName) {
 
   TestRunner runner_with_rule;
   EXPECT_TRUE(
-      runner_with_rule.AddFsRule(Semantics::kFilesAllowAny, path.c_str()));
+      runner_with_rule.AllowFileAccess(FileSemantics::kAllowAny, path.c_str()));
   EXPECT_EQ(SBOX_TEST_SUCCEEDED, runner_with_rule.RunTest(command));
 
   DeleteFile(temp_file_name);
@@ -397,7 +397,7 @@ TEST(FilePolicyTest, AllowWildcard) {
   ASSERT_NE(::GetTempFileName(temp_directory, L"test", 0, temp_file_name), 0u);
 
   wcscat_s(temp_directory, MAX_PATH, L"*");
-  EXPECT_TRUE(runner.AddFsRule(Semantics::kFilesAllowAny, temp_directory));
+  EXPECT_TRUE(runner.AllowFileAccess(FileSemantics::kAllowAny, temp_directory));
 
   wchar_t command_write[MAX_PATH + 20] = {};
   wsprintf(command_write, L"File_Create Write \"%ls\"", temp_file_name);
@@ -410,7 +410,7 @@ TEST(FilePolicyTest, AllowWildcard) {
 
 std::unique_ptr<TestRunner> AllowNtCreatePatternRunner() {
   auto runner = std::make_unique<TestRunner>();
-  EXPECT_TRUE(runner->AddRuleSys32(Semantics::kFilesAllowAny, L"App*.dll"));
+  EXPECT_TRUE(runner->AddRuleSys32(FileSemantics::kAllowAny, L"App*.dll"));
   return runner;
 }
 
@@ -434,7 +434,7 @@ TEST(FilePolicyTest, AllowNtCreatePatternRule) {
 
 TEST(FilePolicyTest, CheckNotFound) {
   TestRunner runner;
-  EXPECT_TRUE(runner.AddRuleSys32(Semantics::kFilesAllowAny, L"n*.dll"));
+  EXPECT_TRUE(runner.AddRuleSys32(FileSemantics::kAllowAny, L"n*.dll"));
 
   EXPECT_EQ(SBOX_TEST_NOT_FOUND,
             runner.RunTest(L"File_OpenSys32 notfound.dll"));
@@ -447,11 +447,11 @@ TEST(FilePolicyTest, CheckNoLeak) {
 
 std::unique_ptr<TestRunner> QueryAttributesFileRunner() {
   auto runner = std::make_unique<TestRunner>();
-  EXPECT_TRUE(runner->AddRuleSys32(Semantics::kFilesAllowAny, L"apphelp.dll"));
-  EXPECT_TRUE(runner->AddRuleSys32(Semantics::kFilesAllowAny, L"notfound.exe"));
-  EXPECT_TRUE(runner->AddRuleSys32(Semantics::kFilesAllowAny, L"drivers"));
+  EXPECT_TRUE(runner->AddRuleSys32(FileSemantics::kAllowAny, L"apphelp.dll"));
+  EXPECT_TRUE(runner->AddRuleSys32(FileSemantics::kAllowAny, L"notfound.exe"));
+  EXPECT_TRUE(runner->AddRuleSys32(FileSemantics::kAllowAny, L"drivers"));
   EXPECT_TRUE(
-      runner->AddRuleSys32(Semantics::kFilesAllowReadonly, L"ipconfig.exe"));
+      runner->AddRuleSys32(FileSemantics::kAllowReadonly, L"ipconfig.exe"));
   return runner;
 }
 
@@ -494,19 +494,19 @@ std::unique_ptr<TestRunner> RenameRunner(
     std::vector<std::wstring>& temp_files) {
   auto runner = std::make_unique<TestRunner>();
   // Add rules to make file0->file1 succeed.
-  runner->AddFsRule(Semantics::kFilesAllowAny, temp_files[0].c_str());
-  runner->AddFsRule(Semantics::kFilesAllowAny, temp_files[1].c_str());
+  runner->AllowFileAccess(FileSemantics::kAllowAny, temp_files[0].c_str());
+  runner->AllowFileAccess(FileSemantics::kAllowAny, temp_files[1].c_str());
 
   // Add rules to make file2->file3 fail.
-  runner->AddFsRule(Semantics::kFilesAllowAny, temp_files[2].c_str());
-  runner->AddFsRule(Semantics::kFilesAllowReadonly, temp_files[3].c_str());
+  runner->AllowFileAccess(FileSemantics::kAllowAny, temp_files[2].c_str());
+  runner->AllowFileAccess(FileSemantics::kAllowReadonly, temp_files[3].c_str());
 
   // Add rules to make file4->file5 fail.
-  runner->AddFsRule(Semantics::kFilesAllowReadonly, temp_files[4].c_str());
-  runner->AddFsRule(Semantics::kFilesAllowAny, temp_files[5].c_str());
+  runner->AllowFileAccess(FileSemantics::kAllowReadonly, temp_files[4].c_str());
+  runner->AllowFileAccess(FileSemantics::kAllowAny, temp_files[5].c_str());
 
   // Add rules to make file6->no_pol_file fail.
-  runner->AddFsRule(Semantics::kFilesAllowAny, temp_files[6].c_str());
+  runner->AllowFileAccess(FileSemantics::kAllowAny, temp_files[6].c_str());
   return runner;
 }
 
@@ -557,7 +557,7 @@ TEST(FilePolicyTest, TestRename) {
 
 std::unique_ptr<TestRunner> AllowNotepadRunner() {
   auto runner = std::make_unique<TestRunner>();
-  runner->AddRuleSys32(Semantics::kFilesAllowAny, L"notepad.exe");
+  runner->AddRuleSys32(FileSemantics::kAllowAny, L"notepad.exe");
   return runner;
 }
 
@@ -581,7 +581,7 @@ TEST(FilePolicyTest, OpenSys32FilesAllowNotepad) {
 
 std::unique_ptr<TestRunner> FileGetDiskSpaceRunner() {
   auto runner = std::make_unique<TestRunner>();
-  runner->AddRuleSys32(Semantics::kFilesAllowReadonly, L"");
+  runner->AddRuleSys32(FileSemantics::kAllowReadonly, L"");
   return runner;
 }
 
@@ -612,7 +612,7 @@ TEST(FilePolicyTest, FileGetDiskSpace) {
 std::unique_ptr<TestRunner> ReparsePointRunner(
     std::wstring& temp_dir_wildcard) {
   auto runner = std::make_unique<TestRunner>();
-  runner->AddFsRule(Semantics::kFilesAllowAny, temp_dir_wildcard.c_str());
+  runner->AllowFileAccess(FileSemantics::kAllowAny, temp_dir_wildcard.c_str());
   return runner;
 }
 
@@ -720,7 +720,7 @@ TEST(FilePolicyTest, TestCopyFile) {
   runner.SetTimeout(2000);
 
   // Allow read access to calc.exe, this should be on all Windows versions.
-  ASSERT_TRUE(runner.AddRuleSys32(Semantics::kFilesAllowReadonly, L"calc.exe"));
+  ASSERT_TRUE(runner.AddRuleSys32(FileSemantics::kAllowReadonly, L"calc.exe"));
 
   sandbox::TargetPolicy* policy = runner.GetPolicy();
 

sandbox/win/src/filesystem_policy.cc

@@ -81,10 +81,8 @@ NTSTATUS NtCreateFileInTarget(HANDLE* target_file_handle,
 }  // namespace.
 
 bool FileSystemPolicy::GenerateRules(const wchar_t* name,
-                                     Semantics semantics,
+                                     FileSemantics semantics,
                                      LowLevelPolicy* policy) {
-  CHECK(semantics == Semantics::kFilesAllowReadonly ||
-        semantics == Semantics::kFilesAllowAny);
   std::wstring mod_name(name);
   if (mod_name.empty()) {
     return false;
@@ -113,7 +111,7 @@ bool FileSystemPolicy::GenerateRules(const wchar_t* name,
   PolicyRule query(result);
   PolicyRule query_full(result);
 
-  if (semantics == Semantics::kFilesAllowReadonly) {
+  if (semantics == FileSemantics::kAllowReadonly) {
     // We consider all flags that are not known to be readonly as potentially
     // used for write.
     DWORD allowed_flags = FILE_READ_DATA | FILE_READ_ATTRIBUTES | FILE_READ_EA |
@@ -147,7 +145,7 @@ bool FileSystemPolicy::GenerateRules(const wchar_t* name,
   }
 
   // Rename is not allowed for read-only and does not make sense for pipes.
-  if (semantics == Semantics::kFilesAllowAny && !is_pipe) {
+  if (semantics == FileSemantics::kAllowAny && !is_pipe) {
     PolicyRule rename(result);
     if (!rename.AddStringMatch(IF, OpenFile::NAME, name, CASE_INSENSITIVE) ||
         !policy->AddRule(IpcTag::NTSETINFO_RENAME, &rename)) {

sandbox/win/src/filesystem_policy.h

@@ -25,7 +25,7 @@ class FileSystemPolicy {
   // 'semantics' is the desired semantics for the open or create.
   // 'policy' is the policy generator to which the rules are going to be added.
   static bool GenerateRules(const wchar_t* name,
-                            Semantics semantics,
+                            FileSemantics semantics,
                             LowLevelPolicy* policy);
 
   // Performs the desired policy action on a create request with an