morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #7013] [MERGED] fix(targets): keep original DOS header padding out of e_lfanew #7574

New issue

Closed

opened 2026-11-03 13:38:49 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-03 13:38:49 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/7013
Author: @dhruvac29
Created: 10/24/2026
Status: ✅ Merged
Merged: 10/24/2026
Merged by: @morluto

Base: main ← Head: fix/6510-dos-header-padding

📝 Commits (4)

e37e678 fix(targets): keep original DOS padding out of e_lfanew
417032a test(ghidra): cover DOS padding and unresolved header candidates
bd4980d fix(targets): read Windows signatures before dismissing DOS padding
af037b7 docs(targets): reconcile DOS padding admission contract

📊 Changes

8 files changed (+156 additions, -47 deletions)

View changed files

📝 docs/ghidra-dos.md (+12 -7)
📝 docs/installation.md (+7 -3)
📝 src/artifacts/inventory/ArtifactGraphConstruction.test.ts (+3 -3)
📝 src/domain/dosMz.test.ts (+61 -8)
📝 src/domain/dosMz.ts (+13 -1)
📝 src/ghidra/GhidraLoadImageValues.test.ts (+26 -1)
📝 src/native/metadata/PeImage.test.ts (+2 -2)
📝 tests/boundary/filesystem/binaryTarget.test.ts (+32 -22)

📄 Description

Summary

An original DOS MZ executable with a complete 64-byte header and a nonempty relocation table was refused as a malformed Windows target when its header padding at 0x3c–0x3f was nonzero. observeMzWindowsHeader treated every nonzero dword in a complete header as a Windows new-header candidate. The observation now requests an unread Windows signature before applying that padding rule, so a real PE/NE/LE/LX header beyond the initial probe still declares Windows; when the signature is observed as non-Windows, the remaining complete-header bytes are DOS padding.

Fixes #6510

Problem and expected behavior

Smallest trigger: a valid DOS MZ image whose page counts declare 128 image bytes, whose 64-byte header holds two relocation records at 0x1c–0x24, and whose remaining header padding contains a nonzero dword at 0x3c. Before this change, public inspect returned target_unavailable with invalid or truncated PE header/unresolved-candidate behavior. After this change, the unchanged file resolves as dos-mz / x86 and reaches provider selection. Confirmed PE remains PE, NE/LE/LX remain unsupported, and malformed Windows declarations without a delimiting relocation table remain rejected. A Windows signature outside the initial 4096-byte probe is read before the padding rule can dismiss the pointer, so a PE/NE/LE/LX header beyond the probe is not reclassified as DOS.

Change and scope

src/domain/dosMz.ts: factor the relocation-table end, and after signature confirmation treat a nonempty table ending at or before 0x3c inside a complete header as owning the remaining header padding. The padding dword is absent Windows evidence only after the signature is observed as non-Windows or no signature bytes remain; an unread signature still requests the bounded follow-up read.
src/domain/dosMz.test.ts: replace the old candidate expectation with DOS admission, add a control that keeps an unconfirmed pointer unresolved when no relocation table delimits the padding, and cover an unread signature outside the bounded prefix.
tests/boundary/filesystem/binaryTarget.test.ts: exercise the reported layout through parseBinaryTarget, including a PE signature beyond the initial probe behind DOS padding.
src/native/metadata/PeImage.test.ts and src/artifacts/inventory/ArtifactGraphConstruction.test.ts: update their expectations now that the fixture is a validated DOS image, not an unresolved candidate.
docs/ghidra-dos.md and docs/installation.md: document the padding-ownership boundary and keep the installation guide aligned with the admission contract.

Intentionally not included: changes to confirmed PE/NE/LE/LX handling, DOS relocation validation, provider behavior, or automatic target selection outside the reported ambiguous-pointer case.

Contract and boundary impact

Semantic owner and earliest changed stage: MZ Windows-header observation in src/domain/dosMz.ts; request parsing / target admission.
Contribution scope fit and linked maintainer decision: ordinary fix within existing boundaries; no maintainer scope decision required.
CLI and MCP/tool-catalog contract: no schema or tool-catalog change. The reported DOS fixture now classifies as dos-mz instead of returning an unresolved-target refusal.
Provider, bridge, target-format, or platform compatibility: DOS MZ remains Ghidra-only; no provider code changed. Confirmed PE, NE, LE, LX, and short-header behavior are unchanged.
Evidence, artifact, provenance, or reconstruction contract: DOS structural facts and Windows-header evidence remain separate; the input bytes are not modified.
Target selection, authority, upstream behavior, and cleanup details: read-only header classification; no target execution, mutation, or cleanup path is involved.
Generated metadata (docs/public/product-catalog.json), package, or installation impact: none.

Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.

Read existing files or captured data.
Launch or attach to a process.
Inject instrumentation or run scripts/code.
Change process memory, registers, files, or configuration.
Contact network services.
Recover protected code or work around protections.
Documentation or usability only; no target interaction.
Not sure yet; investigation or maintainer guidance needed.

No provider or target was launched; the CLI reproduction used deliberately absent provider installations.

Evidence and regression coverage

Tests added or updated: src/domain/dosMz.test.ts, tests/boundary/filesystem/binaryTarget.test.ts, src/native/metadata/PeImage.test.ts, src/artifacts/inventory/ArtifactGraphConstruction.test.ts, and src/ghidra/GhidraLoadImageValues.test.ts.
Review follow-up: the Greptile P1 finding about signatures outside the initial buffer is covered by the new bounded-prefix domain assertion and the PE-beyond-the-probe filesystem regression. The Greptile/Codex P2 findings about temporary debug logging were resolved by removing the logging entirely. The Codex documentation finding is covered by the aligned docs/ghidra-dos.md and docs/installation.md contract text. Current main already contains the analysis-view cancellation test repair from #7022, so the duplicate branch commit was dropped during rebase.
Base reproduction or other evidence: the new domain assertion failed on the unmodified parser with { state: "candidate", offset: 1145258561 }; after the change the same fixture observes { state: "absent" } and parses as dos-mz. The boundary test exercises the same fixture through parseBinaryTarget; its previous version asserted the unresolved-candidate refusal, and the updated version observes the validated DOS target.
User-visible CLI/MCP output (if applicable): ran the issue's inline fixtures through scripts/rea.mjs inspect ... --provider ghidra --format json with provider installations deliberately absent:

padding-nonzero: exit=1 code=provider_unavailable target_formats=["dos-mz"]
padding-zero:    exit=1 code=provider_unavailable target_formats=["dos-mz"]
truncated-pe:    exit=1 code=target_unavailable reason="invalid or truncated PE header"
invalid-windows-signature: exit=1 code=target_unavailable reason="MZ Windows new-header candidate is unresolved"

Remaining proof gaps: no real Ghidra provider, DOS execution, or real-world original DOS sample was exercised. The malformed-signature control reports an unresolved candidate on current main rather than the older revision's invalid or truncated PE header, but remains a target_unavailable refusal.

For evidence-bearing changes:

Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

Rebased onto current main (422ca5a8f) and reran the affected checks on Node.js v24.22.1. Current main carries the Windows source-path test alignment from #7021 and the cancellation test repair from #7022, so this branch does not duplicate either change.
npm run test:local -- src/domain/dosMz.test.ts src/native/metadata/PeImage.test.ts src/artifacts/inventory/ArtifactGraphConstruction.test.ts src/ghidra/GhidraLoadImageValues.test.ts — 4 files, 127 tests passed.
npx vitest run --project boundary tests/boundary/filesystem/binaryTarget.test.ts — 1 file, 80 tests passed.
npx vitest run --project process-global tests/process-global/commandFinalizationCancellation.test.ts — 20 tests passed.
npm run check:fast — typecheck and lint passed.
npm run docs:check — generated documentation checks and 9 README consistency tests passed.
CLI reproduction above was executed before the latest rebase on Node.js v24.22.1 with the compiled runtime produced by the focused test build.

Compatibility, safety, and release

Breaking changes or migration steps: none. A previously rejected ambiguous DOS layout is now admitted; confirmed Windows declarations and malformed declarations without padding ownership are unchanged.
Real Hopper/Ghidra, browser, or OS coverage: not run; provider installations were deliberately absent.
Package or release metadata impact: none.
Security, privacy, process, or containment review: read-only header classification; no execution, mutation, or sensitive data handling change.

Author readiness

Human owner: @dhruvac29

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare this PR ready for initial maintainer review.

Before merge

This PR addresses a concrete problem or an agreed enhancement.
Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
The PR has one focused outcome and the title follows type(scope): outcome.
Related issue is linked, or the reason for not linking one is stated above.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed.
User-visible CLI/MCP changes include representative output.
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/7013 **Author:** [@dhruvac29](https://github.com/dhruvac29) **Created:** 10/24/2026 **Status:** ✅ Merged **Merged:** 10/24/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/6510-dos-header-padding` --- ### 📝 Commits (4) - [`e37e678`](https://github.com/morluto/rea/commit/e37e6786958b64ab1fd32e511586e82cafb81de1) fix(targets): keep original DOS padding out of e_lfanew - [`417032a`](https://github.com/morluto/rea/commit/417032a4593af4ebbbdb7f4a92f756a0e53859e7) test(ghidra): cover DOS padding and unresolved header candidates - [`bd4980d`](https://github.com/morluto/rea/commit/bd4980d2e7a931e7dbd37f2023155b102dc3d40d) fix(targets): read Windows signatures before dismissing DOS padding - [`af037b7`](https://github.com/morluto/rea/commit/af037b7a7a7181056882a6bc31d995710b44e98c) docs(targets): reconcile DOS padding admission contract ### 📊 Changes **8 files changed** (+156 additions, -47 deletions) <details> <summary>View changed files</summary> 📝 `docs/ghidra-dos.md` (+12 -7) 📝 `docs/installation.md` (+7 -3) 📝 `src/artifacts/inventory/ArtifactGraphConstruction.test.ts` (+3 -3) 📝 `src/domain/dosMz.test.ts` (+61 -8) 📝 `src/domain/dosMz.ts` (+13 -1) 📝 `src/ghidra/GhidraLoadImageValues.test.ts` (+26 -1) 📝 `src/native/metadata/PeImage.test.ts` (+2 -2) 📝 `tests/boundary/filesystem/binaryTarget.test.ts` (+32 -22) </details> ### 📄 Description ## Summary An original DOS MZ executable with a complete 64-byte header and a nonempty relocation table was refused as a malformed Windows target when its header padding at `0x3c`–`0x3f` was nonzero. `observeMzWindowsHeader` treated every nonzero dword in a complete header as a Windows new-header candidate. The observation now requests an unread Windows signature before applying that padding rule, so a real PE/NE/LE/LX header beyond the initial probe still declares Windows; when the signature is observed as non-Windows, the remaining complete-header bytes are DOS padding. Fixes #6510 ## Problem and expected behavior Smallest trigger: a valid DOS MZ image whose page counts declare 128 image bytes, whose 64-byte header holds two relocation records at `0x1c`–`0x24`, and whose remaining header padding contains a nonzero dword at `0x3c`. Before this change, public `inspect` returned `target_unavailable` with `invalid or truncated PE header`/unresolved-candidate behavior. After this change, the unchanged file resolves as `dos-mz` / `x86` and reaches provider selection. Confirmed PE remains PE, NE/LE/LX remain unsupported, and malformed Windows declarations without a delimiting relocation table remain rejected. A Windows signature outside the initial 4096-byte probe is read before the padding rule can dismiss the pointer, so a PE/NE/LE/LX header beyond the probe is not reclassified as DOS. ## Change and scope - `src/domain/dosMz.ts`: factor the relocation-table end, and after signature confirmation treat a nonempty table ending at or before `0x3c` inside a complete header as owning the remaining header padding. The padding dword is absent Windows evidence only after the signature is observed as non-Windows or no signature bytes remain; an unread signature still requests the bounded follow-up read. - `src/domain/dosMz.test.ts`: replace the old candidate expectation with DOS admission, add a control that keeps an unconfirmed pointer unresolved when no relocation table delimits the padding, and cover an unread signature outside the bounded prefix. - `tests/boundary/filesystem/binaryTarget.test.ts`: exercise the reported layout through `parseBinaryTarget`, including a PE signature beyond the initial probe behind DOS padding. - `src/native/metadata/PeImage.test.ts` and `src/artifacts/inventory/ArtifactGraphConstruction.test.ts`: update their expectations now that the fixture is a validated DOS image, not an unresolved candidate. - `docs/ghidra-dos.md` and `docs/installation.md`: document the padding-ownership boundary and keep the installation guide aligned with the admission contract. Intentionally not included: changes to confirmed PE/NE/LE/LX handling, DOS relocation validation, provider behavior, or automatic target selection outside the reported ambiguous-pointer case. ## Contract and boundary impact - Semantic owner and earliest changed stage: MZ Windows-header observation in `src/domain/dosMz.ts`; request parsing / target admission. - Contribution scope fit and linked maintainer decision: ordinary fix within existing boundaries; no maintainer scope decision required. - CLI and MCP/tool-catalog contract: no schema or tool-catalog change. The reported DOS fixture now classifies as `dos-mz` instead of returning an unresolved-target refusal. - Provider, bridge, target-format, or platform compatibility: DOS MZ remains Ghidra-only; no provider code changed. Confirmed PE, NE, LE, LX, and short-header behavior are unchanged. - Evidence, artifact, provenance, or reconstruction contract: DOS structural facts and Windows-header evidence remain separate; the input bytes are not modified. - Target selection, authority, upstream behavior, and cleanup details: read-only header classification; no target execution, mutation, or cleanup path is involved. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [x] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. No provider or target was launched; the CLI reproduction used deliberately absent provider installations. ## Evidence and regression coverage - Tests added or updated: `src/domain/dosMz.test.ts`, `tests/boundary/filesystem/binaryTarget.test.ts`, `src/native/metadata/PeImage.test.ts`, `src/artifacts/inventory/ArtifactGraphConstruction.test.ts`, and `src/ghidra/GhidraLoadImageValues.test.ts`. - Review follow-up: the Greptile P1 finding about signatures outside the initial buffer is covered by the new bounded-prefix domain assertion and the PE-beyond-the-probe filesystem regression. The Greptile/Codex P2 findings about temporary debug logging were resolved by removing the logging entirely. The Codex documentation finding is covered by the aligned `docs/ghidra-dos.md` and `docs/installation.md` contract text. Current `main` already contains the analysis-view cancellation test repair from #7022, so the duplicate branch commit was dropped during rebase. - Base reproduction or other evidence: the new domain assertion failed on the unmodified parser with `{ state: "candidate", offset: 1145258561 }`; after the change the same fixture observes `{ state: "absent" }` and parses as `dos-mz`. The boundary test exercises the same fixture through `parseBinaryTarget`; its previous version asserted the unresolved-candidate refusal, and the updated version observes the validated DOS target. - User-visible CLI/MCP output (if applicable): ran the issue's inline fixtures through `scripts/rea.mjs inspect ... --provider ghidra --format json` with provider installations deliberately absent: ```text padding-nonzero: exit=1 code=provider_unavailable target_formats=["dos-mz"] padding-zero: exit=1 code=provider_unavailable target_formats=["dos-mz"] truncated-pe: exit=1 code=target_unavailable reason="invalid or truncated PE header" invalid-windows-signature: exit=1 code=target_unavailable reason="MZ Windows new-header candidate is unresolved" ``` - Remaining proof gaps: no real Ghidra provider, DOS execution, or real-world original DOS sample was exercised. The malformed-signature control reports an unresolved candidate on current `main` rather than the older revision's `invalid or truncated PE header`, but remains a `target_unavailable` refusal. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - Rebased onto current `main` (`422ca5a8f`) and reran the affected checks on Node.js v24.22.1. Current `main` carries the Windows source-path test alignment from #7021 and the cancellation test repair from #7022, so this branch does not duplicate either change. - `npm run test:local -- src/domain/dosMz.test.ts src/native/metadata/PeImage.test.ts src/artifacts/inventory/ArtifactGraphConstruction.test.ts src/ghidra/GhidraLoadImageValues.test.ts` — 4 files, 127 tests passed. - `npx vitest run --project boundary tests/boundary/filesystem/binaryTarget.test.ts` — 1 file, 80 tests passed. - `npx vitest run --project process-global tests/process-global/commandFinalizationCancellation.test.ts` — 20 tests passed. - `npm run check:fast` — typecheck and lint passed. - `npm run docs:check` — generated documentation checks and 9 README consistency tests passed. - CLI reproduction above was executed before the latest rebase on Node.js v24.22.1 with the compiled runtime produced by the focused test build. ## Compatibility, safety, and release - Breaking changes or migration steps: none. A previously rejected ambiguous DOS layout is now admitted; confirmed Windows declarations and malformed declarations without padding ownership are unchanged. - Real Hopper/Ghidra, browser, or OS coverage: not run; provider installations were deliberately absent. - Package or release metadata impact: none. - Security, privacy, process, or containment review: read-only header classification; no execution, mutation, or sensitive data handling change. ## Author readiness Human owner: @dhruvac29 - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-11-03 13:38:49 +00:00

closed this issue
added the
pull-request
label

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#7574

No description provided.

Rows
Columns