From a2f2d585eac97c04ee555cd7020c0d1957925b3d Mon Sep 17 00:00:00 2001 From: Toshihito Kikuchi Date: Wed, 27 May 2020 21:48:35 +0000 Subject: [PATCH] Bug 1509748 - Do not touch ntdll's PE header directly if EAF+ is enabled. r=mhowell,mstange If EAF+ is enabled for firefox.exe, the process does not launch because we parse the PE headers of ntdll.dll at startup, which is prohibited by EAF+. With this patch, we skip two operations when EAF+ is enabled. The first one is to cache ntdll's IAT at startup. Because EAF+ is expected to prevent an injected module from parsing PE headers and modifying IAT, we can skip this caching safely. The second one is to load ntdll's debug information for the profiler. With this patch, the profiler's callstack will not show a raw address instead of a symbol name. It's a bad side effect, but much better than startup crash. Differential Revision: https://phabricator.services.mozilla.com/D76959 --- mozglue/misc/WindowsProcessMitigations.cpp | 44 +++++++++++++++++++ mozglue/misc/WindowsProcessMitigations.h | 1 + toolkit/xre/nsAppRunner.cpp | 8 +++- tools/profiler/core/shared-libraries-win32.cc | 8 +++- 4 files changed, 59 insertions(+), 2 deletions(-) diff --git a/mozglue/misc/WindowsProcessMitigations.cpp b/mozglue/misc/WindowsProcessMitigations.cpp index 2e0be3a360a7..f4150ae98eb5 100644 --- a/mozglue/misc/WindowsProcessMitigations.cpp +++ b/mozglue/misc/WindowsProcessMitigations.cpp @@ -15,6 +15,34 @@ BOOL WINAPI GetProcessMitigationPolicy( SIZE_T dwLength); #endif // (_WIN32_WINNT < 0x0602) +// MinGW does not have these definitions in winnt.h yet +#if defined(__MINGW32__) +constexpr PROCESS_MITIGATION_POLICY ProcessPayloadRestrictionPolicy = + static_cast(12); + +typedef struct _PROCESS_MITIGATION_PAYLOAD_RESTRICTION_POLICY { + union { + DWORD Flags; + struct { + DWORD EnableExportAddressFilter : 1; + DWORD AuditExportAddressFilter : 1; + DWORD EnableExportAddressFilterPlus : 1; + DWORD AuditExportAddressFilterPlus : 1; + DWORD EnableImportAddressFilter : 1; + DWORD AuditImportAddressFilter : 1; + DWORD EnableRopStackPivot : 1; + DWORD AuditRopStackPivot : 1; + DWORD EnableRopCallerCheck : 1; + DWORD AuditRopCallerCheck : 1; + DWORD EnableRopSimExec : 1; + DWORD AuditRopSimExec : 1; + DWORD ReservedFlags : 20; + } DUMMYSTRUCTNAME; + } DUMMYUNIONNAME; +} PROCESS_MITIGATION_PAYLOAD_RESTRICTION_POLICY, + *PPROCESS_MITIGATION_PAYLOAD_RESTRICTION_POLICY; +#endif // defined(__MINGW32__) + namespace mozilla { static decltype(&::GetProcessMitigationPolicy) @@ -58,4 +86,20 @@ MFBT_API bool IsDynamicCodeDisabled() { return polInfo.ProhibitDynamicCode; } +MFBT_API bool IsEafPlusEnabled() { + auto pGetProcessMitigationPolicy = FetchGetProcessMitigationPolicyFunc(); + if (!pGetProcessMitigationPolicy) { + return false; + } + + PROCESS_MITIGATION_PAYLOAD_RESTRICTION_POLICY polInfo; + if (!pGetProcessMitigationPolicy(::GetCurrentProcess(), + ProcessPayloadRestrictionPolicy, &polInfo, + sizeof(polInfo))) { + return false; + } + + return polInfo.EnableExportAddressFilterPlus; +} + } // namespace mozilla diff --git a/mozglue/misc/WindowsProcessMitigations.h b/mozglue/misc/WindowsProcessMitigations.h index b8721080ea4b..31a93f9b698b 100644 --- a/mozglue/misc/WindowsProcessMitigations.h +++ b/mozglue/misc/WindowsProcessMitigations.h @@ -13,6 +13,7 @@ namespace mozilla { MFBT_API bool IsWin32kLockedDown(); MFBT_API bool IsDynamicCodeDisabled(); +MFBT_API bool IsEafPlusEnabled(); } // namespace mozilla diff --git a/toolkit/xre/nsAppRunner.cpp b/toolkit/xre/nsAppRunner.cpp index e71272923e65..322793b11c2f 100644 --- a/toolkit/xre/nsAppRunner.cpp +++ b/toolkit/xre/nsAppRunner.cpp @@ -101,6 +101,7 @@ # include # include "cairo/cairo-features.h" # include "mozilla/WindowsDllBlocklist.h" +# include "mozilla/WindowsProcessMitigations.h" # include "mozilla/WinHeaderOnlyUtils.h" # include "mozilla/mscom/ProcessRuntime.h" # include "mozilla/widget/AudioSession.h" @@ -4764,7 +4765,12 @@ int XREMain::XRE_main(int argc, char* argv[], const BootstrapConfig& aConfig) { // this browser process fails to launch sandbox processes because we cannot // copy a modified IAT into a remote process (See SandboxBroker::LaunchApp). // To prevent that, we cache the intact IAT before COM initialization. - mozilla::ipc::GeckoChildProcessHost::CacheNtDllThunk(); + // If EAF+ is enabled, CacheNtDllThunk() causes a crash, but EAF+ will + // also prevent an injected module from parsing the PE headers and modifying + // the IAT. Therefore, we can skip CacheNtDllThunk(). + if (!mozilla::IsEafPlusEnabled()) { + mozilla::ipc::GeckoChildProcessHost::CacheNtDllThunk(); + } // Some COM settings are global to the process and must be set before any non- // trivial COM is run in the application. Since these settings may affect diff --git a/tools/profiler/core/shared-libraries-win32.cc b/tools/profiler/core/shared-libraries-win32.cc index d2fdefa1b98a..b8c95d2e7b63 100644 --- a/tools/profiler/core/shared-libraries-win32.cc +++ b/tools/profiler/core/shared-libraries-win32.cc @@ -12,6 +12,7 @@ #include "nsWindowsHelpers.h" #include "mozilla/UniquePtr.h" #include "mozilla/Unused.h" +#include "mozilla/WindowsProcessMitigations.h" #include "mozilla/WindowsVersion.h" #include "nsNativeCharsetUtils.h" #include "nsPrintfCString.h" @@ -182,6 +183,11 @@ SharedLibraryInfo SharedLibraryInfo::GetInfoForSelf() { } #endif // !defined(_M_ARM64) + // If EAF+ is enabled, parsing ntdll's PE header via GetPdbInfo() causes + // a crash. We don't include PDB information in SharedLibrary. + bool canGetPdbInfo = (!mozilla::IsEafPlusEnabled() || + !moduleNameStr.LowerCaseEqualsLiteral("ntdll.dll")); + nsCString breakpadId; // Load the module again to make sure that its handle will remain // valid as we attempt to read the PDB information from it. We load the @@ -205,7 +211,7 @@ SharedLibraryInfo SharedLibraryInfo::GetInfoForSelf() { if (handleLock && sizeof(vmemInfo) == VirtualQuery(module.lpBaseOfDll, &vmemInfo, sizeof(vmemInfo)) && - vmemInfo.State == MEM_COMMIT && + vmemInfo.State == MEM_COMMIT && canGetPdbInfo && GetPdbInfo((uintptr_t)module.lpBaseOfDll, pdbSig, pdbAge, &pdbName)) { MOZ_ASSERT(breakpadId.IsEmpty()); breakpadId.AppendPrintf(