feat(windows-sandbox): place sandboxed children on a private desktop - #3174
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review. 📝 WalkthroughProblem solvedThe PR places Windows AppContainer children on a per-launch alternate desktop on the existing window station. Production launches and readiness probes create the desktop with a protected DACL. They assign it through The guarantee covers initial desktop placement and DACL protection. It does not provide escape-proof confinement. Clipboard isolation, dedicated window-station separation, token and Win32k escape prevention, and verified Low-IL desktop rights remain deferred. The PR also adds a production-identity readiness probe. The probe verifies the AppContainer SID, Job membership, timeout handling, cleanup, and repeatable execution. Runtime availability now depends on a successful readiness probe instead of launcher file presence alone. Source of truth and solution scopeThe PR extends the existing Windows launcher and sandbox manager. It does not create a parallel launch path. The readiness probe reuses the existing production identity, Job setup, launch protocol, and desktop-placement logic. The runtime uses the launcher probe before it reports Windows sandbox availability. The manifest digest remains unchanged. This is the smallest coherent solution for the stated initial-placement guarantee. Desktop creation, protected DACL configuration, desktop assignment, verification, and fail-closed handling are required to prevent fallback to the interactive desktop. The readiness cache adds required lifecycle handling, bounded negative caching, and synchronous availability checks. Deletion or simplification opportunitiesNo safe deletion is evident from the supplied change summary. The Rust tests cover desktop security helpers, AppContainer permissions, desktop-name validation, JSON escaping, readiness profile naming, mutex naming, and request validation. Removing them would weaken security and regression coverage. The runtime tests cover probe success, failures, timeouts, memoization, negative-result expiry, permanent positive caching, cache-only reads, and missing launchers. Removing these cases would weaken readiness-cache coverage. The smoke tests and workflow step validate the production-identity path, argument validation, denied network access, and non-default desktop placement. They should remain unless equivalent Windows CI coverage exists elsewhere. Validation
Required-check status is unverified from the supplied evidence. Complexity delta
Total maintenance complexity increases. The increase is justified by stronger launch-boundary enforcement and deterministic, fail-closed readiness checks. The implementation also removes file-presence-only readiness. Optional follow-up items remain separate and are not implementation requirements for this PR. Review-relevant risks
The person performing the merge reviews the final diff. A maintainer makes the final determination. WalkthroughThe launcher adds production-identity readiness checks and private desktop enforcement. Runtime availability uses cached readiness results. Smoke tests, unit tests, CI, and RFC documentation cover the preview behavior and deferred capabilities. ChangesWindows sandbox readiness
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The PR adds private-desktop confinement, but the current implementation still has an unsafe unaligned read in the AppContainer readiness path and can block availability checks for up to 15 seconds while permanently caching transient failures, so these issues should be fixed or explicitly accepted before merge. Sequence Diagram(s)sequenceDiagram
participant Runtime as default-sandbox-manager
participant Launcher as Windows launcher
participant Sandbox as AppContainer child
Runtime->>Launcher: Run --readiness-probe
Launcher->>Sandbox: Start with exact AppContainer SID, Job, and private desktop
Sandbox-->>Launcher: Return exit status and boundary state
Launcher-->>Runtime: Report clean status or failure
Runtime-->>Runtime: Cache readiness by launcher path
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoConfine Windows AppContainer children to per-launch private desktops
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Request changes on the current head c999eefd4.
The alternate-desktop implementation is a useful partial hardening step, but the security guarantee currently documented by this PR is stronger than the mechanism:
CreateDesktopW creates the new desktop on the launcher's current window station, and the RFC explicitly defers a dedicated window station. On Windows, windows/messages/hooks are desktop-scoped, but the clipboard belongs to the window station. Therefore this change can isolate the child from Default windows and desktop hooks, but it does not by itself establish the claimed clipboard isolation while both desktops remain in the same window station.
The current desktopIsolated attestation only proves that the desktop name is non-empty and not Default; the smoke additionally proves the generated prefix. Neither test attempts OpenClipboard/GetClipboardData, proves a different window station, or proves that the child cannot reopen Winsta0\\Default.
Please either:
- narrow the RFC/code comments and PR claim to the desktop-scoped guarantees actually enforced here, with clipboard isolation remaining deferred; or
- create a dedicated window station plus desktop under one RAII lifecycle, pass the full
winsta\\desktopname, and add negative tests for clipboard access, opening/switching toDefault, child escape, and residual object cleanup.
This PR is stacked on #3161, whose readiness ownership and failure-cleanup blockers also need to be resolved before this can merge.
M4n5ter
left a comment
M4n5ter
left a comment
There was a problem hiding this comment.
English
Follow-up review on current head c999eefd4.
The previously reported window-station/clipboard blocker remains and is not duplicated here. One additional blocker was found: the new desktop receives a DACL but no explicit Low mandatory integrity label. Since AppContainer processes run at Low IL, the current probes prove only desktop assignment and name visibility—not that the advertised create-window/write rights are usable by production children.
Please add and verify the appropriate integrity label, with a child-side window-creation test, or explicitly narrow the backend to a no-Win32k contract.
The commit also discloses material Claude authorship but contains only Co-Authored-By; project policy requires a Generated-by: <tool> trailer on every materially AI-authored commit.
All current GitHub checks pass, including 41 Rust tests and the Windows smoke, but those tests do not exercise clipboard access, reopening Winsta0\Default, desktop write/create access, or concurrent desktop-heap pressure.
简体中文
对当前 head c999eefd4 的补充审查。
先前指出的 window-station/clipboard blocker 仍然成立，此处不重复评论。另发现一个 blocker：新 desktop 配置了 DACL，却没有显式设置 Low mandatory integrity label。AppContainer 进程运行在 Low IL，因此当前 probe 只证明了 child 被分配到指定名称的 desktop，不能证明生产 child 实际拥有所声明的 create-window/write 权限。
请设置并验证正确的 integrity label，同时增加 child-side window-creation 测试；或者明确把 backend 收窄为禁止 Win32k 的合同。
此外，该提交披露了 Claude 的实质性参与，但只有 Co-Authored-By。项目政策要求每个包含实质性 AI 内容的提交带有 Generated-by: <tool> trailer。
当前 GitHub checks 全部通过，包括 41 个 Rust 测试和 Windows smoke；但这些测试没有覆盖 clipboard、重新打开 Winsta0\Default、desktop write/create 权限以及并发 desktop-heap 压力。
The Windows AppContainer backend advertised availability from file existence alone (`existsSync(clientPath)`), which never proves the host can actually stand up the sandbox identity. Replace that with a real readiness probe (RFC §6.4) and align the RFC to the shipped behavior. - launcher: add `--readiness-probe`, which creates the real AppContainer identity/token and a kill-on-close Job and launches a throwaway confined child, failing closed if the host cannot create or enforce the boundary. - runtime: back the Windows backend's `isAvailable` with a memoized spawnSync of `--readiness-probe`, so `auto`/`require` fail closed on hosts where the OS cannot create the boundary rather than trusting the packaged binary's presence. - CI: add readiness-probe-smoke.ps1 to the W0 lane (exit 0, repeatable, argument-rejecting). - docs: align RFC §6.4/§6.5 (EN + zh-CN) — mark the readiness probe as implemented and enforced, and keep private desktop, full per-profile policy at readiness, and launcher signature/version as later gates. Advances apache#2142 Phase 4 (align the RFC's guarantees with the shipped slice). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
…entity spoofing Address adversarial review on the production-identity readiness probe: - Make the throwaway AppContainer profile name unique per invocation with a monotonic nonce (SystemTime nanos + PID) so a recycled PID can never collide with a still-registered profile and deadlock the probe (finding A). - Prove enforcement, not mere presence: verify the confined child runs under the SPECIFIC requested Job (IsProcessInJob against our handle) and carries the EXACT requested AppContainer SID (TokenAppContainerSid + EqualSid), failing closed otherwise. "some job / some AppContainer" is not evidence the boundary we asked for is real (finding B). - Cache Windows readiness at module scope keyed by client path so backends sharing a launcher share one probe result; distinct paths stay independent (finding D). - Sync RFC (EN + zh) Updated: date to 2026-08-17 (finding C). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
…eadiness probe deterministically Address maintainer review on the readiness-probe PR: - Availability: make `isBuiltinFilesystemWorkerSandboxAvailable(win32)` consume the memoized readiness probe instead of treating launcher-file presence as a second authority. Runtime Host composition calls this at startup, warming the readiness cache so the backend's later synchronous `isAvailable()` on the transform hot path hits the cache rather than spawning on the event loop. Export `probeWindowsReadiness` with an injectable spawn seam and cover it with unit tests (clean/non-zero/spawn-error/external-timeout/memoized/missing). - Settlement: route the readiness child's post-resume outcomes (verification failure, timeout, clean exit) through explicit TerminateProcess plus the existing `terminate_and_drain_job` primitive, so a child found outside the Job is settled rather than relying on Job close as the only backstop. - Cleanup: give the readiness AppContainer profile one fixed, self-reconciling name that best-effort-deletes any leftover before create, so a profile leaked by an externally-killed probe is reclaimed by the next probe. Replaces the PID+nonce scheme. - Docs: mark the structured unavailable-reason / diagnostics surface as a deferred later gate in both RFC language variants (§6.4/§6.5). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
Non-interactive AppContainer workers previously inherited the creator's interactive Winsta0\Default desktop, leaving the same-session GUI attack surface (window-message shatter into the user's interactive windows, desktop hooks) open — AppContainer confines files/network/token but not the desktop scope. Per RFC v1 §6.3 the private-desktop promise was designed but deferred. Enforce the desktop-scoped half under an explicit no-Win32k contract: each AppContainer launch (production create + readiness probe) now stands up a per-launch alternate desktop on the existing window station, with a protected DACL that grants only the launching user, Local System, and the child's AppContainer SID the minimal DESKTOP_* rights (create window/menu, read/write objects, enumerate) and never DESKTOP_SWITCHDESKTOP / HOOKCONTROL / journal. STARTUPINFOEXW.lpDesktop points the child at it. If the desktop cannot be created or the DACL cannot be granted (CreateProcessW then fails ACCESS_DENIED), the launch fails closed — auto/require never fall back to the host desktop. Scope, stated honestly (per maintainer review on apache#3174): - This keeps the worker off the interactive Default desktop, so it cannot enumerate/post messages to the user's interactive windows or install desktop hooks against them. - It does NOT isolate the clipboard: the clipboard belongs to the window station, which both desktops still share. Clipboard isolation with a dedicated window station is a later hardening gate (§6.5). - The create-window/write DACL rights are granted but not relied upon; the worker does no GUI work (no-Win32k contract). A verified Low mandatory- integrity label proving those rights are usable at AppContainer's Low IL is deferred (§6.5). The boundary probe self-attests its own desktop name and refuses to run on Default; appcontainer-smoke asserts the private-desktop name prefix and rejects Default. Digest is unchanged: the desktop is a launch-time detail and does not enter the manifest. Stacked on apache#3161 (readiness probe). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
c999eef to
0df8547
Compare
|
Thanks both — pushed @hqhq1025 — I took option (a): narrow the claim to the desktop-scoped guarantees actually enforced, with clipboard deferred.
@M4n5ter — the DACL-vs-Low-IL / integrity-label finding: I took your "explicitly narrow the backend to a no-Win32k contract" option.
Both the readiness-ownership and failure-cleanup blockers you referenced on #3161 are addressed in 简体中文感谢两位 —— 已推 @hqhq1025 —— 采用 option (a):把声明收窄到实际强制的 desktop 作用域,clipboard 延后。
@M4n5ter —— DACL vs Low-IL / integrity-label 那条:采用你的"明确把 backend 收窄为 no-Win32k 契约"选项。
你在 #3161 提到的 readiness-ownership 与 failure-cleanup blocker,已在那边 |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Follow-up review on current head 0df8547d7.
The private-desktop-specific concerns from my previous review are resolved at the contract level:
- the RFC and code comments now explicitly state that the alternate desktop does not isolate the window-station clipboard;
- dedicated window-station/clipboard isolation is correctly deferred;
- Low-IL create-window/write usability is no longer claimed, and the shipped behavior is narrowed to a no-Win32k confinement contract;
- the AI attribution trailers are present.
I would accept this PR's private-desktop delta on that narrowed scope. I cannot approve the stacked revision yet because it includes #3161 head 4506446f2, whose fixed readiness-profile implementation still has blocking namespace, cross-process ownership, unsettled-cleanup, and permanent-negative-cache issues. See my follow-up review on #3161 for the exact failure paths.
Once #3161 lands with those lifecycle issues resolved and this stack is rebased onto that accepted head, I will re-check the final desktop delta and CI. The current #3174 Windows W0 check is green; some package/E2E checks are still running at review time.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/architecture/windows-sandbox-rfc-v1.zh-CN.md (1)
128-172: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win中文版 §6.5 结尾缺少英文版的最后一句，且新增段落混用半角逗号。
两处与英文版不一致：
- 第 172 行只保留了「暂缓收窄的是 readiness 丰富度……绝不回退到宿主执行」。英文版
docs/architecture/windows-sandbox-rfc-v1.md第 252-254 行还有一句：cancellation、parent-death、concurrency、process-drain 与残留 ACL/state 释放的生命周期证据由 W1（§9）与 Phase 4（#2142）跟踪，属于发布证据而非假设。中文读者会漏掉这条发布 gate。- 新增文本在中文句中使用半角逗号
,（如第 128、137、140-141、143、158-159、164-166、168-170 行），与文档其余部分的全角，不一致。处置：第 1 点 fix-now（翻译内容缺失会改变含义）；第 2 点 optional。
🧹 Nitpick comments (9)
experiments/windows-sandbox/launcher/src/windows_launcher.rs (2)
471-507: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: consolidate the two
CreateAppContainerProfilecall sites.
create_readinessrepeats the full creation sequence fromcreate. Only the display strings, the pre-delete reclaim, and the error prefix differ. Two call sites now own the profile-creation contract, so a later change to that contract can drift.Disposition: optional. There is no correctness defect today.
♻️ Suggested shape
impl AppContainerProfile { unsafe fn create_named( name: Vec<u16>, display_name: &str, description: &str, label: &str, ) -> Result<Self, String> { let display_name = wide(display_name); let description = wide(description); let mut sid = null_mut(); let result = unsafe { CreateAppContainerProfile( name.as_ptr(), display_name.as_ptr(), description.as_ptr(), null(), 0, &mut sid, ) }; if result < 0 { return Err(format!( "CreateAppContainerProfile{label} failed closed: HRESULT 0x{:08x}", result as u32 )); } Ok(Self { sid, name }) } }
createcalls it with"";create_readinesscallsDeleteAppContainerProfilefirst and passes"(readiness)".Source: Path instructions
596-645: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDerive the desktop name from
request_id.
CreateDesktopWopens an existing desktop and ignores the supplied security descriptor. A clock rollback or coarse clock resolution can reuse this name while the old desktop remains referenced. The AppContainer child can then fail withACCESS_DENIEDbecause its SID is absent from the existing DACL. Hashrequest_idfor the desktop name, as for the AppContainer profile.experiments/windows-sandbox/launcher/src/main.rs (2)
286-298: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: make the child-side predicate the single authority on desktop identity.
desktop_is_isolatedrejects only an empty name andDefault.experiments/windows-sandbox/appcontainer-smoke.ps1Lines 84 and 88 assert the stronger property, that the name starts withmaka-sandbox-desktop.. Two places now define what an isolated desktop is, and the weaker definition is the one the launcher enforces.If the child must always land on a launcher-created desktop, assert the prefix here and let the smoke script rely on
desktopIsolated.Disposition: optional.
♻️ Suggested change
-/// A confined worker must run on a private desktop, never the shared -/// interactive `Default` desktop, and never an empty/unnamed one. -fn desktop_is_isolated(name: &str) -> bool { - !name.is_empty() && !name.eq_ignore_ascii_case("Default") -} +/// A confined worker must run on a desktop this launcher created, never the +/// shared interactive `Default` desktop, and never an empty/unnamed one. +fn desktop_is_isolated(name: &str) -> bool { + name.starts_with("maka-sandbox-desktop.") +}The desktop-name prefix is produced by
create_confined_desktopinexperiments/windows-sandbox/launcher/src/windows_launcher.rsLines 603-606, so extract it into one shared constant if you take this.Source: Path instructions
300-319: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
serde_jsoninstead of a hand-written JSON escaper.
main.rsalready depends onserde_json(seeserde_json::from_strat Line 137 andserde_json::to_vecat Line 385).serde_json::to_string(&desktop)produces the same quoted, escaped string for any&str. That deletes this function and thedesktop_name_is_json_escapedtest inexperiments/windows-sandbox/launcher/src/windows_launcher_desktop_tests.rs.Disposition: optional. The current implementation is correct for the desktop-name input.
♻️ Suggested change
- "{{...,\"desktop\":{desktop_json},\"desktopIsolated\":{desktop_isolated}}}", - desktop_json = json_string(&desktop) + "{{...,\"desktop\":{desktop_json},\"desktopIsolated\":{desktop_isolated}}}", + desktop_json = serde_json::to_string(&desktop).map_err(|error| error.to_string())?Source: Path instructions
experiments/windows-sandbox/launcher/src/windows_launcher_tests.rs (1)
102-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test does not protect the behavior its comment describes.
appcontainer_profile_nameis a pure SHA-256 of its input.assert_eq!(first, second)therefore asserts that a pure function is deterministic, and theassert_ne!against"request-one"asserts SHA-256 collision resistance. Neither statement can fail while the function is a hash of its argument.The invariant the comment states — a leaked readiness profile is reclaimed rather than accumulated — lives in
AppContainerProfile::create_readiness, which callsDeleteAppContainerProfilebefore creating. This test does not reach that code.The distinctness and name-format properties are already covered by
appcontainer_profile_identity_is_unique_and_bounded_per_requeston Lines 92-100.Disposition: optional. Consider deleting this test, or replacing it with an assertion that the readiness name is stable across builds by pinning the expected literal, which would catch an accidental change to the fixed request id.
Source: Path instructions
experiments/windows-sandbox/appcontainer-smoke.ps1 (1)
87-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis check cannot fire. Delete it.
Line 84 already requires
"desktop":"maka-sandbox-desktop\., so a"desktop":"Default"payload throws at Line 85 before reaching Line 88. The launcher also rejects a non-isolated desktop itself and exits non-zero, which Line 75 catches. Three layers assert the same invariant and this one is unreachable.Disposition: optional.
🧹 Proposed deletion
- # The confined child must never land on the shared interactive desktop. - if ($rendered -match '"desktop":"Default"') { - throw "Confined child ran on the interactive Default desktop: $rendered" - }Source: Path instructions
packages/runtime/src/__tests__/default-sandbox-manager.test.ts (1)
153-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the cache-warming claim across both entry points.
This test proves memoization within
probeWindowsReadiness. The load-bearing claim inpackages/runtime/src/sandbox/default-sandbox-manager.tsLines 134-139 is stronger: a call throughisBuiltinFilesystemWorkerSandboxAvailablewarms the cache so the backend's later synchronousisAvailablenever spawns. No test asserts that the two entry points share one cache entry for the same resolved launcher path.Disposition: optional. It protects the property that keeps the 15-second spawn off the operation path.
experiments/windows-sandbox/readiness-probe-smoke.ps1 (1)
17-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the em dashes to clear the PSScriptAnalyzer BOM warning.
Lines 19 and 20 contain em dash characters, which makes the file non-ASCII. PSScriptAnalyzer reports
PSUseBOMForUnicodeEncodedFilefor it. Windows PowerShell 5.1 reads a BOM-less UTF-8 file as ANSI and mangles those characters. The CI step usesshell: pwsh, so CI is unaffected, and the characters appear only in comments.Either use ASCII hyphens or save the file with a UTF-8 BOM.
Disposition: optional.
Source: Linters/SAST tools
packages/runtime/src/sandbox/default-sandbox-manager.ts (1)
74-91: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDo not cache timeout results permanently
probeWindowsReadiness()cachesfalsefor timeouts (status === null). A transient startup timeout disables Windows sandboxing until process restart. Keep permanent caching for missing launchers and non-timeout failures, but retry timeouts with a bounded policy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 99058c3f-425a-491b-b950-2f910b0f32de
📒 Files selected for processing (12)
.github/workflows/windows-sandbox-w0.ymldocs/architecture/windows-sandbox-rfc-v1.mddocs/architecture/windows-sandbox-rfc-v1.zh-CN.mdexperiments/windows-sandbox/appcontainer-smoke.ps1experiments/windows-sandbox/launcher/Cargo.tomlexperiments/windows-sandbox/launcher/src/main.rsexperiments/windows-sandbox/launcher/src/windows_launcher.rsexperiments/windows-sandbox/launcher/src/windows_launcher_desktop_tests.rsexperiments/windows-sandbox/launcher/src/windows_launcher_tests.rsexperiments/windows-sandbox/readiness-probe-smoke.ps1packages/runtime/src/__tests__/default-sandbox-manager.test.tspackages/runtime/src/sandbox/default-sandbox-manager.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.
Address four readiness-probe lifecycle blockers on the availability path: 1. Reserve the `readiness-probe` request_id in protocol and reject it in LaunchRequest::validate; derive the probe profile under a disjoint `maka.readiness.` namespace so no production launch can resolve to the profile the probe deletes and recreates. 2. Serialize the whole delete->create->probe->settle->drop window across processes with the DACL-hardened named mutex the ACL ledger uses (LedgerLock), scoped by SID under Global\Maka.WindowsSandbox.ReadinessProfile.v1, failing closed on timeout. 3. Preserve the AppContainer registration on drop when the Job could not be proven empty (keep_on_drop + preserve()), mirroring the production Unsettled contract, instead of deleting a possibly-live identity. 4. Cache negative Windows readiness for a bounded TTL (60s) rather than the process lifetime, and make the synchronous isAvailable hot path strictly cache-only (readCachedWindowsReadiness) so it never spawns on the event loop; positive results stay cached permanently. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
Non-interactive AppContainer workers previously inherited the creator's interactive Winsta0\Default desktop, leaving the same-session GUI attack surface (window-message shatter into the user's interactive windows, desktop hooks) open — AppContainer confines files/network/token but not the desktop scope. Per RFC v1 §6.3 the private-desktop promise was designed but deferred. Enforce the desktop-scoped half under an explicit no-Win32k contract: each AppContainer launch (production create + readiness probe) now stands up a per-launch alternate desktop on the existing window station, with a protected DACL that grants only the launching user, Local System, and the child's AppContainer SID the minimal DESKTOP_* rights (create window/menu, read/write objects, enumerate) and never DESKTOP_SWITCHDESKTOP / HOOKCONTROL / journal. STARTUPINFOEXW.lpDesktop points the child at it. If the desktop cannot be created or the DACL cannot be granted (CreateProcessW then fails ACCESS_DENIED), the launch fails closed — auto/require never fall back to the host desktop. Scope, stated honestly (per maintainer review on apache#3174): - This keeps the worker off the interactive Default desktop, so it cannot enumerate/post messages to the user's interactive windows or install desktop hooks against them. - It does NOT isolate the clipboard: the clipboard belongs to the window station, which both desktops still share. Clipboard isolation with a dedicated window station is a later hardening gate (§6.5). - The create-window/write DACL rights are granted but not relied upon; the worker does no GUI work (no-Win32k contract). A verified Low mandatory- integrity label proving those rights are usable at AppContainer's Low IL is deferred (§6.5). The boundary probe self-attests its own desktop name and refuses to run on Default; appcontainer-smoke asserts the private-desktop name prefix and rejects Default. Digest is unchanged: the desktop is a launch-time detail and does not enter the manifest. Stacked on apache#3161 (readiness probe). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
0df8547 to
d861e1e
Compare
|
Rebased onto the updated #3161 head ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
experiments/windows-sandbox/launcher/src/windows_launcher.rs (1)
618-640: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winOne constant serves two different authorities. Split it, and drop the rights the no-Win32k contract does not use.
CONFINED_DESKTOP_APP_MASKis used for two unrelated decisions:
- Line 638: the access mask granted to the AppContainer package SID in the desktop DACL.
- Line 719: the
dwDesiredAccessthe launcher requests for its ownHDESK.The launcher only needs a handle that keeps the desktop alive and can be closed. It does not need
DESKTOP_CREATEWINDOW,DESKTOP_CREATEMENU,DESKTOP_WRITEOBJECTS, orDESKTOP_ENUMERATE. Coupling the two means narrowing the child's grant also narrows the launcher's handle, so neither can be tightened independently.The doc comment at Lines 667-670 states the create-window and write rights are granted but not relied upon, because the worker performs no GUI work. Under that stated contract those grants are unused authority in a security boundary. Granting
DESKTOP_READOBJECTSalone matches the shipped guarantee and removes the ambiguity the past Low-IL review raised.Disposition: follow-up. The current code is coherent, but it grants rights the PR explicitly says it does not depend on.
♻️ Proposed split
-const CONFINED_DESKTOP_APP_MASK: u32 = DESKTOP_READOBJECTS - | DESKTOP_CREATEWINDOW - | DESKTOP_CREATEMENU - | DESKTOP_WRITEOBJECTS - | DESKTOP_ENUMERATE; +/// Rights granted to the confined child. Under the no-Win32k contract the +/// worker performs no GUI work, so only the read right is granted. +const CONFINED_DESKTOP_CHILD_MASK: u32 = DESKTOP_READOBJECTS; + +/// Rights the launcher requests for its own handle: enough to keep the desktop +/// alive and close it. +const CONFINED_DESKTOP_OWNER_MASK: u32 = DESKTOP_READOBJECTS;Then use
CONFINED_DESKTOP_CHILD_MASKindesktop_sddlandCONFINED_DESKTOP_OWNER_MASKin theCreateDesktopWcall.Also applies to: 710-722
Source: Path instructions
experiments/windows-sandbox/launcher/src/acl_ledger.rs (1)
219-230: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe lease kind is encoded twice: in the name generator and in this substring match.
acquireinfers the lease kind fromname.contains(".ReadinessProfile.").readiness_mutex_nameat Lines 54-56 is the source of truth for that substring. If the object name changes, this branch silently falls through to "acquire ACL ledger mutex timed out" and the diagnostic becomes wrong. The compiler cannot catch it.Passing the label explicitly consolidates the authority in one place.
Disposition: optional. The existing
.AclLease.branch has the same shape, so this is not a regression introduced here.♻️ Proposed consolidation
- pub(crate) fn acquire(name: &str, user_sid: &str, timeout_ms: u32) -> Result<Self, String> { - Self::try_acquire(name, user_sid, timeout_ms)?.ok_or_else(|| { - if name.contains(".AclLease.") { - "acquire ACL ledger lease timed out".to_owned() - } else if name.contains(".ReadinessProfile.") { - "acquire readiness profile lease timed out".to_owned() - } else { - "acquire ACL ledger mutex timed out".to_owned() - } - }) + pub(crate) fn acquire( + name: &str, + user_sid: &str, + timeout_ms: u32, + kind: &'static str, + ) -> Result<Self, String> { + Self::try_acquire(name, user_sid, timeout_ms)? + .ok_or_else(|| format!("acquire {kind} timed out")) }Callers then pass
"readiness profile lease","ACL ledger lease", or"ACL ledger mutex".Source: Path instructions
packages/runtime/src/__tests__/default-sandbox-manager.test.ts (1)
211-231: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test duplicates coverage already asserted at Lines 203-207.
The preceding test ends by advancing the clock ten minutes and asserting
callsstays at 2. That proves the spawn is not called at all after a positive result. This test asserts the weaker consequence that a failing spawn is not consulted, which follows from the spawn never running.Deleting it removes one test to maintain without losing an invariant.
Disposition: optional.
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 54c9eb67-1666-41be-aa25-c87d5ceec9f3
📒 Files selected for processing (8)
docs/architecture/windows-sandbox-rfc-v1.mddocs/architecture/windows-sandbox-rfc-v1.zh-CN.mdexperiments/windows-sandbox/launcher/src/acl_ledger.rsexperiments/windows-sandbox/launcher/src/protocol.rsexperiments/windows-sandbox/launcher/src/windows_launcher.rsexperiments/windows-sandbox/launcher/src/windows_launcher_tests.rspackages/runtime/src/__tests__/default-sandbox-manager.test.tspackages/runtime/src/sandbox/default-sandbox-manager.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/architecture/windows-sandbox-rfc-v1.md
- docs/architecture/windows-sandbox-rfc-v1.zh-CN.md
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Follow-up review on current head d861e1ee7.
The earlier clipboard and Low-IL overclaims are fixed, but the private-desktop delta still does not establish confinement from the interactive desktop.
STARTUPINFO.lpDesktop selects the child's initial desktop; it does not prevent code inside that child from calling OpenDesktopW("Default") and moving a thread with no windows/hooks via SetThreadDesktop. This PR creates and protects the private desktop, but it neither structurally denies access to Winsta0\Default nor enables a no-Win32k mitigation. The current smoke proves only the initial desktop name, so desktopIsolated overstates what was verified.
Please make one of the intended boundaries real: preferably enable and validate the no-Win32k mitigation for the Node worker, or use a token/window-station design that prevents reopening Default. Add child-side negative tests for OpenDesktop(Default), SetThreadDesktop, window enumeration, and hooks. Until then this is initial desktop placement, not malicious-code desktop confinement.
This stack also includes #3161, which still has the readiness retry and unsettled-profile lifecycle blockers noted in my latest review there. All current CI is green, including the Windows W0 lane, but it does not cover the escape path above. Not ready to merge.
…to what the code enforces hqhq1025's round-3 review is factually correct on both points; this converges the claims rather than building the heavier machinery, matching the preview's fail-closed scope. Blocker 1 (TTL recovery overclaim): the filesystem worker is published once when a composition is built, so a negative readiness result is not recovered within a running host — the 60s negative TTL only bounds how long a stale negative poisons the module cache so the *next* composition build re-probes. De-claim the RFC (EN+zh §6.4/§6.5) and default-sandbox-manager.ts comments from 're-probes on the next composition rather than disabling until restart' to honest new-composition/restart-scoped recovery, and mark an active running-host readiness retry with dynamic worker publication as a deferred gate. Add a test asserting the hot path never self-recovers a negative; only an explicit re-probe (a new composition build) can. Blocker 2 (ineffective preserve/keep_on_drop): create_readiness deletes the fixed identity unconditionally next cycle, so preserve() only deferred deletion by one probe and never durably quarantined. Remove preserve()/keep_on_drop and the unsettled out-param; the settlement-failure surfacing in the match stays, so an unsettled probe still fails closed (reports unavailable). Document that cleanup relies on the kill-on-close Job's tree termination and a zero-filesystem-root identity, and mark durable quarantine (or unique probe identities plus a reconciliation ledger) as a deferred gate (RFC §6.5). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
M4n5ter
left a comment
M4n5ter
left a comment
There was a problem hiding this comment.
English
Request changes on current head d861e1ee7.
I found one additional blocker not covered by the existing threads: allocating a default-size WinSta0 desktop for every live launch puts ordinary sustained parallel workloads close to the system desktop-heap limit.
I am not repeating hqhq1025’s valid current-head finding that lpDesktop only selects the initial desktop and that no Win32k/UI restriction prevents malicious code from reopening Default.
No other new material correctness or design findings remain after reviewing the private-desktop delta, its composition with stacked #3161, existing discussions, and the simplification surface. The simplest coherent design may be to enforce the stated no-Win32k boundary and remove the per-launch desktop state, subject to Node-worker compatibility evidence.
Verification on d861e1ee7:
- All current GitHub checks are successful, including the Windows W0 lane.
git diff --checkandcargo fmt --all -- --checkpass.- Windows-target production and test code compile with
cargo check. - Desktop-heap exhaustion was established from the documented Windows resource contract and reachable sustained-concurrency path; the current CI does not reproduce it because its ten child processes exit quickly.
简体中文
在当前 head d861e1ee7 上请求修改。
我发现了一个现有讨论尚未覆盖的 blocker：每个存活的 launch 都在 WinSta0 上分配默认大小的 desktop，会让普通的持续并发 workload 很快逼近系统 desktop-heap 上限。
hqhq1025 已经准确指出：lpDesktop 只选择初始 desktop，当前也没有 Win32k/UI restriction 阻止恶意代码重新打开 Default。这里不重复该 finding。
检查 private-desktop 增量、它与 stacked #3161 的组合、全部现有讨论和简化空间后，没有其他新的实质性 correctness/design finding。一个可能更简单且完整的方向是强制落实文档声明的 no-Win32k boundary，并删除按 launch 创建 desktop 的状态；前提是补齐 Node worker 的兼容性证据。
在 d861e1ee7 上的验证：
- 当前所有 GitHub checks 均成功，包括 Windows W0 lane。
git diff --check与cargo fmt --all -- --check通过。- Windows target 的生产代码和测试代码均通过
cargo check。 - Desktop-heap 问题基于微软公开的资源契约和可达的持续并发路径确认；当前 CI 的十个 child 很快退出，因此没有复现持有全部 desktop 时的耗尽场景。
Non-interactive AppContainer workers previously inherited the creator's interactive Winsta0\Default desktop, exposing the same-session GUI attack surface (window-message shatter into the user's interactive windows, desktop hooks) — AppContainer confines files/network/token but not the desktop scope. Per RFC v1 §6.3 the private-desktop promise was designed but deferred. This change enforces initial-desktop *placement* plus DACL protection, not escape-proof confinement. Each AppContainer launch (production create + readiness probe) stands up a per-launch alternate desktop on the existing window station, with a protected DACL that grants only the launching user, Local System, and the child's AppContainer SID the minimal DESKTOP_* rights (create window/menu, read/write objects, enumerate) and never DESKTOP_SWITCHDESKTOP / HOOKCONTROL / journal. STARTUPINFOEXW.lpDesktop points the child at it. If the desktop cannot be created or the DACL cannot be granted (CreateProcessW then fails ACCESS_DENIED), the launch fails closed — auto/require never fall back to the host desktop. Scope, stated honestly (per maintainer review on apache#3174): - lpDesktop selects only the child's *initial* desktop. This places the worker off the interactive Default desktop and DACL-protects the private one, but it does NOT structurally confine: absent a no-Win32k mitigation, a dedicated window station, or a token boundary, in-process code can still OpenDesktopW("Default") + SetThreadDesktop to re-attach. Enforcing a no-Win32k mitigation is a deferred gate (§6.5). - It does NOT isolate the clipboard: the clipboard belongs to the window station, which both desktops still share. Clipboard isolation with a dedicated window station is a later hardening gate (§6.5). - The create-window/write DACL rights are granted but not relied upon; the worker does no GUI work. A verified Low mandatory-integrity label proving those rights are usable at AppContainer's Low IL is deferred (§6.5). The boundary probe self-attests its own initial desktop name (desktopPrivatePlacement) and refuses to run on Default; appcontainer-smoke asserts the private-desktop name prefix and rejects Default. Digest is unchanged: the desktop is a launch-time detail and does not enter the manifest. Stacked on apache#3161 (readiness probe). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
d861e1e to
c000c64
Compare
|
@hqhq1025 Your latest point is correct and I've converged on de-claiming rather than shipping the escape-proof boundary in this PR. New head:
Why de-claim rather than enable no-Win32k now: making one boundary real (no-Win32k for the Node worker, or a token/window-station design) plus child-side escape tests is a genuine hardening slice with its own compatibility surface across Node/PowerShell/cmd/Git. Bundling it here would blow this PR's scope; the honest shipped guarantee is placement + DACL, and the worker does no GUI work. I'd rather ship that truthfully now and land the escape-proof boundary as its own gated slice than overclaim. Since your prior review already accepted the private-desktop delta on the narrowed scope, and the stack is now rebased onto the fixed #3161 head, this should be re-checkable once #3161 lands. Not self-merging. 中文你的最新意见正确,本 PR 选择降级声明而非在此塞入防逃逸边界。新 head:
为何不在此启用 no-Win32k:把某条边界做实(Node worker 的 no-Win32k,或 token/window-station 设计)加 child 侧逃逸负测,是一块有自身跨 Node/PowerShell/cmd/Git 兼容面的硬化切片,塞进本 PR 会撑爆作用域;当前如实交付的保证是 placement + DACL,且 worker 不做任何 GUI 工作。我宁愿现在如实交付它、把防逃逸边界作为独立门禁切片落地,也不 overclaim。 你上一轮已在收窄作用域上接受 private-desktop delta,现栈已 rebase 到修好的 #3161 head,待 #3161 落地即可复检。不自合。 |
|
Thanks for the rework — the desktop confinement is genuinely the right mechanism for the gap (AppContainer fences files/network/token but not desktop scope: window-message shatter and desktop hooks stay open on the interactive desktop), and the implementation is lean: ~120 lines of Rust ( Conclusion: PASS — no P0/P1. P2-1 (handle or explicitly defer): the DACL claim vs the child's effective permissions don't match. The P3 (optional): AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on 中文摘要（AI 辅助审查）结论：PASS（无 P0/P1）。桌面隔离正是这个缺口的正确机制（AppContainer 只围文件/网络/token，不围 desktop scope：window-message shatter 与 desktop hooks 在交互桌面仍开放），实现精简：~120 行 Rust（create_confined_desktop + 两条 launch 路径接线 lpDesktop + 子进程侧自证）扩展现有 create_appcontainer_child/probe_appcontainer_child seam，无并行机制。已核实：CreateDesktopW+逐对象受保护 DACL（D:P）符合 #3182 教训；handle 全路径 RAII 关闭；两条路径都在 CreateProcessW 前建桌面并设 lpDesktop；探针 child 同落私有桌面且 mutex lease 覆盖全程；失败 fail-closed 无 fallback；私有桌面在 launcher 的 winsta 上、剪贴板共享已声明；windows-2025 CI lane 真跑 cargo test + smoke（断言 desktopPrivatePlacement:true、前缀匹配、拒绝 "desktop":"Default"）在真实 Windows 全绿；broker 两条生产路径都经 launch_appcontainer 全覆盖。P2-1：DACL 声明与子进程有效权限不符——desktop_sddl 给 user SID 授 GA（D:P(A;;GA;;;owner)(A;;GA;;;SY)(A;;0xc7;;;app)），commit/RFC 声明"绝不给 SWITCHDESKTOP/HOOKCONTROL/journal"；但 AppContainer token 保留 user SID 为有效 SID（仅 Everyone 等为 deny-only），首条 ACE 即给 child 全部桌面权限（含 SWITCHDESKTOP/HOOKCONTROL/journal 录制回放/WRITE_DAC/WRITE_OWNER）；"最小权限绝不给"只在 app SID ACE 层面成立，测试还把 GA-for-user 固化为意图。影响有限（child 对自身桌面 GA 不跨任何桌面边界——切换需目标/输入桌面的权限，其 DACL 系统所有），是声明-有效权限不一致而非可利用漏洞，但未来给 user 增权会静默流入 child。修复二选一：grant ACE 后追加 user SID deny ACE 覆盖 0x138（SWITCHDESKTOP|HOOKCONTROL|JOURNALRECORD|JOURNALPLAYBACK，launcher 不需这些权限、CreateProcessW 只需 0xc7 子集），或修正 commit/RFC/注释措辞并显式延后。P3（可选）：desktop_is_private_placement 只排除 Default/空名，任何其他桌面名都通过子进程自证——建议直接匹配 maka-sandbox-desktop. 前缀（冒烟脚本前缀断言才是真兜底）；readiness gate 只证 SID+Job 不证桌面放置（探针 child 是 cmd /c exit 0，未来重构丢 lpDesktop 门禁仍绿仅 CI 冒烟能抓）；遗留 restricted-token 路径（--atomic/CreateProcessWithTokenW/CreateProcessAsUserW 无 lpDesktop）仍在交互桌面裸奔——生产 broker 不用它们，但建议加注释或复用 create_confined_desktop。 |
|
Thanks @Astro-Han — took the deny-ACE option and swept the whole PR; head is now P2-1 (owner P3 attestation prefix — fixed. P3 legacy restricted-token paths — documented. P3 readiness gate doesn't prove placement — acknowledged, deferred. True: the probe child ( Self-sweep result (beyond the quoted lines): your P2-1 plus the pattern across previous rounds — claims exceeding enforcement — prompted a full sweep of this PR instead of another spot-fix. The sweep caught one more instance we had missed: the 中文P2-1（owner P3 自证前缀 — 已修。 P3 遗留 restricted-token 路径 — 已注释。 P3 readiness gate 不证 placement — 认同并延后。 探针子进程（ 自查结果（超出被引用行）： 你的 P2-1 加上前几轮反复出现的同一模式——声明超出强制——促使这次对整个 PR 做全量清扫而非再打一个点补丁。清扫抓到一处我们此前漏掉的实例： |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The private-desktop direction strengthens the production AppContainer boundary, and the protected DACL, exact SID/Job checks, separate readiness namespace, and fail-closed Runtime cache are well aligned. Existing public reviews already identify the unaligned token-information UB, WinSta0 desktop-heap exhaustion, and possible existing-desktop reuse; I am not duplicating those inline threads, but they remain blockers.
From first principles, release evidence must attest the boundary properties, not only the launcher's success bit. The additional inline finding asks the independent readiness path to emit and verify its exact desktop/SID/Job attestation, matching what production confinement claims.
Review performed with three Codex reviewer agents and DeepSeek V4 Flash as advisory tools; I verified the novel finding against the latest head and live green CI.
中文评论
private desktop 方向强化了生产 AppContainer 边界；protected DACL、exact SID/Job 校验、独立 readiness namespace 和 fail-closed Runtime cache 均符合职责边界。现有公开 review 已指出 token information 未对齐读取 UB、WinSta0 desktop heap 耗尽和旧 desktop 复用风险，我不重复这些行内线程，但它们仍是 blocker。
从第一性原理看，release evidence 必须证明边界属性，而不只是 launcher 返回成功。以下新增行内问题建议让独立 readiness 路径输出并验证其 exact desktop/SID/Job attestation，与生产 confinement 的声明一致。
本次审查使用了三位 Codex reviewer agents 与 DeepSeek V4 Flash 作为辅助工具；我已依据最新 head 和实时绿色 CI 复核新增问题。
The named-lock comment claimed a squatted Global\ name "fails closed at acquisition", but that only held for a squatter with a restrictive DACL: CreateMutexW ignores the supplied security descriptor when the name already exists, so a local user pre-creating the predictable mutex name with a *permissive* DACL would hand the launcher an attacker-owned arbitration object -- readiness and ACL-ledger operations would then block on a mutex the squatter can hold forever (availability DoS), while the code read as if the DACL had rejected them. Close the gap where it is checkable: when CreateMutexW reports ERROR_ALREADY_EXISTS (also the normal same-user contention path), read the existing object's owner via GetSecurityInfo before any wait and require it to be the current user or SYSTEM. Ownership is the one property a permissive squatter cannot forge -- re-owning an object to another SID requires SeTakeOwnership/SeRestore, which standard users do not hold. Any other owner fails closed with an explicit squatted-mutex error instead of blocking. The 10-way concurrency smoke (same-user contention) passes unchanged; the cross-user negative path cannot be exercised by an unprivileged CI runner and is enforced by the owner check itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
Non-interactive AppContainer workers previously inherited the creator's interactive Winsta0\Default desktop, exposing the same-session GUI attack surface (window-message shatter into the user's interactive windows, desktop hooks) — AppContainer confines files/network/token but not the desktop scope. Per RFC v1 §6.3 the private-desktop promise was designed but deferred. This change enforces initial-desktop *placement* plus DACL protection, not escape-proof confinement. Each AppContainer launch (production create + readiness probe) stands up a per-launch alternate desktop on the existing window station, with a protected DACL: a leading deny ACE strips DESKTOP_SWITCHDESKTOP / HOOKCONTROL / journal record+playback (0x138) from the launching-user SID — which the AppContainer child's token still carries as an effective SID, so without the deny the owner's full-control allow would name the child as a grantee of those rights (maintainer review, P2-1) — then allow ACEs grant the launching user and Local System control for cleanup and the child's AppContainer SID only the minimal DESKTOP_* rights (create window/menu, read/write objects, enumerate). STARTUPINFOEXW.lpDesktop points the child at it. If the desktop cannot be created or the DACL cannot be granted (CreateProcessW then fails ACCESS_DENIED), the launch fails closed — auto/require never fall back to the host desktop. Scope, stated honestly (per maintainer review on apache#3174): - lpDesktop selects only the child's *initial* desktop. This places the worker off the interactive Default desktop and DACL-protects the private one, but it does NOT structurally confine: absent a no-Win32k mitigation, a dedicated window station, or a token boundary, in-process code can still OpenDesktopW("Default") + SetThreadDesktop to re-attach. Enforcing a no-Win32k mitigation is a deferred gate (§6.5). - It does NOT isolate the clipboard: the clipboard belongs to the window station, which both desktops still share. Clipboard isolation with a dedicated window station is a later hardening gate (§6.5). - The create-window/write DACL rights are granted but not relied upon; the worker does no GUI work. A verified Low mandatory-integrity label proving those rights are usable at AppContainer's Low IL is deferred (§6.5). The boundary probe self-attests its own initial desktop name (desktopPrivatePlacement) and requires the launcher-owned maka-sandbox-desktop. prefix — not merely "not Default", so landing on any other pre-existing desktop also fails the placement check; appcontainer-smoke asserts the same prefix and rejects Default. Digest is unchanged: the desktop is a launch-time detail and does not enter the manifest. Stacked on apache#3161 (readiness probe). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
…ttested Follow-ups from inline review on apache#3174, each closing a spot where the desktop's documented properties exceeded (or under-stated) what the code enforced: - Pin a Low no-write-up mandatory label (S:(ML;;NW;;;LW)) on the private desktop. Without it the desktop inherited the creator's Medium integrity, and because MIC is evaluated before the DACL, the Low-IL AppContainer child's granted create-window/write rights were unusable in exactly the case the DACL claimed to grant them. The rights are now labeled-usable; an in-child window-creation check proving them end to end stays a deferred gate (RFC S6.5) - the worker still does no GUI work, so the shipped guarantee remains placement. - Bound the per-launch desktop heap: CreateDesktopExW with a 512 KiB budget instead of CreateDesktopW's default 3,072 KiB interactive allocation, so the supported ten-way concurrency costs ~5 MiB of the documented 48 MiB system desktop heap instead of ~30 MiB. A real-OS test holds ten confined desktops live simultaneously to prove the budget. - Fix an unaligned read: TokenAppContainerSid was read through a Vec<u8> buffer cast to TOKEN_APPCONTAINER_INFORMATION, whose leading pointer field requires pointer alignment - UB in Rust. The buffer is now sized in usize words (same pattern as current_user_sid_string). - Attest the readiness boundary instead of exiting 0: --readiness-probe now emits a machine-readable JSON of the facts it verified (exact-SID match, specific-Job membership, settlement drain, private-desktop placement), and readiness-probe-smoke.ps1 asserts those fields, so removing any verification would turn release evidence red rather than leaving a hollow exit-0 gate green. RFC (EN + zh) updated to match: label and heap bound recorded as enforced, the deferred gate narrowed to no-Win32k plus in-child window-creation coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
3ec5183 to
70eea18
Compare
|
Head is now 中文head 更新为 |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The latest readiness attestation, Low-IL label, aligned token storage, and bounded desktop heap address the earlier findings. The new mutex-owner hardening currently rejects a legitimate elevated owner in required CI, and the desktop name still lacks a fail-closed collision contract.
The simplest authority is explicit and consistent identity: create the mutex with the exact owner SID the validator accepts, and use a GUID-quality desktop nonce while rejecting/retrying ERROR_ALREADY_EXISTS. Do not broaden trust to every Administrators-group owner merely to make CI pass.
Reviewed with Codex using two independent review passes and DeepSeek V4 Flash as an external adversarial pass; I verified the Windows object-identity paths and live required-check failure against this exact head.
中文
最新 readiness attestation、Low-IL label、token 对齐存储和 desktop heap 上限已修复旧问题。新的 mutex owner 加固在 required CI 中拒绝了合法 elevated owner；desktop 名称也仍缺少 fail-closed 的冲突契约。
最小权威方案是显式且一致的 identity：创建 mutex 时设置 validator 接受的精确 owner SID；desktop 使用 GUID 级 nonce，并在 ERROR_ALREADY_EXISTS 时拒绝或重试。不要为了 CI 直接信任所有 Administrators group owner。
本次由 Codex 两轮独立审查，并使用 DeepSeek V4 Flash 做外部对抗审查；我核对了当前 head 和 required check。
…ting locks
The squatted-lock owner check rejected locks owned by
BUILTIN\Administrators (S-1-5-32-544), which broke on elevated hosts:
an elevated administrator token stamps the Administrators group -- not
the user SID -- as the default owner on objects it creates, so a mutex
this very code created earlier on the elevated CI runner failed the
{user, SYSTEM} check and the W0 protocol lane went red
(unsettled_launch_preserves_grants_and_quarantines_the_ledger).
Accept the process token's default-owner SID (TokenOwner) as a third
legitimate owner. This stays inside the threat model: only an elevated
administrator can create objects owned by Administrators, and RFC
S1/S5 explicitly does not defend against administrators. Standard-user
squatters are still rejected -- they cannot forge either the user SID
or the Administrators owner. Adds a unit test pinning the helper to
{user SID, S-1-5-32-544} so both elevation states stay covered.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Generated-by: Claude Fable 5
Non-interactive AppContainer workers previously inherited the creator's interactive Winsta0\Default desktop, exposing the same-session GUI attack surface (window-message shatter into the user's interactive windows, desktop hooks) — AppContainer confines files/network/token but not the desktop scope. Per RFC v1 §6.3 the private-desktop promise was designed but deferred. This change enforces initial-desktop *placement* plus DACL protection, not escape-proof confinement. Each AppContainer launch (production create + readiness probe) stands up a per-launch alternate desktop on the existing window station, with a protected DACL: a leading deny ACE strips DESKTOP_SWITCHDESKTOP / HOOKCONTROL / journal record+playback (0x138) from the launching-user SID — which the AppContainer child's token still carries as an effective SID, so without the deny the owner's full-control allow would name the child as a grantee of those rights (maintainer review, P2-1) — then allow ACEs grant the launching user and Local System control for cleanup and the child's AppContainer SID only the minimal DESKTOP_* rights (create window/menu, read/write objects, enumerate). STARTUPINFOEXW.lpDesktop points the child at it. If the desktop cannot be created or the DACL cannot be granted (CreateProcessW then fails ACCESS_DENIED), the launch fails closed — auto/require never fall back to the host desktop. Scope, stated honestly (per maintainer review on apache#3174): - lpDesktop selects only the child's *initial* desktop. This places the worker off the interactive Default desktop and DACL-protects the private one, but it does NOT structurally confine: absent a no-Win32k mitigation, a dedicated window station, or a token boundary, in-process code can still OpenDesktopW("Default") + SetThreadDesktop to re-attach. Enforcing a no-Win32k mitigation is a deferred gate (§6.5). - It does NOT isolate the clipboard: the clipboard belongs to the window station, which both desktops still share. Clipboard isolation with a dedicated window station is a later hardening gate (§6.5). - The create-window/write DACL rights are granted but not relied upon; the worker does no GUI work. A verified Low mandatory-integrity label proving those rights are usable at AppContainer's Low IL is deferred (§6.5). The boundary probe self-attests its own initial desktop name (desktopPrivatePlacement) and requires the launcher-owned maka-sandbox-desktop. prefix — not merely "not Default", so landing on any other pre-existing desktop also fails the placement check; appcontainer-smoke asserts the same prefix and rejects Default. Digest is unchanged: the desktop is a launch-time detail and does not enter the manifest. Stacked on apache#3161 (readiness probe). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
…ttested Follow-ups from inline review on apache#3174, each closing a spot where the desktop's documented properties exceeded (or under-stated) what the code enforced: - Pin a Low no-write-up mandatory label (S:(ML;;NW;;;LW)) on the private desktop. Without it the desktop inherited the creator's Medium integrity, and because MIC is evaluated before the DACL, the Low-IL AppContainer child's granted create-window/write rights were unusable in exactly the case the DACL claimed to grant them. The rights are now labeled-usable; an in-child window-creation check proving them end to end stays a deferred gate (RFC S6.5) - the worker still does no GUI work, so the shipped guarantee remains placement. - Bound the per-launch desktop heap: CreateDesktopExW with a 512 KiB budget instead of CreateDesktopW's default 3,072 KiB interactive allocation, so the supported ten-way concurrency costs ~5 MiB of the documented 48 MiB system desktop heap instead of ~30 MiB. A real-OS test holds ten confined desktops live simultaneously to prove the budget. - Fix an unaligned read: TokenAppContainerSid was read through a Vec<u8> buffer cast to TOKEN_APPCONTAINER_INFORMATION, whose leading pointer field requires pointer alignment - UB in Rust. The buffer is now sized in usize words (same pattern as current_user_sid_string). - Attest the readiness boundary instead of exiting 0: --readiness-probe now emits a machine-readable JSON of the facts it verified (exact-SID match, specific-Job membership, settlement drain, private-desktop placement), and readiness-probe-smoke.ps1 asserts those fields, so removing any verification would turn release evidence red rather than leaving a hollow exit-0 gate green. RFC (EN + zh) updated to match: label and heap bound recorded as enforced, the deferred gate narrowed to no-Win32k plus in-child window-creation coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
70eea18 to
2797e0e
Compare
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Follow-up review on current head 2797e0eb4.
Findings
- P2 — The RFC still states an escape-proof result before immediately disclaiming it.
docs/architecture/windows-sandbox-rfc-v1.md:148-170 normatively says workers “cannot enumerate or post window messages to the user's interactive windows, or install desktop hooks against them,” but the same paragraph correctly explains that lpDesktop is only initial placement and in-process code can reopen Winsta0\Default with OpenDesktopW + SetThreadDesktop. The PR description also still calls this “desktop-scoped confinement” and says the worker is kept off Default, despite the latest commit deliberately narrowing the implementation to placement. This PR exists partly to align the RFC with shipped behavior, so the remaining top-level guarantee must be narrowed to “starts on a launcher-created alternate desktop” and must not use an unconditional “cannot” claim. Apply the same correction to the Chinese RFC and PR description/title.
- P2 — Desktop creation has no fail-closed uniqueness contract.
experiments/windows-sandbox/launcher/src/windows_launcher.rs:779-826 derives the object name from PID plus wall-clock nanoseconds and collapses clock failure to 0. Microsoft documents that CreateDesktopExW succeeds by returning the existing desktop when the specified name already exists; the supplied security descriptor and heap parameters therefore do not establish a new per-launch object in that case. PID reuse, clock rollback/failure, or same-process calls at the same clock tick can reopen an older desktop while the code and attestation accept the launcher prefix as proof of this launch's object. Use a GUID/cryptographic nonce and avoid the unwrap_or(0) fallback. The Windows test should retain ten handles and assert ten distinct names/objects, not only that ten calls returned handles.
Problem and architecture assessment
The underlying problem is correctly narrowed now: AppContainer alone does not choose a non-interactive initial desktop, and this PR adds initial placement plus a protected per-launch DACL. That is a useful defense-in-depth layer, but it is not confinement from Default; no-Win32k or a dedicated window-station/token boundary remains the correct deeper architecture for that stronger requirement. The current implementation is otherwise direct and uses the existing launch paths rather than introducing a parallel broker.
Deletion opportunities
The hand-written json_string helper and its test can be replaced with the existing serde_json::to_string. The explicit "desktop":"Default" smoke rejection is redundant after the stronger launcher-prefix assertion. These are non-blocking cleanup items.
Verification
All live GitHub checks are green. On this exact head I ran cargo fmt --check, Windows MSVC cross-target cargo check --locked, the full repository build, focused runtime readiness/Windows sandbox tests (22 passed), Biome, diff checks, and merge-conflict checks. Real Windows CI exercises creation, the Low label, the 512 KiB heap, and the smoke paths, but it does not exercise name collision or escape back to Default.
Merge verdict: not ready until the two claim/object-identity issues above are fixed. After that, the remaining no-Win32k/window-station work can stay explicitly deferred.
liugddx
changed the title
|
Thanks @hqhq1025 — both points fixed at head 1. Normative claim vs disclaimed reality — fixed at the sentence that mattered. You're right that the earlier narrowing only fixed the annotation layer while the top-level §6.3 guarantee still said workers "cannot enumerate or post window messages … or install desktop hooks." The normative sentence now reads: workers start on a launcher-created private (alternate) desktop, never the interactive 2. Desktop-name uniqueness — now fail-closed. Accepted in full: Deletion opportunities — both taken. Local verification on 中文1. 规范句与免责说明矛盾 — 已在真正的规范句上修复。 此前的收窄只改了注解层,§6.3 顶层保证仍写"cannot enumerate/post/hook"。现在规范句为:worker 以 launcher 创建的 private (alternate) desktop 作为初始桌面,绝不落在交互 2. 桌面名唯一性 — 已 fail-closed。 完全接受: 删除机会 — 两条都采纳。 本机 |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Follow-up review on current head cecc80542.
No actionable finding remains in the latest delta. The two blockers from my previous review are resolved.
Problem, root cause, and mechanism
The PR now defines the problem correctly: AppContainer does not choose a non-interactive initial desktop, so production children previously started on Winsta0\Default. The patch adds initial placement on a per-launch alternate desktop with a protected DACL, Low mandatory label, bounded heap, fail-closed creation, and placement attestation. It explicitly does not claim escape-proof confinement; no-Win32k and dedicated window-station/token boundaries remain deferred.
The latest commit fixes the object-identity gap directly. Desktop names now use a 128-bit nonce from BCryptGenRandom; RNG failure aborts the launch instead of falling back to a predictable value, and the real-Windows test holds ten desktops live while asserting ten distinct launcher-owned names. The RFC, Chinese RFC, PR title, and description now consistently describe initial placement rather than an unconditional inability to reopen Default.
First-principles and optimality assessment
For the stated placement guarantee, this is the simplest coherent architecture: reuse the existing production/readiness launch paths, create one scoped desktop object per launch, keep its handle alive through settlement, and fail closed on every setup error. Adding a no-Win32k state machine or dedicated window station here would solve a stronger problem than this PR claims and should remain a separate compatibility-tested hardening slice.
Deletion and test quality
The hand-written JSON escaper and duplicate Default smoke assertions were correctly deleted. The remaining SDDL, placement, heap, distinct-name, production smoke, and readiness-attestation tests protect observable boundary properties rather than mirroring implementation details. No further production code or low-quality test needs deletion before merge.
Verification
On this exact head, git diff --check, merge-conflict inspection, cargo fmt --check, and Windows MSVC cross-target cargo check --locked pass. Live Windows W0, Windows recovery, installed Windows CLI, package, typecheck, and CodeRabbit checks are green. At review time, test_workspaces, test_runtime_host, storybook, and e2e are still queued or running; they must finish successfully before merge.
Residual risks
The delivered boundary is initial placement, not prevention of OpenDesktopW("Default")/SetThreadDesktop; clipboard remains window-station scoped; Low-IL GUI rights are labeled but not exercised by an in-child window test; and the readiness lifecycle retains the documented #3161 deferred gaps. These are explicitly documented and do not contradict the current guarantee.
Merge verdict: the reviewed revision is ready once the remaining required checks complete successfully.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The current head resolves the earlier private-desktop blockers while keeping the security claim narrow and accurate. It proves initial placement on a launcher-created desktop, uses unpredictable names, protects the DACL and Low-IL label, verifies the exact AppContainer SID/Job identity, and keeps the desktop handle owned through child settlement. The RFC correctly leaves window-station clipboard isolation and stronger escape resistance out of this W0 claim.
I found no reproducible P0–P3 issue in the code on this head. The implementation extends the existing Windows launcher/readiness path and does not create a second sandbox authority.
CI note: the required run is not currently green. The E2E failure is the unrelated slash-command-menu.spec.ts compact-session assertion (Fake backend received: after compact was not observed), and Storybook remains stuck in progress. This approval is for the reviewed code; the PR is not merge-ready until required CI is resolved.
AI-assisted review disclosure: Codex performed the final review using two independent reviewer passes and OpenCode Go DeepSeek V4 Flash (high effort) as an adversarial advisory pass. I verified the desktop security/lifecycle paths against exact head cecc80542 and inspected the live E2E log. No local tests were run.
中文评论
当前 head 已解决此前 private desktop 的阻塞项，同时把安全声明保持在准确、有限的范围：它证明 child 初始位于 launcher 创建的 desktop，使用不可预测名称，保护 DACL 与 Low-IL label，校验精确 AppContainer SID/Job 身份，并由 launcher 持有 desktop handle 直到 child settle。RFC 也正确地没有把 window-station clipboard isolation 和更强 escape resistance 纳入本 W0 声明。
当前 head 的代码未发现可复现 P0–P3。实现扩展既有 Windows launcher/readiness 路径，没有新增第二个 sandbox authority。
CI 说明：必需 run 目前不是绿色。E2E 失败来自无关的 slash-command-menu.spec.ts compact-session 断言（未观察到 Fake backend received: after compact），Storybook 仍卡在 in progress。本次 approve 针对已审代码；必需 CI 解决前不能合并。
AI 辅助审查说明：Codex 使用两轮独立 reviewer 审查，并以 OpenCode Go DeepSeek V4 Flash（high effort）进行对抗性辅助审查；我已针对精确 head cecc80542 核对 desktop security/lifecycle 路径并检查实时 E2E 日志。本轮未运行本地测试。
The owner check accepted {user SID, SYSTEM, this token's default owner},
but the default owner differs between elevation states: a mutex created
by an *elevated* instance is owned by BUILTIN\Administrators, while a
concurrent *non-elevated* instance of the same user resolves its own
token owner to the user SID -- so it rejected the elevated instance's
legitimate lock and readiness/ACL-ledger acquisition failed closed
whenever elevated and unelevated Maka processes overlapped.
Make the lock's identity elevation-independent at the source: the lock
security descriptor now pins an explicit owner (O:<user SID> -- always
an assignable owner for the user's own token, elevated or not) ahead of
the protected DACL, so every lock this code creates carries the same
owner in every elevation state. Validation of a pre-existing lock
accepts exactly {user SID, SYSTEM, BUILTIN\Administrators} -- the last
for locks created by builds that predate the pinning during side-by-side
overlap, and safe because only an elevated administrator (outside the
RFC S1/S5 threat model) can create Administrators-owned objects. The
per-token default-owner query is deleted. Adds a lock_sddl unit test
asserting the pinned owner precedes the protected DACL.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Generated-by: Claude Fable 5
Non-interactive AppContainer workers previously inherited the creator's interactive Winsta0\Default desktop, exposing the same-session GUI attack surface (window-message shatter into the user's interactive windows, desktop hooks) — AppContainer confines files/network/token but not the desktop scope. Per RFC v1 §6.3 the private-desktop promise was designed but deferred. This change enforces initial-desktop *placement* plus DACL protection, not escape-proof confinement. Each AppContainer launch (production create + readiness probe) stands up a per-launch alternate desktop on the existing window station, with a protected DACL: a leading deny ACE strips DESKTOP_SWITCHDESKTOP / HOOKCONTROL / journal record+playback (0x138) from the launching-user SID — which the AppContainer child's token still carries as an effective SID, so without the deny the owner's full-control allow would name the child as a grantee of those rights (maintainer review, P2-1) — then allow ACEs grant the launching user and Local System control for cleanup and the child's AppContainer SID only the minimal DESKTOP_* rights (create window/menu, read/write objects, enumerate). STARTUPINFOEXW.lpDesktop points the child at it. If the desktop cannot be created or the DACL cannot be granted (CreateProcessW then fails ACCESS_DENIED), the launch fails closed — auto/require never fall back to the host desktop. Scope, stated honestly (per maintainer review on apache#3174): - lpDesktop selects only the child's *initial* desktop. This places the worker off the interactive Default desktop and DACL-protects the private one, but it does NOT structurally confine: absent a no-Win32k mitigation, a dedicated window station, or a token boundary, in-process code can still OpenDesktopW("Default") + SetThreadDesktop to re-attach. Enforcing a no-Win32k mitigation is a deferred gate (§6.5). - It does NOT isolate the clipboard: the clipboard belongs to the window station, which both desktops still share. Clipboard isolation with a dedicated window station is a later hardening gate (§6.5). - The create-window/write DACL rights are granted but not relied upon; the worker does no GUI work. A verified Low mandatory-integrity label proving those rights are usable at AppContainer's Low IL is deferred (§6.5). The boundary probe self-attests its own initial desktop name (desktopPrivatePlacement) and requires the launcher-owned maka-sandbox-desktop. prefix — not merely "not Default", so landing on any other pre-existing desktop also fails the placement check; appcontainer-smoke asserts the same prefix and rejects Default. Digest is unchanged: the desktop is a launch-time detail and does not enter the manifest. Stacked on apache#3161 (readiness probe). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
…ttested Follow-ups from inline review on apache#3174, each closing a spot where the desktop's documented properties exceeded (or under-stated) what the code enforced: - Pin a Low no-write-up mandatory label (S:(ML;;NW;;;LW)) on the private desktop. Without it the desktop inherited the creator's Medium integrity, and because MIC is evaluated before the DACL, the Low-IL AppContainer child's granted create-window/write rights were unusable in exactly the case the DACL claimed to grant them. The rights are now labeled-usable; an in-child window-creation check proving them end to end stays a deferred gate (RFC S6.5) - the worker still does no GUI work, so the shipped guarantee remains placement. - Bound the per-launch desktop heap: CreateDesktopExW with a 512 KiB budget instead of CreateDesktopW's default 3,072 KiB interactive allocation, so the supported ten-way concurrency costs ~5 MiB of the documented 48 MiB system desktop heap instead of ~30 MiB. A real-OS test holds ten confined desktops live simultaneously to prove the budget. - Fix an unaligned read: TokenAppContainerSid was read through a Vec<u8> buffer cast to TOKEN_APPCONTAINER_INFORMATION, whose leading pointer field requires pointer alignment - UB in Rust. The buffer is now sized in usize words (same pattern as current_user_sid_string). - Attest the readiness boundary instead of exiting 0: --readiness-probe now emits a machine-readable JSON of the facts it verified (exact-SID match, specific-Job membership, settlement drain, private-desktop placement), and readiness-probe-smoke.ps1 asserts those fields, so removing any verification would turn release evidence red rather than leaving a hollow exit-0 gate green. RFC (EN + zh) updated to match: label and heap bound recorded as enforced, the deferred gate narrowed to no-Win32k plus in-child window-creation coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
…ive claim Maintainer follow-up on the private-desktop delta, both points fixing a gap between what was stated and what the object model guarantees: - Desktop-name uniqueness is now fail-closed. CreateDesktopExW *opens* an existing desktop when the name collides -- silently ignoring the supplied DACL and heap budget -- and the old nonce was wall-clock nanoseconds with a unwrap_or(0) fallback, so PID reuse, clock rollback, same-tick calls, or a failed clock could reopen an older desktop while the prefix attestation still passed. The nonce is now 128 bits from the OS CSPRNG (BCryptGenRandom), collision is cryptographically negligible, and RNG failure fails the launch closed instead of collapsing to a constant name. The ten-desktop live test now also asserts ten *distinct* names that each pass the placement attestation, not merely ten returned handles. - The RFC's top-level S6.3 guarantee no longer states an unconditional "cannot": it now reads "workers start on a launcher-created private desktop, never Default", with the enumerate/message/hook resistance explicitly tied to the deferred no-Win32k/window-station gates (EN + zh). The PR title/description are narrowed the same way. Cleanups from the same review: json_string now delegates to serde_json (hand-rolled escaping and its test deleted), and the redundant '"desktop":"Default"' smoke rejections are removed -- the stronger launcher-prefix assertions already exclude Default. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
cecc805 to
e7f560e
Compare
4 participants
What & why
Closes the private-desktop half of the umbrella #2142 Phase 4 ③ sandbox hardening — specifically initial private-desktop placement plus a hardened per-launch desktop object. #3161 delivered the production-identity readiness probe and deferred the private desktop to a follow-up (RFC v1 §6.5); this is that follow-up.
Non-interactive AppContainer workers previously inherited the creator's interactive
Winsta0\Defaultdesktop as their initial desktop. AppContainer fences files, network and token, but does not choose a non-interactive desktop — so code that never re-attaches still ran amid the user's interactive windows. RFC v1 §6.3 promised a private desktop; it was designed but not enforced.What this does
create_appcontainer_childpath and theprobe_appcontainer_childreadiness path — creates a per-launch alternate desktop on the existing window station (CreateDesktopExW, bounded 512 KiB heap so ten-way concurrency stays ~5 MiB of the documented 48 MiB system budget; name =maka-sandbox-desktop.<pid>.<128-bit CSPRNG nonce>, RNG failure fails the launch closed).DESKTOP_SWITCHDESKTOP/HOOKCONTROL/journal (0x138) from the launching-user SID (which the AppContainer child carries as an effective SID); allow ACEs grant the launching user and Local System control for cleanup and the child's AppContainer SID only the minimalDESKTOP_*rights; an explicit Low no-write-up mandatory label makes the granted rights pass MIC at the child's Low IL.STARTUPINFOEXW.lpDesktoppoints the child at it; the handle lives until the child settles, thenCloseDesktop.auto/requirenever fall back to the host desktop.--boundary-probeself-attests its initial desktop name against the launcher-ownedmaka-sandbox-desktop.prefix;--readiness-probeemits a machine-readable attestation (exact SID, specific Job, settlement, desktop placement) thatreadiness-probe-smoke.ps1asserts field by field.windows_launcher_desktop_tests.rscovers the SDDL builder (deny-before-allow, minimal app-SID mask, Low label), the placement predicate, and a real-OS test holding ten live desktops with ten distinct names inside the heap budget.Scope of the guarantee (narrowed per review)
This PR ships initial-desktop placement plus a DACL/label-protected per-launch desktop object — not escape-proof confinement:
Default; the desktop object is protected (deny ACE, minimal app mask, Low label, protected DACL) and per-launch unique (CSPRNG nonce;CreateDesktopExWwould silently reopen an existing name, so uniqueness is part of the object contract).lpDesktopselects only the initial desktop. Nothing structural stops in-process code fromOpenDesktopW("Default")+SetThreadDesktopre-attaching — keeping a worker unable to enumerate/message/hook the user's windows after an escape attempt requires the deferred no-Win32k / dedicated window-station / token-boundary gates (§6.5).Digest is unchanged — the desktop is a launch-time detail and does not enter the manifest, so no
profile_digest_mismatch.Stacked on #3161
This PR is stacked on #3161 (it modifies the
probe_appcontainer_childcode that #3161 introduces). Until #3161 merges, the diff here includes its commits; they drop out automatically once #3161 lands. Please merge #3161 first.Verification (Windows 11 x64, local)
cargo fmt --checkclean;cargo build --locked;cargo test --locked→ 49 pass (incl. desktop SDDL/placement tests and the ten-distinct-desktops live test).maka-sandbox-desktop.*, file/network/Job boundary intact, readiness attestation fields) is proven byappcontainer-smoke.ps1andreadiness-probe-smoke.ps1on the realwindows-sandbox-w0CI lane.@maka/core+@maka/runtimerebuild clean;default-sandbox-managernode test → 11 pass.RFC
windows-sandbox-rfc-v1.mdand.zh-CN.md§6.3/§6.4/§6.5 updated: the normative guarantee is stated as initial-desktop placement (no unconditional "cannot" claims); the deny ACE, Low label, heap bound, and readiness attestation are recorded as enforced; no-Win32k, dedicated window station, clipboard isolation, and in-child window-creation proof stay on the deferred-gates list.AI use
Select exactly one:
Tool(s) and scope: Claude Fable 5 (Anthropic), driven by the human contributor of record, authored the private-desktop placement (
create_confined_desktop, the two launch-path wirings, the boundary-probe self-attestation), the smoke assertions, the Rust unit tests, and the RFC updates, all under human review. The commit carries aGenerated-by: Claude Fable 5trailer; the final squash commit must retain it.