From de7e14368fa8aa6881dc8dfbc41c50ce44596dbf Mon Sep 17 00:00:00 2001 From: Yan Date: Sun, 9 Aug 2026 23:03:11 +0000 Subject: [PATCH 1/2] Stop indexing PE and ELF header tables past their declared length Loading aborted with IndexError on two header-controlled numbers that were used as list indices without being checked against the list. PE: NumberOfRvaAndSizes states how many data directories an image has, and fewer than 16 is legal - EFI stub images commonly declare 6 - so pefile parses a short DATA_DIRECTORY. The IAT (12) and .NET descriptor (14) lookups indexed it with fixed constants. An index past the end means the image does not have that directory, which is what a zero VirtualAddress already means, so treat it the same way. Both lookups now go through _meta_dd, the one place that decides whether a directory is present, so is_dotnet no longer keeps its own copy of that decision; it now also wants a nonzero Size, the way every other directory does. ELF: st_shndx values from SHN_LORESERVE up are reserved tags, not section header table indices. pyelftools decodes only SHN_UNDEF, SHN_ABS and SHN_COMMON into strings and passes the rest through as ints, so a symbol in the processor-specific SHN_X86_64_LCOMMON indexed the section list. Such a symbol names no section, so it gets neither a section nor a remap offset. It stays an export: SHN_X86_64_LCOMMON is what a large common symbol gets instead of SHN_COMMON, and cle already exports those. Both cases need an input no binary in angr/binaries had, so both fixtures are new there: efi_short_data_directory.efi, a UEFI application whose optional header declares six directories, and large_common.o, built by gcc -mcmodel=medium. Each reproduces its IndexError on the unfixed loader. Requires the angr/binaries branch feature/short-data-directory-and-lcommon. Co-Authored-By: Claude Opus 5 --- cle/backends/elf/symbol.py | 18 +++++++++++---- cle/backends/pe/pe.py | 17 +++++++------- tests/test_elf_symbols.py | 45 ++++++++++++++++++++++++++++++++++++++ tests/test_pe.py | 14 ++++++++++++ 4 files changed, 82 insertions(+), 12 deletions(-) create mode 100644 tests/test_elf_symbols.py diff --git a/cle/backends/elf/symbol.py b/cle/backends/elf/symbol.py index bd0788d24..4976518ac 100644 --- a/cle/backends/elf/symbol.py +++ b/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: diff --git a/cle/backends/pe/pe.py b/cle/backends/pe/pe.py index c6fab9f5d..5b6b8e48c 100644 --- a/cle/backends/pe/pe.py +++ b/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 = {} @@ -512,9 +507,15 @@ def _meta_pe_context(self) -> tuple[pefile.PE, int, bool, int]: 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.""" + """Return the named data directory entry if the image has one with a nonzero VirtualAddress and Size.""" 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 diff --git a/tests/test_elf_symbols.py b/tests/test_elf_symbols.py new file mode 100644 index 000000000..0ec30703a --- /dev/null +++ b/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() diff --git a/tests/test_pe.py b/tests/test_pe.py index 7b32cabb9..d228d33cd 100644 --- a/tests/test_pe.py +++ b/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" From 43c897a6c363151643ab7808fe9c275c404397db Mon Sep 17 00:00:00 2001 From: Yan Date: Sat, 29 Aug 2026 19:13:23 +0000 Subject: [PATCH 2/2] Let _meta_dd return the data directory entry's own type The Typecheck job scores each changed file against master's copy of it: badness is (10*errors + warnings)/lines and must not increase. Its environment installs types-pefile, whose stub types OPTIONAL_HEADER.DATA_DIRECTORY as a list of entries carrying VirtualAddress and Size. _meta_dd declared pefile.Structure, the base class that has neither, which threw that away for every caller: 39 of pe.py's 52 errors are a caller reading VirtualAddress or Size off the result. That is what makes this branch red. It adds no diagnostic of its own - it scores the same 52 errors as master - but its pe.py is 26 lines shorter than master's, and the same error count over a smaller line count is a regression by that measure. Leaving the return type to inference gives the entry type where the stub is installed and an unknown one where it is not, so the callers type-check and nothing about them changes at run time. pe.py goes from 52 errors to 13. Co-Authored-By: Claude Opus 5 (1M context) --- cle/backends/pe/pe.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/cle/backends/pe/pe.py b/cle/backends/pe/pe.py index 5b6b8e48c..95fe43a81 100644 --- a/cle/backends/pe/pe.py +++ b/cle/backends/pe/pe.py @@ -506,8 +506,14 @@ 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 the named data directory entry if the image has one with a nonzero VirtualAddress and Size.""" + 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] data_directory = self._pe.OPTIONAL_HEADER.DATA_DIRECTORY # NumberOfRvaAndSizes may be smaller than the 16 directories the format defines - EFI stub images