[PR #6721] [MERGED] fix(pe): accept WIN_CERTIFICATE padding counted in dwLength #7398

Closed
opened 2026-11-01 21:47:34 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6721
Author: @Deepak-Kambala
Created: 10/23/2026
Status: ✅ Merged
Merged: 10/23/2026
Merged by: @morluto

Base: main ← Head: fix/pe-certificate-dwlength-padding


📝 Commits (5)

  • e850dcf fix(pe): accept WIN_CERTIFICATE padding counted in dwLength
  • f4cdff4 perf(pe): reuse decoded PKCS#7 ASN.1 tree
  • dbc31e2 Merge branch 'main' of https://github.com/morluto/rea into fix/pe-certificate-dwlength-padding
  • 49c75e0 fix(pe): reconcile counted certificate padding with main
  • f538cd2 style: format files inherited from main for CI

📊 Changes

12 files changed (+165 additions, -109 deletions)

View changed files

📝 THIRD_PARTY_NOTICES.md (+20 -20)
📝 docs/pe-certificates.md (+3 -2)
📝 src/application/javascript/JavaScriptArtifactGraphFindings.ts (+2 -1)
📝 src/domain/reconstructionReadinessJourneyFindings.ts (+4 -1)
📝 src/ida/IdaDoctor.test.ts (+2 -1)
📝 src/native/pe/PeCertificateParser.test.ts (+80 -35)
📝 src/native/pe/PeCertificateParser.ts (+15 -9)
📝 src/native/pe/PeCertificates.fixture.ts (+2 -1)
📝 tests/acceptance/peCertificatesCli.test.ts (+12 -15)
📝 tests/boundary/cli/idaDoctorRegistration.test.ts (+1 -3)
📝 tests/boundary/filesystem/referenceSourceQueuedDirectories.test.ts (+4 -1)
📝 tests/boundary/mcp/nativeUiFactsMcp.test.ts (+20 -20)

📄 Description

Summary

Fixes #6694. inspect-pe-certificates now accepts valid PE32 and PE32+ certificate entries when WIN_CERTIFICATE.dwLength includes the required zero-filled alignment padding.

Problem and expected behavior

When dwLength included alignment padding, the parser treated all dwLength - 8 bytes as CMS data. The padding then appeared as trailing ASN.1 data, and structural inspection failed. The encoded CMS payload should end at its ASN.1 root boundary; only format-permitted alignment padding may follow it.

Change and scope

Use the ASN.1 root end offset to bound the encoded CMS payload. Accept trailing bytes only when they are zero-filled and exactly the padding needed to align the full certificate entry. Preserve the original artifact bytes and digest, dwLength, and entry range; report the CMS payload range and hash without alignment padding. Malformed CMS and other trailing data remain rejected. Reuse main’s decoded-root CMS interface, with trailing-byte validation owned by the PE entry parser. Document payload ranges and hashes. Cover every alignment gap and consecutive entries. Refresh the two stale test expectations identified in review for TeletexString compatibility and retained filesystem diagnostics.

This is a routine static PE-format inspection fix within the existing contribution scope; no maintainer scope decision is required. No CLI/MCP contract or generated metadata changes are included. Structural inspection does not verify cryptographic signatures.

Follow the contribution scope.
Unresolved scope, authority, side effects, or required verification block
integration. Link the required maintainer scope decision for new providers or
capabilities involving target execution, injection, mutation, or protection
recovery; ordinary fixes within existing boundaries may state "not applicable".
Draft PRs may leave the scope decision pending and the readiness and merge checklists
unchecked. Maintainers can help resolve open questions.

Contract and boundary impact

  • Semantic owner and earliest changed stage: PeCertificateParser, at PE certificate-table entry parsing.
  • Contribution scope fit and linked maintainer decision: ordinary format inspection bug fix; maintainer decision not applicable.
  • CLI and MCP/tool-catalog contract: no contract or catalog changes.
  • Provider, bridge, target-format, or platform compatibility: built-in PE32 and PE32+ parser; BER support remains covered.
  • Evidence, artifact, provenance, or reconstruction contract: preserve artifact identity and entry range; payload hash and range exclude padding.
  • Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable; this reads the selected local artifact and does not execute it.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact (CI generates and validates ignored catalogs and skills/; edit authored instructions in .agents/skills/reverse-engineer-anything/ and never commit build outputs): 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.

Evidence and regression coverage

  • Tests added or updated: parser regressions cover PE32 and PE32+ counted-padding fixtures, existing excluded-padding controls, nonzero and excessive trailing bytes, and overlong entry length. Existing parser coverage rejects malformed CMS, truncated entries, and short tables. Tests also check complete coverage, one certificate and signer, unchanged artifact digest and size, unchanged entry length/range, and CMS payload range/hash excluding padding.
  • Base reproduction or other evidence: the issue report's source CLI/service reproduction reports the original trailing-data failure for counted padding and success for excluded-padding controls. The new parser regression cases exercise the counted-padding boundary.
  • User-visible CLI/MCP output (if applicable): existing focused CLI and MCP tests passed. The counted-padding cases are covered at the parser boundary, not through CLI/MCP.
  • Remaining proof gaps: the full repository suite was not run; tests use synthetic fixtures, with no real-world PE artifact or external provider verification.

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

Current merge with main (960b74258), Node.js 24.22.1:

  • npm run test:local -- src/native/pe/PeCertificateParser.test.ts src/domain/native/peCertificates.test.ts src/application/binaryDiagnostics/PeCertificatesService.test.ts — 38 passed.
  • npm run test:focused -- tests/acceptance/peCertificatesCli.test.ts tests/boundary/mcp/peCertificatesMcp.test.ts tests/boundary/filesystem/peCertificates.test.ts tests/boundary/filesystem/referenceSourceQueuedDirectories.test.ts — 10 passed after updating the two stale assertions noted in review.
  • npm run check:fast — typecheck and lint passed.
  • CI follow-up f538cd2aa: formatted six files inherited from main after CI reported format:check failures. npm run check:ci:fast — passed locally, including formatting, typecheck, compilation, lint and module boundaries.
  • git diff --check — passed.
  • Synthetic parser coverage includes all gaps 0–7 for PE32/PE32+ and consecutive counted-padding entries. Compiled CLI/MCP tests use synthetic artifacts; counted-padding regressions run at the parser boundary.
  • Full repository suite, real-world signed artifacts, cryptographic validity, and fresh performance measurements were not run. Earlier timing claims have been removed because the implementation now uses main’s decoded-root interface.

Compatibility, safety, and release

  • Breaking changes or migration steps: none.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: static inspection only; no target execution or artifact modification. Signature validity is not established.

Author readiness

Human owner: @Deepak-Kambala

  • 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/6721 **Author:** [@Deepak-Kambala](https://github.com/Deepak-Kambala) **Created:** 10/23/2026 **Status:** ✅ Merged **Merged:** 10/23/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/pe-certificate-dwlength-padding` --- ### 📝 Commits (5) - [`e850dcf`](https://github.com/morluto/rea/commit/e850dcfaa08fefbeee1c686a40e4e605aaabefac) fix(pe): accept WIN_CERTIFICATE padding counted in dwLength - [`f4cdff4`](https://github.com/morluto/rea/commit/f4cdff45276e0b8fcf84ebd7e6d34a8618d85558) perf(pe): reuse decoded PKCS#7 ASN.1 tree - [`dbc31e2`](https://github.com/morluto/rea/commit/dbc31e271b7214417cae5cbe58342a7fa97fc10e) Merge branch 'main' of https://github.com/morluto/rea into fix/pe-certificate-dwlength-padding - [`49c75e0`](https://github.com/morluto/rea/commit/49c75e05af4b93b91a81dcbb82eaac94c5e2b09d) fix(pe): reconcile counted certificate padding with main - [`f538cd2`](https://github.com/morluto/rea/commit/f538cd2aa64f9c16e2e46e052dddacfa4bfe212b) style: format files inherited from main for CI ### 📊 Changes **12 files changed** (+165 additions, -109 deletions) <details> <summary>View changed files</summary> 📝 `THIRD_PARTY_NOTICES.md` (+20 -20) 📝 `docs/pe-certificates.md` (+3 -2) 📝 `src/application/javascript/JavaScriptArtifactGraphFindings.ts` (+2 -1) 📝 `src/domain/reconstructionReadinessJourneyFindings.ts` (+4 -1) 📝 `src/ida/IdaDoctor.test.ts` (+2 -1) 📝 `src/native/pe/PeCertificateParser.test.ts` (+80 -35) 📝 `src/native/pe/PeCertificateParser.ts` (+15 -9) 📝 `src/native/pe/PeCertificates.fixture.ts` (+2 -1) 📝 `tests/acceptance/peCertificatesCli.test.ts` (+12 -15) 📝 `tests/boundary/cli/idaDoctorRegistration.test.ts` (+1 -3) 📝 `tests/boundary/filesystem/referenceSourceQueuedDirectories.test.ts` (+4 -1) 📝 `tests/boundary/mcp/nativeUiFactsMcp.test.ts` (+20 -20) </details> ### 📄 Description ## Summary Fixes #6694. `inspect-pe-certificates` now accepts valid PE32 and PE32+ certificate entries when `WIN_CERTIFICATE.dwLength` includes the required zero-filled alignment padding. ## Problem and expected behavior When `dwLength` included alignment padding, the parser treated all `dwLength - 8` bytes as CMS data. The padding then appeared as trailing ASN.1 data, and structural inspection failed. The encoded CMS payload should end at its ASN.1 root boundary; only format-permitted alignment padding may follow it. ## Change and scope Use the ASN.1 root end offset to bound the encoded CMS payload. Accept trailing bytes only when they are zero-filled and exactly the padding needed to align the full certificate entry. Preserve the original artifact bytes and digest, `dwLength`, and entry range; report the CMS payload range and hash without alignment padding. Malformed CMS and other trailing data remain rejected. Reuse main’s decoded-root CMS interface, with trailing-byte validation owned by the PE entry parser. Document payload ranges and hashes. Cover every alignment gap and consecutive entries. Refresh the two stale test expectations identified in review for TeletexString compatibility and retained filesystem diagnostics. This is a routine static PE-format inspection fix within the existing contribution scope; no maintainer scope decision is required. No CLI/MCP contract or generated metadata changes are included. Structural inspection does not verify cryptographic signatures. Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). Unresolved scope, authority, side effects, or required verification block integration. Link the required maintainer scope decision for new providers or capabilities involving target execution, injection, mutation, or protection recovery; ordinary fixes within existing boundaries may state "not applicable". Draft PRs may leave the scope decision pending and the readiness and merge checklists unchecked. Maintainers can help resolve open questions. ## Contract and boundary impact - Semantic owner and earliest changed stage: `PeCertificateParser`, at PE certificate-table entry parsing. - Contribution scope fit and linked maintainer decision: ordinary format inspection bug fix; maintainer decision not applicable. - CLI and MCP/tool-catalog contract: no contract or catalog changes. - Provider, bridge, target-format, or platform compatibility: built-in PE32 and PE32+ parser; BER support remains covered. - Evidence, artifact, provenance, or reconstruction contract: preserve artifact identity and entry range; payload hash and range exclude padding. - Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable; this reads the selected local artifact and does not execute it. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact (CI generates and validates ignored catalogs and `skills/`; edit authored instructions in `.agents/skills/reverse-engineer-anything/` and never commit build outputs): 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. ## Evidence and regression coverage - Tests added or updated: parser regressions cover PE32 and PE32+ counted-padding fixtures, existing excluded-padding controls, nonzero and excessive trailing bytes, and overlong entry length. Existing parser coverage rejects malformed CMS, truncated entries, and short tables. Tests also check complete coverage, one certificate and signer, unchanged artifact digest and size, unchanged entry length/range, and CMS payload range/hash excluding padding. - Base reproduction or other evidence: the issue report's source CLI/service reproduction reports the original trailing-data failure for counted padding and success for excluded-padding controls. The new parser regression cases exercise the counted-padding boundary. - User-visible CLI/MCP output (if applicable): existing focused CLI and MCP tests passed. The counted-padding cases are covered at the parser boundary, not through CLI/MCP. - Remaining proof gaps: the full repository suite was not run; tests use synthetic fixtures, with no real-world PE artifact or external provider verification. 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 Current merge with main (`960b74258`), Node.js 24.22.1: - `npm run test:local -- src/native/pe/PeCertificateParser.test.ts src/domain/native/peCertificates.test.ts src/application/binaryDiagnostics/PeCertificatesService.test.ts` — 38 passed. - `npm run test:focused -- tests/acceptance/peCertificatesCli.test.ts tests/boundary/mcp/peCertificatesMcp.test.ts tests/boundary/filesystem/peCertificates.test.ts tests/boundary/filesystem/referenceSourceQueuedDirectories.test.ts` — 10 passed after updating the two stale assertions noted in review. - `npm run check:fast` — typecheck and lint passed. - CI follow-up `f538cd2aa`: formatted six files inherited from main after CI reported `format:check` failures. `npm run check:ci:fast` — passed locally, including formatting, typecheck, compilation, lint and module boundaries. - `git diff --check` — passed. - Synthetic parser coverage includes all gaps 0–7 for PE32/PE32+ and consecutive counted-padding entries. Compiled CLI/MCP tests use synthetic artifacts; counted-padding regressions run at the parser boundary. - Full repository suite, real-world signed artifacts, cryptographic validity, and fresh performance measurements were not run. Earlier timing claims have been removed because the implementation now uses main’s decoded-root interface. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not applicable. - Package or release metadata impact: none. - Security, privacy, process, or containment review: static inspection only; no target execution or artifact modification. Signature validity is not established. ## Author readiness Human owner: @Deepak-Kambala - [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. - [ ] 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-01 21:47:34 +00:00
Sign in to join this conversation.
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#7398
No description provided.