From 4d23f98bbb31d8a33d759d0f2f03c3aff283d561 Mon Sep 17 00:00:00 2001 From: Greg Stoll Date: Fri, 31 Jul 2026 18:10:11 +0000 Subject: [PATCH] Bug 2058208 r=win-reviewers,handyman Differential Revision: https://phabricator.services.mozilla.com/D315348 --- mozglue/misc/NativeNt.h | 101 +++++++++++++++++--- mozglue/tests/TestNativeNt.cpp | 167 +++++++++++++++++++++++++++++++++ 2 files changed, 257 insertions(+), 11 deletions(-) diff --git a/mozglue/misc/NativeNt.h b/mozglue/misc/NativeNt.h index 997eeaa551bd..7bd8de7c57eb 100644 --- a/mozglue/misc/NativeNt.h +++ b/mozglue/misc/NativeNt.h @@ -11,6 +11,7 @@ #include #include +#include #include #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 +constexpr size_t PointeeSize() { + static_assert(std::is_pointer_v, + "RVAToPtr and friends resolve an RVA to a pointer, so T must " + "be a pointer type"); + + using Pointee = std::remove_pointer_t; + if constexpr (std::is_void_v || std::is_function_v) { + 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 + template ()> 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(mMzHeader); char* absAddress = reinterpret_cast(aBase) + aRva; - if (absAddress < reinterpret_cast(mMzHeader) || - absAddress > reinterpret_cast(mImageLimit)) { + if (absAddress < imageBase) { return nullptr; } - return reinterpret_cast(absAddress); + return RVAToPtrChecked(static_cast(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 + T RVAToPtrChecked(R aRva, size_t aSize) const { + if (!mImageLimit || !aSize) { + return nullptr; + } + + uintptr_t base = reinterpret_cast(mMzHeader); + // mImageLimit points at the last valid byte of the mapping. + uintptr_t available = reinterpret_cast(mImageLimit) - base + 1u; + + uintptr_t rva = static_cast(aRva); + if (rva >= available || aSize > available - rva) { + return nullptr; + } + + return reinterpret_cast(base + rva); } Maybe> GetBounds() const { @@ -933,16 +983,45 @@ class MOZ_RAII PEHeaders final { } const CodeViewRecord70* GetPdbInfo() const { - PIMAGE_DEBUG_DIRECTORY debugDirectory = - GetImageDirectoryEntry( - 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( + dirEntry->VirtualAddress, sizeof(IMAGE_DEBUG_DIRECTORY)); if (!debugDirectory) { return nullptr; } - const CodeViewRecord70* debugInfo = - RVAToPtr(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( + 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: diff --git a/mozglue/tests/TestNativeNt.cpp b/mozglue/tests/TestNativeNt.cpp index 56110677c3f5..94edb0b9fcb8 100644 --- a/mozglue/tests/TestNativeNt.cpp +++ b/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(::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(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(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( + 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;