Repository navigation
fix: normalize new Ghidra PIE imports - #310
Conversation
branover
left a comment
There was a problem hiding this comment.
REQUEST-CHANGES
I completed both merge-gate passes against PR head 8b9371a and base c1b4ce7, using both gh pr diff 310 and git diff origin/main...HEAD.
Findings
-
HIGH —
src/hexgraph/sandbox/probes/pyghidra_lib.py:300: an unverified cold ELF mapping is still committed. When_elf_load_map()returns anunverifiedreport (for example a valid ELF using the explicitly unsupported extended program-header count, or a truncated/otherwise unsupported ELF header),validate_address_mapping(..., allow_existing_base=False)returns normally.open_target()then completes analysis and commits the warm marker even though normalization was skipped and the promised ELF coordinate system was never validated. The report has no warning on the cold path, so a PIE can retain Ghidra's legacy bias silently despite the PR's contract. Impact: a newly created project can publish wrong addresses as if the normalized import succeeded. Fix: raiseAddressMappingMismatchfor an unverified ELF on the cold path (while continuing to preserve and warn for warm projects), and add a cold-path regression test for unsupported/truncated ELF metadata. -
HIGH —
src/hexgraph/engine/re/ghidra_bridge.py:227: managed decompile responses discard the new mapping and warning. The server now attachesaddress_mapping/warning, but_ManagedOps.decompile()rebuildsoutfrom selected fields and omits both. This affects decompile/list-functions andenrich_target()calls routed through a resident managed bridge. Impact: an existing warm project with a legacy load bias produces user-visible addresses without the required warning, and its Observations lack the coordinate fingerprint, so they can deduplicate against a different coordinate system. Fix: copy validatedaddress_mappingandwarninginto the managed decompile result and cover the client reconstruction path, not only_serve_one(). -
HIGH —
src/hexgraph/sandbox/decompiler.py:50: radare2 fallback addresses are recorded under the legacy Ghidra mapping. When Ghidra misses a requested focus,GhidraDecompiler.decompile()substitutes the radare2 focus and tags it withfocus_fallback=True;focus_only_payload()now unconditionally copies the original Ghidraaddress_mappingwhile dropping the fallback provenance. For a preserved legacy project, the focus address is the normalized radare2 address, but the Observation is fingerprinted as legacy-biased Ghidra output. Impact: the payload mixes coordinate systems, warns that its displayed address includes a bias when it does not, and a later real Ghidra result with the same mapping can incorrectly deduplicate to the fallback Observation. Fix: preserve the focus engine/coordinate provenance and only attach the Ghidra mapping to a focus that actually came from Ghidra (or model inventory and focus mappings separately); add a legacy-mapping fallback regression test.
Pass results
- Correctness/test quality: the three blockers above prevent the coordinate and warning contracts from holding on cold-parser failures, managed bridge decompiles, and fallback decompiles. The focused affected suite passed:
108 passed. - Security/repository invariants: no loopback, sandbox, secret-handling, or policy-seam regression found. ELF parsing remains inside the sandbox probe and is bounded to fixed-size headers with a 4096-entry cap; no execution, egress, or rehosting gate changed. No public-repository confidential target details were found.
- Docs/migrations/UX: MCP docs and generated VR-skill guidance ship with the behavior change. There is no DB model or frontend behavior change, so no migration or UX-contract update is required.
- Broad test note:
just testcompleted the suite except for the known environment-only Docker bridge readiness error intest_live_session_auth_flow_across_calls(172.17.x.x:8080unreachable from the host); this is not attributed to this PR.
Addresses review comments r3789589252, r3789589270, and r3789589291 on PR #310.
branover
left a comment
There was a problem hiding this comment.
APPROVE
Follow-up review at PR head c9229f3: all three blocking findings from the REQUEST-CHANGES review are fixed.
- Cold ELF mappings that cannot be verified now raise before a new warm marker can be committed. Existing warm projects remain preserve-and-warn, and relocatable ELF files without
PT_LOADranges remain explicitly supported asnot_applicable. - Managed bridge decompile reconstruction now propagates
address_mappingandwarning, so user-facing decompile/list/enrichment behavior and Observation identity retain the actual coordinate system. - A radare2 fallback focus now retains its engine/fallback provenance, is not fingerprinted with a legacy Ghidra mapping, and does not receive a misleading Ghidra-bias warning.
Verification: 110 passed in the affected address-mapping, managed-bridge, fallback, Observation, re_script, and tool-contract suites. The worktree is clean and commit c9229f3 was pushed. The three inline review threads include fix replies and are resolved.
The security/repository-invariant pass remains clean: hostile ELF parsing stays in the sandbox; loopback binding, secrets, and the execution/network/rehosting policy seams are unchanged; no confidential target details are present. Docs are current, and this PR has no model, migration, frontend, or UX-contract obligation.
CI must complete successfully on the new head before merge; the dispatching owner retains merge responsibility.
Summary
re_scriptWhy
Ghidra 12.x commonly imports a PIE whose ELF load base is zero at
0x100000. That made otherwise identical virtual addresses disagree across Ghidra, radare2, and debugger output. New imports now use the ELF coordinate system, while expensive existing projects remain usable and clearly identify their historical coordinate system.Verification
1868 passed, 47 skipped, 14 deselectedin the offline non-slow gate213 passed, 5 skippedin the affected offline suites4 passedin live Ghidra decompilation, emulation, and taint testsjust testreached the unrelated live HTTP fixture but the local host could not connect to its Docker bridge IP; the focused reproduction timed out identically at the fixture readiness check