Bug 1604008 - Use a target process's export table to cross-process detour. r=aklotz

When `WindowsDllInterceptor` detours a function in a remote process, it calculates
a target address via `GetProcAddress` in the caller's process first, and detours
that address in the target process.  If the caller's export table was modified, the
target address might be invalid in the target process.

With this patch, `WindowsDllInterceptor` uses the target process's export table to
calculate a target function address.

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

--HG--
extra : moz-landing-system : lando

mozglue/misc/NativeNt.h

@@ -16,6 +16,7 @@
 
 #include "mozilla/ArrayUtils.h"
 #include "mozilla/Attributes.h"
+#include "mozilla/BinarySearch.h"
 #include "mozilla/DebugOnly.h"
 #include "mozilla/Maybe.h"
 #include "mozilla/Move.h"
@@ -409,6 +410,17 @@ inline int StricmpASCII(const char* aLeft, const char* aRight) {
   return curLeft - curRight;
 }
 
+inline int StrcmpASCII(const char* aLeft, const char* aRight) {
+  char curLeft, curRight;
+
+  do {
+    curLeft = *(aLeft++);
+    curRight = *(aRight++);
+  } while (curLeft && curLeft == curRight);
+
+  return curLeft - curRight;
+}
+
 class MOZ_RAII PEHeaders final {
   /**
    * This structure is documented on MSDN as VS_VERSIONINFO, but is not present
@@ -508,6 +520,70 @@ class MOZ_RAII PEHeaders final {
     return Some(MakeSpan(base, imageSize));
   }
 
+  PIMAGE_EXPORT_DIRECTORY GetExportDirectory() {
+    return GetImageDirectoryEntry<PIMAGE_EXPORT_DIRECTORY>(
+        IMAGE_DIRECTORY_ENTRY_EXPORT);
+  }
+
+  /**
+   * This functions searches the export table for a given string as
+   * GetProcAddress does.  Instead of a function address, this returns a matched
+   * entry of the Export Address Table i.e. a pointer to an RVA of a matched
+   * function.  If the entry is forwarded, this function returns nullptr.
+   */
+  const DWORD* FindExportAddressTableEntry(const char* aFunctionNameASCII) {
+    struct NameTableComparator {
+      NameTableComparator(PEHeaders& aPEHeader, const char* aTarget)
+          : mPEHeader(aPEHeader), mTarget(aTarget) {}
+
+      int operator()(DWORD aOther) const {
+        return StrcmpASCII(mTarget, mPEHeader.RVAToPtr<const char*>(aOther));
+      }
+
+      PEHeaders& mPEHeader;
+      const char* mTarget;
+    };
+
+    DWORD rvaDirStart, rvaDirEnd;
+    const auto exportDir = GetImageDirectoryEntry<PIMAGE_EXPORT_DIRECTORY>(
+        IMAGE_DIRECTORY_ENTRY_EXPORT, &rvaDirStart, &rvaDirEnd);
+    if (!exportDir) {
+      return nullptr;
+    }
+
+    const auto exportAddressTable =
+        RVAToPtr<const DWORD*>(exportDir->AddressOfFunctions);
+    const auto exportNameTable =
+        RVAToPtr<const DWORD*>(exportDir->AddressOfNames);
+    const auto exportOrdinalTable =
+        RVAToPtr<const WORD*>(exportDir->AddressOfNameOrdinals);
+
+    const NameTableComparator comp(*this, aFunctionNameASCII);
+
+    size_t match;
+    if (!BinarySearchIf(exportNameTable, 0, exportDir->NumberOfNames, comp,
+                        &match)) {
+      return nullptr;
+    }
+
+    WORD index = exportOrdinalTable[match];
+
+    if (index >= exportDir->NumberOfFunctions) {
+      return nullptr;
+    }
+
+    DWORD rvaFunction = exportAddressTable[index];
+    if (rvaFunction >= rvaDirStart && rvaFunction < rvaDirEnd) {
+      // If an entry points to an address within the export section, the
+      // field is a forwarder RVA.  We return nullptr because the entry is
+      // not a function address but a null-terminated string used for export
+      // forwarding.
+      return nullptr;
+    }
+
+    return &exportAddressTable[index];
+  }
+
   PIMAGE_IMPORT_DESCRIPTOR GetImportDirectory() {
     // If the import directory is already tampered, we skip bounds check
     // because it could be located outside the mapped image.
@@ -724,12 +800,28 @@ class MOZ_RAII PEHeaders final {
   enum class BoundsCheckPolicy { Default, Skip };
 
   template <typename T, BoundsCheckPolicy Policy = BoundsCheckPolicy::Default>
-  T GetImageDirectoryEntry(const uint32_t aDirectoryIndex) {
+  T GetImageDirectoryEntry(const uint32_t aDirectoryIndex,
+                           DWORD* aOutRvaStart = nullptr,
+                           DWORD* aOutRvaEnd = nullptr) {
+    if (aOutRvaStart) {
+      *aOutRvaStart = 0;
+    }
+    if (aOutRvaEnd) {
+      *aOutRvaEnd = 0;
+    }
+
     PIMAGE_DATA_DIRECTORY dirEntry = GetImageDirectoryEntryPtr(aDirectoryIndex);
     if (!dirEntry) {
       return nullptr;
     }
 
+    if (aOutRvaStart) {
+      *aOutRvaStart = dirEntry->VirtualAddress;
+    }
+    if (aOutRvaEnd) {
+      *aOutRvaEnd = dirEntry->VirtualAddress + dirEntry->Size;
+    }
+
     return Policy == BoundsCheckPolicy::Skip
                ? RVAToPtrUnchecked<T>(dirEntry->VirtualAddress)
                : RVAToPtr<T>(dirEntry->VirtualAddress);

mozglue/misc/interceptor/MMPolicies.h

@@ -11,6 +11,7 @@
 #include "mozilla/DynamicallyLinkedFunctionPtr.h"
 #include "mozilla/MathAlgorithms.h"
 #include "mozilla/Maybe.h"
+#include "mozilla/NativeNt.h"
 #include "mozilla/Span.h"
 #include "mozilla/TypedEnumBits.h"
 #include "mozilla/Types.h"
@@ -429,6 +430,10 @@ class MOZ_TRIVIAL_CTOR_DTOR MMPolicyInProcess : public MMPolicyBase {
            mbi.State == MEM_COMMIT && mbi.Protect != PAGE_NOACCESS;
   }
 
+  FARPROC GetProcAddress(HMODULE aModule, const char* aName) const {
+    return ::GetProcAddress(aModule, aName);
+  }
+
   bool FlushInstructionCache() const {
     return !!::FlushInstructionCache(::GetCurrentProcess(), nullptr, 0);
   }
@@ -665,6 +670,37 @@ class MMPolicyOutOfProcess : public MMPolicyBase {
            mbi.State == MEM_COMMIT && mbi.Protect != PAGE_NOACCESS;
   }
 
+  /**
+   * This searches the target process's export address table for a given name
+   * instead of simply calling ::GetProcAddress because the local export table
+   * might be modified and the value of a table entry might be different from
+   * the target process.  If we fail to get an entry for some reason, we fall
+   * back to using ::GetProcAddress.
+   */
+  FARPROC GetProcAddress(HMODULE aModule, const char* aName) const {
+    nt::PEHeaders moduleHeaders(aModule);
+    const DWORD* funcEntry = moduleHeaders.FindExportAddressTableEntry(aName);
+    if (!funcEntry) {
+      // FindExportAddressTableEntry returns nullptr if a matched entry is
+      // forwarded to another module.  Because a forwarder entry needs to point
+      // a null-terminated string within the export section, it's less likely to
+      // be modified by a third-party code.  We safely use the local table.
+      return ::GetProcAddress(aModule, aName);
+    }
+
+    SIZE_T numBytes = 0;
+    DWORD rvaTargetFunction = 0;
+    BOOL ok = ::ReadProcessMemory(mProcess, funcEntry, &rvaTargetFunction,
+                                  sizeof(rvaTargetFunction), &numBytes);
+    if (!ok || numBytes != sizeof(rvaTargetFunction)) {
+      // If we fail to read the table entry in the target process for unexpected
+      // reason, we fall back to ::GetProcAddress.
+      return ::GetProcAddress(aModule, aName);
+    }
+
+    return moduleHeaders.RVAToPtr<FARPROC>(rvaTargetFunction);
+  }
+
   bool FlushInstructionCache() const {
     return !!::FlushInstructionCache(mProcess, nullptr, 0);
   }

mozglue/misc/interceptor/PatcherBase.h

@@ -95,6 +95,11 @@ class WindowsDllPatcherBase {
     return ReadOnlyTargetFunction<MMPolicyT>(mVMPolicy, aRedirAddress);
   }
 
+ public:
+  FARPROC GetProcAddress(HMODULE aModule, const char* aName) const {
+    return mVMPolicy.GetProcAddress(aModule, aName);
+  }
+
  protected:
   VMPolicy mVMPolicy;
 };

mozglue/misc/nsWindowsDllInterceptor.h

@@ -373,7 +373,7 @@ class WindowsDllInterceptor final
       return false;
     }
 
-    FARPROC proc = ::GetProcAddress(mModule, aName);
+    FARPROC proc = mDetourPatcher.GetProcAddress(mModule, aName);
     if (!proc) {
       return false;
     }
@@ -404,7 +404,7 @@ class WindowsDllInterceptor final
       return false;
     }
 
-    FARPROC proc = ::GetProcAddress(mModule, aName);
+    FARPROC proc = mDetourPatcher.GetProcAddress(mModule, aName);
     if (!proc) {
       return false;
     }

mozglue/tests/TestNativeNt.cpp

@@ -173,6 +173,40 @@ int main(int argc, char* argv[]) {
     return 1;
   }
 
+  // Use ntdll.dll because it does not have any forwarder RVA.
+  HMODULE ntdllImageBase = ::GetModuleHandleW(L"ntdll.dll");
+  PEHeaders ntdllHeaders(ntdllImageBase);
+
+  auto exportDir = ntdllHeaders.GetExportDirectory();
+  auto tableOfNames = ntdllHeaders.RVAToPtr<PDWORD>(exportDir->AddressOfNames);
+  for (DWORD i = 0; i < exportDir->NumberOfNames; ++i) {
+    const auto name = ntdllHeaders.RVAToPtr<const char*>(tableOfNames[i]);
+    const DWORD* funcEntry = ntdllHeaders.FindExportAddressTableEntry(name);
+    if (ntdllHeaders.RVAToPtr<const void*>(*funcEntry) !=
+        ::GetProcAddress(ntdllImageBase, name)) {
+      printf(
+          "TEST-FAILED | NativeNt | FindExportAddressTableEntry returned "
+          "a wrong value.\n");
+      return 1;
+    }
+  }
+
+  // Test a known forwarder RVA.
+  if (k32headers.FindExportAddressTableEntry("HeapAlloc")) {
+    printf(
+        "TEST-FAILED | NativeNt | kernel32!HeapAlloc should be forwarded to "
+        "ntdll!RtlAllocateHeap.\n");
+    return 1;
+  }
+
+  // Test an invalid name.
+  if (k32headers.FindExportAddressTableEntry("Invalid name")) {
+    printf(
+        "TEST-FAILED | NativeNt | FindExportAddressTableEntry should return "
+        "null for an non-existent name.\n");
+    return 1;
+  }
+
   printf("TEST-PASS | NativeNt | All tests ran successfully\n");
   return 0;
 }