From 4f5c21acd0a924e3d516c8c8e8da6944a1a9a5e1 Mon Sep 17 00:00:00 2001 From: Bob Owen Date: Tue, 13 May 2025 19:52:21 +0000 Subject: [PATCH] Bug 1963797: Don't try and use heap in chromium sandbox code in early start-up. r=handyman Using the heap too early can cause crashes, in particular when certain exploit mitigations cause the application verifier DLL to load. This is a cherry-pick and rebase of chromium commit: f1abab03e72635bf675f41715faadd78579086fc Differential Revision: https://phabricator.services.mozilla.com/D249042 --- ..._dont_use_heap_too_early_in_start_up.patch | 167 ++++++++++++++++++ .../sandbox/win/src/sandbox_nt_util.h | 8 + .../sandbox/win/src/signed_interception.cc | 27 ++- .../sandbox/win/src/target_interceptions.cc | 17 +- 4 files changed, 202 insertions(+), 17 deletions(-) create mode 100644 security/sandbox/chromium-shim/patches/upstream/10_dont_use_heap_too_early_in_start_up.patch diff --git a/security/sandbox/chromium-shim/patches/upstream/10_dont_use_heap_too_early_in_start_up.patch b/security/sandbox/chromium-shim/patches/upstream/10_dont_use_heap_too_early_in_start_up.patch new file mode 100644 index 000000000000..c5f7d36e2ca7 --- /dev/null +++ b/security/sandbox/chromium-shim/patches/upstream/10_dont_use_heap_too_early_in_start_up.patch @@ -0,0 +1,167 @@ +# HG changeset patch +# User Bob Owen +# Date 1747070870 -3600 +# Mon May 12 18:27:50 2025 +0100 +# Node ID 73d6e5ffce5bd487521b4a28e98ffa6c2e1fc54c +# Parent a6117fd7a8ce0ad6b6d5313d7d7b2abd83a57f95 +Bug 1963797: Don't try and use heap in chromium sandbox code in early start-up. r=handyman! + +Using the heap too early can cause crashes, in particular when certain exploit +mitigations cause the application verifier DLL to load. + +This is a cherry-pick and rebase of chromium commit: +f1abab03e72635bf675f41715faadd78579086fc + +diff --git a/security/sandbox/chromium/sandbox/win/src/sandbox_nt_util.h b/security/sandbox/chromium/sandbox/win/src/sandbox_nt_util.h +--- a/security/sandbox/chromium/sandbox/win/src/sandbox_nt_util.h ++++ b/security/sandbox/chromium/sandbox/win/src/sandbox_nt_util.h +@@ -210,11 +210,19 @@ class AutoProtectMemory { + }; + + // Returns true if the file_rename_information structure is supported by our + // rename handler. + bool IsSupportedRenameCall(FILE_RENAME_INFORMATION* file_info, + DWORD length, + uint32_t file_info_class); + ++// Version of memset that can be called before the CRT is initialized. ++__forceinline void Memset(void* ptr, int value, size_t num_bytes) { ++ unsigned char* byte_ptr = static_cast(ptr); ++ while (num_bytes--) { ++ *byte_ptr++ = static_cast(value); ++ } ++} ++ + } // namespace sandbox + + #endif // SANDBOX_SRC_SANDBOX_NT_UTIL_H__ +diff --git a/security/sandbox/chromium/sandbox/win/src/signed_interception.cc b/security/sandbox/chromium/sandbox/win/src/signed_interception.cc +--- a/security/sandbox/chromium/sandbox/win/src/signed_interception.cc ++++ b/security/sandbox/chromium/sandbox/win/src/signed_interception.cc +@@ -13,16 +13,19 @@ + #include "sandbox/win/src/sandbox_factory.h" + #include "sandbox/win/src/sandbox_nt_util.h" + #include "sandbox/win/src/sharedmem_ipc_client.h" + #include "sandbox/win/src/target_services.h" + #include "mozilla/sandboxing/sandboxLogging.h" + + 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, + ACCESS_MASK desired_access, + POBJECT_ATTRIBUTES object_attributes, + PLARGE_INTEGER maximum_size, + ULONG section_page_protection, + ULONG allocation_attributes, +@@ -45,34 +48,46 @@ TargetNtCreateSection(NtCreateSectionFun + + mozilla::sandboxing::LogBlocked("NtCreateSection"); + + // IPC must be fully started. + void* memory = GetGlobalIPCMemory(); + if (!memory) + break; + +- std::unique_ptr 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); ++ // Avoid memset inserted by -ftrivial-auto-var-init=pattern. ++ STACK_UNINITIALIZED char path_buffer[path_buffer_size]; ++ OBJECT_NAME_INFORMATION* path = ++ reinterpret_cast(path_buffer); ++ ULONG out_buffer_size = 0; ++ NTSTATUS status = g_nt.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 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())) + break; + + if (!ValidParameter(section_handle, sizeof(HANDLE), WRITE)) + break; + +- CrossCallReturn answer = {0}; ++ // Avoid memset inserted by -ftrivial-auto-var-init=pattern on debug builds. ++ STACK_UNINITIALIZED CrossCallReturn answer; ++ Memset(&answer, 0, sizeof(answer)); ++ + answer.nt_status = STATUS_INVALID_IMAGE_HASH; + SharedMemIPCClient ipc(memory); + ResultCode code = + CrossCall(ipc, IpcTag::NTCREATESECTION, file_handle, &answer); + + if (code != SBOX_ALL_OK) + break; + +diff --git a/security/sandbox/chromium/sandbox/win/src/target_interceptions.cc b/security/sandbox/chromium/sandbox/win/src/target_interceptions.cc +--- a/security/sandbox/chromium/sandbox/win/src/target_interceptions.cc ++++ b/security/sandbox/chromium/sandbox/win/src/target_interceptions.cc +@@ -41,44 +41,39 @@ TargetNtMapViewOfSection(NtMapViewOfSect + + do { + if (!NT_SUCCESS(ret)) + break; + + 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(*base)); + + // _strnicmp below may hit read access violations for some sections. We + // 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 && +- (g_nt._strnicmp( +- ansi_module_name, base::win::kApplicationVerifierDllName, +- g_nt.strlen(base::win::kApplicationVerifierDllName) + 1) == 0)) +- break; +- + if (ansi_module_name && + (g_nt._strnicmp(ansi_module_name, KERNEL32_DLL_NAME, + sizeof(KERNEL32_DLL_NAME)) == 0)) { + s_state = kAfterKernel32; + } + } __except (EXCEPTION_EXECUTE_HANDLER) { + } + } + ++ // 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; + + if (!IsValidImageSection(section, base, offset, view_size)) + break; + + UINT image_flags; + UNICODE_STRING* module_name = diff --git a/security/sandbox/chromium/sandbox/win/src/sandbox_nt_util.h b/security/sandbox/chromium/sandbox/win/src/sandbox_nt_util.h index 70d011dfb475..1e17370ceb57 100644 --- a/security/sandbox/chromium/sandbox/win/src/sandbox_nt_util.h +++ b/security/sandbox/chromium/sandbox/win/src/sandbox_nt_util.h @@ -215,6 +215,14 @@ bool IsSupportedRenameCall(FILE_RENAME_INFORMATION* file_info, DWORD length, uint32_t file_info_class); +// Version of memset that can be called before the CRT is initialized. +__forceinline void Memset(void* ptr, int value, size_t num_bytes) { + unsigned char* byte_ptr = static_cast(ptr); + while (num_bytes--) { + *byte_ptr++ = static_cast(value); + } +} + } // namespace sandbox #endif // SANDBOX_SRC_SANDBOX_NT_UTIL_H__ diff --git a/security/sandbox/chromium/sandbox/win/src/signed_interception.cc b/security/sandbox/chromium/sandbox/win/src/signed_interception.cc index 4f4310d6052b..4a5811db8254 100644 --- a/security/sandbox/chromium/sandbox/win/src/signed_interception.cc +++ b/security/sandbox/chromium/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, @@ -50,15 +53,24 @@ TargetNtCreateSection(NtCreateSectionFunction orig_CreateSection, if (!memory) break; - std::unique_ptr 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); + // Avoid memset inserted by -ftrivial-auto-var-init=pattern. + STACK_UNINITIALIZED char path_buffer[path_buffer_size]; + OBJECT_NAME_INFORMATION* path = + reinterpret_cast(path_buffer); + ULONG out_buffer_size = 0; + NTSTATUS status = g_nt.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 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())) @@ -67,7 +79,10 @@ TargetNtCreateSection(NtCreateSectionFunction orig_CreateSection, if (!ValidParameter(section_handle, sizeof(HANDLE), WRITE)) break; - CrossCallReturn answer = {0}; + // Avoid memset inserted by -ftrivial-auto-var-init=pattern on debug builds. + STACK_UNINITIALIZED CrossCallReturn answer; + Memset(&answer, 0, sizeof(answer)); + answer.nt_status = STATUS_INVALID_IMAGE_HASH; SharedMemIPCClient ipc(memory); ResultCode code = diff --git a/security/sandbox/chromium/sandbox/win/src/target_interceptions.cc b/security/sandbox/chromium/sandbox/win/src/target_interceptions.cc index 1b467814c6cf..7073161d1105 100644 --- a/security/sandbox/chromium/sandbox/win/src/target_interceptions.cc +++ b/security/sandbox/chromium/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(*base)); @@ -56,15 +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 && - (g_nt._strnicmp( - ansi_module_name, base::win::kApplicationVerifierDllName, - g_nt.strlen(base::win::kApplicationVerifierDllName) + 1) == 0)) - break; - if (ansi_module_name && (g_nt._strnicmp(ansi_module_name, KERNEL32_DLL_NAME, sizeof(KERNEL32_DLL_NAME)) == 0)) { @@ -74,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;