Bug 1733532 - Avoid using stack buffers during calls to Thread32Next. r=gstoll

Thread32Next relies on NtMapViewOfSection to map the snapshot that it works with.
We hook NtMapViewOfSection, so calls to Thread32Next reach our patched_NtMapViewOfSection.

With some third-party software, this results in a crash if we use stack buffers (see bug 1733532),
because for some reason the stack cookie check code is not mapped executable. If we can avoid using
stack buffers in that case, then the third-party DLL should get its result from NtMapViewOfSection
without error.

This change thus splits patched_NtMapViewOfSection so that we only use stack buffers when necessary,
i.e. when an executable mapping is asked. Hopefully this can fix bug 1733532.

Differential Revision: https://phabricator.services.mozilla.com/D169450

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<uintptr_t>(&freestanding::patched_NtMapViewOfSection)));
+#  endif  // #if defined(DEBUG) && defined(_M_X64) && !defined(__MINGW64__)
+
   bool ok = freestanding::stub_NtMapViewOfSection.SetDetour(
       aTransferMgr, intcpt, "NtMapViewOfSection",
       &freestanding::patched_NtMapViewOfSection);

browser/app/winlauncher/freestanding/DllBlocklist.cpp

@@ -364,25 +364,15 @@ NTSTATUS NTAPI patched_LdrLoadDll(PWCHAR aDllPath, PULONG aFlags,
 
 CrossProcessDllInterceptor::FuncHookType<NtMapViewOfSectionPtr>
     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

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 <windows.h>
+#  include <winnt.h>
+
+#  include <cstdint>
+
+#  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<DWORD64>(aFunctionAddress), &imageBase, nullptr);
+  if (entry && entry->EndAddress > entry->BeginAddress + 14) {
+    auto begin = reinterpret_cast<uint8_t*>(imageBase + entry->BeginAddress);
+    auto end = reinterpret_cast<uint8_t*>(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

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 += [

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 <stdio.h>
+#include <windows.h>
+
+#include <array>
+#include <utility>
+
+#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<std::pair<uintptr_t, bool>, 3> testCases{
+      std::make_pair<uintptr_t, bool>(
+          reinterpret_cast<uintptr_t>(NoStackCookie), false),
+      std::make_pair<uintptr_t, bool>(
+          reinterpret_cast<uintptr_t>(StackCookieWithSmallStackSpace), true),
+      std::make_pair<uintptr_t, bool>(
+          reinterpret_cast<uintptr_t>(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<void*>(functionAddress), expectStackCookieCheck);
+      return false;
+    }
+    printf(
+        "TEST-PASS | StackCookie | Correct output from HasStackCookieCheck for "
+        "function at %p (expected %d).\n",
+        reinterpret_cast<void*>(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;
+}

mozglue/tests/moz.build

@@ -32,6 +32,7 @@ if CONFIG["OS_ARCH"] == "WINNT":
         [
             "TestNativeNt",
             "TestPEExportSection",
+            "TestStackCookie",
             "TestTimeStampWin",
         ],
         linkage=None,

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]

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;