feat(windows): windows gap closure (v2.0 milestone)#725
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements the Windows parity milestone for the nono sandboxing system, introducing process containment using Job Objects, low-integrity token management, and network enforcement via Windows Firewall and WFP. It also enables ETW-based learning for path discovery. Review feedback identifies several security vulnerabilities related to relative path usage for system commands like icacls, netsh, and sc, which could lead to path hijacking. Other critical issues include a race condition in the logging thread's handle management, a malformed Job Object name containing a newline, unquoted paths in service creation, and logic gaps regarding DLL staging and directory access on Windows.
93ec86b to
fbab60f
Compare
fe4576c to
2be6853
Compare
Squash of 212 commits on windows-squash that delivered Windows/Unix parity for everyday CLI usage, network policy, and developer tooling. Milestone: v2.0 Windows Gap Closure (tag `v2.0` on source branch). Shipped 2026-04-18. See .planning/milestones/v2.0-ROADMAP.md for the full 11-phase, 29-plan archive. Scope: - Phase 5: Windows detach readiness fix (WaitNamedPipeW readiness probe in run_detached_launch). - Phase 6: WFP enforcement activation — SID-based kernel filtering promoted to primary network backend; driver gate removed; duplicate activation path cleaned. - Phase 7: Quick wins — `nono wrap` (Direct strategy + Job Object + WFP) and `nono logs` / `nono inspect` / `nono prune` session commands on Windows. - Phase 8: Interactive `nono shell` via ConPTY (CreatePseudoConsole) inside Job Object + WFP sandbox on Windows 10 build 17763+. - Phase 9: Port-granular WFP policy (--allow-port with bind/connect independence) + proxy credential injection (--network-profile / --credential / --upstream-proxy). - Phase 10: `nono learn` on Windows via ETW (ferrisetw + Win32-format path conversion, file + network events, admin-gated). - Phase 11 (stretch): Runtime capability expansion over named pipe with constant-time token auth + interactive approval. - Phase 12: Milestone bookkeeping cleanup. - Phase 13: v2.0 human verification UAT — 10 items resolved to terminal verdicts (3 pass, 7 waived). - Phase 14: v2.0 fix pass (detached-console-grandchild partial fix + help-text correction + runbook flag repair). - Phase 15: Detached Console + ConPTY architecture investigation — direction-b closure of the v2.0 carry-forward (gated PTY-disable + null-token + AppID WFP on detached Windows path only). 5-row smoke gate pass; 4 Phase 13 UAT items promoted to pass. Also includes earlier upstream sync work (WSL2 feature matrix + security hardening, release 0.26.0-0.29.1 bookkeeping, keystore file:// URI support, macOS proxy NO_PROXY fix, other fork maintenance) that lived on the same branch. Security-critical notes: - Direction-b scoped waivers for detached Windows path: Low-IL isolation waived on detached path (Job Object + filesystem sandbox remain primary); per-session-SID WFP replaced by AppID WFP on detached path (still kernel-enforced; requires nono-wfp-service). Non-detached path keeps WRITE_RESTRICTED + session-SID + ConPTY unchanged. - WRITE_RESTRICTED narrows the restricting-SID access-check gate to writes only so DLL loads and console init aren't blocked. - All paths canonicalized at grant time; path-component comparison (not string starts_with) used throughout to prevent `/home-evil/...` style escapes. 191 files changed, +50,204 / -5,244. Supersedes pre-squash branches: - pr/windows-epic12-clean-v2 (PR always-further#555) - pr555/windows-epic12-clean-v3 (PR always-further#583) - win-101-windows-build-foundation (PR always-further#530) Signed-off-by: oscarmackjr-twg <oscar.mack.jr@gmail.com>
2be6853 to
3ec7fdf
Compare
Address high-severity race from gemini-code-assist PR always-further#725 review on `start_logging` (supervisor.rs). The old pattern was a two-step lookup: lock active_attachment, clone the HANDLE out, release the lock, then raw WriteFile on the cloned HANDLE. Between the release and the WriteFile, the pipe-sink thread (start_data_pipe_server) could acquire the same mutex on attach-client disconnect, set the slot to None, release the lock, and drop its File wrapper — invoking CloseHandle(h_pipe). Windows then recycles the numeric HANDLE value for any subsequently-created kernel object in the process; the logging thread's WriteFile silently writes child stdout bytes into that unrelated resource. Fix: keep the mutex guard live across the WriteFile call. The pipe- sink thread must re-acquire the same mutex before clearing the slot, so while the logging thread holds the guard the File wrapper cannot drop. Contention is negligible — pipe-sink's lock scopes are straight- line swap-in / swap-out of the Option. Chose "hold the lock" (gemini's first suggestion) over DuplicateHandle (second suggestion). The dup-handle design needs a second owned HANDLE on the logging side, lifecycle management for when to close it, and a separate mechanism to detect disconnect — substantially more state for the same correctness gain. Raw FFI WriteFile preserved (vs File::write_all) so the bridge thread keeps tolerating ERROR_NO_DATA / ERROR_BROKEN_PIPE without dying. Verification gates: cargo fmt --all -- --check, cargo build --workspace, cargo clippy --workspace --all-targets -- -D warnings -D clippy::unwrap_used all exit 0. Signed-off-by: oscarmackjr-twg <oscar.mack.jr@gmail.com>
…de program Address medium-severity finding from gemini-code-assist PR always-further#725 on `stage_program_for_blocked_network_launch` in `crates/nono-cli/src/exec_strategy_windows/network.rs`. The blocked-network Windows Firewall fallback path stages the program into a temp dir so the firewall rule targets a unique image path (the staged copy) without permanently affecting the real program location. Previously only the program binary was copied — any .dll / .manifest / .config / .pdb that the program depended on via same-dir loading was missing from the staged layout, and the program would fail to start. Fix: new helper `copy_program_siblings` walks the program's parent directory and copies regular files whose extension matches an allowlist (dll / pdb / manifest / config / xml). Chosen to cover the common Windows startup-dependency shapes without blindly copying unrelated bytes when the program lives in e.g. `C:\Program Files\...`. Fail-secure: any copy error aborts the stage. WFP remains the primary backend (SID-based, no staging needed); this fix only affects the legacy firewall fallback path. Verification gates: cargo fmt --all -- --check, cargo build --workspace, cargo clippy --workspace --all-targets -- -D warnings -D clippy::unwrap_used all exit 0. Signed-off-by: oscarmackjr-twg <oscar.mack.jr@gmail.com>
Address high-severity finding from gemini-code-assist PR always-further#725 on `build_wfp_driver_create_args`. The driver image path was passed unquoted to `sc create binPath= <path>`; SCM stored it verbatim and at service-start time parsed whitespace in the path as argument separators (e.g. `C:\Program Files\nono-wfp-driver.sys` became image `C:\Program` with args `Files\nono-wfp-driver.sys`). Service failed to start whenever the driver was installed under a whitespace-containing path. Fix: embed literal `"` around the path in the value passed to sc, matching the existing treatment of the service-binary side in `format_wfp_service_command` at line 214. Verification: cargo fmt --all -- --check and cargo build --workspace both green. Signed-off-by: oscarmackjr-twg <oscar.mack.jr@gmail.com>
…pen_windows_supervisor_path Address medium-severity finding from gemini-code-assist PR always-further#725 on `open_windows_supervisor_path`. `std::fs::OpenOptions` on Windows wraps `CreateFileW` without `FILE_FLAG_BACKUP_SEMANTICS`, so directory paths fail with an opaque `ERROR_ACCESS_DENIED` that looks like an ACL problem but is really a platform quirk. The cap-pipe protocol is file-scoped by design (brokers file handles via `DuplicateHandle`, not directory enumeration handles). Added a pre-open `metadata().is_dir()` check that returns a clear capability-broker-boundary error instead of surfacing the platform error. Deliberate choice over transparent directory support: - Setting `FILE_FLAG_BACKUP_SEMANTICS` via `OpenOptionsExt` would let directories open, but silently granting directory handles to sandboxed children is a capability-scope expansion beyond the existing broker contract — directory handles enable enumeration and change-notification beyond the approved path's scope. - If future work needs directory brokering, it should add an explicit `is_dir` discriminator to `CapabilityRequest`. Verification: cargo fmt --all -- --check and cargo build --workspace both green. Signed-off-by: oscarmackjr-twg <oscar.mack.jr@gmail.com>
|
Hi — wanted to give a status update on this PR if you have a moment. This PR was branched from I aborted the rebase rather than guess at how you'd want those resolved. Before I sink time into resolving them, I wanted to ask: would you prefer I
No rush at all — I know reviewer time is the scarce resource here. The phase-level archive is at Thanks! |
Resumed quick task after PR always-further#785 (claude-pack-migration) merged upstream on 2026-04-29 — fired trigger #1 in CONTEXT.md `<deferred>` block. Sigstore-bump triggers (always-further#777, always-further#778) are dead (PRs closed). Rebase attempt via `git rebase --onto upstream/main 063ebad origin/v2.0-pr` surfaced 77 conflicts across 504 upstream commits (v0.41 → v0.44): - 49 UU content conflicts - 26 AA add/add (16 of which are `crates/nono-cli/src/*_runtime.rs` — parallel-evolution architectural collision; common ancestor has none of them, both branches added independently with different content) - 2 DU (policy_cmd.rs upstream-deleted per always-further#594, needs remap onto split policy.rs / network_policy.rs / deprecated_policy.rs) Aborted the rebase rather than improvise architectural decisions on upstream's runtime refactor. Posted deferential outreach comments on PR always-further#725 and always-further#726 asking the maintainer to pick between (1) push through rebase with inline guidance, (2) close+restage as smaller per-phase PRs, or (3) alternate path. Comments: - always-further#725 (comment) - always-further#726 (comment) Re-deferred until maintainer response. Locked decisions from 2026-04-28 still hold (phase22 stays on local main; force-push gate never crossed; windows-squash untouched). Signed-off-by: oscarmackjr-twg <oscar.mack.jr@gmail.com>
|
Closing. Please speak with the maintainers on Discord or other platforms before opening anymore PR's so we have an idea of what these PR's are doing, otherwise they are just bloating the PR's on this project. Thanks |
Squash of 212 commits on windows-squash that delivered Windows/Unix parity for everyday CLI usage, network policy, and developer tooling.
Milestone: v2.0 Windows Gap Closure (tag
v2.0on source branch). Shipped 2026-04-18. See .planning/milestones/v2.0-ROADMAP.md for the full 11-phase, 29-plan archive.Scope:
nono wrap(Direct strategy + Job Object + WFP) andnono logs/nono inspect/nono prunesession commands on Windows.nono shellvia ConPTY (CreatePseudoConsole) inside Job Object + WFP sandbox on Windows 10 build 17763+.nono learnon Windows via ETW (ferrisetw + Win32-format path conversion, file + network events, admin-gated).Also includes earlier upstream sync work (WSL2 feature matrix + security hardening, release 0.26.0-0.29.1 bookkeeping, keystore file:// URI support, macOS proxy NO_PROXY fix, other fork maintenance) that lived on the same branch.
Security-critical notes:
/home-evil/...style escapes.191 files changed, +50,204 / -5,244.
Supersedes pre-squash branches: