Bug 1587539 - Skip bounds check when getting IAT if Import Directory is tampered. r=aklotz
authorToshihito Kikuchi <tkikuchi@mozilla.com>
Wed, 06 Nov 2019 21:54:55 +0000
changeset 500981 848a87a2c19b66818aab4936655e59a0997c9863
parent 500980 1e710184eb57d8decb388aee8efdac082d9b48a1
child 500982 eaee8979fdc423b979a8d1dc3915e45eea79a34d
push id99916
push userdluca@mozilla.com
push dateWed, 06 Nov 2019 22:56:04 +0000
treeherderautoland@848a87a2c19b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaklotz
bugs1587539
milestone72.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 1587539 - Skip bounds check when getting IAT if Import Directory is tampered. r=aklotz Some applications tamper Import Directory entry of a loaded executable image to pretend static dependency on their module. We have `RestoreImportDirectory` to revert it in the browser process. If tampering happened in the launcher process, however, we failed to get an IAT thunk address via `GetIATThunksForModule` because it could be located outside the mapped image. With this patch, we skip bounds check in `GetIATThunksForModule` if we detect tampering in the launcher process. We can proceed safely because it's expected that Import Directory is still valid though it's located outside. Differential Revision: https://phabricator.services.mozilla.com/D49690
mozglue/misc/ImportDir.h
mozglue/misc/NativeNt.h
--- a/mozglue/misc/ImportDir.h
+++ b/mozglue/misc/ImportDir.h
@@ -44,19 +44,20 @@ inline LauncherResult<nt::DataDirectoryE
  *                       process for examination.
  * @param aTargetProcess Handle to the child process whose import table we are
  *                       touching.
  * @param aRemoteExeImage HMODULE referencing the child process's executable
  *                        binary that we are touching. This value is used to
  *                        determine the base address of the binary within the
  *                        target process.
  */
-inline LauncherVoidResult RestoreImportDirectory(
-    const wchar_t* aFullImagePath, const nt::PEHeaders& aLocalExeImage,
-    HANDLE aTargetProcess, HMODULE aRemoteExeImage) {
+inline LauncherVoidResult RestoreImportDirectory(const wchar_t* aFullImagePath,
+                                                 nt::PEHeaders& aLocalExeImage,
+                                                 HANDLE aTargetProcess,
+                                                 HMODULE aRemoteExeImage) {
   uint32_t importDirEntryRva;
   PIMAGE_DATA_DIRECTORY importDirEntry =
       aLocalExeImage.GetImageDirectoryEntryPtr(IMAGE_DIRECTORY_ENTRY_IMPORT,
                                                &importDirEntryRva);
   if (!importDirEntry) {
     return LAUNCHER_ERROR_FROM_WIN32(ERROR_BAD_EXE_FORMAT);
   }
 
@@ -76,16 +77,20 @@ inline LauncherVoidResult RestoreImportD
   LauncherResult<nt::DataDirectoryEntry> realImportDirectory =
       detail::GetImageDirectoryViaFileIo(file, importDirEntryRva);
   if (realImportDirectory.isErr()) {
     return LAUNCHER_ERROR_FROM_RESULT(realImportDirectory);
   }
 
   nt::DataDirectoryEntry toWrite = realImportDirectory.unwrap();
 
+  if (toWrite != *importDirEntry) {
+    aLocalExeImage.SetImportDirectoryTampered();
+  }
+
   void* remoteAddress = reinterpret_cast<char*>(
                             nt::PEHeaders::HModuleToBaseAddr(aRemoteExeImage)) +
                         importDirEntryRva;
 
   {  // Scope for prot
     AutoVirtualProtect prot(remoteAddress, sizeof(IMAGE_DATA_DIRECTORY),
                             PAGE_READWRITE, aTargetProcess);
     if (!prot) {
--- a/mozglue/misc/NativeNt.h
+++ b/mozglue/misc/NativeNt.h
@@ -399,17 +399,20 @@ class MOZ_RAII PEHeaders final {
   }
 
   explicit PEHeaders(void* aBaseAddress)
       : PEHeaders(reinterpret_cast<PIMAGE_DOS_HEADER>(aBaseAddress)) {}
 
   explicit PEHeaders(HMODULE aModule) : PEHeaders(HModuleToBaseAddr(aModule)) {}
 
   explicit PEHeaders(PIMAGE_DOS_HEADER aMzHeader)
-      : mMzHeader(aMzHeader), mPeHeader(nullptr), mImageLimit(nullptr) {
+      : mMzHeader(aMzHeader),
+        mPeHeader(nullptr),
+        mImageLimit(nullptr),
+        mIsImportDirectoryTampered(false) {
     if (!mMzHeader || mMzHeader->e_magic != IMAGE_DOS_SIGNATURE) {
       return;
     }
 
     mPeHeader = RVAToPtrUnchecked<PIMAGE_NT_HEADERS>(mMzHeader->e_lfanew);
     if (!mPeHeader || mPeHeader->Signature != IMAGE_NT_SIGNATURE) {
       return;
     }
@@ -466,18 +469,24 @@ class MOZ_RAII PEHeaders final {
     }
 
     auto base = reinterpret_cast<const uint8_t*>(mMzHeader);
     DWORD imageSize = mPeHeader->OptionalHeader.SizeOfImage;
     return Some(MakeSpan(base, imageSize));
   }
 
   PIMAGE_IMPORT_DESCRIPTOR GetImportDirectory() {
-    return GetImageDirectoryEntry<PIMAGE_IMPORT_DESCRIPTOR>(
-        IMAGE_DIRECTORY_ENTRY_IMPORT);
+    // If the import directory is already tampered, we skip bounds check
+    // because it could be located outside the mapped image.
+    return mIsImportDirectoryTampered
+               ? GetImageDirectoryEntry<PIMAGE_IMPORT_DESCRIPTOR,
+                                        BoundsCheckPolicy::Skip>(
+                     IMAGE_DIRECTORY_ENTRY_IMPORT)
+               : GetImageDirectoryEntry<PIMAGE_IMPORT_DESCRIPTOR>(
+                     IMAGE_DIRECTORY_ENTRY_IMPORT);
   }
 
   PIMAGE_RESOURCE_DIRECTORY GetResourceTable() {
     return GetImageDirectoryEntry<PIMAGE_RESOURCE_DIRECTORY>(
         IMAGE_DIRECTORY_ENTRY_RESOURCE);
   }
 
   PIMAGE_DATA_DIRECTORY GetImageDirectoryEntryPtr(
@@ -531,17 +540,19 @@ class MOZ_RAII PEHeaders final {
     aResult = mPeHeader->FileHeader.TimeDateStamp;
     return true;
   }
 
   PIMAGE_IMPORT_DESCRIPTOR
   GetIATForModule(const char* aModuleNameASCII) {
     for (PIMAGE_IMPORT_DESCRIPTOR curImpDesc = GetImportDirectory();
          IsValid(curImpDesc); ++curImpDesc) {
-      auto curName = RVAToPtr<const char*>(curImpDesc->Name);
+      auto curName = mIsImportDirectoryTampered
+                         ? RVAToPtrUnchecked<const char*>(curImpDesc->Name)
+                         : RVAToPtr<const char*>(curImpDesc->Name);
       if (!curName) {
         return nullptr;
       }
 
       if (StricmpASCII(aModuleNameASCII, curName)) {
         continue;
       }
 
@@ -667,25 +678,31 @@ class MOZ_RAII PEHeaders final {
   static bool IsValid(PIMAGE_IMPORT_DESCRIPTOR aImpDesc) {
     return aImpDesc && aImpDesc->OriginalFirstThunk != 0;
   }
 
   static bool IsValid(PIMAGE_THUNK_DATA aImgThunk) {
     return aImgThunk && aImgThunk->u1.Ordinal != 0;
   }
 
+  void SetImportDirectoryTampered() { mIsImportDirectoryTampered = true; }
+
  private:
-  template <typename T>
+  enum class BoundsCheckPolicy { Default, Skip };
+
+  template <typename T, BoundsCheckPolicy Policy = BoundsCheckPolicy::Default>
   T GetImageDirectoryEntry(const uint32_t aDirectoryIndex) {
     PIMAGE_DATA_DIRECTORY dirEntry = GetImageDirectoryEntryPtr(aDirectoryIndex);
     if (!dirEntry) {
       return nullptr;
     }
 
-    return RVAToPtr<T>(dirEntry->VirtualAddress);
+    return Policy == BoundsCheckPolicy::Skip
+               ? RVAToPtrUnchecked<T>(dirEntry->VirtualAddress)
+               : RVAToPtr<T>(dirEntry->VirtualAddress);
   }
 
   // This private variant does not have bounds checks, because we need to be
   // able to resolve the bounds themselves.
   template <typename T, typename R>
   T RVAToPtrUnchecked(R aRva) const {
     return reinterpret_cast<T>(reinterpret_cast<char*>(mMzHeader) + aRva);
   }
@@ -769,16 +786,17 @@ class MOZ_RAII PEHeaders final {
 
     return result;
   }
 
  private:
   PIMAGE_DOS_HEADER mMzHeader;
   PIMAGE_NT_HEADERS mPeHeader;
   void* mImageLimit;
+  bool mIsImportDirectoryTampered;
 };
 
 inline HANDLE RtlGetProcessHeap() {
   PTEB teb = ::NtCurrentTeb();
   PPEB peb = teb->ProcessEnvironmentBlock;
   return peb->Reserved4[1];
 }
 
@@ -814,16 +832,24 @@ inline LauncherResult<DWORD> GetParentPr
 
 struct DataDirectoryEntry : public _IMAGE_DATA_DIRECTORY {
   DataDirectoryEntry() : _IMAGE_DATA_DIRECTORY() {}
 
   MOZ_IMPLICIT DataDirectoryEntry(const _IMAGE_DATA_DIRECTORY& aOther)
       : _IMAGE_DATA_DIRECTORY(aOther) {}
 
   DataDirectoryEntry(const DataDirectoryEntry& aOther) = default;
+
+  bool operator==(const DataDirectoryEntry& aOther) const {
+    return VirtualAddress == aOther.VirtualAddress && Size == aOther.Size;
+  }
+
+  bool operator!=(const DataDirectoryEntry& aOther) const {
+    return !(*this == aOther);
+  }
 };
 
 inline LauncherResult<void*> GetProcessPebPtr(HANDLE aProcess) {
   ULONG returnLength;
   PROCESS_BASIC_INFORMATION pbi;
   NTSTATUS status = ::NtQueryInformationProcess(
       aProcess, ProcessBasicInformation, &pbi, sizeof(pbi), &returnLength);
   if (!NT_SUCCESS(status)) {