Repository navigation
feat(macho): report per-slice encryption status in inspect_macho - #5359
Merged
Merged
Conversation
…d readers Split readMachoImage into readMachoContainer, which classifies a file as thin, FAT or not Mach-O and keeps FAT range issues per slice, and readSliceLoadCommands, which reads one slice header and frames its load commands, returning every command framed before a failure. readMachoImage keeps its strict policy on top of them: commands framed before a framing defect still decode first, an unsupported FAT slice still yields to a later malformed one, and the first FAT range issue is still reported. A characterization test pins that precedence for inputs with several defects; it passes against both the previous and the new implementation. Part of morluto#4306.
inspect_macho now reports the LC_ENCRYPTION_INFO(_64) state of every slice in a new `encryption` collection, read in process from load commands without reading code bytes. - Each slice is encrypted, not-encrypted, absent, malformed or unknown. Absent requires a completed load-command walk; malformed and unknown carry a reason and keep commands read before the failure. - The named range is end-exclusive in slice and input-file coordinates and maps to file-backed segment fragments, VM addresses and the overlapping nonempty sections. - A ciphertext range adds a limitation naming its input-file bytes. - The in-process read and the native tools are bracketed by one regular file descriptor and stat identity checks; a change reports artifact_changed. A missing or inaccessible target keeps the tool facts and reports encryption as non-exhaustive, never absent. - Retained Evidence recorded before this field is parsed by the owning schema and reports encryption as unknown in the macho view and in managed/native verification. Fixes morluto#4306
chordpli
marked this pull request as ready for review
October 20, 2026 05:53
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Contributor
 [Medium impact] The PR appears safe to merge; the latest change correctly selects the existing macOS test job. SummaryAdds per-slice encryption facts to
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart LR
A["Bound target file"] --> B["Shared container and command reader"]
B --> C["Per-slice encryption facts"]
C --> D["inspect_macho report"]
D --> E["CLI and MCP Evidence"]
E --> F["Summary and paged encryption view"]
Reviews (24) · Last reviewed commit: "fix(ci): own the Mach-O encryption test ..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…e errors - Frame load commands through a visitor instead of retaining every command, restoring the pre-refactor memory profile (2M eight-byte commands: peak heap 370 MiB -> 0.9 MiB). - Confirm a large FAT container from a valid slice's Mach-O header so a sibling's range fault stays per slice instead of hiding healthy slices. - Name only sections whose bytes the range and their segment both cover. - Throw inspection failures inside the owned descriptor callback so a close failure keeps them as its primary error.
…-info # Conflicts: # src/artifacts/apple/MachoLoadCommandReader.ts
This was referenced Oct 20, 2026
…-info # Conflicts: # src/artifacts/apple/MachoLoadCommandReader.ts # src/domain/apple/machoContainer.ts # src/native/NativeMacOSProvider.ts # src/native/NativeMachoInspection.ts
2 tasks done
Contributor
Author
|
@morluto I merged current Conflict resolution
Test changes
Validation at
E2E: built CLI, isolated environment (macOS 26.5.2 arm64, Apple clang 21.0.0)
Not rerun at this commit: full |
…-info # Conflicts: # src/native/NativeMacOSProvider.ts
…-info # Conflicts: # src/domain/managed/managedNativeVerificationMatch.ts
2 tasks done
mapRange gave every file-backed segment fragment VM addresses, even for a segment with no VM size, whose file bytes dyld never maps, and past vmsize when a segment declared more file bytes than memory. Read vmsize with the segment. A segment with no VM size keeps its fragment with null VM addresses. A file size beyond the VM size, or a VM range past the address space, makes the mapping unknown with a reason while the slice keeps its encryption status. The docs also claimed the encryption read never reads code bytes. Like every Mach-O container read, a 0xcafebabe input of at most 16 MiB is read whole to rule out a JVM class file; say so.
Main's withArtifactRegularFile now hands callers the admitted handle and its initial stats. Use those facts instead of re-stating the handle, and pass the legacy Evidence payload straight to createEvidence.
Contributor
Author
|
I merged current What broke
Validation at
E2E: built CLI
E2E: real stdio MCP server on Not rerun at this commit: full |
The file owner keeps the completed callback value when the descriptor close fails or cancellation arrives during close. inspect_macho now validates that value and retains the report as the error's partial observation, as inspect_signature does, instead of returning only the cleanup or cancellation error.
# Conflicts: # src/artifacts/apple/MachoLoadCommandReader.ts # src/domain/apple/machoContainer.ts
# Conflicts: # src/artifacts/apple/MachoLoadCommandReader.ts # src/domain/analysisView/machoView.ts # src/domain/apple/machoContainer.ts # src/domain/managed/managedNativeVerificationSource.ts # src/native/NativeMachoInspection.ts
# Conflicts: # src/artifacts/apple/MachoLoadCommandReader.ts
main's new dyld refusal cases build NativeMachoInspectionContext without the encryption reader this branch requires.
main's facet-ownership case builds inspect_macho Evidence without the encryption coverage this branch requires; the fixture now carries one slice row and the loop checks the encryption page like the other facets.
3 tasks done
A slice retains at most two encryption load commands, since a second one already makes it malformed; the reason keeps the full count. Range mapping in every slice shares one budget of segment and section records, charged from each segment command's capacity before it is decoded. A slice past the budget keeps its encryption status and reports an unknown mapping.
Segment commands are collected during framing only while their mapping records still fit in the budget shared by all slices, and the budget is charged only when the slice maps its range.
Collaborator
|
Final CI audit for
The operator explicitly authorized bypass for demonstrated main failures. The bypass covers that baseline assertion and its dependent/cancelled CI outcomes, with no known PR bug left unresolved. Original contributor readiness declarations are preserved unchanged; no new human-review attestation was made. |
3 tasks done
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
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.
Summary
Fixes #4306.
inspect_machonow reports the declaredLC_ENCRYPTION_INFO(_64)state of every slice, so analysts can distinguish a recorded ciphertext range from program bytes before interpreting disassembly. Output includes observed command fields, CPU identity, slice/input offsets, segment/section overlaps and VM mappings. CLI and MCP share the same provider/application facts;inspect_analysis_viewexposes the pagedmachoencryption facet with retained Evidence.This is static metadata inspection within the existing native provider. Status derives only from load commands: no entropy guess, payload-encryption test, target execution, decryption or mutation. Artifact inventory's separate container-encryption meaning remains intact.
absentrequires a complete walk; malformed/unknown states preserve their reason and earlier commands. Older retained Evidence remains readable.Implementation and continuation
The contributor's history is preserved. Common container/slice framing owns identity and load-command parsing; the strict existing reader retains its defect precedence. Only encryption/segment commands are retained. Mapping uses the existing shared record allowance and checks the framing bound before retaining segment commands.
Review found that a failed early mapping still spent its entire reservation, dropping a healthy later slice's VM/section facts. The producing boundary now commits the reservation only after mapping succeeds. Four tiny double-slice regressions reproduce the original failure for missing section bytes, VM-size mismatch, address-space overflow and command-width mismatch; all pass after the fix. The existing successful-mapping test still proves that retained results consume the shared budget. No downstream repair or parallel interpretation was added.
The bound descriptor and target SHA remain checked around native tools. Cleanup retains the exact owner and primary failure. Native provenance verification now requires complete per-slice absence through real CLI/MCP, and the owned encryptable-image CLI test is included in the macOS CI lane. This tests declared
cryptid=1metadata; it is not a genuine FairPlay sample.Executed validation
Continuation on Linux, Node 24.22.1 /npm 11.16.0. Runtime commit
26280c746; head0eba2e7faadditionally merges maind3ecf6800(website-only changes) and clarifies documentation/comments.check:fast, formatting of the changed paths, docs (9 README tests), MCP catalog and discovery (2,834 test files exactly once) pass. Docs reran after the website-only main merge and final documentation changes.git diff --checkpasses.c2c2e93fc; it is attributed rather than claimed as rerun here. Real Apple CI now passes: 402 tests pass /5 skip in 29 files, plus all artifact-format/metadata verifiers. Its real clang/lipo/otool/codesign encryption CLI test passes (5.980 s including fixture preparation); the actual stdio CLI/MCP provenance verifier confirms complete slice absence and unchanged target bytes. The separate macOS-only summary fixture is not selected in this lane. No genuine FairPlay-encrypted input has been verified.Final CI 40851338708, merge
d60f8bc/maind3ecf6800: source tests 21,846 pass /16 skip in 1,176 files. Static checks, build and docs pass; the first run found an additional CI integration omission: the new Apple test path was in the workflow but absent from its classifier owner. Final head5ffb40d26adds that Apple scope entry; all 310 scope/workflow tests and type/lint pass. Runtime code is unchanged. This keeps future test-only edits from silently skipping real-host verification.Two other failures in that run are baseline text assertions:
bundleComparisonCli.test.ts:223andflutterMcp.test.ts:259still expect legacy colon-delimited errors. A clean worktree on pure maind3ecf6800reproduces both identical failures (17 pass /2 fail,npm run test:focused -- tests/boundary/cli/bundleComparisonCli.test.ts tests/boundary/mcp/flutterMcp.test.ts). These unrelated assertions were not changed. Matrix fail-fast and the superseding push cancel unfinished jobs; those jobs are not counted as passing. The final run passes shard 7; shard 3 fails only the unchangedbundleComparisonMcp.test.ts:168legacy message assertion. Pure maind3ecf6800reproduces this too (8 pass /1 identical failure). The other six shards are matrix-cancelled and unverified in this run, not reported as passing. Final-head Greptile gives 5/5 with no unresolved threads. Platform results are available in the linked run. The operator explicitly permits bypass for demonstrated main failures; the PR-owned scope omission has been fixed and locally validated separately.Completed parser-task measurements
Before
9bd7dddcdis the original contribution integrated on the same main base; after is runtime26280c746. Two alternating/reversed repetitions, fresh Node processes, warm OS caches. Each first-run task answers for all three real files; the repeated task answers the same three questions six times in one process, including the first pass. Every read uses a fresh descriptor, verifies unchanged file metadata and closes it. No new cache is retained.All answer hashes and read counts match: 15 reads /9,560,612 bytes per three-file task. The shared JVM discriminator reads small
CAFEBABEcontainers whole; thin images read their structural metadata. Framing/read costs remain bounded; no speedup is claimed from these short runs.These measure the shared reader task with a 4,096-record allowance, not whole CLI/MCP agent tasks. Native commands, MCP discovery/schemas/Evidence wrappers and true cold filesystem caches are unmeasured locally. Tokenx 1.3.0 estimates serialized requests/results, not billed tokens. RSS is a 10 ms process-group sample and may miss brief peaks; CPU includes the complete Node task and excludes the sampler. A 256 MiB Node heap /512 MiB family /60 s guard bounds each run. All eight runs complete without a guard stop or survivor; downloaded inputs and measurement artifacts remain outside Git.
Author readiness
Human owner: @chordpli
Before merge
type(scope): outcome.