COFF: bound the offsets and counts a COFF object declares - #807
Open
zardus wants to merge 2 commits into
Open
Conversation
Member
Author
|
THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS Validation record for head This replaces the record for
Known and not addressed here: two indexes taken from the file are still unguarded, and both behave identically on the two revisions. CI prediction: every cle job should be green while the |
Member
Author
|
THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS Full output of Before — the out-of-section relocation silently rewrites the first three bytes of angr/cle masterAfter — the two relocations are skipped with a warning and the three malformed objects are with this change |
Member
|
Corpus decompilation diffs can be found at angr/dec-snapshots@master...angr/cle_807 |
Coff._add_relocs took the patch address as section.PointerToRawData plus reloc.VirtualAddress and registered a relocation there without checking it. Neither bound was tested, and the two fail differently. A field past the end of the file crashes. The backend maps the object as one backer covering the file, so CoffRelocationDIR32.value asks Clemory for four bytes at an address nothing maps, and cle.Loader(..., perform_relocations=True) raises KeyError out of Clemory.load. A field merely past the end of its own section does not crash, and that is the worse half. Every offset in the file is mapped, so the store lands wherever the arithmetic points -- another section's raw data, the relocation table, the symbol table -- and the load returns normally with those bytes rewritten. Check both bounds where the relocation is registered rather than in relocate(). A relocation that cannot be applied should not reach self.relocs at all: it is handed to the symbol resolver, it can produce an extern symbol for a field that will never be written, and it is visible to every consumer that iterates an object's relocations. It is also where the PE backend drops a section whose raw data the file does not hold. The field's width comes from struct.calcsize on the relocation class's PACK_FORMAT -- four bytes normally, eight for ADDR64, two for SECTION -- so a four-byte field starting on the last byte of a section is out of bounds, which a bound on the start offset alone would miss. PACK_FORMAT is declared on CoffRelocation rather than on Relocation, so RELOC_CLASSES is annotated with the class it actually holds. This leaves the section mapping loop alone. Bounding a section's raw data by the size of the file is #806; the two compose, because _add_relocs walks self._coff.sections itself and would still register the relocations of a section that loop has skipped. Two details keep this bound correct against the other open COFF branches, and change nothing on this one. The section comes out of self._coff.sections by index rather than off the loop variable. Both name the same object here, by the definition of enumerate. #764 rewrites this loop to walk indices and drops the variable, and the two branches merge with no textual conflict, so with both applied and the loop variable read _add_relocs raises NameError on the first relocation of a supported type. Of the five COFF objects angr/binaries tracks that this backend loads, four carry such a relocation and stop loading; the fifth has none. #804 is stacked on #764 and carries the same rewrite. The file-size half of the bound is taken against self._image_vmem, the bytes the backend maps, rather than against self._data. Here the two are the same object: _image_vmem is assigned from _data in __init__, never rebound, and cle defines no subclass of Coff. #804 places a section whose file offset does not satisfy its alignment past the end of the file and extends the image to cover it, so a relocation into a moved section is past len(self._data) and inside the image, and bounding on the file would skip it. With both applied and the file used, x86/fauxware.obj keeps 177 of its 225 relocations and x86_64/fauxware.obj 66 of 126, and the test below asserting 225 fails.
CoffParser._parse reads three tables at offsets and counts that come out of the file, and bounds none of them. A truncated or malformed object therefore leaves cle.Loader by an exception a caller cannot name: struct.error from the string table size read, and ValueError from ctypes for the section and relocation tables. Neither is a CLEError, so nothing catching CLEError catches them. Bound all three against the size of the file and raise CLEInvalidBinaryError naming the field, the bytes it wanted and the size of the file. The message follows the register of the PE backend's out-of-bounds section warning. A table with no entries is exempt, because nothing dereferences its pointer: a section declaring zero relocations and a PointerToRelocations past the end of the file loads on master and has to keep loading. The bounds change no acceptance decision. What changes is the exception: a rejection that was struct.error or ValueError is now CLEInvalidBinaryError. One bound covers two of the reads. The string table begins on the byte after the last symbol, so a file that holds the four-byte string table size also holds every symbol. Loading all 16677 truncations of x86/fauxware.obj, from zero bytes to the whole file: on master 15457 of them -- every length from 20 to 15476 -- fail at the string table size read and not one reaches the symbol table read. 15477 is the first length that holds that size field, and the 1200 lengths from there up load. A separate bound on the symbol table would be unreachable. Those same 1200 lengths still load with these bounds applied. The 15457 that failed now fail as CLEInvalidBinaryError, 1160 of them on the section table and 14297 on the symbol and string table. Malformed here is fatal rather than skippable, because the parser has no partial product to hand back. _add_relocs indexes self._coff.symbols by an index taken from the file, so a short symbol list turns a truncated object into an IndexError somewhere with no information about why. That index is itself unbounded on both revisions and stays that way here; bounding it is a separate change. The constructor already treats an unusable COFF as fatal for an unsupported machine type and for a /GL object, so this replaces an accidental exception with a named one rather than adding a new failure. Not bounded: CoffFileHeader.from_buffer_copy on the first line of _parse, which still raises ValueError for 18 of the 20 truncations shorter than the twenty-byte header. The other two never reach the COFF backend at all. That read takes no field from the file. Whether a two-byte file should reach the parser is Coff.is_compatible's question, since it claims a file on its first two bytes, and it is a separate change.
zardus
force-pushed
the
feature/coff-reloc-bounds
branch
from
September 6, 2026 05:27
d1943dc to
ac80e76
Compare
zardus
added a commit
to angr/binaries
that referenced
this pull request
Sep 6, 2026
cle's tests on master now load tests/aarch64/langdetect_go.macho and tests/aarch64/relocatable_object.macho, which #193 and #224 added after this branch was cut. angr/cle#807 names this pull request in its sync: line, so CI checks this branch out instead of master and those two files were missing: 3 failed, 264 passed on the macOS job. Merging master in supplies them and leaves this branch's own five objects and build script untouched.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS
Problem
The COFF backend reads and writes at addresses it computes from fields in the object's own
header, and checks none of them against the file. Three failures follow.
A relocation whose field lies past the end of its own section is applied anyway. On
tests/x86/coff_reloc_outside_section.obj, a relocation belonging to a.textof0x10byteshas its four-byte field at offset
0x10, which is the first byte of.data. The load returnsnormally,
.datacomes back rewritten withaa aa aabecome16 ab ea, and nothing isreported.
A relocation whose field lies past the end of the file crashes. On
tests/x86/coff_reloc_outside_file.objthe field is at0x4000000in a 108-byte object, andcle.Loader(path, auto_load_libs=False, perform_relocations=True)raises:And
CoffParser._parsereads the section table, the symbol and string table, and eachsection's relocation table at offsets and counts the file supplies, bounding none of them.
Each of those three reads can leave
cle.Loaderasstruct.erroror asValueErrorfromctypes, neither of which is aCLEError, so nothing catchingCLEErrorcatches them.Loading all 16677 truncations of the tracked
tests/x86/fauxware.obj, 15457 fail that way.Root cause
The backend maps the whole file as one backer at address 0, so every offset inside the file
resolves and nothing outside it does.
_add_relocsadds a relocation'sVirtualAddressto itssection's
PointerToRawDataand registers the result unchecked, so an offset the section doesnot contain still lands somewhere live: another section's raw data, the relocation table, the
symbol table.
_parsehas the same shape one layer earlier, takingNumberOfSections,PointerToSymbolTable,NumberOfSymbolsandPointerToRelocationsas read.Fix
Two bounds.
A relocation is registered only if its field lies wholly inside its own section's raw data and
wholly inside the file; otherwise it is skipped with a
log.warning, which is what the PEbackend does with a section whose raw data the file does not hold. The width comes from
struct.calcsizeon the relocation class'sPACK_FORMAT, so a four-byte field starting on thelast byte of a section is out of bounds.
Two details in that bound serve the other open COFF branches and change nothing here: the
section is read by index out of
self._coff.sectionsrather than off the loop variable, andthe file-size half is bounded by
self._image_vmemrather than byself._data. Both are thesame object here. Without them, #764 and #804, which conflict with this nowhere, stop four of
the COFF objects
angr/binariestracks from loading.The parser refuses a header table the file does not hold, raising
CLEInvalidBinaryErrorthatnames the field, the bytes it wanted and the size of the file. A table with no entries is
exempt, because nothing dereferences its pointer. Nothing else changes about which objects
load: across a sweep of the five fields patched into
tests/x86/coff_reloc_dir32.obj, masterand this branch accept the same objects and reject the same ones, and what changes is only the
exception. One bound covers two reads, because the string table begins on the byte after the
last symbol, so a file holding the four-byte string table size holds every symbol — across
those 16677 truncations, not one reaches the symbol table read.
A malformed table is fatal rather than skippable because the parser has no partial product:
_add_relocsindexesself._coff.symbolsby an index from the file, so a short symbol listbecomes an
IndexErrorelsewhere.pe.pytakes the other option for its analogue, warning andreturning no symbols when a PE symbol table ends past the file. A COFF object has no second
source.
Deliberately not done: bounding a section's own raw data by the file size, which is #806; and
the fixed-size header read on the first line of
_parse, which still raisesValueErrorfor18 of the 20 truncations shorter than twenty bytes, the other two never reaching this backend.
That read takes no field from the file, and whether such a file should reach the parser is
Coff.is_compatible's question.Testing
Five regressions in
tests/test_coff.py, all loading committed objects. The out-of-section oneasserts
.datastill readsaasixteen times, which is what a fix that stopped the crash butkept the silent write would fail; the three parser ones assert
CLEInvalidBinaryErrorand thefield it names, so each pins a different bound. All five fail on master, on the byte
comparison, on the
KeyErrorabove, and three times onstruct.errororValueError.A sixth test checks that an unmodified
tests/x86/fauxware.objstill yields 29 sections and225 relocations, and the cle suite goes from 261 to 267 passed with 9 skipped. Of the 16677
truncations, the same 1200 lengths load before and after. Of the ten COFF objects in
angr/binariesat the fixture head, the five well-formed ones keep their relocation counts exactly, 225 and 126among them, and the two carrying a loose relocation drop from one registered relocation to
none. With
perform_relocations=True, the default, master loads six of the ten and this branchloads seven.
The fixtures live in the
angr/binariespull request below, which must merge first.Validation: #807 (comment)
sync: angr/binaries#223
session: sharpen