From 0701946b9d4ebf67a0406015551691031a71f6f8 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Tue, 9 Feb 2021 15:55:57 -0800 Subject: [PATCH 1/4] Implement new way of return address hijacking This change implements a new way of return address hijacking for Intel CET enabled Windows devices. It uses a mechanism created by the Windows team for this purpose. The existing mechanism that just patches the return address by an address of our helper routine would result in process killing as it would seem like a ROP exploit. --- src/coreclr/debug/ee/wks/CMakeLists.txt | 2 +- src/coreclr/vm/amd64/AsmHelpers.asm | 9 ++++-- src/coreclr/vm/amd64/cgenamd64.cpp | 4 +++ src/coreclr/vm/amd64/cgencpu.h | 7 +++-- src/coreclr/vm/excep.cpp | 33 +++++++++++++++++++++ src/coreclr/vm/threadsuspend.cpp | 39 +++++++++++++++++++++---- src/coreclr/vm/threadsuspend.h | 11 +++++++ src/coreclr/vm/wks/CMakeLists.txt | 1 + 8 files changed, 95 insertions(+), 11 deletions(-) diff --git a/src/coreclr/debug/ee/wks/CMakeLists.txt b/src/coreclr/debug/ee/wks/CMakeLists.txt index 6411b64d436ff7..42042082bfb63e 100644 --- a/src/coreclr/debug/ee/wks/CMakeLists.txt +++ b/src/coreclr/debug/ee/wks/CMakeLists.txt @@ -18,7 +18,7 @@ if (CLR_CMAKE_TARGET_WIN32) else () - set(ASM_OPTIONS /c /Zi /W3 /errorReport:prompt) + set(ASM_OPTIONS /c /Zi /W3 /errorReport:prompt /guard:ehcont) if (CLR_CMAKE_HOST_ARCH_I386) list (APPEND ASM_OPTIONS /safeseh) diff --git a/src/coreclr/vm/amd64/AsmHelpers.asm b/src/coreclr/vm/amd64/AsmHelpers.asm index 172b800f3feac2..0fd77a277f58b3 100644 --- a/src/coreclr/vm/amd64/AsmHelpers.asm +++ b/src/coreclr/vm/amd64/AsmHelpers.asm @@ -432,13 +432,15 @@ endif ; _DEBUG NESTED_ENTRY OnHijackTripThread, _TEXT ; Don't fiddle with this unless you change HijackFrame::UpdateRegDisplay - ; and HijackObjectArgs + ; and HijackArgs + mov rdx, rsp push rax ; make room for the real return address (Rip) + push rdx PUSH_CALLEE_SAVED_REGISTERS push_vol_reg rax mov rcx, rsp - alloc_stack 30h ; make extra room for xmm0 + alloc_stack 38h ; make extra room for xmm0, argument home slots and align the SP save_xmm128_postrsp xmm0, 20h @@ -448,9 +450,10 @@ NESTED_ENTRY OnHijackTripThread, _TEXT movdqa xmm0, [rsp + 20h] - add rsp, 30h + add rsp, 38h pop rax POP_CALLEE_SAVED_REGISTERS + pop rdx ret ; return to the correct place, adjusted by our caller NESTED_END OnHijackTripThread, _TEXT diff --git a/src/coreclr/vm/amd64/cgenamd64.cpp b/src/coreclr/vm/amd64/cgenamd64.cpp index d00f7b74df0d48..af91cc3a525f37 100644 --- a/src/coreclr/vm/amd64/cgenamd64.cpp +++ b/src/coreclr/vm/amd64/cgenamd64.cpp @@ -286,7 +286,11 @@ void HijackFrame::UpdateRegDisplay(const PREGDISPLAY pRD) pRD->IsCallerSPValid = FALSE; // Don't add usage of this field. This is only temporary. pRD->pCurrentContext->Rip = m_ReturnAddress; +#ifdef TARGET_WINDOWS + pRD->pCurrentContext->Rsp = m_Args->Rsp; +#else pRD->pCurrentContext->Rsp = PTR_TO_MEMBER_TADDR(HijackArgs, m_Args, Rip) + sizeof(void *); +#endif UpdateRegDisplayFromCalleeSavedRegisters(pRD, &(m_Args->Regs)); diff --git a/src/coreclr/vm/amd64/cgencpu.h b/src/coreclr/vm/amd64/cgencpu.h index 6300876fa330e3..93797a4e84910e 100644 --- a/src/coreclr/vm/amd64/cgencpu.h +++ b/src/coreclr/vm/amd64/cgencpu.h @@ -465,7 +465,7 @@ struct HijackArgs ULONG64 Rax; ULONG64 ReturnValue[1]; }; -#else // FEATURE_MULTIREG_RETURN +#else // !FEATURE_MULTIREG_RETURN union { struct @@ -475,8 +475,11 @@ struct HijackArgs }; ULONG64 ReturnValue[2]; }; -#endif // TARGET_UNIX +#endif // !FEATURE_MULTIREG_RETURN CalleeSavedRegisters Regs; +#ifdef TARGET_WINDOWS + ULONG64 Rsp; +#endif union { ULONG64 Rip; diff --git a/src/coreclr/vm/excep.cpp b/src/coreclr/vm/excep.cpp index 7ee6baad9a4a63..eba2d60bff7aec 100644 --- a/src/coreclr/vm/excep.cpp +++ b/src/coreclr/vm/excep.cpp @@ -7186,6 +7186,39 @@ LONG WINAPI CLRVectoredExceptionHandler(PEXCEPTION_POINTERS pExceptionInfo) } } +#ifdef TARGET_AMD64 + +#ifndef STATUS_RETURN_ADDRESS_HIJACK_ATTEMPT +#define STATUS_RETURN_ADDRESS_HIJACK_ATTEMPT ((DWORD)0x80000033L) +#endif + + if (pExceptionInfo->ExceptionRecord->ExceptionCode == STATUS_RETURN_ADDRESS_HIJACK_ATTEMPT) + { + HijackArgs hijackArgs; + hijackArgs.Rax = pExceptionInfo->ContextRecord->Rax; + hijackArgs.Rsp = pExceptionInfo->ContextRecord->Rsp; + if (Thread::AreCetShadowStacksEnabled()) + { + // When the CET is enabled, the return address is still on stack, so we need to set the Rsp as + // if it was popped. + hijackArgs.Rsp += 8; + } + hijackArgs.Rip = 0 ; // The OnHijackWorker sets this + #define CALLEE_SAVED_REGISTER(regname) hijackArgs.Regs.regname = pExceptionInfo->ContextRecord->regname; + ENUM_CALLEE_SAVED_REGISTERS(); + #undef CALLEE_SAVED_REGISTER + + OnHijackWorker(&hijackArgs); + + #define CALLEE_SAVED_REGISTER(regname) pExceptionInfo->ContextRecord->regname = hijackArgs.Regs.regname; + ENUM_CALLEE_SAVED_REGISTERS(); + #undef CALLEE_SAVED_REGISTER + + pExceptionInfo->ContextRecord->Rax = hijackArgs.Rax; + + return EXCEPTION_CONTINUE_EXECUTION; + } +#endif // We need to unhijack the thread here if it is not unhijacked already. On x86 systems, // we do this in Thread::StackWalkFramesEx, but on amd64 systems we have the OS walk the diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index 02e4747fceaf10..fc10e2aeec11fb 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -28,6 +28,10 @@ CLREvent* ThreadSuspend::g_pGCSuspendEvent = NULL; ThreadSuspend::SUSPEND_REASON ThreadSuspend::m_suspendReason; +#if defined(TARGET_WINDOWS) && defined(TARGET_AMD64) +void* ThreadSuspend::g_returnAddressHijackTarget = NULL; +#endif + // If you add any thread redirection function, make sure the debugger can 1) recognize the redirection // function, and 2) retrieve the original CONTEXT. See code:Debugger.InitializeHijackFunctionAddress and // code:DacDbiInterfaceImpl.RetrieveHijackedContext. @@ -4678,7 +4682,17 @@ void Thread::HijackThread(ReturnKind returnKind, ExecutionState *esb) CONTRACTL_END; _ASSERTE(IsValidReturnKind(returnKind)); + VOID *pvHijackAddr = reinterpret_cast(OnHijackTripThread); + +#if defined(TARGET_WINDOWS) && defined(TARGET_AMD64) + void* returnAddressHijackTarget = ThreadSuspend::GetReturnAddressHijackTarget(); + if (returnAddressHijackTarget != NULL) + { + pvHijackAddr = returnAddressHijackTarget; + } +#endif // TARGET_WINDOWS && TARGET_AMD64 + #ifdef TARGET_X86 if (returnKind == RT_Float) { @@ -4978,10 +4992,8 @@ void STDCALL OnHijackWorker(HijackArgs * pArgs) Thread *thread = GetThread(); - thread->ResetThreadState(Thread::TS_Hijacked); - - // Fix up our caller's stack, so it can resume from the hijack correctly pArgs->ReturnAddress = (size_t)thread->m_pvHJRetAddr; + thread->UnhijackThreadNoAlloc(); // Build a frame so that stack crawling can proceed from here back to where // we will resume execution. @@ -5975,9 +5987,26 @@ bool Thread::InjectActivation(ActivationReason reason) // Initialize thread suspension support void ThreadSuspend::Initialize() { -#if defined(FEATURE_HIJACK) && defined(TARGET_UNIX) +#ifdef FEATURE_HIJACK +#if defined(TARGET_UNIX) ::PAL_SetActivationFunction(HandleSuspensionForInterruptedThread, CheckActivationSafePoint); -#endif +#elif defined(TARGET_WINDOWS) && defined(TARGET_AMD64) + // Only versions of Windows that have the special user mode APC have a correct implementation of the return address hijack handling + if (Thread::UseSpecialUserModeApc()) + { + HMODULE hModNtdll = WszLoadLibrary(W("ntdll.dll")); + if (hModNtdll != NULL) + { + typedef ULONG_PTR (NTAPI *PFN_RtlGetReturnAddressHijackTarget)(); + PFN_RtlGetReturnAddressHijackTarget pfnRtlGetReturnAddressHijackTarget = (PFN_RtlGetReturnAddressHijackTarget)GetProcAddress(hModNtdll, "RtlGetReturnAddressHijackTarget"); + if (pfnRtlGetReturnAddressHijackTarget != NULL) + { + g_returnAddressHijackTarget = (void*)pfnRtlGetReturnAddressHijackTarget(); + } + } + } +#endif // TARGET_WINDOWS && TARGET_AMD64 +#endif // FEATURE_HIJACK } #ifdef _DEBUG diff --git a/src/coreclr/vm/threadsuspend.h b/src/coreclr/vm/threadsuspend.h index f36b39f4f7f7a8..9274327682a6b9 100644 --- a/src/coreclr/vm/threadsuspend.h +++ b/src/coreclr/vm/threadsuspend.h @@ -194,6 +194,10 @@ class ThreadSuspend private: static CLREvent * g_pGCSuspendEvent; +#if defined(TARGET_WINDOWS) && defined(TARGET_AMD64) + static void* g_returnAddressHijackTarget; +#endif // TARGET_WINDOWS && TARGET_AMD64 + // This is true iff we're currently in the process of suspending threads. Once the // threads have been suspended, this is false. This is set via an instance of // SuspendRuntimeInProgressHolder placed in SuspendRuntime, SysStartSuspendForDebug, @@ -247,6 +251,13 @@ class ThreadSuspend return g_pSuspensionThread; } +#if defined(TARGET_WINDOWS) && defined(TARGET_AMD64) + static void* GetReturnAddressHijackTarget() + { + return g_returnAddressHijackTarget; + } +#endif // TARGET_WINDOWS && TARGET_AMD64 + private: static LONG m_DebugWillSyncCount; }; diff --git a/src/coreclr/vm/wks/CMakeLists.txt b/src/coreclr/vm/wks/CMakeLists.txt index cb7ba8ca2cc997..43b81d4fd96e6f 100644 --- a/src/coreclr/vm/wks/CMakeLists.txt +++ b/src/coreclr/vm/wks/CMakeLists.txt @@ -32,6 +32,7 @@ if (CLR_CMAKE_TARGET_WIN32) if (CLR_CMAKE_HOST_ARCH_I386) set_source_files_properties(${VM_SOURCES_WKS_ARCH_ASM} PROPERTIES COMPILE_FLAGS "/Zm /safeseh") endif (CLR_CMAKE_HOST_ARCH_I386) + set_source_files_properties(${VM_SOURCES_WKS_ARCH_ASM} PROPERTIES COMPILE_FLAGS "/guard:ehcont") # Convert AsmConstants.h into AsmConstants.inc find_program(POWERSHELL powershell) From 63fb1066778d85de335a1e0edf00a63856f6e73b Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Tue, 20 Jul 2021 10:09:38 +0200 Subject: [PATCH 2/4] Add assembler options for amd64 only --- src/coreclr/debug/ee/wks/CMakeLists.txt | 6 +++++- src/coreclr/vm/wks/CMakeLists.txt | 5 ++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/coreclr/debug/ee/wks/CMakeLists.txt b/src/coreclr/debug/ee/wks/CMakeLists.txt index 42042082bfb63e..01e74ad18444c4 100644 --- a/src/coreclr/debug/ee/wks/CMakeLists.txt +++ b/src/coreclr/debug/ee/wks/CMakeLists.txt @@ -18,12 +18,16 @@ if (CLR_CMAKE_TARGET_WIN32) else () - set(ASM_OPTIONS /c /Zi /W3 /errorReport:prompt /guard:ehcont) + set(ASM_OPTIONS /c /Zi /W3 /errorReport:prompt) if (CLR_CMAKE_HOST_ARCH_I386) list (APPEND ASM_OPTIONS /safeseh) endif (CLR_CMAKE_HOST_ARCH_I386) + if (CLR_CMAKE_HOST_ARCH_AMD64) + list (APPEND ASM_OPTIONS /guard:ehcont) + endif (CLR_CMAKE_HOST_ARCH_AMD64) + set_source_files_properties(${ASM_FILE} PROPERTIES COMPILE_OPTIONS "${ASM_OPTIONS}") add_library_clr(cordbee_wks_obj OBJECT ${CORDBEE_SOURCES_WKS} ${ASM_FILE}) diff --git a/src/coreclr/vm/wks/CMakeLists.txt b/src/coreclr/vm/wks/CMakeLists.txt index 43b81d4fd96e6f..096a0b57d327c1 100644 --- a/src/coreclr/vm/wks/CMakeLists.txt +++ b/src/coreclr/vm/wks/CMakeLists.txt @@ -32,7 +32,10 @@ if (CLR_CMAKE_TARGET_WIN32) if (CLR_CMAKE_HOST_ARCH_I386) set_source_files_properties(${VM_SOURCES_WKS_ARCH_ASM} PROPERTIES COMPILE_FLAGS "/Zm /safeseh") endif (CLR_CMAKE_HOST_ARCH_I386) - set_source_files_properties(${VM_SOURCES_WKS_ARCH_ASM} PROPERTIES COMPILE_FLAGS "/guard:ehcont") + + if (CLR_CMAKE_HOST_ARCH_AMD64) + set_source_files_properties(${VM_SOURCES_WKS_ARCH_ASM} PROPERTIES COMPILE_FLAGS "/guard:ehcont") + endif (CLR_CMAKE_HOST_ARCH_AMD64) # Convert AsmConstants.h into AsmConstants.inc find_program(POWERSHELL powershell) From a203418e447282d5e6bc35a10c5bae5dfe00bfc3 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 22 Jul 2021 18:26:44 +0200 Subject: [PATCH 3/4] Little cleanup around OnHijackWorker for CET --- src/coreclr/vm/excep.cpp | 17 +++++++++++++++-- src/coreclr/vm/threadsuspend.cpp | 2 +- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/coreclr/vm/excep.cpp b/src/coreclr/vm/excep.cpp index eba2d60bff7aec..74a1f9f6be7822 100644 --- a/src/coreclr/vm/excep.cpp +++ b/src/coreclr/vm/excep.cpp @@ -7197,7 +7197,9 @@ LONG WINAPI CLRVectoredExceptionHandler(PEXCEPTION_POINTERS pExceptionInfo) HijackArgs hijackArgs; hijackArgs.Rax = pExceptionInfo->ContextRecord->Rax; hijackArgs.Rsp = pExceptionInfo->ContextRecord->Rsp; - if (Thread::AreCetShadowStacksEnabled()) + + bool areCetShadowStacksEnabled = Thread::AreCetShadowStacksEnabled(); + if (areCetShadowStacksEnabled) { // When the CET is enabled, the return address is still on stack, so we need to set the Rsp as // if it was popped. @@ -7213,9 +7215,20 @@ LONG WINAPI CLRVectoredExceptionHandler(PEXCEPTION_POINTERS pExceptionInfo) #define CALLEE_SAVED_REGISTER(regname) pExceptionInfo->ContextRecord->regname = hijackArgs.Regs.regname; ENUM_CALLEE_SAVED_REGISTERS(); #undef CALLEE_SAVED_REGISTER - pExceptionInfo->ContextRecord->Rax = hijackArgs.Rax; + if (areCetShadowStacksEnabled) + { + // The context refers to the return instruction + // Set the return address on the stack to the original one + *(size_t *)pExceptionInfo->ContextRecord->Rsp = hijackArgs.ReturnAddress; + } + else + { + // The context refers to the location after the return was processed + pExceptionInfo->ContextRecord->Rip = hijackArgs.ReturnAddress; + } + return EXCEPTION_CONTINUE_EXECUTION; } #endif diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index fc10e2aeec11fb..a53f444e5aca5f 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -4992,8 +4992,8 @@ void STDCALL OnHijackWorker(HijackArgs * pArgs) Thread *thread = GetThread(); + thread->ResetThreadState(Thread::TS_Hijacked); pArgs->ReturnAddress = (size_t)thread->m_pvHJRetAddr; - thread->UnhijackThreadNoAlloc(); // Build a frame so that stack crawling can proceed from here back to where // we will resume execution. From 0dde1a8a3a3ff39803049c7abc3f9ccbedfe2476 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Fri, 23 Jul 2021 00:56:16 +0200 Subject: [PATCH 4/4] Remove unnecessary assembler options --- src/coreclr/debug/ee/wks/CMakeLists.txt | 4 ---- src/coreclr/vm/threadsuspend.cpp | 2 ++ src/coreclr/vm/wks/CMakeLists.txt | 4 ---- 3 files changed, 2 insertions(+), 8 deletions(-) diff --git a/src/coreclr/debug/ee/wks/CMakeLists.txt b/src/coreclr/debug/ee/wks/CMakeLists.txt index 01e74ad18444c4..6411b64d436ff7 100644 --- a/src/coreclr/debug/ee/wks/CMakeLists.txt +++ b/src/coreclr/debug/ee/wks/CMakeLists.txt @@ -24,10 +24,6 @@ if (CLR_CMAKE_TARGET_WIN32) list (APPEND ASM_OPTIONS /safeseh) endif (CLR_CMAKE_HOST_ARCH_I386) - if (CLR_CMAKE_HOST_ARCH_AMD64) - list (APPEND ASM_OPTIONS /guard:ehcont) - endif (CLR_CMAKE_HOST_ARCH_AMD64) - set_source_files_properties(${ASM_FILE} PROPERTIES COMPILE_OPTIONS "${ASM_OPTIONS}") add_library_clr(cordbee_wks_obj OBJECT ${CORDBEE_SOURCES_WKS} ${ASM_FILE}) diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index a53f444e5aca5f..8359706d948f70 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -4993,6 +4993,8 @@ void STDCALL OnHijackWorker(HijackArgs * pArgs) Thread *thread = GetThread(); thread->ResetThreadState(Thread::TS_Hijacked); + + // Fix up our caller's stack, so it can resume from the hijack correctly pArgs->ReturnAddress = (size_t)thread->m_pvHJRetAddr; // Build a frame so that stack crawling can proceed from here back to where diff --git a/src/coreclr/vm/wks/CMakeLists.txt b/src/coreclr/vm/wks/CMakeLists.txt index 096a0b57d327c1..cb7ba8ca2cc997 100644 --- a/src/coreclr/vm/wks/CMakeLists.txt +++ b/src/coreclr/vm/wks/CMakeLists.txt @@ -33,10 +33,6 @@ if (CLR_CMAKE_TARGET_WIN32) set_source_files_properties(${VM_SOURCES_WKS_ARCH_ASM} PROPERTIES COMPILE_FLAGS "/Zm /safeseh") endif (CLR_CMAKE_HOST_ARCH_I386) - if (CLR_CMAKE_HOST_ARCH_AMD64) - set_source_files_properties(${VM_SOURCES_WKS_ARCH_ASM} PROPERTIES COMPILE_FLAGS "/guard:ehcont") - endif (CLR_CMAKE_HOST_ARCH_AMD64) - # Convert AsmConstants.h into AsmConstants.inc find_program(POWERSHELL powershell) if (POWERSHELL STREQUAL "POWERSHELL-NOTFOUND")