fix(control): verify Windows control anchor owner (D062 F1) - #85
fix(control): verify Windows control anchor owner (D062 F1)#85LogicDuke wants to merge 7 commits into
Conversation
Decision 062 Amendment A (PR #84 F1). The control-anchor gate verified the DACL but not the OWNER SID, so a foreign owner could rewrite the DACL, grant itself access, and read the descriptor token. verifyControlAnchor now also requires the anchor OWNER SID to equal the exact runtime operator SID (SYSTEM is allowed as a DACL principal, never as owner), fail-closed. The owner SID is read by a single source-in-repo native helper built from reviewed C by a trusted Windows build; its identity and SHA-256 are generated build metadata (a built JS artifact, not a committed literal or .sha256 sidecar) and the helper bytes are hash-verified before it is executed via the existing bounded, shell-free runner. This adds one read-only executable (whoami, icacls, owner helper) and no more. F2 (control-runtime.ts) and F3 (icacls parser) are unchanged. Full suite 2010/2010; typecheck/lint/build/diff-check clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GC2pVoLmjUX5FPLq4urhJT
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The real Windows owner-helper positive test assumed mkdtempSync(tmpdir()) yields an operator-owned directory (owner SID == whoami SID). That holds in a non-elevated context but not on GitHub's elevated Windows runner, where a freshly created directory is owned by Administrators (or SYSTEM) — a non-operator SID — so production correctly fails closed and the ok:true assertion breaks. The test now reads the temp directory's ACTUAL owner SID via the same real, build-provenanced helper binary, then asserts the corresponding deterministic gate behaviour: owner == operator must accept and echo that SID; a non-operator owner must fail closed with the exact reason (OWNER_IS_SYSTEM for SYSTEM, OWNER_MISMATCH otherwise). Meaningful in both elevated and non-elevated contexts, and never treats a foreign owner as success. Test-only. Production (ownerSid === operatorSid gate) unchanged; F1/F2/F3 and the six protected files byte-identical. Full suite 2010/2010; typecheck/lint/build/diff-check clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PPAMmQpdQbunE8kGbezgih
…context test(control): make owner helper integration context-aware
The control runtime unlinked the fixed runtime-descriptor.json unconditionally in close() and in the listen-failure cleanup. Under an overlapped start (runtime B rotates and publishes its descriptor before runtime A closes), A's cleanup deleted B's descriptor, leaving B's live pipe undiscoverable by the official CLI. Availability/correctness only; no authority or token exposure. Cleanup is now bound to the runtime instance identity: the descriptor is read and validated through the existing trusted parser and unlinked only on an exact pipeName match (the per-process 128-bit-random identity; a pid can be reused, a pipeName cannot). A missing, malformed, unreadable, or successor-owned descriptor is left untouched, so cleanup fails safe for the successor in both lifecycle locations. The startup rotation of a stale crash descriptor is intentionally unchanged. Read-compare-unlink is not atomic; the residual race narrows from the successor's whole lifetime to the sub-millisecond match-to-unlink gap and is not closable with JS fs primitives. Adds deterministic adversarial tests: A/B overlap survival, missing, malformed, and unreadable descriptors, same-pid foreign descriptor, and listen-failure protection of a foreign descriptor alongside own-cleanup. F1/F3 and the protected files byte-identical. tests/control 126/126 (real owner-helper binary included), full suite 2017/2017; typecheck/lint/build/diff-check clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ZN6c4sZ6k4RSNigL5CWxG
…ship fix(control): preserve successor runtime descriptor
Decision 062 Amendment B (PR #85 F3): replace the localized icacls DACL read with a single build-provenanced native OWNER + DACL security-descriptor snapshot emitted as canonical SIDs only, so control-anchor authorization is identical on any Windows locale. SYSTEM is recognized solely as S-1-5-18 and the operator by exact canonical SID; a display name can never enter the decision. F1 owner-only mode and its provenance/hash gate are preserved; F2 is untouched. The deferred P3 (operator effective-rights type/mask semantics) is intentionally out of scope and not repaired here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KE5bkGWp2jPzaJaLvFhPPu
fix(control): make anchor ACL verification locale-independent
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aaa7ddc4b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "cockpit": "npm run build && node dist/cockpit-host/server.js", | ||
| "cockpit:live": "npm run build && node dist/runtime/live-cockpit.js", | ||
| "control": "npm run build && node dist/control/cli.js", | ||
| "helper:build": "node tools/control-owner/build.mjs", |
There was a problem hiding this comment.
On a clean Windows checkout, both npm run cockpit:live and npm run control execute only the TypeScript build, while the newly mandatory executable and owner-helper-provenance.js are produced only by this separate script. Consequently, defaultLoadProvenance() returns null and every normal production invocation disables the control channel with HELPER_PROVENANCE_MISSING unless the operator knows to run an undocumented extra build requiring MSVC first. Integrate the helper into the Windows production build/launch or package its generated artifacts.
Useful? React with 👍 / 👎.
1 participant
Isolated F1 repair for PR #84 (Decision 062 Amendment A): the control-anchor gate verified the DACL but not the owner SID, so a foreign owner could rewrite the DACL, grant itself access, and read the descriptor token.
verifyControlAnchornow also requires the anchor OWNER SID to equal the exact runtime operator SID (SYSTEM allowed as a DACL principal, never as owner), fail-closed — the owner SID is read by a build-provenanced, hash-verified native helper run through the existing bounded, shell-free runner.Identity
756fb5ecba5ab6140ba6acd51e6f3f48eae16b262b00c91c5217b62d2e238fa28018d43969284694fix(control): verify Windows control anchor owner)13132c0686e167601e96695de4a3f64c10e3fc69a0aefc7c525592a7badb275d(44183 bytes)feature/d062-post-start-control-channel(PR D62 — Authenticated post-start Autoflow control channel #84's current head)Scope guarantees
src/control/control-runtime.tsbyte-identical (0c1ab700…182); the older-runtime-close defect is out of scope here.icaclsparser is unchanged; owner query and DACL query remain separate/non-atomic (TOCTOU preserved, not absorbed).Governance
2b00c91c5217b62d2e238fa28018d43969284694.🤖 Generated with Claude Code
https://claude.ai/code/session_01GC2pVoLmjUX5FPLq4urhJT