diff --git a/browser/app/winlauncher/DllBlocklistInit.cpp b/browser/app/winlauncher/DllBlocklistInit.cpp index d05365f9347f..fcb22d47da2f 100644 --- a/browser/app/winlauncher/DllBlocklistInit.cpp +++ b/browser/app/winlauncher/DllBlocklistInit.cpp @@ -16,6 +16,7 @@ #include "mozilla/ScopeExit.h" #include "mozilla/Types.h" #include "mozilla/WindowsDllBlocklist.h" +#include "mozilla/WindowsStackCookie.h" #include "mozilla/WinHeaderOnlyUtils.h" #include "DllBlocklistInit.h" @@ -51,6 +52,13 @@ static LauncherVoidResultWithLineInfo InitializeDllBlocklistOOPInternal( CrossProcessDllInterceptor intcpt(aTransferMgr.RemoteProcess()); intcpt.Init(L"ntdll.dll"); +# if defined(DEBUG) && defined(_M_X64) && !defined(__MINGW64__) + // This debug check preserves compatibility with third-parties (see bug + // 1733532). + MOZ_ASSERT(!HasStackCookieCheck( + reinterpret_cast(&freestanding::patched_NtMapViewOfSection))); +# endif // #if defined(DEBUG) && defined(_M_X64) && !defined(__MINGW64__) + bool ok = freestanding::stub_NtMapViewOfSection.SetDetour( aTransferMgr, intcpt, "NtMapViewOfSection", &freestanding::patched_NtMapViewOfSection); diff --git a/browser/app/winlauncher/freestanding/DllBlocklist.cpp b/browser/app/winlauncher/freestanding/DllBlocklist.cpp index 2141c145fda7..d9e42a308ecc 100644 --- a/browser/app/winlauncher/freestanding/DllBlocklist.cpp +++ b/browser/app/winlauncher/freestanding/DllBlocklist.cpp @@ -364,25 +364,15 @@ NTSTATUS NTAPI patched_LdrLoadDll(PWCHAR aDllPath, PULONG aFlags, CrossProcessDllInterceptor::FuncHookType stub_NtMapViewOfSection; +constexpr DWORD kPageExecutable = PAGE_EXECUTE | PAGE_EXECUTE_READ | + PAGE_EXECUTE_READWRITE | + PAGE_EXECUTE_WRITECOPY; -NTSTATUS NTAPI patched_NtMapViewOfSection( - HANDLE aSection, HANDLE aProcess, PVOID* aBaseAddress, ULONG_PTR aZeroBits, - SIZE_T aCommitSize, PLARGE_INTEGER aSectionOffset, PSIZE_T aViewSize, - SECTION_INHERIT aInheritDisposition, ULONG aAllocationType, - ULONG aProtectionFlags) { - // We always map first, then we check for additional info after. - NTSTATUS stubStatus = stub_NtMapViewOfSection( - aSection, aProcess, aBaseAddress, aZeroBits, aCommitSize, aSectionOffset, - aViewSize, aInheritDisposition, aAllocationType, aProtectionFlags); - if (!NT_SUCCESS(stubStatus)) { - return stubStatus; - } - - if (aProcess != nt::kCurrentProcess) { - // We're only interested in mapping for the current process. - return stubStatus; - } - +// All the code for patched_NtMapViewOfSection that relies on stack buffers +// (e.g. mbi and sectionFileName) should be put in this helper function (see +// bug 1733532). +MOZ_NEVER_INLINE NTSTATUS AfterMapExecutableViewOfSection( + HANDLE aProcess, PVOID* aBaseAddress, NTSTATUS aStubStatus) { // Do a query to see if the memory is MEM_IMAGE. If not, continue MEMORY_BASIC_INFORMATION mbi; NTSTATUS ntStatus = @@ -397,11 +387,8 @@ NTSTATUS NTAPI patched_NtMapViewOfSection( // We check for the AllocationProtect, not the Protect field because // the first section of a mapped image is always PAGE_READONLY even // when it's mapped as an executable. - constexpr DWORD kPageExecutable = PAGE_EXECUTE | PAGE_EXECUTE_READ | - PAGE_EXECUTE_READWRITE | - PAGE_EXECUTE_WRITECOPY; if (!(mbi.Type & MEM_IMAGE) || !(mbi.AllocationProtect & kPageExecutable)) { - return stubStatus; + return aStubStatus; } // Get the section name @@ -443,7 +430,7 @@ NTSTATUS NTAPI patched_NtMapViewOfSection( // use it to bypass CIG. In a sandbox process, this addition fails // because we cannot map the section to a writable region, but it's // ignorable because the paths have been added by the browser process. - Unused << gSharedSection.AddDependentModule(sectionFileName); + Unused << SharedSection::AddDependentModule(sectionFileName); bool attemptToBlockViaRedirect; #if defined(NIGHTLY_BUILD) @@ -511,18 +498,50 @@ NTSTATUS NTAPI patched_NtMapViewOfSection( if (nt::RtlGetProcessHeap()) { ModuleLoadFrame::NotifySectionMap( - nt::AllocatedUnicodeString(sectionFileName), *aBaseAddress, stubStatus, + nt::AllocatedUnicodeString(sectionFileName), *aBaseAddress, aStubStatus, loadStatus, isInjectedDependent); } if (loadStatus == ModuleLoadInfo::Status::Loaded || loadStatus == ModuleLoadInfo::Status::Redirected) { - return stubStatus; + return aStubStatus; } ::NtUnmapViewOfSection(aProcess, *aBaseAddress); return STATUS_ACCESS_DENIED; } +// To preserve compatibility with third-parties, calling into this function +// must not use stack buffers when reached through Thread32Next (see bug +// 1733532). Therefore, all code relying on stack buffers should be put in the +// dedicated helper function AfterMapExecutableViewOfSection. +NTSTATUS NTAPI patched_NtMapViewOfSection( + HANDLE aSection, HANDLE aProcess, PVOID* aBaseAddress, ULONG_PTR aZeroBits, + SIZE_T aCommitSize, PLARGE_INTEGER aSectionOffset, PSIZE_T aViewSize, + SECTION_INHERIT aInheritDisposition, ULONG aAllocationType, + ULONG aProtectionFlags) { + // We always map first, then we check for additional info after. + NTSTATUS stubStatus = stub_NtMapViewOfSection( + aSection, aProcess, aBaseAddress, aZeroBits, aCommitSize, aSectionOffset, + aViewSize, aInheritDisposition, aAllocationType, aProtectionFlags); + if (!NT_SUCCESS(stubStatus)) { + return stubStatus; + } + + if (aProcess != nt::kCurrentProcess) { + // We're only interested in mapping for the current process. + return stubStatus; + } + + if (!(aProtectionFlags & kPageExecutable)) { + // Bail out early if an executable mapping was not asked. In particular, + // we will not use stack buffers during calls to Thread32Next, which can + // result in crashes with third-party software (see bug 1733532). + return stubStatus; + } + + return AfterMapExecutableViewOfSection(aProcess, aBaseAddress, stubStatus); +} + } // namespace freestanding } // namespace mozilla diff --git a/mozglue/misc/WindowsStackCookie.h b/mozglue/misc/WindowsStackCookie.h new file mode 100644 index 000000000000..c4196b385385 --- /dev/null +++ b/mozglue/misc/WindowsStackCookie.h @@ -0,0 +1,63 @@ +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ +/* vim: set ts=8 sts=2 et sw=2 tw=80: */ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#ifndef mozilla_WindowsStackCookie_h +#define mozilla_WindowsStackCookie_h + +#if defined(DEBUG) && defined(_M_X64) && !defined(__MINGW64__) + +# include +# include + +# include + +# include "mozilla/Types.h" + +namespace mozilla { + +// This function does pattern matching on the instructions generated for a +// given function, to detect whether it uses stack buffers. More specifically, +// it looks for instructions that characterize the presence of stack cookie +// checks. When this function returns true, it can be a false positive, but we +// use a rather long pattern to make false positives very unlikely. +// Note: Do not use this function inside the function that lives at +// aFunctionAddress, as that could introduce stack buffers. +// Note: The pattern we use does not work for MinGW builds. +inline bool HasStackCookieCheck(uintptr_t aFunctionAddress) { + DWORD64 imageBase{}; + auto entry = ::RtlLookupFunctionEntry( + reinterpret_cast(aFunctionAddress), &imageBase, nullptr); + if (entry && entry->EndAddress > entry->BeginAddress + 14) { + auto begin = reinterpret_cast(imageBase + entry->BeginAddress); + auto end = reinterpret_cast(imageBase + entry->EndAddress - 14); + for (auto pc = begin; pc != end; ++pc) { + // 48 8b 05 XX XX XX XX: mov rax, qword ptr [rip + XXXXXXXX] + if ((pc[0] == 0x48 && pc[1] == 0x8b && pc[2] == 0x05) && + // 48 31 e0: xor rax, rsp + (pc[7] == 0x48 && pc[8] == 0x31 && pc[9] == 0xe0) && + // 48 89 (8|4)4 24 ...: mov qword ptr [rsp + ...], rax + (pc[10] == 0x48 && pc[11] == 0x89 && + (pc[12] == 0x44 || pc[12] == 0x84) && pc[13] == 0x24)) { + return true; + } + } + } + // In x64, if there is no entry, then there is no stack allocation, hence + // there is no stack cookie check: "Table-based exception handling requires a + // table entry for all functions that allocate stack space or call another + // function (for example, nonleaf functions)." + // https://learn.microsoft.com/en-us/cpp/build/exception-handling-x64 + // Similarly, if the gap between begin and end is less than 14 bytes, then + // the function cannot contain the pattern we are looking for, therefore it + // has no cookie check either. + return false; +} + +} // namespace mozilla + +#endif // defined(DEBUG) && defined(_M_X64) && !defined(__MINGW64__) + +#endif // mozilla_WindowsStackCookie_h diff --git a/mozglue/misc/moz.build b/mozglue/misc/moz.build index ce83ea8a593d..7b79612e549d 100644 --- a/mozglue/misc/moz.build +++ b/mozglue/misc/moz.build @@ -81,6 +81,7 @@ if CONFIG["OS_ARCH"] == "WINNT": "WindowsEnumProcessModules.h", "WindowsMapRemoteView.h", "WindowsProcessMitigations.h", + "WindowsStackCookie.h", "WindowsUnwindInfo.h", ] EXPORTS.mozilla.glue += [ diff --git a/mozglue/tests/TestStackCookie.cpp b/mozglue/tests/TestStackCookie.cpp new file mode 100644 index 000000000000..6970fbc240ef --- /dev/null +++ b/mozglue/tests/TestStackCookie.cpp @@ -0,0 +1,89 @@ +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ +/* vim: set ts=8 sts=2 et sw=2 tw=80: */ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include +#include + +#include +#include + +#include "mozilla/WindowsStackCookie.h" + +#if defined(DEBUG) && defined(_M_X64) && !defined(__MINGW64__) + +uint64_t NoStackCookie(const uint64_t* aArray, size_t aSize) { + uint64_t result = 0; + for (size_t i = 0; i < aSize; ++i) { + result += aArray[i]; + } + result /= aSize; + return result; +} + +// We expect the following instructions to be generated: +// +// 48 8b 05 XX XX XX XX mov rax,qword ptr [__security_cookie] +// 48 31 e0 xor rax,rsp 48 +// 89 44 24 38 mov qword ptr [rsp+38h],rax +uint64_t StackCookieWithSmallStackSpace(const uint64_t* aArray, size_t aSize) { + uint64_t array[0x2]{}; + for (size_t i = 0; i < aSize; ++i) { + array[aArray[i]] += aArray[i]; + } + return array[0] + array[1]; +} + +// We expect the following instructions to be generated: +// +// 48 8b 05 XX XX XX XX mov rax,qword ptr [__security_cookie] +// 48 31 e0 xor rax,rsp +// 48 89 84 24 28 40 00 00 mov qword ptr [rsp+4028h],rax +uint64_t StackCookieWithLargeStackSpace(const uint64_t* aArray, size_t aSize) { + uint64_t array[0x800]{}; + for (size_t i = 0; i < aSize; ++i) { + array[aArray[i]] += aArray[i]; + } + return array[0] + array[0x7FF]; +} + +bool TestStackCookieCheck() { + std::array, 3> testCases{ + std::make_pair( + reinterpret_cast(NoStackCookie), false), + std::make_pair( + reinterpret_cast(StackCookieWithSmallStackSpace), true), + std::make_pair( + reinterpret_cast(StackCookieWithLargeStackSpace), true), + }; + for (auto [functionAddress, expectStackCookieCheck] : testCases) { + if (mozilla::HasStackCookieCheck(functionAddress) != + expectStackCookieCheck) { + printf( + "TEST-FAILED | StackCookie | Wrong output from HasStackCookieCheck " + "for function at %p (expected %d).\n", + reinterpret_cast(functionAddress), expectStackCookieCheck); + return false; + } + printf( + "TEST-PASS | StackCookie | Correct output from HasStackCookieCheck for " + "function at %p (expected %d).\n", + reinterpret_cast(functionAddress), expectStackCookieCheck); + } + return true; +} + +#endif // defined(DEBUG) && defined(_M_X64) && !defined(__MINGW64__) + +int wmain(int argc, wchar_t* argv[]) { +#if defined(DEBUG) && defined(_M_X64) && !defined(__MINGW64__) + if (!TestStackCookieCheck()) { + return 1; + } +#endif // defined(DEBUG) && defined(_M_X64) && !defined(__MINGW64__) + + printf("TEST-PASS | StackCookie | All tests ran successfully\n"); + return 0; +} diff --git a/mozglue/tests/moz.build b/mozglue/tests/moz.build index 15aa2581f271..ac68508ebd48 100644 --- a/mozglue/tests/moz.build +++ b/mozglue/tests/moz.build @@ -32,6 +32,7 @@ if CONFIG["OS_ARCH"] == "WINNT": [ "TestNativeNt", "TestPEExportSection", + "TestStackCookie", "TestTimeStampWin", ], linkage=None, diff --git a/testing/cppunittest.ini b/testing/cppunittest.ini index 9a1191595177..864ec02d0596 100644 --- a/testing/cppunittest.ini +++ b/testing/cppunittest.ini @@ -80,6 +80,8 @@ skip-if = asan || tsan # Not built on sanitizer builds [TestSaturate] [TestSplayTree] [TestSPSCQueue] +[TestStackCookie] +skip-if = os != 'win' [TestTemplateLib] [TestTextUtils] [TestThreadSafeWeakPtr] diff --git a/toolkit/xre/dllservices/tests/gtest/TestDLLBlocklist.cpp b/toolkit/xre/dllservices/tests/gtest/TestDLLBlocklist.cpp index b815fbe6decb..fdfdecc44227 100644 --- a/toolkit/xre/dllservices/tests/gtest/TestDLLBlocklist.cpp +++ b/toolkit/xre/dllservices/tests/gtest/TestDLLBlocklist.cpp @@ -14,6 +14,7 @@ #include "mozilla/ArrayUtils.h" #include "mozilla/Char16.h" #include "mozilla/gtest/MozAssertions.h" +#include "mozilla/WindowsStackCookie.h" #include "nsDirectoryServiceDefs.h" #include "nsDirectoryServiceUtils.h" #include "nsString.h" @@ -113,8 +114,10 @@ TEST(TestDllBlocklist, UtilityProcessOnly_AllowInMainProcess) EXPECT_TRUE(!!::GetModuleHandleW(kLeafName.get())); } -// RedirectToNoOpEntryPoint needs the launcher process. #if defined(MOZ_LAUNCHER_PROCESS) +// RedirectToNoOpEntryPoint needs the launcher process. +// This test will fail in debug x64 if we mistakenly reintroduce stack buffers +// in patched_NtMapViewOfSection (see bug 1733532). TEST(TestDllBlocklist, NoOpEntryPoint) { // DllMain of this dll has MOZ_RELEASE_ASSERT. This test makes sure we load @@ -136,7 +139,9 @@ TEST(TestDllBlocklist, NoOpEntryPoint) # endif } -// User blocklist needs the launcher process +// User blocklist needs the launcher process. +// This test will fail in debug x64 if we mistakenly reintroduce stack buffers +// in patched_NtMapViewOfSection (see bug 1733532). TEST(TestDllBlocklist, UserBlocked) { constexpr auto kLeafName = u"TestDllBlocklist_UserBlocked.dll"_ns;