Bug 1599015 - Graciously return a failure if we fail to change the attribute of a trampoline. r=handyman,dmajor
authorToshihito Kikuchi <tkikuchi@mozilla.com>
Thu, 02 Jan 2020 17:08:33 +0000
changeset 508634 a39663ca4c56a717e467bf8b7e22c8b45c32ce06
parent 508633 f79f9ec7908363b967bd63b53209f548470d30ac
child 508635 81a5256f8c72b8e6ccec359f4d72d1a210ed5b26
push id104132
push usercbrindusan@mozilla.com
push dateThu, 02 Jan 2020 17:36:07 +0000
treeherderautoland@a39663ca4c56 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershandyman, dmajor
bugs1599015
milestone73.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1599015 - Graciously return a failure if we fail to change the attribute of a trampoline. r=handyman,dmajor Our detour allocates a trampoline with `PAGE_EXECUTE_READ` first, and then makes it writable before use. If the dynamic code policy is enabled after allocation, we fail to change the attribute, and crash the process because we try to write data into a readonly page. We need to check the validity of a trampoline before writing data. Differential Revision: https://phabricator.services.mozilla.com/D56983
mozglue/misc/interceptor/PatcherDetour.h
mozglue/tests/interceptor/TestDllInterceptor.cpp
mozglue/tests/interceptor/TestDllInterceptor.exe.manifest
--- a/mozglue/misc/interceptor/PatcherDetour.h
+++ b/mozglue/misc/interceptor/PatcherDetour.h
@@ -710,16 +710,19 @@ class WindowsDllDetourPatcher final : pu
   }
 
   void CreateTrampoline(ReadOnlyTargetFunction<MMPolicyT>& origBytes,
                         TrampPoolT* aTrampPool, Trampoline<MMPolicyT>& aTramp,
                         intptr_t aDest, void** aOutTramp) {
     *aOutTramp = nullptr;
 
     Trampoline<MMPolicyT>& tramp = aTramp;
+    if (!tramp) {
+      return;
+    }
 
     // The beginning of the trampoline contains two pointer-width slots:
     // [0]: |this|, so that we know whether the trampoline belongs to us;
     // [1]: Pointer to original function, so that we can reset the hooked
     // function to its original behavior upon destruction.  In rare cases
     // where the function was already a different trampoline, this is
     // just a pointer to that trampoline's target address.
     tramp.WriteEncodedPointer(this);
--- a/mozglue/tests/interceptor/TestDllInterceptor.cpp
+++ b/mozglue/tests/interceptor/TestDllInterceptor.cpp
@@ -7,16 +7,17 @@
 #include <shlobj.h>
 #include <stdio.h>
 #include <commdlg.h>
 #define SECURITY_WIN32
 #include <security.h>
 #include <wininet.h>
 #include <schnlsp.h>
 #include <winternl.h>
+#include <processthreadsapi.h>
 
 #include "AssemblyPayloads.h"
 #include "mozilla/DynamicallyLinkedFunctionPtr.h"
 #include "mozilla/TypeTraits.h"
 #include "mozilla/UniquePtr.h"
 #include "mozilla/WindowsVersion.h"
 #include "nsWindowsDllInterceptor.h"
 #include "nsWindowsHelpers.h"
@@ -49,16 +50,22 @@ NTSTATUS NTAPI NtMapViewOfSection(
 PVOID NTAPI LdrResolveDelayLoadedAPI(PVOID, PVOID, PVOID, PVOID, PVOID, ULONG);
 void CALLBACK ProcessCaretEvents(HWINEVENTHOOK, DWORD, HWND, LONG, LONG, DWORD,
                                  DWORD);
 void __fastcall BaseThreadInitThunk(BOOL aIsInitialThread, void* aStartAddress,
                                     void* aThreadParam);
 
 BOOL WINAPI ApiSetQueryApiSetPresence(PCUNICODE_STRING, PBOOLEAN);
 
+#if (_WIN32_WINNT < 0x0602)
+BOOL WINAPI
+SetProcessMitigationPolicy(PROCESS_MITIGATION_POLICY aMitigationPolicy,
+                           PVOID aBuffer, SIZE_T aBufferLen);
+#endif  // (_WIN32_WINNT < 0x0602)
+
 using namespace mozilla;
 
 struct payload {
   UINT64 a;
   UINT64 b;
   UINT64 c;
 
   bool operator==(const payload& other) const {
@@ -70,21 +77,36 @@ extern "C" __declspec(dllexport) __decls
     rotatePayload(payload p) {
   UINT64 tmp = p.a;
   p.a = p.b;
   p.b = p.c;
   p.c = tmp;
   return p;
 }
 
+// payloadNotHooked is a target function for a test to expect a negative result.
+// We cannot use rotatePayload for that purpose because our detour cannot hook
+// a function detoured already.  Please keep this function always unhooked.
+extern "C" __declspec(dllexport) __declspec(noinline) payload
+    payloadNotHooked(payload p) {
+  // Do something different from rotatePayload to avoid ICF.
+  p.a ^= p.b;
+  p.b ^= p.c;
+  p.c ^= p.a;
+  return p;
+}
+
 static bool patched_func_called = false;
 
 static WindowsDllInterceptor::FuncHookType<decltype(&rotatePayload)>
     orig_rotatePayload;
 
+static WindowsDllInterceptor::FuncHookType<decltype(&payloadNotHooked)>
+    orig_payloadNotHooked;
+
 static payload patched_rotatePayload(payload p) {
   patched_func_called = true;
   return orig_rotatePayload(p);
 }
 
 // Invoke aFunc by taking aArg's contents and using them as aFunc's arguments
 template <typename OrigFuncT, typename... Args,
           typename ArgTuple = Tuple<Args...>, size_t... Indices>
@@ -762,16 +784,66 @@ bool TestAssemblyFunctions() {
     }
 
     printf("TEST-PASS | WindowsDllInterceptor | %s\n", testCase.functionName);
   }
 
   return true;
 }
 
+bool TestDynamicCodePolicy() {
+  if (!IsWin8Point1OrLater()) {
+    // Skip if a platform does not support this policy.
+    return true;
+  }
+
+  PROCESS_MITIGATION_DYNAMIC_CODE_POLICY policy = {};
+  policy.ProhibitDynamicCode = true;
+
+  mozilla::DynamicallyLinkedFunctionPtr<decltype(&SetProcessMitigationPolicy)>
+      pSetProcessMitigationPolicy(L"kernel32.dll",
+                                  "SetProcessMitigationPolicy");
+  if (!pSetProcessMitigationPolicy) {
+    printf(
+        "TEST-UNEXPECTED-FAIL | WindowsDllInterceptor | "
+        "SetProcessMitigationPolicy does not exist.\n");
+    fflush(stdout);
+    return false;
+  }
+
+  if (!pSetProcessMitigationPolicy(ProcessDynamicCodePolicy, &policy,
+                                   sizeof(policy))) {
+    printf(
+        "TEST-UNEXPECTED-FAIL | WindowsDllInterceptor | "
+        "Fail to enable ProcessDynamicCodePolicy.\n");
+    fflush(stdout);
+    return false;
+  }
+
+  WindowsDllInterceptor ExeIntercept;
+  ExeIntercept.Init("TestDllInterceptor.exe");
+
+  // Make sure we fail to hook a function if ProcessDynamicCodePolicy is on
+  // because we cannot create an executable trampoline region.
+  if (orig_payloadNotHooked.Set(ExeIntercept, "payloadNotHooked",
+                                &patched_rotatePayload)) {
+    printf(
+        "TEST-UNEXPECTED-FAIL | WindowsDllInterceptor | "
+        "ProcessDynamicCodePolicy is not working.\n");
+    fflush(stdout);
+    return false;
+  }
+
+  printf(
+      "TEST-PASS | WindowsDllInterceptor | "
+      "Successfully passed TestDynamicCodePolicy.\n");
+  fflush(stdout);
+  return true;
+}
+
 extern "C" int wmain(int argc, wchar_t* argv[]) {
   LARGE_INTEGER start;
   QueryPerformanceCounter(&start);
 
   // We disable this part of the test because the code coverage instrumentation
   // injects code in rotatePayload in a way that WindowsDllInterceptor doesn't
   // understand.
 #ifndef MOZ_CODE_COVERAGE
@@ -949,17 +1021,20 @@ extern "C" int wmain(int argc, wchar_t* 
       TEST_HOOK("wininet.dll", HttpSendRequestExA, Equals, FALSE) &&
       TEST_HOOK("wininet.dll", HttpEndRequestA, Equals, FALSE) &&
       TEST_HOOK("wininet.dll", InternetQueryOptionA, Equals, FALSE) &&
       TEST_HOOK("sspicli.dll", AcquireCredentialsHandleA, NotEquals,
                 SEC_E_OK) &&
       TEST_HOOK_PARAMS("sspicli.dll", QueryCredentialsAttributesA, Equals,
                        SEC_E_INVALID_HANDLE, &credHandle, 0, nullptr) &&
       TEST_HOOK_PARAMS("sspicli.dll", FreeCredentialsHandle, Equals,
-                       SEC_E_INVALID_HANDLE, &credHandle)) {
+                       SEC_E_INVALID_HANDLE, &credHandle) &&
+      // Run TestDynamicCodePolicy() at the end because the policy is
+      // irreversible.
+      TestDynamicCodePolicy()) {
     printf("TEST-PASS | WindowsDllInterceptor | all checks passed\n");
 
     LARGE_INTEGER end, freq;
     QueryPerformanceCounter(&end);
 
     QueryPerformanceFrequency(&freq);
 
     LARGE_INTEGER result;
new file mode 100644
--- /dev/null
+++ b/mozglue/tests/interceptor/TestDllInterceptor.exe.manifest
@@ -0,0 +1,17 @@
+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+<assembly xmlns="urn:schemas-microsoft-com:asm.v1"
+          manifestVersion="1.0"
+          xmlns:asmv3="urn:schemas-microsoft-com:asm.v3">
+  <assemblyIdentity type="win32"
+                    name="TestDllInterceptor"
+                    version="1.0.0.0" />
+  <compatibility xmlns="urn:schemas-microsoft-com:compatibility.v1">
+    <application>
+      <!-- Need this to use functions in WindowsVersion.h -->
+      <supportedOS Id="{8e0f7a12-bfb3-4fe8-b9a5-48fd50a15a9a}"/> <!-- Win10 -->
+      <supportedOS Id="{1f676c76-80e1-4239-95bb-83d0f6d0da78}"/> <!-- Win8.1 -->
+      <supportedOS Id="{4a2f28e3-53b9-4441-ba9c-d69d4a4a6e38}"/> <!-- Win8 -->
+      <supportedOS Id="{35138b9a-5d96-4fbd-8e2d-a2440225f93a}"/> <!-- Win7 -->
+    </application>
+  </compatibility>
+</assembly>