Enable sandbox shims to work regardless of timing

There are several limitations with the sandbox shims, root caused as
chromium sometimes accessing the Windows heap before it is safe to do
so, when OS code attempts to call NtCreateSection or NtMapViewOfSection
during super-early process start-up.

NtCreateSection - used to bypass CIG checks for binaries not signed by
Microsoft by brokering the syscall.

NtMapViewOfSection - used to listen to DLL loads in case we want to
also shim those DLLs.

This change adds early exits to the NtMapViewOfSection when these APIs
are called before kernel32 is loaded, to avoid needing to use the heap
while the process is in this super-early startup phase. There is not a
great publicly documented API we can use earlier, as Windows public API
is generally in kernel32, ntdll is mostly undocumented.

It should be safe to wait this long, as kernel32.dll should get loaded
before any other DLLs we try to shim at runtime.

For NtCreateSection shim, we modify it to only use stack memory instead
of heap memory. We did not apply the same approach as the
NtMapViewOfSection shim because NtCreateSection is not called for every
DLL load, so the approach to wait for kernel32 would mean the shims have
to depend on each other, though they are not always applied at the same
time.

Modified CheckWin10MsSignedPolicySuccess test so it actually applies the
shims used to load other DLLs on release build. Validated that this test
fails with AppVerifier enabled without these changes and passes with the
changes.

Verified manually with a smoke test that we can run chromium under
page heap, app verifier, and under the conditions from crbug.com/1519187

Also verified manually that sbox integration tests fail under these
conditions without the changes to the shims, and they pass with the
shim changes.

Previously attempted to land here, but reverted due to 1520945:
https://chromium-review.googlesource.com/c/chromium/src/+/5214980

Validated that per the previous revert, the change works under
--enable-features=NetworkServiceCodeIntegrity,NetworkServiceSandbox

Bug: 1519187, 465515, 1004989, 1520945, 1081080
Change-Id: I5dc65db4dca84cff998ea412dc14f76a680a7173
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5241720
Reviewed-by: Alex Gough <ajgo@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Commit-Queue: Stefan Smolen <ssmole@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1255734}

base/win/win_util.cc

@@ -778,28 +778,6 @@ bool IsCurrentSessionRemote() {
   return current_session_id != glass_session_id;
 }
 
-#if !defined(OFFICIAL_BUILD)
-bool IsAppVerifierEnabled(const std::wstring& process_name) {
-  RegKey key;
-
-  // Look for GlobalFlag in the IFEO\chrome.exe key. If it is present then
-  // Application Verifier or gflags.exe are configured. Most GlobalFlag
-  // settings are experimentally determined to be incompatible with renderer
-  // code integrity and a safe set is not known so any GlobalFlag entry is
-  // assumed to mean that Application Verifier (or pageheap) are enabled.
-  // The settings are propagated to both 64-bit WOW6432Node versions of the
-  // registry on 64-bit Windows, so only one check is needed.
-  return key.Open(
-             HKEY_LOCAL_MACHINE,
-             (L"SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion\\Image File "
-              L"Execution Options\\" +
-              process_name)
-                 .c_str(),
-             KEY_READ | KEY_WOW64_64KEY) == ERROR_SUCCESS &&
-         key.HasValue(L"GlobalFlag");
-}
-#endif  // !defined(OFFICIAL_BUILD)
-
 bool IsAppVerifierLoaded() {
   return GetModuleHandleA(kApplicationVerifierDllName);
 }

base/win/win_util.h

@@ -228,13 +228,6 @@ BASE_EXPORT bool IsRunningUnderDesktopName(std::wstring_view desktop_name);
 // Returns true if current session is a remote session.
 BASE_EXPORT bool IsCurrentSessionRemote();
 
-#if !defined(OFFICIAL_BUILD)
-// IsAppVerifierEnabled() indicates whether a newly created process will get
-// Application Verifier or pageheap injected into it. Only available in
-// unofficial builds to prevent abuse.
-BASE_EXPORT bool IsAppVerifierEnabled(const std::wstring& process_name);
-#endif  // !defined(OFFICIAL_BUILD)
-
 // IsAppVerifierLoaded() indicates whether Application Verifier is *already*
 // loaded into the current process.
 BASE_EXPORT bool IsAppVerifierLoaded();

chrome/browser/chrome_content_browser_client.cc

@@ -4709,13 +4709,6 @@ bool ChromeContentBrowserClient::PreSpawnChild(
       break;
   }
 
-#if !defined(OFFICIAL_BUILD)
-  // Disable renderer code integrity when Application Verifier or pageheap are
-  // enabled for chrome.exe to avoid renderer crashes. https://crbug.com/1004989
-  if (base::win::IsAppVerifierEnabled(chrome::kBrowserProcessExecutableName))
-    enforce_code_integrity = false;
-#endif  // !defined(OFFICIAL_BUILD)
-
   if (!enforce_code_integrity)
     return true;
 

sandbox/win/src/process_mitigations_unittest.cc

@@ -717,9 +717,10 @@ TEST(ProcessMitigationsTest, CheckWin10MsSignedPolicySuccessDelayed) {
 
 // This test validates that setting the MITIGATION_FORCE_MS_SIGNED_BINS
 // mitigation enables the setting on a process when non-delayed.
-
-// Disabled due to crbug.com/1081080
-TEST(ProcessMitigationsTest, DISABLED_CheckWin10MsSignedPolicySuccess) {
+TEST(ProcessMitigationsTest, CheckWin10MsSignedPolicySuccess) {
+  // AllowExtraDlls shims may run before ASAN has a chance to initialize its
+  // internal state, namely __asan_shadow_memory_dynamic_address.
+#if !defined(ADDRESS_SANITIZER)
   if (base::win::GetVersion() < base::win::Version::WIN10_TH2)
     return;
 
@@ -749,6 +750,96 @@ TEST(ProcessMitigationsTest, DISABLED_CheckWin10MsSignedPolicySuccess) {
 #endif  // defined(COMPONENT_BUILD)
 
   EXPECT_EQ(SBOX_TEST_SUCCEEDED, runner2.RunTest(test_command.c_str()));
+#endif  // !defined(ADDRESS_SANITIZER)
+}
+
+// This test attempts to load an unsigned dll, which should succeed only if
+// allowed by the CIG shims, and validate that the MicrosoftSignedOnly CIG
+// mitigation is applied.
+SBOX_TESTS_COMMAND int TestMsSignedLoadUnsignedDll(int argc, wchar_t** argv) {
+  PROCESS_MITIGATION_BINARY_SIGNATURE_POLICY policy = {};
+  if (!::GetProcessMitigationPolicy(::GetCurrentProcess(),
+                                    ProcessSignaturePolicy, &policy,
+                                    sizeof(policy))) {
+    return SBOX_TEST_NOT_FOUND;
+  }
+  if (!policy.MicrosoftSignedOnly) {
+    return SBOX_TEST_FIRST_ERROR;
+  }
+
+  base::FilePath hook_dll_path(hooking_dll::g_hook_dll_file);
+  base::ScopedNativeLibrary dll(hook_dll_path);
+  if (!dll.is_valid()) {
+    return SBOX_TEST_SECOND_ERROR;
+  }
+
+  return SBOX_TEST_SUCCEEDED;
+}
+
+// This test validates that setting the MITIGATION_FORCE_MS_SIGNED_BINS
+// mitigation enables the setting on a process when non-delayed, and that
+// process fails load a dll not signed by Microsoft.
+TEST(ProcessMitigationsTest, CheckWin10MsSignedPolicyAndDllLoadFailure) {
+  // AllowExtraDlls shims may run before ASAN has a chance to initialize its
+  // internal state, namely __asan_shadow_memory_dynamic_address.
+  // With component build we would have to allow all DLLs to load, which
+  // invalidates the test.
+#if !defined(ADDRESS_SANITIZER) && !defined(COMPONENT_BUILD)
+  if (base::win::GetVersion() < base::win::Version::WIN10_TH2) {
+    return;
+  }
+
+  std::wstring test_command = L"TestMsSignedLoadUnsignedDll";
+
+  TestRunner runner;
+  // After the sandbox is applied, the sandbox will prevent DLL loads. CIG
+  // should prevent the DLL load as well.
+  runner.SetTestState(BEFORE_REVERT);
+  sandbox::TargetConfig* config = runner.GetPolicy()->GetConfig();
+
+  EXPECT_EQ(config->SetProcessMitigations(MITIGATION_FORCE_MS_SIGNED_BINS),
+            SBOX_ALL_OK);
+
+  EXPECT_EQ(SBOX_TEST_SECOND_ERROR, runner.RunTest(test_command.c_str()));
+#endif  // !defined(ADDRESS_SANITIZER) && !defined(COMPONENT_BUILD)
+}
+
+// This test validates that setting the MITIGATION_FORCE_MS_SIGNED_BINS
+// mitigation enables the setting on a process when non-delayed, and that
+// process can load a dll.
+TEST(ProcessMitigationsTest, CheckWin10MsSignedPolicyAndDllLoadSuccess) {
+  // AllowExtraDlls shims may run before ASAN has a chance to initialize its
+  // internal state, namely __asan_shadow_memory_dynamic_address.
+#if !defined(ADDRESS_SANITIZER)
+  if (base::win::GetVersion() < base::win::Version::WIN10_TH2) {
+    return;
+  }
+
+  std::wstring test_command = L"TestMsSignedLoadUnsignedDll";
+
+  TestRunner runner;
+  // After the sandbox is applied, the sandbox will prevent DLL loads. Validate
+  // we can load a DLL specified in AllowExtraDlls before sandbox is applied.
+  runner.SetTestState(BEFORE_REVERT);
+  sandbox::TargetConfig* config = runner.GetPolicy()->GetConfig();
+
+  EXPECT_EQ(config->SetProcessMitigations(MITIGATION_FORCE_MS_SIGNED_BINS),
+            SBOX_ALL_OK);
+  // In a component build, allow all *.dll in current directory to load. On a
+  // release build, specify the name of the hooking dll that the test tries to
+  // load.
+  base::FilePath exe_path;
+  EXPECT_TRUE(base::PathService::Get(base::DIR_EXE, &exe_path));
+  EXPECT_EQ(sandbox::SBOX_ALL_OK,
+            config->AllowExtraDlls(
+#if defined(COMPONENT_BUILD)
+                exe_path.DirName().AppendASCII("*.dll").value().c_str()));
+#else
+                exe_path.Append(hooking_dll::g_hook_dll_file).value().c_str()));
+#endif
+
+  EXPECT_EQ(SBOX_TEST_SUCCEEDED, runner.RunTest(test_command.c_str()));
+#endif  // !defined(ADDRESS_SANITIZER)
 }
 
 //------------------------------------------------------------------------------

sandbox/win/src/sandbox_nt_util.cc

@@ -692,41 +692,6 @@ bool IsSupportedRenameCall(FILE_RENAME_INFORMATION* file_info,
   return true;
 }
 
-bool NtGetPathFromHandle(HANDLE handle,
-                         std::unique_ptr<wchar_t, NtAllocDeleter>* path) {
-  OBJECT_NAME_INFORMATION initial_buffer;
-  OBJECT_NAME_INFORMATION* name;
-  ULONG size = 0;
-  // Query the name information a first time to get the size of the name.
-  NTSTATUS status = GetNtExports()->QueryObject(handle, ObjectNameInformation,
-                                                &initial_buffer, size, &size);
-
-  if (!NT_SUCCESS(status) && status != STATUS_INFO_LENGTH_MISMATCH)
-    return false;
-
-  std::unique_ptr<BYTE[], NtAllocDeleter> name_ptr;
-  if (!size)
-    return false;
-  name_ptr.reset(new (NT_ALLOC) BYTE[size]);
-  name = reinterpret_cast<OBJECT_NAME_INFORMATION*>(name_ptr.get());
-
-  // Query the name information a second time to get the name of the
-  // object referenced by the handle.
-  status = GetNtExports()->QueryObject(handle, ObjectNameInformation, name,
-                                       size, &size);
-
-  if (STATUS_SUCCESS != status)
-    return false;
-  size_t num_path_wchars = (name->ObjectName.Length / sizeof(wchar_t)) + 1;
-  path->reset(new (NT_ALLOC) wchar_t[num_path_wchars]);
-  status =
-      CopyData(path->get(), name->ObjectName.Buffer, name->ObjectName.Length);
-  path->get()[num_path_wchars - 1] = L'\0';
-  if (STATUS_SUCCESS != status)
-    return false;
-  return true;
-}
-
 CLIENT_ID GetCurrentClientId() {
   return reinterpret_cast<PARTIAL_TEB*>(NtCurrentTeb())->ClientId;
 }

sandbox/win/src/sandbox_nt_util.h

@@ -184,10 +184,6 @@ bool IsValidImageSection(HANDLE section,
 // Converts an ansi string to an UNICODE_STRING.
 UNICODE_STRING* AnsiToUnicode(const char* string);
 
-// Resolves a handle to an nt path. Returns true if the handle can be resolved.
-bool NtGetPathFromHandle(HANDLE handle,
-                         std::unique_ptr<wchar_t, NtAllocDeleter>* path);
-
 // Provides a simple way to temporarily change the protection of a memory page.
 class AutoProtectMemory {
  public:

sandbox/win/src/sandbox_nt_util_unittest.cc

@@ -241,26 +241,6 @@ TEST(SandboxNtUtil, ValidParameter) {
   EXPECT_TRUE(verify_buffer());
 }
 
-TEST(SandboxNtUtil, NtGetPathFromHandle) {
-  base::FilePath exe;
-  ASSERT_TRUE(base::PathService::Get(base::FILE_EXE, &exe));
-  base::File exe_file(exe, base::File::FLAG_OPEN);
-  ASSERT_TRUE(exe_file.IsValid());
-  std::unique_ptr<wchar_t, NtAllocDeleter> path;
-  EXPECT_TRUE(NtGetPathFromHandle(exe_file.GetPlatformFile(), &path));
-
-  // Basic sanity test, the functionality of NtGetPathFromHandle to return
-  // the correct value is already tested from win_utils_unittest.cc.
-  EXPECT_TRUE(base::EndsWith(base::AsStringPiece16(path.get()),
-                             base::AsStringPiece16(exe.BaseName().value()),
-                             base::CompareCase::INSENSITIVE_ASCII));
-
-  // Compare to GetNtPathFromWin32Path for extra check.
-  auto nt_path = GetNtPathFromWin32Path(exe.value());
-  EXPECT_TRUE(nt_path);
-  EXPECT_STREQ(path.get(), nt_path->c_str());
-}
-
 TEST(SandboxNtUtil, CopyNameAndAttributes) {
   OBJECT_ATTRIBUTES object_attributes;
   InitializeObjectAttributes(&object_attributes, nullptr, 0, nullptr, nullptr);

sandbox/win/src/signed_interception.cc

@@ -18,6 +18,9 @@
 
 namespace sandbox {
 
+// Note that this shim may be called before the heap is available, we must get
+// as far as |QueryBroker| without using the heap, for example when AppVerifier
+// is enabled.
 NTSTATUS WINAPI
 TargetNtCreateSection(NtCreateSectionFunction orig_CreateSection,
                       PHANDLE section_handle,
@@ -48,15 +51,24 @@ TargetNtCreateSection(NtCreateSectionFunction orig_CreateSection,
     if (!memory)
       break;
 
-    std::unique_ptr<wchar_t, NtAllocDeleter> path;
+    // As mentioned at the top of the function, we need to use the stack here
+    // because the heap may not be available.
+    constexpr ULONG path_buffer_size =
+        (MAX_PATH * sizeof(wchar_t)) + sizeof(OBJECT_NAME_INFORMATION);
+    char path_buffer[path_buffer_size];
+    OBJECT_NAME_INFORMATION* path =
+        reinterpret_cast<OBJECT_NAME_INFORMATION*>(path_buffer);
+    ULONG out_buffer_size = 0;
+    NTSTATUS status =
+        GetNtExports()->QueryObject(file_handle, ObjectNameInformation, path,
+                                    path_buffer_size, &out_buffer_size);
 
-    if (!NtGetPathFromHandle(file_handle, &path))
+    if (!NT_SUCCESS(status)) {
       break;
-
-    const wchar_t* const_name = path.get();
+    }
 
     CountedParameterSet<NameBased> params;
-    params[NameBased::NAME] = ParamPickerMake(const_name);
+    params[NameBased::NAME] = ParamPickerMake(path->ObjectName.Buffer);
 
     // Check if this will be sent to the broker.
     if (!QueryBroker(IpcTag::NTCREATESECTION, params.GetBase()))

sandbox/win/src/target_interceptions.cc

@@ -46,8 +46,6 @@ TargetNtMapViewOfSection(NtMapViewOfSectionFunction orig_MapViewOfSection,
     if (!IsSameProcess(process))
       break;
 
-    // Only check for verifier.dll or kernel32.dll loading if we haven't moved
-    // past that state yet.
     if (s_state == kBeforeKernel32) {
       const char* ansi_module_name =
           GetAnsiImageInfoFromModule(reinterpret_cast<HMODULE>(*base));
@@ -56,17 +54,6 @@ TargetNtMapViewOfSection(NtMapViewOfSectionFunction orig_MapViewOfSection,
       // find what looks like a valid export directory for a PE module but the
       // pointer to the module name will be pointing to invalid memory.
       __try {
-        // Don't initialize the heap if verifier.dll is being loaded. This
-        // indicates Application Verifier is enabled and we should wait until
-        // the next module is loaded.
-        if (ansi_module_name &&
-            (GetNtExports()->_strnicmp(
-                 ansi_module_name, base::win::kApplicationVerifierDllName,
-                 GetNtExports()->strlen(
-                     base::win::kApplicationVerifierDllName) +
-                     1) == 0)) {
-          break;
-        }
         if (ansi_module_name &&
             (GetNtExports()->_strnicmp(ansi_module_name, KERNEL32_DLL_NAME,
                                        sizeof(KERNEL32_DLL_NAME)) == 0)) {
@@ -76,6 +63,12 @@ TargetNtMapViewOfSection(NtMapViewOfSectionFunction orig_MapViewOfSection,
       }
     }
 
+    // Assume the heap may not be initialized before kernel32 loads, which is
+    // the case when AppVerifier is enabled.
+    if (s_state == kBeforeKernel32) {
+      break;
+    }
+
     if (!InitHeap())
       break;