Bug 2058208 r=win-reviewers,handyman

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

mozglue/misc/NativeNt.h

@@ -11,6 +11,7 @@
 #include <winternl.h>
 
 #include <algorithm>
+#include <type_traits>
 #include <utility>
 
 #include "mozilla/Attributes.h"
@@ -540,6 +541,27 @@ struct CodeViewRecord70 {
   char pdbFileName[1];
 };
 
+// The size of the object that T points at, used to bounds-check a pointer
+// against the mapped image.
+//
+// void and function pointees have no meaningful compile-time extent (the
+// former is opaque and the latter is machine code) so they fall back to a
+// one-byte "this address is inside the image" check, which is all the caller
+// can meaningfully assert.
+template <typename T>
+constexpr size_t PointeeSize() {
+  static_assert(std::is_pointer_v<T>,
+                "RVAToPtr and friends resolve an RVA to a pointer, so T must "
+                "be a pointer type");
+
+  using Pointee = std::remove_pointer_t<T>;
+  if constexpr (std::is_void_v<Pointee> || std::is_function_v<Pointee>) {
+    return 1;
+  } else {
+    return sizeof(Pointee);
+  }
+}
+
 class MOZ_RAII PEHeaders final {
   /**
    * This structure is documented on MSDN as VS_VERSIONINFO, but is not present
@@ -620,23 +642,51 @@ class MOZ_RAII PEHeaders final {
   }
 
   /**
-   * This overload computes a result by adding aRva to aBase, but also ensures
+   * This overload computes a result by adding aRva to aBase and ensures
    * that the resulting pointer falls within the bounds of this binary's memory
    * mapping.
    */
-  template <typename T, typename R>
+  template <typename T, typename R, size_t Size = PointeeSize<T>()>
   T RVAToPtr(void* aBase, R aRva) const {
     if (!mImageLimit) {
       return nullptr;
     }
 
+    // aBase is not always the image base (some callers resolve offsets that
+    // are relative to a resource directory or to the optional header), so
+    // convert back to an image-relative RVA before bounds checking.
+    char* imageBase = reinterpret_cast<char*>(mMzHeader);
     char* absAddress = reinterpret_cast<char*>(aBase) + aRva;
-    if (absAddress < reinterpret_cast<char*>(mMzHeader) ||
-        absAddress > reinterpret_cast<char*>(mImageLimit)) {
+    if (absAddress < imageBase) {
       return nullptr;
     }
 
-    return reinterpret_cast<T>(absAddress);
+    return RVAToPtrChecked<T>(static_cast<uintptr_t>(absAddress - imageBase),
+                              Size);
+  }
+
+  /**
+   * Like RVAToPtr, but ensures that the whole range [aRva, aRva + aSize) --
+   * not just its first byte -- falls within the bounds of this binary's memory
+   * mapping. Use this whenever the caller is going to dereference more than a
+   * single byte at the resulting address.
+   */
+  template <typename T, typename R>
+  T RVAToPtrChecked(R aRva, size_t aSize) const {
+    if (!mImageLimit || !aSize) {
+      return nullptr;
+    }
+
+    uintptr_t base = reinterpret_cast<uintptr_t>(mMzHeader);
+    // mImageLimit points at the last valid byte of the mapping.
+    uintptr_t available = reinterpret_cast<uintptr_t>(mImageLimit) - base + 1u;
+
+    uintptr_t rva = static_cast<uintptr_t>(aRva);
+    if (rva >= available || aSize > available - rva) {
+      return nullptr;
+    }
+
+    return reinterpret_cast<T>(base + rva);
   }
 
   Maybe<Range<const uint8_t>> GetBounds() const {
@@ -933,16 +983,45 @@ class MOZ_RAII PEHeaders final {
   }
 
   const CodeViewRecord70* GetPdbInfo() const {
-    PIMAGE_DEBUG_DIRECTORY debugDirectory =
-        GetImageDirectoryEntry<PIMAGE_DEBUG_DIRECTORY>(
-            IMAGE_DIRECTORY_ENTRY_DEBUG);
+    PIMAGE_DATA_DIRECTORY dirEntry =
+        GetImageDirectoryEntryPtr(IMAGE_DIRECTORY_ENTRY_DEBUG);
+    if (!dirEntry || dirEntry->Size < sizeof(IMAGE_DEBUG_DIRECTORY)) {
+      return nullptr;
+    }
+
+    auto debugDirectory = RVAToPtrChecked<PIMAGE_DEBUG_DIRECTORY>(
+        dirEntry->VirtualAddress, sizeof(IMAGE_DEBUG_DIRECTORY));
     if (!debugDirectory) {
       return nullptr;
     }
 
-    const CodeViewRecord70* debugInfo =
-        RVAToPtr<CodeViewRecord70*>(debugDirectory->AddressOfRawData);
-    return (debugInfo && debugInfo->signature == 'SDSR') ? debugInfo : nullptr;
+    // The record needs to be big enough for the fixed-size fields plus at
+    // least the null terminator of pdbFileName.
+    constexpr size_t kMinRecordSize =
+        offsetof(CodeViewRecord70, pdbFileName) + 1;
+    if (debugDirectory->SizeOfData < kMinRecordSize) {
+      return nullptr;
+    }
+
+    auto debugInfo = RVAToPtrChecked<const CodeViewRecord70*>(
+        debugDirectory->AddressOfRawData, debugDirectory->SizeOfData);
+    if (!debugInfo || debugInfo->signature != 'SDSR') {
+      return nullptr;
+    }
+
+    // Callers treat pdbFileName as a C string, so it must be terminated inside
+    // the record.
+    const size_t nameCapacity =
+        debugDirectory->SizeOfData - offsetof(CodeViewRecord70, pdbFileName);
+    for (size_t i = 0; i < nameCapacity; ++i) {
+      if (!debugInfo->pdbFileName[i]) {
+        // terminated, so succeed
+        return debugInfo;
+      }
+    }
+
+    // not terminated, so fail
+    return nullptr;
   }
 
  private:

mozglue/tests/TestNativeNt.cpp

@@ -400,6 +400,169 @@ bool TestCheckStack() {
 }
 #endif  // _M_X64
 
+// A synthetic PE image, used to feed PEHeaders::GetPdbInfo() malformed debug
+// directories. The page following the image is reserved but never committed,
+// so that a read past the end of the image faults rather than silently
+// succeeding.
+class FakePEImage final {
+ public:
+  FakePEImage() {
+    SYSTEM_INFO sysInfo;
+    ::GetSystemInfo(&sysInfo);
+    mImageSize = sysInfo.dwPageSize * 2;
+    mBase = reinterpret_cast<uint8_t*>(::VirtualAlloc(
+        nullptr, mImageSize + sysInfo.dwPageSize, MEM_RESERVE, PAGE_NOACCESS));
+    if (mBase &&
+        !::VirtualAlloc(mBase, mImageSize, MEM_COMMIT, PAGE_READWRITE)) {
+      ::VirtualFree(mBase, 0, MEM_RELEASE);
+      mBase = nullptr;
+    }
+  }
+
+  ~FakePEImage() {
+    if (mBase) {
+      ::VirtualFree(mBase, 0, MEM_RELEASE);
+    }
+  }
+
+  FakePEImage(const FakePEImage&) = delete;
+  FakePEImage& operator=(const FakePEImage&) = delete;
+
+  explicit operator bool() const { return !!mBase; }
+  DWORD ImageSize() const { return mImageSize; }
+  uint8_t* At(DWORD aRva) const { return mBase + aRva; }
+  PIMAGE_DOS_HEADER DosHeader() const {
+    return reinterpret_cast<PIMAGE_DOS_HEADER>(mBase);
+  }
+
+  // Zeroes the image and writes headers whose debug data directory points at
+  // aDebugDirRva with the given size.
+  void Reset(DWORD aDebugDirRva, DWORD aDebugDirSize) {
+    constexpr DWORD kNtHeadersRva = 0x40;
+    memset(mBase, 0, mImageSize);
+
+    DosHeader()->e_magic = IMAGE_DOS_SIGNATURE;
+    DosHeader()->e_lfanew = kNtHeadersRva;
+
+    auto ntHeaders = reinterpret_cast<PIMAGE_NT_HEADERS>(At(kNtHeadersRva));
+    ntHeaders->Signature = IMAGE_NT_SIGNATURE;
+    ntHeaders->FileHeader.SizeOfOptionalHeader = sizeof(IMAGE_OPTIONAL_HEADER);
+    ntHeaders->OptionalHeader.Magic = IMAGE_NT_OPTIONAL_HDR_MAGIC;
+    ntHeaders->OptionalHeader.SizeOfImage = mImageSize;
+    ntHeaders->OptionalHeader.NumberOfRvaAndSizes =
+        IMAGE_NUMBEROF_DIRECTORY_ENTRIES;
+
+    auto& dataDir =
+        ntHeaders->OptionalHeader.DataDirectory[IMAGE_DIRECTORY_ENTRY_DEBUG];
+    dataDir.VirtualAddress = aDebugDirRva;
+    dataDir.Size = aDebugDirSize;
+  }
+
+ private:
+  uint8_t* mBase = nullptr;
+  DWORD mImageSize = 0;
+};
+
+bool TestPdbInfoBounds() {
+  FakePEImage image;
+  if (!image) {
+    printf("TEST-FAIL | NativeNt | Failed to create a fake PE image\n");
+    return false;
+  }
+
+  const DWORD imageSize = image.ImageSize();
+  constexpr DWORD kDebugDirRva = 0x400;
+  constexpr DWORD kRecordRva = 0x500;
+  constexpr DWORD kNameOffset = offsetof(CodeViewRecord70, pdbFileName);
+  const char kPdbName[] = "test.pdb";
+
+  struct TestCase {
+    const char* mDescription;
+    DWORD mDebugDirRva;
+    DWORD mDebugDirSize;
+    DWORD mRawDataRva;
+    DWORD mRawDataSize;
+    bool mTerminateName;
+    bool mExpectSuccess;
+  };
+
+  const TestCase kTestCases[] = {
+      {"well-formed record", kDebugDirRva, sizeof(IMAGE_DEBUG_DIRECTORY),
+       kRecordRva, kNameOffset + sizeof(kPdbName), true, true},
+      {"record extending past the end of the image", kDebugDirRva,
+       sizeof(IMAGE_DEBUG_DIRECTORY), imageSize - 8, 64, true, false},
+      {"record too small to hold a file name", kDebugDirRva,
+       sizeof(IMAGE_DEBUG_DIRECTORY), kRecordRva, kNameOffset, true, false},
+      // A size below the fixed-size fields would underflow the remaining
+      // capacity of pdbFileName if it were not rejected up front.
+      {"record smaller than the fixed-size fields", kDebugDirRva,
+       sizeof(IMAGE_DEBUG_DIRECTORY), kRecordRva, kNameOffset / 2, true, false},
+      {"unterminated file name", kDebugDirRva, sizeof(IMAGE_DEBUG_DIRECTORY),
+       kRecordRva, kNameOffset + 8, false, false},
+      {"debug directory extending past the end of the image", imageSize - 4,
+       sizeof(IMAGE_DEBUG_DIRECTORY), kRecordRva,
+       kNameOffset + sizeof(kPdbName), true, false},
+      {"debug directory smaller than one entry", kDebugDirRva,
+       sizeof(IMAGE_DEBUG_DIRECTORY) - 1, kRecordRva,
+       kNameOffset + sizeof(kPdbName), true, false},
+  };
+
+  for (const auto& testCase : kTestCases) {
+    image.Reset(testCase.mDebugDirRva, testCase.mDebugDirSize);
+
+    if (testCase.mDebugDirRva + sizeof(IMAGE_DEBUG_DIRECTORY) <= imageSize) {
+      auto debugDir = reinterpret_cast<PIMAGE_DEBUG_DIRECTORY>(
+          image.At(testCase.mDebugDirRva));
+      debugDir->Type = IMAGE_DEBUG_TYPE_CODEVIEW;
+      debugDir->AddressOfRawData = testCase.mRawDataRva;
+      debugDir->SizeOfData = testCase.mRawDataSize;
+    }
+
+    // Write as much of the record as fits in the image, so that a bounds check
+    // that only looks at the record's first bytes would still be fooled.
+    if (testCase.mRawDataRva < imageSize) {
+      DWORD writable =
+          std::min(testCase.mRawDataSize, imageSize - testCase.mRawDataRva);
+      uint8_t* record = image.At(testCase.mRawDataRva);
+      memset(record, 'A', writable);
+      if (writable >= sizeof(uint32_t)) {
+        const uint32_t signature = 'SDSR';
+        memcpy(record, &signature, sizeof(signature));
+      }
+      if (testCase.mTerminateName && writable > kNameOffset) {
+        DWORD nameSize =
+            std::min(DWORD(sizeof(kPdbName)), DWORD(writable - kNameOffset));
+        memcpy(record + kNameOffset, kPdbName, nameSize);
+        record[kNameOffset + nameSize - 1] = '\0';
+      }
+    }
+
+    PEHeaders headers(image.DosHeader());
+    if (!headers) {
+      printf("TEST-FAIL | NativeNt | Failed to parse the fake PE image (%s)\n",
+             testCase.mDescription);
+      return false;
+    }
+
+    const CodeViewRecord70* pdbInfo = headers.GetPdbInfo();
+    if (!pdbInfo != !testCase.mExpectSuccess) {
+      printf("TEST-FAIL | NativeNt | GetPdbInfo() unexpectedly %s for %s\n",
+             pdbInfo ? "succeeded" : "failed", testCase.mDescription);
+      return false;
+    }
+
+    if (pdbInfo && strcmp(pdbInfo->pdbFileName, kPdbName)) {
+      printf(
+          "TEST-FAIL | NativeNt | GetPdbInfo() returned the wrong name for "
+          "%s: %s\n",
+          testCase.mDescription, pdbInfo->pdbFileName);
+      return false;
+    }
+  }
+
+  return true;
+}
+
 int wmain(int argc, wchar_t* argv[]) {
   UNICODE_STRING normal;
   ::RtlInitUnicodeString(&normal, kNormal);
@@ -636,6 +799,10 @@ int wmain(int argc, wchar_t* argv[]) {
     return 1;
   }
 
+  if (!TestPdbInfoBounds()) {
+    return 1;
+  }
+
 #if defined(_M_X64)
   if (!TestCheckStack()) {
     return 1;