Added ESP Unpacker#412
Open
Jepson2k wants to merge 26 commits into
Open
Conversation
dannyp303
reviewed
Feb 6, 2024
EdwardLarson
reviewed
Feb 8, 2024
rbs-jacob
requested changes
Jun 6, 2025
rbs-jacob
left a comment
rbs-jacob
left a comment
Member
There was a problem hiding this comment.
I did not review all of this in great detail, but there are some substantial changes needed before I'd be comfortable merging this in
Comment on lines
+47
to
+62
| def determine_chip(f: _TemporaryFileWrapper) -> str: | ||
| """ | ||
| Determines the chip type based on the firmware image. | ||
|
|
||
| :param f: A temporary file object containing the firmware image | ||
| :return: The chip name as a string, defaults to 'esp8266' if not determined | ||
| """ | ||
| extended_header = f.read(16) | ||
| if extended_header[-1] not in [0, 1]: | ||
| return "esp8266" | ||
|
|
||
| chip_id = int.from_bytes(extended_header[4:5], "little") | ||
| for rom in [n for n in ROM_LIST if n.CHIP_NAME != "ESP8266"]: | ||
| if chip_id == rom.IMAGE_CHIP_ID: | ||
| return rom.CHIP_NAME.lower() | ||
| return "esp8266" |
Member
There was a problem hiding this comment.
For type-checking purposes, this should probably return an enum instead of a string.
Member
There was a problem hiding this comment.
Also, why use a temporary file wrapper instead of indexing into the data itself? All of the data is already stored in memory.
Member
There was a problem hiding this comment.
Philosophically, there are a lot of child types here that we don't need to define.
To make an analogy: the OFRAK tar unpacker unpacks files, not tar blocks. We want to do the equivalent here and unpack only what will actually be semantically useful for further layers of unpacking or analysis. We don't necessarily need tagged children for each part of the file type, especially if those parts are metadata.
Unfortunately, this guiding philosophy isn't documented anywhere, so there isn't a way you could have known this before. Also if any of this is based on the Elf unpacker, that one deviates from this philosophy a bit.
…cker # Conflicts: # ofrak_core/tests/components/assets/esp/esp32_basic_flash.bin # ofrak_core/tests/components/assets/esp/esp32_hello.bin # ofrak_core/tests/components/assets/esp/esp32_ota_flash.bin # ofrak_core/tests/components/assets/esp/esp32_storage_flash.bin # ofrak_core/tests/components/assets/esp/esp32s3_basic_flash.bin # ofrak_core/tests/components/assets/esp/esp32s3_hello.bin # ofrak_core/tests/components/assets/esp/esp8266_hello.bin # ofrak_core/tests/components/test_esp_app_component.py # ofrak_core/tests/components/test_esp_flash_component.py
Address reviewer feedback on the ESP app components: - Remove the runtime esptool dependency. The unpacker/analyzer/packer now parse the ESP app image format statelessly by indexing into the resource bytes (struct + hashlib) instead of writing a NamedTemporaryFile and handing it to esptool's LoadFirmwareImage. This eliminates the temp file, the in-process esptool call, the stateful file seeks, and the try/finally cleanup. Per-chip segment memory maps (sourced from esptool) are baked in as constants. - Restructure to segments-only children: only loadable segments become ESPAppSection children (IRAM/IROM tagged CodeRegion). Header, extended header, checksum and SHA256 digest are exposed as ESPAppAttributes rather than as tagged child resources. - determine_chip returns an ESPChip enum (not a string); chip is stored on ESPAppAttributes as that enum. - Move esptool to test-only requirements (used only by the ESP app tests to independently validate repacked images via `python -m esptool image_info`). - Rewrite the ESP app tests for the attributes-based model and fix the esptool verification helper to use sys.executable so it actually runs. - Add CHANGELOG entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address findings from a multi-agent review of the esptool-removal refactor: - Add ESP8266 "v2" (magic 0xEA) image support: a unified stateless _parse_image handles the two-header v2 layout (irom0 segment excluded from the checksum, a second 0xE9 header with the loadable segments, and a trailing CRC32) as well as the v1/ESP32 layout. Adds image_version/crc32/crc32_valid to ESPAppAttributes. - Validate image structure up front: _parse_image raises a clean UnpackerError on too-small / truncated / malformed-segment / bad-magic input instead of IndexError/struct.error or silently truncating the segment list (so the checksum is never computed over a partial image). The packer validates the checksum offset. - ESPAppIdentifier now guards on header length, so a 1-3 byte resource starting with 0xE9/0xEA no longer raises IndexError during recursive identify. - Add per-chip memory maps for ESP32-C2/C6/H2/P4/C5 (recognized by ESPChip but previously missing), restoring segment naming and CodeRegion tagging for them. - _determine_chip reads the full 16-bit chip_id, matching the analyzer. - Fix ESPPartition.get_header to navigate flash -> partition table -> entry (upstream removed Resource.get_only_sibling_as_view; this also clears mypy). - Tests: add ESP8266 v2 unpack + unpack/modify/pack round-trips (built via esptool), malformed-input rejection, and the short-resource identifier guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ESPAppUnpacker carried id=b"ESP32AppUnpacker" (class name ESPAppUnpacker). OFRAK's GUI server skips any component whose class name != get_id() (server.py _get_specific_components), so the unpacker was dropped from /get_components and would not be runnable from the web API. Align the id with the class name. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…xes, chip-aware flash decode, 100% tests - flash.py/flash_model.py: stateless byte-indexed rewrite. Fix partition-entry data_range to be parent-relative (was absolute -> wrong bytes), size the partition table to one 4 KB sector, rename the colliding SectionIndex index, make ESPPartitionTableEntryModifier byte-accurate (preserve partition offset/size), and fix ESPPartitionTable.get_section_by_name to query the flash. - app.py/app_model.py: tag CodeRegion only for instruction-only regions (no mistag where IRAM/DRAM coincide on C6/H2/C5/P4), decode SPI drive strengths as 4-bit nibbles (matches esptool), guard against trailing data, drop dead code, and decode flash size/frequency chip-aware into ESPAppAttributes (shown in GUI). - Replace in-code image generation with real committed assets (esp32c3, esp32c6, esp8266 v2); real-data error-path + robustness tests; 100% coverage of ofrak.core.esp. - Pin esptool as a test dependency; broaden .gitignore.
Collaborator
Author
|
@rbs-jacob I believe I've resolved all prior comments and issues with the latest commits. The macOS reds appear to be related to a no_autobump! infra issue thats affecting the nightly as well. |
…irements.txt esptool is a test-only tool (used by the ESP app tests to independently validate repacked images), not a component dependency. It is already declared in requirements-test.txt; the entry in requirements.txt was a leftover from the pre-stateless draft and mirrored no install_requires entry in setup.py. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Jepson2k
added a commit
to Jepson2k/ofrak
that referenced
this pull request
Jun 17, 2026
Same fix as on the redballoonsecurity#412 branch: esptool is a test-only tool (declared in requirements-test.txt), not a component dependency, so it does not belong in requirements.txt. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
4 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.
One sentence summary of this PR (This should go in the CHANGELOG!)
Added an ESP unpacker to support unpacking Espressif binaries.
Link to Related Issue(s)
#410
Please describe the changes in your request.
Added esp.py to ofrak_core/ofrak/core which contains multiple resource view for ESP binaries as well as identification and unpacking. Uses ESPTool.py for most of the work.
Anyone you think should look at this, specifically?
@rbs-jacob
There are multiple "TODO:"s located in the code, a few of which are notes for things I'm unsure about, and the rest are questions that I couldn't find answers to in the contribution guidelines.