COFF: bound the offsets and counts a COFF object declares

cle/backends/coff.py

@@ -11,6 +11,7 @@
 
 import archinfo
 
+from cle.errors import CLEInvalidBinaryError
 from cle.utils import extract_null_terminated_bytestr
 
 from .backend import Backend, register_backend
@@ -173,6 +174,14 @@ def __init__(self, data: bytes):
         self.data: bytes = data
         self._parse()
 
+    def _require_in_file(self, offset: int, size: int, what: str) -> None:
+        # A table with no entries is never read, so nothing dereferences its pointer.
+        if size and offset + size > len(self.data):
+            raise CLEInvalidBinaryError(
+                f"The {what} needs {size:#x} bytes at {offset:#x}, "
+                f"which a {len(self.data):#x} byte file does not hold"
+            )
+
     def _parse(self) -> None:
         self.header = CoffFileHeader.from_buffer_copy(self.data)
         if self.header.Machine not in {
@@ -181,8 +190,20 @@ def _parse(self) -> None:
         }:
             raise NotImplementedError("Unsupported machine type")
 
-        strings_offset = (
-            self.header.PointerToSymbolTable + ctypes.sizeof(CoffSymbolTableEntry) * self.header.NumberOfSymbols
+        self._require_in_file(
+            ctypes.sizeof(self.header),
+            ctypes.sizeof(CoffSectionTableEntry) * self.header.NumberOfSections,
+            f"section table of {self.header.NumberOfSections} entries",
+        )
+
+        symbols_size = ctypes.sizeof(CoffSymbolTableEntry) * self.header.NumberOfSymbols
+        strings_offset = self.header.PointerToSymbolTable + symbols_size
+        # The string table begins on the byte after the last symbol and opens with its own size,
+        # so bounding that size field bounds every symbol table read as well.
+        self._require_in_file(
+            self.header.PointerToSymbolTable,
+            symbols_size + 4,
+            f"symbol table of {self.header.NumberOfSymbols} entries and the string table size after it",
         )
         strings_size = struct.unpack("<I", self.data[strings_offset : strings_offset + 4])[0]
         self.strings: bytes = self.data[strings_offset : strings_offset + strings_size]
@@ -224,6 +245,11 @@ def _parse(self) -> None:
             # Relocations
             relocs = []
             offset = section.PointerToRelocations
+            self._require_in_file(
+                offset,
+                ctypes.sizeof(CoffRelocationTableEntry) * section.NumberOfRelocations,
+                f"relocation table of section {i} with {section.NumberOfRelocations} entries",
+            )
             for i in range(section.NumberOfRelocations):
                 reloc = CoffRelocationTableEntry.from_buffer_copy(self.data, offset)
                 relocs.append(reloc)
@@ -413,7 +439,7 @@ def value(self):
         return offset_to_symbol
 
 
-RELOC_CLASSES: dict[IntEnum, dict[IntEnum, type[Relocation]]] = {
+RELOC_CLASSES: dict[IntEnum, dict[IntEnum, type[CoffRelocation]]] = {
     IMAGE_FILE_MACHINE.I386: {
         IMAGE_REL_I386.REL32: CoffRelocationREL32,
         IMAGE_REL_I386.DIR32: CoffRelocationDIR32,
@@ -514,6 +540,21 @@ def _add_relocs(self) -> None:
                 }:
                     reloc_class = RELOC_CLASSES[self._coff.header.Machine].get(reloc.Type, None)
                     if reloc_class is not None:
+                        patch_size = struct.calcsize(reloc_class.PACK_FORMAT)
+                        section_size = self._coff.sections[section_idx].SizeOfRawData
+                        image_size = len(self._image_vmem)
+                        if reloc.VirtualAddress + patch_size > section_size or patch_offset + patch_size > image_size:
+                            log.warning(
+                                "Section %s has a relocation of type %#x at %#x patching %#x bytes, which is out of "
+                                "bounds for its SizeOfRawData %#x or for the image size %#x. Skipping this relocation.",
+                                self._coff.get_section_name(section_idx),
+                                reloc.Type,
+                                reloc.VirtualAddress,
+                                patch_size,
+                                section_size,
+                                image_size,
+                            )
+                            continue
                         cle_symbol = self.get_symbol(sym_name, produce_extern_symbols=True)
                         self.relocs.append(reloc_class(self, cle_symbol, patch_offset))
                         continue

tests/test_coff.py

@@ -37,6 +37,35 @@ def test_x86_64(self):
         assert "rejected" in symbol_names
         assert "authenticate" in symbol_names
 
+    def test_a_section_table_longer_than_the_file_is_rejected(self):
+        # The first 512 bytes of x86/fauxware.obj. Its header declares 29 sections, whose table
+        # needs 0x49c bytes counted from the start of the file.
+        exe = os.path.join(TEST_BASE, "tests", "x86", "coff_truncated_section_table.obj")
+        with self.assertRaisesRegex(cle.CLEInvalidBinaryError, "section table"):
+            cle.Loader(exe, auto_load_libs=False)
+
+    def test_a_symbol_table_past_the_end_of_the_file_is_rejected(self):
+        # The first 2048 bytes of x86/fauxware.obj, which is long enough to hold the whole
+        # section table and not the 152 symbols at 0x31c1 or the string table after them.
+        exe = os.path.join(TEST_BASE, "tests", "x86", "coff_truncated_symbol_table.obj")
+        with self.assertRaisesRegex(cle.CLEInvalidBinaryError, "symbol table"):
+            cle.Loader(exe, auto_load_libs=False)
+
+    def test_a_relocation_table_past_the_end_of_the_file_is_rejected(self):
+        # An otherwise well-formed 108-byte object whose .text points its relocation table at
+        # 0x4000000, so everything else the parser reads is in range.
+        exe = os.path.join(TEST_BASE, "tests", "x86", "coff_reloc_table_past_file.obj")
+        with self.assertRaisesRegex(cle.CLEInvalidBinaryError, "relocation table"):
+            cle.Loader(exe, auto_load_libs=False)
+
+    def test_the_whole_object_still_loads(self):
+        # The bounds above are on what the header declares, so an object that declares only what
+        # it holds is unaffected.
+        exe = os.path.join(TEST_BASE, "tests", "x86", "fauxware.obj")
+        ld = cle.Loader(exe, auto_load_libs=False)
+        assert len(ld.main_object.sections) == 29
+        assert len(ld.main_object.relocs) == 225
+
     def test_long_section_names_come_from_the_string_table(self):
         exe = os.path.join(TEST_BASE, "tests", "x86", "coff_long_section_names.obj")
         ld = cle.Loader(exe, auto_load_libs=False)
@@ -59,6 +88,25 @@ def test_dir32_relocation_wraps_at_the_field_width(self):
         field_addr = section_vaddr(ld.main_object, ".text")
         assert ld.memory.load(field_addr, 4) == struct.pack("<I", (target_symbol.rebased_addr + addend) % 2**32)
 
+    def test_a_relocation_past_the_end_of_its_section_is_skipped(self):
+        # The object's one relocation sits at offset 0x10 of a .text section holding 0x10 bytes, so
+        # its four-byte field falls on the start of .data, which is filled with 0xaa.
+        exe = os.path.join(TEST_BASE, "tests", "x86", "coff_reloc_outside_section.obj")
+        ld = cle.Loader(exe, auto_load_libs=False, perform_relocations=True)
+        assert ld.memory.load(section_vaddr(ld.main_object, ".data"), 0x10) == b"\xaa" * 0x10
+        assert ld.main_object.relocs == []
+
+    def test_a_relocation_past_the_end_of_the_file_is_skipped(self):
+        # The object's one relocation sits at offset 0x4000000 of .text, which the whole file does
+        # not reach.
+        exe = os.path.join(TEST_BASE, "tests", "x86", "coff_reloc_outside_file.obj")
+        with open(exe, "rb") as f:
+            raw = f.read()
+
+        ld = cle.Loader(exe, auto_load_libs=False, perform_relocations=True)
+        assert ld.main_object.relocs == []
+        assert ld.main_object.memory.load(0, len(raw)) == raw
+
     def test_rel32_relocation_encodes_a_negative_displacement(self):
         # The object holds a backwards call: _callee at offset 0, and a displacement field at
         # offset 5 storing the addend -4. Both live in .text, so the result does not depend on