From 85a6b05b421e42ab95b7c72d9ae8712017ae321e Mon Sep 17 00:00:00 2001 From: Bob Owen Date: Tue, 4 Jul 2023 17:17:06 +0000 Subject: [PATCH] Bug 1839463 p2: Add TargetNtImpersonateAnonymousToken and remove TargetNtSetInformationThread patch. r=handyman Depends on D181611 Differential Revision: https://phabricator.services.mozilla.com/D181612 --- ...nateAnonymousToken_before_LowerToken.patch | 189 ++++++++++++++++++ .../patches/with_update/patch_order.txt | 2 +- ..._TargetNtSetInformationThread_change.patch | 39 ---- 3 files changed, 190 insertions(+), 40 deletions(-) create mode 100644 security/sandbox/chromium-shim/patches/with_update/block_NtImpersonateAnonymousToken_before_LowerToken.patch delete mode 100644 security/sandbox/chromium-shim/patches/with_update/revert_TargetNtSetInformationThread_change.patch diff --git a/security/sandbox/chromium-shim/patches/with_update/block_NtImpersonateAnonymousToken_before_LowerToken.patch b/security/sandbox/chromium-shim/patches/with_update/block_NtImpersonateAnonymousToken_before_LowerToken.patch new file mode 100644 index 000000000000..735b12685882 --- /dev/null +++ b/security/sandbox/chromium-shim/patches/with_update/block_NtImpersonateAnonymousToken_before_LowerToken.patch @@ -0,0 +1,189 @@ +# HG changeset patch +# User Bob Owen +# Date 1687248452 -3600 +# Tue Jun 20 09:07:32 2023 +0100 +# Node ID a07e3be35d9e5558fb95a18e3b858d5f4654dce9 +# Parent 5f4aecabd0376981a5f837f5de593c106c194712 +Bug 1839463: Block NtImpersonateAnonymousToken before RevertToSelf. + +Note this patch is slightly different from what landed for Bug 1839463, because +that also included the reversion of a different patch. + +diff --git a/security/sandbox/chromium/sandbox/win/src/interceptors.h b/security/sandbox/chromium/sandbox/win/src/interceptors.h +--- a/security/sandbox/chromium/sandbox/win/src/interceptors.h ++++ b/security/sandbox/chromium/sandbox/win/src/interceptors.h +@@ -11,16 +11,17 @@ + + namespace sandbox { + + enum InterceptorId { + // Internal use: + MAP_VIEW_OF_SECTION_ID = 0, + UNMAP_VIEW_OF_SECTION_ID, + // Policy broker: ++ IMPERSONATE_ANONYMOUS_TOKEN_ID, + SET_INFORMATION_THREAD_ID, + OPEN_THREAD_TOKEN_ID, + OPEN_THREAD_TOKEN_EX_ID, + OPEN_THREAD_ID, + OPEN_PROCESS_ID, + OPEN_PROCESS_TOKEN_ID, + OPEN_PROCESS_TOKEN_EX_ID, + // Filesystem dispatcher: +diff --git a/security/sandbox/chromium/sandbox/win/src/interceptors_64.cc b/security/sandbox/chromium/sandbox/win/src/interceptors_64.cc +--- a/security/sandbox/chromium/sandbox/win/src/interceptors_64.cc ++++ b/security/sandbox/chromium/sandbox/win/src/interceptors_64.cc +@@ -46,16 +46,24 @@ NTSTATUS WINAPI TargetNtUnmapViewOfSecti + reinterpret_cast( + g_originals[UNMAP_VIEW_OF_SECTION_ID]); + return TargetNtUnmapViewOfSection(orig_fn, process, base); + } + + // ----------------------------------------------------------------------- + + NTSTATUS WINAPI ++TargetNtImpersonateAnonymousToken64(HANDLE thread) { ++ NtImpersonateAnonymousTokenFunction orig_fn = ++ reinterpret_cast( ++ g_originals[IMPERSONATE_ANONYMOUS_TOKEN_ID]); ++ return TargetNtImpersonateAnonymousToken(orig_fn, thread); ++} ++ ++NTSTATUS WINAPI + TargetNtSetInformationThread64(HANDLE thread, + NT_THREAD_INFORMATION_CLASS thread_info_class, + PVOID thread_information, + ULONG thread_information_bytes) { + NtSetInformationThreadFunction orig_fn = + reinterpret_cast( + g_originals[SET_INFORMATION_THREAD_ID]); + return TargetNtSetInformationThread(orig_fn, thread, thread_info_class, +diff --git a/security/sandbox/chromium/sandbox/win/src/interceptors_64.h b/security/sandbox/chromium/sandbox/win/src/interceptors_64.h +--- a/security/sandbox/chromium/sandbox/win/src/interceptors_64.h ++++ b/security/sandbox/chromium/sandbox/win/src/interceptors_64.h +@@ -31,16 +31,20 @@ TargetNtMapViewOfSection64(HANDLE sectio + // It should never be called directly. This function provides the means to + // detect dlls being unloaded, so we can clean up our interceptions. + SANDBOX_INTERCEPT NTSTATUS WINAPI TargetNtUnmapViewOfSection64(HANDLE process, + PVOID base); + + // ----------------------------------------------------------------------- + // Interceptors without IPC. + ++// Interception of NtImpersonateAnonymousToken on the child process. ++SANDBOX_INTERCEPT NTSTATUS WINAPI ++TargetNtImpersonateAnonymousToken64(HANDLE thread); ++ + // Interception of NtSetInformationThread on the child process. + SANDBOX_INTERCEPT NTSTATUS WINAPI + TargetNtSetInformationThread64(HANDLE thread, + NT_THREAD_INFORMATION_CLASS thread_info_class, + PVOID thread_information, + ULONG thread_information_bytes); + + // Interception of NtOpenThreadToken on the child process. +diff --git a/security/sandbox/chromium/sandbox/win/src/nt_internals.h b/security/sandbox/chromium/sandbox/win/src/nt_internals.h +--- a/security/sandbox/chromium/sandbox/win/src/nt_internals.h ++++ b/security/sandbox/chromium/sandbox/win/src/nt_internals.h +@@ -299,16 +299,19 @@ typedef enum _NT_THREAD_INFORMATION_CLAS + ThreadIdealProcessor, + ThreadPriorityBoost, + ThreadSetTlsArrayAddress, + ThreadIsIoPending, + ThreadHideFromDebugger + } NT_THREAD_INFORMATION_CLASS, + *PNT_THREAD_INFORMATION_CLASS; + ++typedef NTSTATUS(WINAPI* NtImpersonateAnonymousTokenFunction)( ++ IN HANDLE ThreadHandle); ++ + typedef NTSTATUS(WINAPI* NtSetInformationThreadFunction)( + IN HANDLE ThreadHandle, + IN NT_THREAD_INFORMATION_CLASS ThreadInformationClass, + IN PVOID ThreadInformation, + IN ULONG ThreadInformationLength); + + // Partial definition only: + typedef enum _PROCESSINFOCLASS { +diff --git a/security/sandbox/chromium/sandbox/win/src/policy_broker.cc b/security/sandbox/chromium/sandbox/win/src/policy_broker.cc +--- a/security/sandbox/chromium/sandbox/win/src/policy_broker.cc ++++ b/security/sandbox/chromium/sandbox/win/src/policy_broker.cc +@@ -95,16 +95,18 @@ bool SetupBasicInterceptions(Interceptio + if (!INTERCEPT_NT(manager, NtOpenThread, OPEN_THREAD_ID, 20) || + !INTERCEPT_NT(manager, NtOpenProcess, OPEN_PROCESS_ID, 20) || + !INTERCEPT_NT(manager, NtOpenProcessToken, OPEN_PROCESS_TOKEN_ID, 16)) + return false; + + // Interceptions with neither policy nor IPC. + if (!INTERCEPT_NT(manager, NtSetInformationThread, SET_INFORMATION_THREAD_ID, + 20) || ++ !INTERCEPT_NT(manager, NtImpersonateAnonymousToken, ++ IMPERSONATE_ANONYMOUS_TOKEN_ID, 8) || + !INTERCEPT_NT(manager, NtOpenThreadToken, OPEN_THREAD_TOKEN_ID, 20)) + return false; + + // This one is also provided by process_thread_policy. + if (!INTERCEPT_NT(manager, NtOpenProcessTokenEx, OPEN_PROCESS_TOKEN_EX_ID, + 20)) + return false; + +diff --git a/security/sandbox/chromium/sandbox/win/src/policy_target.cc b/security/sandbox/chromium/sandbox/win/src/policy_target.cc +--- a/security/sandbox/chromium/sandbox/win/src/policy_target.cc ++++ b/security/sandbox/chromium/sandbox/win/src/policy_target.cc +@@ -67,16 +67,30 @@ bool QueryBroker(IpcTag ipc_id, CountedP + processor.Evaluate(kShortEval, params->parameters, params->count); + DCHECK_NT(POLICY_ERROR != result); + + return POLICY_MATCH == result && ASK_BROKER == processor.GetAction(); + } + + // ----------------------------------------------------------------------- + ++// Hooks NtImpersonateAnonymousToken so we can block until call to LowerToken. ++// This means a non-retricted token behaves the same as restricted one before ++// LowerToken and prevents us from being left with an anonymous logon token ++// because we are blocking the RevertToSelf that would undo it. ++NTSTATUS WINAPI TargetNtImpersonateAnonymousToken( ++ NtImpersonateAnonymousTokenFunction orig_ImpersonateAnonymousToken, ++ HANDLE thread) { ++ if (!SandboxFactory::GetTargetServices()->GetState()->RevertedToSelf()) { ++ return STATUS_ACCESS_DENIED; ++ } ++ ++ return orig_ImpersonateAnonymousToken(thread); ++} ++ + // Hooks NtSetInformationThread to block RevertToSelf from being + // called before the actual call to LowerToken. + NTSTATUS WINAPI TargetNtSetInformationThread( + NtSetInformationThreadFunction orig_SetInformationThread, + HANDLE thread, + NT_THREAD_INFORMATION_CLASS thread_info_class, + PVOID thread_information, + ULONG thread_information_bytes) { +diff --git a/security/sandbox/chromium/sandbox/win/src/policy_target.h b/security/sandbox/chromium/sandbox/win/src/policy_target.h +--- a/security/sandbox/chromium/sandbox/win/src/policy_target.h ++++ b/security/sandbox/chromium/sandbox/win/src/policy_target.h +@@ -14,16 +14,22 @@ namespace sandbox { + struct CountedParameterSetBase; + + // Performs a policy lookup and returns true if the request should be passed to + // the broker process. + bool QueryBroker(IpcTag ipc_id, CountedParameterSetBase* params); + + extern "C" { + ++// Interception of NtImpersonateAnonymousToken on the child process. ++// It should never be called directly. ++SANDBOX_INTERCEPT NTSTATUS WINAPI TargetNtImpersonateAnonymousToken( ++ NtImpersonateAnonymousTokenFunction orig_ImpersonateAnonymousToken, ++ HANDLE thread); ++ + // Interception of NtSetInformationThread on the child process. + // It should never be called directly. + SANDBOX_INTERCEPT NTSTATUS WINAPI TargetNtSetInformationThread( + NtSetInformationThreadFunction orig_SetInformationThread, HANDLE thread, + NT_THREAD_INFORMATION_CLASS thread_info_class, PVOID thread_information, + ULONG thread_information_bytes); + + // Interception of NtOpenThreadToken on the child process. diff --git a/security/sandbox/chromium-shim/patches/with_update/patch_order.txt b/security/sandbox/chromium-shim/patches/with_update/patch_order.txt index f5f47138fc95..951b35ba7b97 100755 --- a/security/sandbox/chromium-shim/patches/with_update/patch_order.txt +++ b/security/sandbox/chromium-shim/patches/with_update/patch_order.txt @@ -5,7 +5,6 @@ ifdef_out_FromStringInternal.patch add_option_to_not_use_restricting_sids.patch ifdef_out_SequenceChecker_code.patch allow_read_only_all_paths_rule.patch -revert_TargetNtSetInformationThread_change.patch mingw_copy_s.patch mingw_operator_new.patch mingw_cast_getprocaddress.patch @@ -31,3 +30,4 @@ broker_complex_line_breaks.patch allow_reparse_points.patch derive_sid_from_name.patch add_loongarch_defines.patch +block_NtImpersonateAnonymousToken_before_LowerToken.patch diff --git a/security/sandbox/chromium-shim/patches/with_update/revert_TargetNtSetInformationThread_change.patch b/security/sandbox/chromium-shim/patches/with_update/revert_TargetNtSetInformationThread_change.patch deleted file mode 100644 index 60bb45e3afb3..000000000000 --- a/security/sandbox/chromium-shim/patches/with_update/revert_TargetNtSetInformationThread_change.patch +++ /dev/null @@ -1,39 +0,0 @@ -# HG changeset patch -# User Bob Owen -# Date 1510058662 0 -# Tue Nov 07 12:44:22 2017 +0000 -# Node ID 5b2b8b6c509a1025ef6d6ba208b093d4c4359186 -# Parent 2c3a28eab0bfcaa5a14771454f83703ae938da6c -Revert commit f7540af7428f4b146136ec19b781886693f8c03f changes to policy_target.cc for causing issues with CoInitializeSecurity. r=aklotz - -diff --git a/security/sandbox/chromium/sandbox/win/src/policy_target.cc b/security/sandbox/chromium/sandbox/win/src/policy_target.cc ---- a/security/sandbox/chromium/sandbox/win/src/policy_target.cc -+++ b/security/sandbox/chromium/sandbox/win/src/policy_target.cc -@@ -78,16 +78,26 @@ NTSTATUS WINAPI TargetNtSetInformationTh - NT_THREAD_INFORMATION_CLASS thread_info_class, - PVOID thread_information, - ULONG thread_information_bytes) { - do { - if (SandboxFactory::GetTargetServices()->GetState()->RevertedToSelf()) - break; - if (ThreadImpersonationToken != thread_info_class) - break; -+ if (!thread_information) -+ break; -+ HANDLE token; -+ if (sizeof(token) > thread_information_bytes) -+ break; -+ -+ NTSTATUS ret = CopyData(&token, thread_information, sizeof(token)); -+ if (!NT_SUCCESS(ret) || NULL != token) -+ break; -+ - // This is a revert to self. - return STATUS_SUCCESS; - } while (false); - - return orig_SetInformationThread( - thread, thread_info_class, thread_information, thread_information_bytes); - } - - // Hooks NtOpenThreadToken to force the open_as_self parameter to be set to