Stop indexing PE and ELF header tables past their declared length

cle/backends/elf/symbol.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+from elftools.elf.constants import SHN_INDICES
 from elftools.elf.enums import ENUM_ST_INFO_TYPE
 
 from cle.address_translator import AT
@@ -31,9 +32,15 @@ def __init__(self, owner, symb):
 
         sec_ndx, value = symb.entry.st_shndx, symb.entry.st_value
 
+        # pyelftools decodes SHN_UNDEF, SHN_ABS and SHN_COMMON to strings and passes every other st_shndx
+        # through as an int. Values from SHN_LORESERVE up are reserved tags, such as the processor-specific
+        # SHN_X86_64_LCOMMON, and do not index the section header table.
+        reserved_ndx = isinstance(sec_ndx, int) and sec_ndx >= SHN_INDICES.SHN_LORESERVE
+        section = None if reserved_ndx or isinstance(sec_ndx, str) else sec_ndx
+
         # A relocatable object's symbol's value is relative to its section's addr.
-        if owner.is_relocatable and isinstance(sec_ndx, int):
-            value += owner.sections[sec_ndx].remap_offset
+        if owner.is_relocatable and section is not None:
+            value += owner.sections[section].remap_offset
 
         super().__init__(
             owner, maybedecode(symb.name), AT.from_lva(value, owner).to_rva(), symb.entry.st_size, self.type
@@ -42,14 +49,17 @@ def __init__(self, owner, symb):
         self.version = None
         self.binding = symb.entry.st_info.bind
         self.is_hidden = symb.entry["st_other"]["visibility"] == "STV_HIDDEN"
-        self.section = sec_ndx if not isinstance(sec_ndx, str) else None
+        self.section = section
         self.is_static = self._type == SymbolType.TYPE_SECTION or sec_ndx == "SHN_ABS"
         self.is_common = sec_ndx == "SHN_COMMON"
         self.is_weak = self.binding == "STB_WEAK"
         self.is_local = self.binding == "STB_LOCAL"
 
         self.is_import = sec_ndx == "SHN_UNDEF" and self.binding in ("STB_GLOBAL", "STB_WEAK")
-        self.is_export = (self.section is not None or self.is_common) and self.binding in ("STB_GLOBAL", "STB_WEAK")
+        # A reserved st_shndx names no section, but the symbol is still defined here, the way a SHN_COMMON
+        # one is: SHN_X86_64_LCOMMON is what a large common symbol gets instead of SHN_COMMON.
+        defined_here = self.section is not None or self.is_common or reserved_ndx
+        self.is_export = defined_here and self.binding in ("STB_GLOBAL", "STB_WEAK")
 
     @property
     def subtype(self) -> ELFSymbolType:

cle/backends/pe/pe.py

@@ -195,12 +195,7 @@ def __init__(
         # Go binaries keep a full function table even when stripped
         self.gopclntab = register_gopclntab_symbols(self)
 
-        self.is_dotnet = (
-            self._pe.OPTIONAL_HEADER.DATA_DIRECTORY[
-                pefile.DIRECTORY_ENTRY["IMAGE_DIRECTORY_ENTRY_COM_DESCRIPTOR"]
-            ].VirtualAddress
-            != 0
-        )
+        self.is_dotnet = self._meta_dd("IMAGE_DIRECTORY_ENTRY_COM_DESCRIPTOR") is not None
 
     _pefile_cache = {}
 
@@ -511,10 +506,22 @@ def _meta_pe_context(self) -> tuple[pefile.PE, int, bool, int]:
         ptr_size = 8 if is_64 else 4
         return pe, base, is_64, ptr_size
 
-    def _meta_dd(self, name: str) -> pefile.Structure | None:
-        """Return a data directory entry if it has a nonzero VirtualAddress and Size, else None."""
+    def _meta_dd(self, name: str):
+        """
+        Return the named data directory entry if the image has one with a nonzero VirtualAddress and Size.
+
+        The return type is inferred rather than declared: pefile fills a data directory entry in from the format
+        string it parsed, so the pefile.Structure this used to promise declares neither VirtualAddress nor Size and
+        hides both from every caller.
+        """
         idx = pefile.DIRECTORY_ENTRY[name]
-        dd = self._pe.OPTIONAL_HEADER.DATA_DIRECTORY[idx]
+        data_directory = self._pe.OPTIONAL_HEADER.DATA_DIRECTORY
+        # NumberOfRvaAndSizes may be smaller than the 16 directories the format defines - EFI stub images
+        # commonly declare 6 - so pefile parses a short DATA_DIRECTORY. A directory past the end is absent,
+        # which means the same thing a zero VirtualAddress means.
+        if idx >= len(data_directory):
+            return None
+        dd = data_directory[idx]
         if dd.VirtualAddress and dd.Size:
             return dd
         return None

tests/test_elf_symbols.py

@@ -0,0 +1,45 @@
+#!/usr/bin/env python
+from __future__ import annotations
+
+import os
+
+import cle
+from cle.backends.elf.symbol import ELFSymbol
+
+TESTS_BASE = os.path.join(os.path.dirname(os.path.realpath(__file__)), "..", "..", "binaries", "tests")
+
+
+def test_large_common_symbol():
+    """
+    An st_shndx from SHN_LORESERVE up is a tag, not a section header table index.
+
+    large_common.o is built with -mcmodel=medium, which puts a common symbol too large for the small
+    code model at SHN_X86_64_LCOMMON (0xFF02) instead of SHN_COMMON. Subscripting the section list
+    with that used to raise IndexError before the object finished loading.
+    """
+    ld = cle.Loader(os.path.join(TESTS_BASE, "x86_64", "large_common.o"), auto_load_libs=False)
+    assert isinstance(ld.main_object, cle.ELF)
+    assert ld.main_object.is_relocatable
+
+    symbol = ld.main_object.get_symbol("big_buffer")
+    assert isinstance(symbol, ELFSymbol)
+    # The tag names no section, so the symbol gets none and no section remap offset either.
+    assert symbol.section is None
+    assert not symbol.is_common
+    # It still defines big_buffer for anything linking against this object.
+    assert symbol.is_export
+
+
+def test_common_symbol():
+    """An ordinary SHN_COMMON symbol keeps naming no section, and stays an export."""
+    ld = cle.Loader(os.path.join(TESTS_BASE, "x86_64", "decompiler", "gzip.o"), auto_load_libs=False)
+    symbol = ld.main_object.get_symbol("ofname")
+    assert isinstance(symbol, ELFSymbol)
+    assert symbol.is_common
+    assert symbol.section is None
+    assert symbol.is_export
+
+
+if __name__ == "__main__":
+    test_large_common_symbol()
+    test_common_symbol()

tests/test_pe.py

@@ -300,6 +300,20 @@ def test_load_binary_larger_than_highest_address(self):
         assert ld.main_object.max_addr == 0x44F02D
         assert ld.all_objects[1].min_addr == 0x500000
 
+    def test_short_data_directory(self):
+        # NumberOfRvaAndSizes may be smaller than the 16 directories the format defines, and the
+        # trailing directories are then simply absent. EFI stub images commonly declare 6, which is
+        # what this one does; indexing the IAT (12) or the .NET descriptor (14) has to cope.
+        efi = os.path.join(TEST_BASE, "tests", "x86_64", "efi_short_data_directory.efi")
+        ld = cle.Loader(efi, auto_load_libs=False)
+
+        assert isinstance(ld.main_object, cle.PE)
+        assert [sec.name for sec in ld.main_object.sections] == [".text", ".data"]
+        assert ld.main_object.entry == 0x1000
+        assert not ld.main_object.deps
+        # The .NET descriptor is directory 14, past the end of this image's directories.
+        assert not ld.main_object.is_dotnet
+
     def test_loading_incomplete_pe_file(self):
         exe = os.path.join(
             TEST_BASE, "tests", "i386", "windows", "a94bbeed0ef51db3d3964bb0cc2cbed0adab0e47997d88f34daa92faa1a91e8a"