From 136f8e6b75ecba2057e36482c1ada4190d78dcb1 Mon Sep 17 00:00:00 2001 From: Carmelo Messina Date: Thu, 29 Feb 2024 14:03:52 +0100 Subject: [PATCH] Restore windows network sandbox partial pick of https://chromium-review.googlesource.com/c/chromium/src/+/5270572 --- build/cromite_patches_list.txt | 1 - ...Temp-disable-network-service-windows.patch | 20 -- .../patches/Improve-the-browser-sandbox.patch | 202 +++++++++++++++++- 3 files changed, 199 insertions(+), 24 deletions(-) delete mode 100644 build/patches/00Temp-disable-network-service-windows.patch diff --git a/build/cromite_patches_list.txt b/build/cromite_patches_list.txt index 0c49034a..27d0b0ce 100644 --- a/build/cromite_patches_list.txt +++ b/build/cromite_patches_list.txt @@ -288,7 +288,6 @@ Temp-disable-predictive-back-gesture.patch TEMP-Add-a-log-to-track-strange-behavior.patch Temp-guard-FileSystemAccessPersistentPermissions.patch Fix-chromium-build-bugs.patch -00Temp-disable-network-service-windows.patch eyeo-beta-118.0.5993.48-base.patch eyeo-beta-118.0.5993.48-chrome_integration.patch diff --git a/build/patches/00Temp-disable-network-service-windows.patch b/build/patches/00Temp-disable-network-service-windows.patch deleted file mode 100644 index 83ee7f78..00000000 --- a/build/patches/00Temp-disable-network-service-windows.patch +++ /dev/null @@ -1,20 +0,0 @@ -From: Your Name -Date: Wed, 21 Feb 2024 09:16:27 +0000 -Subject: Temp disable network service windows - ---- - sandbox/policy/features.cc | 2 +- - 1 file changed, 1 insertion(+), 1 deletion(-) - -diff --git a/sandbox/policy/features.cc b/sandbox/policy/features.cc ---- a/sandbox/policy/features.cc -+++ b/sandbox/policy/features.cc -@@ -217,6 +217,6 @@ SET_CROMITE_FEATURE_ENABLED(kUseRendererProcessPolicy); - SET_CROMITE_FEATURE_ENABLED(kRestrictRendererPoliciesInBaseline); - #endif - #if BUILDFLAG(IS_WIN) --SET_CROMITE_FEATURE_ENABLED(kNetworkServiceSandbox); -+//SET_CROMITE_FEATURE_ENABLED(kNetworkServiceSandbox); - #endif - } // namespace sandbox::policy::features --- diff --git a/build/patches/Improve-the-browser-sandbox.patch b/build/patches/Improve-the-browser-sandbox.patch index 0fc4fa2b..e0348d5c 100644 --- a/build/patches/Improve-the-browser-sandbox.patch +++ b/build/patches/Improve-the-browser-sandbox.patch @@ -7,9 +7,12 @@ using the new flags on android License: GPL-2.0-or-later - https://spdx.org/licenses/GPL-2.0-or-later.html --- - chrome/browser/chrome_content_browser_client.cc | 2 +- - sandbox/policy/features.cc | 7 +++++++ - 2 files changed, 8 insertions(+), 1 deletion(-) + .../browser/chrome_content_browser_client.cc | 2 +- + sandbox/policy/features.cc | 7 ++++ + sandbox/win/src/sandbox_nt_util.h | 12 ++++-- + sandbox/win/src/signed_interception.cc | 37 +++++++++------- + sandbox/win/src/target_interceptions.cc | 42 +++++++------------ + 5 files changed, 54 insertions(+), 46 deletions(-) diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc --- a/chrome/browser/chrome_content_browser_client.cc @@ -38,4 +41,197 @@ diff --git a/sandbox/policy/features.cc b/sandbox/policy/features.cc +SET_CROMITE_FEATURE_ENABLED(kNetworkServiceSandbox); +#endif } // namespace sandbox::policy::features +diff --git a/sandbox/win/src/sandbox_nt_util.h b/sandbox/win/src/sandbox_nt_util.h +--- a/sandbox/win/src/sandbox_nt_util.h ++++ b/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* path); +- + // Provides a simple way to temporarily change the protection of a memory page. + class AutoProtectMemory { + public: +@@ -223,6 +219,14 @@ bool IsSupportedRenameCall(FILE_RENAME_INFORMATION* file_info, + // Get the CLIENT_ID from the current TEB. + CLIENT_ID GetCurrentClientId(); + ++// 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_WIN_SRC_SANDBOX_NT_UTIL_H_ +diff --git a/sandbox/win/src/signed_interception.cc b/sandbox/win/src/signed_interception.cc +--- a/sandbox/win/src/signed_interception.cc ++++ b/sandbox/win/src/signed_interception.cc +@@ -14,11 +14,13 @@ + #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_interceptions.h" + #include "sandbox/win/src/target_services.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, +@@ -43,27 +45,31 @@ TargetNtCreateSection(NtCreateSectionFunction orig_CreateSection, + break; + if (allocation_attributes != SEC_IMAGE) + break; +- // We shouldn't need to broker section creations until after kernel32.dll is +- // loaded, and delaying is necessary to avoid using the Windows heap for any +- // sections created before the Windows heap is initialized. +- if (GetSectionLoadState() != SectionLoadState::kAfterKernel32) { +- break; +- } + + // IPC must be fully started. + void* memory = GetGlobalIPCMemory(); + if (!memory) + break; + +- std::unique_ptr path; +- +- if (!NtGetPathFromHandle(file_handle, &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 = ++ GetNtExports()->QueryObject(file_handle, ObjectNameInformation, path, ++ path_buffer_size, &out_buffer_size); ++ ++ 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())) +@@ -72,7 +78,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/sandbox/win/src/target_interceptions.cc b/sandbox/win/src/target_interceptions.cc +--- a/sandbox/win/src/target_interceptions.cc ++++ b/sandbox/win/src/target_interceptions.cc +@@ -4,31 +4,16 @@ + + #include "sandbox/win/src/target_interceptions.h" + +-#include +- + #include + ++#include "base/win/static_constants.h" + #include "sandbox/win/src/interception_agent.h" + #include "sandbox/win/src/sandbox_factory.h" + #include "sandbox/win/src/sandbox_nt_util.h" + + namespace sandbox { + +-namespace { +- +-std::atomic g_section_load_state{ +- SectionLoadState::kBeforeKernel32}; +- +-void UpdateSectionLoadState(SectionLoadState new_state) { +- g_section_load_state = new_state; +-} +- + const char KERNEL32_DLL_NAME[] = "kernel32.dll"; +-} // namespace +- +-SectionLoadState GetSectionLoadState() { +- return g_section_load_state.load(std::memory_order_relaxed); +-} + + // Hooks NtMapViewOfSection to detect the load of DLLs. If hot patching is + // required for this dll, this functions patches it. +@@ -47,6 +32,7 @@ TargetNtMapViewOfSection(NtMapViewOfSectionFunction orig_MapViewOfSection, + NTSTATUS ret = orig_MapViewOfSection(section, process, base, zero_bits, + commit_size, offset, view_size, inherit, + allocation_type, protect); ++ static SectionLoadState s_state = SectionLoadState::kBeforeKernel32; + + do { + if (!NT_SUCCESS(ret)) +@@ -57,7 +43,7 @@ TargetNtMapViewOfSection(NtMapViewOfSectionFunction orig_MapViewOfSection, + + // Only check for verifier.dll or kernel32.dll loading if we haven't moved + // past that state yet. +- if (GetSectionLoadState() == SectionLoadState::kBeforeKernel32) { ++ if (s_state == SectionLoadState::kBeforeKernel32) { + const char* ansi_module_name = + GetAnsiImageInfoFromModule(reinterpret_cast(*base)); + +@@ -65,24 +51,26 @@ 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)) { +- UpdateSectionLoadState(SectionLoadState::kAfterKernel32); ++ s_state = SectionLoadState::kAfterKernel32; + } + } __except (EXCEPTION_EXECUTE_HANDLER) { + } + } + +- // Assume the Windows heap isn't initialized until we've loaded kernel32. We +- // don't expect that we will need to patch any modules before kernel32.dll +- // is loaded as they should all depend on kernel32.dll, and Windows may load +- // sections before it's safe to call into the Windows heap (e.g. AppVerifier +- // or internal Windows feature key checking). +- if (GetSectionLoadState() == SectionLoadState::kBeforeKernel32) { +- break; +- } +- + if (!InitHeap()) + break; + --