From d016eeefc62a766715ec0747ec0f6af291da5b1b Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Thu, 30 Jul 2026 05:15:19 -0700 Subject: [PATCH 1/8] sandbox(win): run lifecycle scripts on a nub-owned copy of the project's Node MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The build jail could not start a single lifecycle script for a standard user with an all-users Node. A leaf read grant is an ACE, which needs WRITE_DAC on the target; the stock MSI installs to %ProgramFiles%\nodejs, where a standard user holds read but not WRITE_DAC. Measured de-elevated on a restricted token, the interpreter grant there fails `Access is denied. (os error 5)` and `?` aborts the launch. C:\hostedtoolcache is the same. Nothing existing rescued it: the already-granted-to-AppContainers skip needs an INHERITABLE ace, which no file can carry. Widening that DACL would not have been the fix even where nub can write it. CreateProcessW opens the image in the CALLER's context, so once the caller is itself in the AppContainer, opening the ambient node.exe by absolute path is a confined open and is refused — measured against the identical command line unconfined. So the interpreter is now a nub-owned copy of the SAME distribution, staged under /jail-bin/- and keyed by the version the interpreter reported. The version is load-bearing rather than incidental: prebuild-install and node-gyp-build both default the ABI they fetch for to process.versions.modules of the running Node, so a newer staged Node would make that family download a prebuild that then fails to load. The ace is written on the EMPTY staging directory and inherited at creation, which is both the cheap direction (24 ms against 426 ms re-granting a populated tree) and what makes the backend's own leaf grant skip, so a lifecycle spawn pays nothing. Populated by copy, never hard link: an NTFS hard link shares one MFT record and therefore one security descriptor, so an ace written on the link lands on the original. Also: a refused READ grant is now skipped rather than fatal, matching the ancestor-chain repair two dozen lines below it. A read grant may legitimately name a toolchain the user does not hold WRITE_DAC on, and aborting every lifecycle script over one unreachable toolchain is the loudest possible failure for the mildest cause. A grant is a reduction from the unconfined spawn's complete access, so skipping one leaves the child with less, never more. Write grants stay fatal — every one is nub's own tmp or the package dir being built. And the extra-read grant's doc described only the POSIX layout, whose rationale does not hold on Windows: that archive ships no include/ and no lib/, so `/lib/node_modules` named a path that does not exist and `include/node` described headers the distribution does not carry. Both halves now state the shape they belong to, with where node-gyp actually reads headers from when nodedir is set and where it downloads them when it is not (node-gyp 13 lib/configure.js getNodeDir, lib/install.js). The pinning test covered only the POSIX shape, which is how the Windows spelling stayed inert; it now covers both, with a sibling refusal so the widened-grant direction is caught too. --- crates/nub-cli/src/pm_engine/build_jail.rs | 32 +- .../nub-cli/src/pm_engine/build_prefetch.rs | 18 +- crates/nub-cli/src/pm_engine/jail_bin.rs | 286 ++++++++++++++++++ crates/nub-cli/src/pm_engine/mod.rs | 3 + crates/nub-sandbox/src/backend/mod.rs | 2 + crates/nub-sandbox/src/backend/windows.rs | 81 ++++- crates/nub-sandbox/src/compiler/preset.rs | 128 ++++++-- crates/nub-sandbox/src/lib.rs | 2 + 8 files changed, 511 insertions(+), 41 deletions(-) create mode 100644 crates/nub-cli/src/pm_engine/jail_bin.rs diff --git a/crates/nub-cli/src/pm_engine/build_jail.rs b/crates/nub-cli/src/pm_engine/build_jail.rs index 70d0e2a19..20bfde383 100644 --- a/crates/nub-cli/src/pm_engine/build_jail.rs +++ b/crates/nub-cli/src/pm_engine/build_jail.rs @@ -112,25 +112,39 @@ impl aube_util::LifecycleSandbox for NubBuildJail { // Windows stamps `NODE_OPTIONS` too — below, where the interpreter's version is // already known. + // Make node-gyp compile offline. It reads Node headers from `npm_config_nodedir/ + // include/node` (default devdir `~/.cache/node-gyp/`, unreadable → network + // fallback the jail denies). Point nodedir at a directory that ACTUALLY HOLDS + // them and grant the toolchain subtrees (the store path is outside `$tooldirs` + + // the interpreter grant). Set-if-absent: an explicit ambient nodedir is a + // deliberate build-against-custom-node choice; the case we fix carries none. + let probe = ProbeScope::new(&spawn); + + // WINDOWS: redirect the interpreter to a nub-owned COPY of the same distribution, + // BEFORE anything else reads `npm_node_execpath`. Two independent reasons the ambient + // one is unusable — nub cannot write the read-grant ACE where the stock MSI installs, + // and a confined caller cannot open that image even where it can — are on + // [`super::jail_bin`]'s module doc with their measurements. Everything below then + // derives from the copy: the interpreter grant, `node_layout`'s `node_modules` and + // header paths, and the version the `NODE_OPTIONS` gate asks for. Declining leaves the + // ambient interpreter, which is the behavior before this existed. + #[cfg(windows)] + if let Some(staged) = super::jail_bin::stage(&ambient, &probe) { + staged.redirect_env(&mut ambient); + } + // The interpreter closure to grant READ. nub provisions its own Node under its // store (not `/usr`), so the tight-read base can't reach it. Under nub a bare // `node` resolves via the PATH-prepended shim (`NODE`) which re-execs the real // binary (`npm_node_execpath`), so BOTH must be readable/executable — grant each - // (compile_build_jail dedups and adds each one's bin dir). + // (compile_build_jail dedups and adds each one's bin dir). On Windows both spellings + // already name the staged copy, so this resolves to one directory. let interpreter: Vec = ["npm_node_execpath", "NODE"] .iter() .filter_map(|k| ambient.get(*k)) .map(PathBuf::from) .collect(); - // Make node-gyp compile offline. It reads Node headers from `npm_config_nodedir/ - // include/node` (default devdir `~/.cache/node-gyp/`, unreadable → network - // fallback the jail denies). Point nodedir at a directory that ACTUALLY HOLDS - // them and grant the toolchain subtrees (the store path is outside `$tooldirs` + - // the interpreter grant). Set-if-absent: an explicit ambient nodedir is a - // deliberate build-against-custom-node choice; the case we fix carries none. - let probe = ProbeScope::new(&spawn); - // WINDOWS: deliver the `child_process` stdio shim. A piped spawn under the // AppContainer does not fail, it SPINS — libuv retries the refused named pipe // forever inside `uv_spawn`, before any timeout can arm — and every `node-gyp` diff --git a/crates/nub-cli/src/pm_engine/build_prefetch.rs b/crates/nub-cli/src/pm_engine/build_prefetch.rs index dc8ce690c..95ca13a5e 100644 --- a/crates/nub-cli/src/pm_engine/build_prefetch.rs +++ b/crates/nub-cli/src/pm_engine/build_prefetch.rs @@ -255,6 +255,22 @@ pub(super) fn node_version( node_facts(ambient, probe).map(|facts| facts.version.as_str()) } +/// The cache key for a copy of this interpreter's distribution: the version it reports plus its +/// architecture. Shares the memo above, so it costs nothing beyond what the header prefetch has +/// already spent. +/// +/// The VERSION is what makes a staged copy ABI-correct by construction rather than by assertion +/// (see `jail_bin`), and the ARCH is what keeps a same-version x64 and arm64 install from +/// colliding on one directory. +#[cfg_attr(not(windows), allow(dead_code))] +pub(super) fn node_dist_key( + ambient: &BTreeMap, + probe: &ProbeScope, +) -> Option { + let facts = node_facts(ambient, probe)?; + Some(format!("{}-{}", facts.version, facts.arch)) +} + /// Separated from the spawn so the parse is unit-testable without a Node on disk. fn parse_node_facts(stdout: &str) -> Option { let line = stdout.lines().next()?; @@ -1320,7 +1336,7 @@ fn host_allowed(url: &str) -> bool { }) } -fn cache_root() -> Option { +pub(super) fn cache_root() -> Option { aube_store::dirs::cache_dir() } diff --git a/crates/nub-cli/src/pm_engine/jail_bin.rs b/crates/nub-cli/src/pm_engine/jail_bin.rs new file mode 100644 index 000000000..2cf9f2bac --- /dev/null +++ b/crates/nub-cli/src/pm_engine/jail_bin.rs @@ -0,0 +1,286 @@ +//! The nub-owned interpreter copy a Windows build jail runs its lifecycle scripts on. +//! +//! WHY A COPY EXISTS AT ALL. The build jail must hold at ZERO privilege — there is no setup +//! command and never will be — and on Windows a leaf read grant is an ACE, which needs +//! `WRITE_DAC` on the target. The stock Node MSI installs to `%ProgramFiles%\nodejs`, where a +//! standard user holds read but not `WRITE_DAC`; `C:\hostedtoolcache` (what `actions/setup-node` +//! unzips into) is the same. Measured de-elevated on a restricted token, granting the interpreter +//! there fails `Access is denied. (os error 5)` and the jail cannot start a single lifecycle +//! script. `%ProgramFiles%\nodejs` is the outlier that makes this nub's problem rather than +//! Windows': 43 of the 44 `C:\Program Files` children publish read to AppContainers already, so +//! an all-users Python or Visual Studio install needs no grant — the Node installer is the one +//! that ships without it. +//! +//! AND A SECOND, INDEPENDENT REASON, which is why widening the DACL would not have been the fix +//! even where nub can: `CreateProcessW` opens the image in the CALLER's context, and inside the +//! jail the caller is itself in the AppContainer. Naming the ambient `%ProgramFiles%\nodejs\ +//! node.exe` by ABSOLUTE path from a confined `cmd.exe` gets `Access is denied.`, while the +//! identical command line unconfined succeeds — one variable, confinement (run 30517334191, both +//! Windows images). Everything a lifecycle script spawns is such a nested spawn. So the +//! interpreter has to be a file the jail can read, not merely one nub can point at. +//! +//! THE VERSION IS NOT NEGOTIABLE — a copy, never an upgrade. `prebuild-install` and +//! `node-gyp-build` both default the ABI they fetch for to `process.versions.modules` of the +//! RUNNING Node (`prebuild-install/util.js`, `node-gyp-build/node-gyp-build.js`), so staging a +//! newer Node than the project's would make that whole family download a prebuild that then fails +//! to load. The copy is byte-identical to the project's own distribution and keyed by the version +//! it reported, which makes the constraint structural rather than a check. +//! +//! GRANT FIRST, THEN POPULATE. [`nub_sandbox::windows_publish_appcontainer_read`] carries why: +//! the ace is inheritable, so writing it on the EMPTY directory has every entry inherit at +//! creation (24 ms) instead of walking a populated 2,435-entry tree (426 ms), and an inheritable +//! AAP ace is exactly what the backend's leaf grant skips on — so a lifecycle spawn pays nothing. +//! +//! WHAT THIS DOES NOT DO. It does not copy Python or the MSVC toolchain, and must not: those are +//! the user's own, versions matter, and they are granted read where they already live. Nor is the +//! child's `PATH` replaced wholesale — only the source distribution is dropped from it. Dropping +//! the user's `git`/`python`/system entries would trade a jail that cannot start for packages that +//! cannot find their tools, and the ambient install needs no removal to be beaten anyway: an +//! un-ACE'd directory on `PATH` is SILENTLY skipped by cmd's probe, measured with the MSI dir +//! listed first and the child's own `execPath` still the nub-owned copy. It is dropped because a +//! shim that re-searches `PATH` (stock `npm.cmd`: `IF NOT EXIST "%~dp0\node.exe" SET +//! "NODE_EXE=node"`) is the class of defect that has defeated a caller-side allowlist before, and +//! removing the one entry closes it without costing the rest. + +// Windows-only in production, but COMPILED EVERYWHERE on purpose: the path rewrite and the +// distribution-shape checks are ordinary logic, and compiling them on the dev host is what lets +// their tests run there instead of only on a Windows runner. +#![cfg_attr(not(windows), allow(dead_code))] + +use std::collections::BTreeMap; +use std::path::{Path, PathBuf}; + +/// The staged distribution: what the child runs, and where it came from. +pub(super) struct JailBin { + /// The staged interpreter — `/node.exe`. + exe: PathBuf, + /// The staged distribution root, which is also the interpreter's bin dir (the Windows + /// layout is FLAT: `node.exe` with `node_modules` beside it, no `bin/`). + dir: PathBuf, + /// The distribution the copy was taken from. Removed from the child's `PATH`. + source_root: PathBuf, +} + +/// Refuse a source tree that is not shaped like a Node distribution before spending a copy on +/// it. The real payload is 2,435 entries / ~101 MiB (measured on both Windows images), so these +/// are loose enough to never bind on a real one and tight enough that a mis-set +/// `npm_node_execpath` cannot turn an install into an unbounded copy. +const MAX_ENTRIES: usize = 20_000; +const MAX_BYTES: u64 = 512 * 1024 * 1024; + +/// Stage the ambient interpreter into a nub-owned, AppContainer-readable directory, reusing a +/// previous stage when one is already published. +/// +/// `None` — any reason at all — leaves the caller's env untouched, i.e. the behavior before this +/// existed. Every failure here is a jail that grants the ambient path instead, which is a worse +/// outcome than staging but a better one than refusing to install. +pub(super) fn stage( + ambient: &BTreeMap, + probe: &super::build_jail::ProbeScope, +) -> Option { + let exec = PathBuf::from(ambient.get("npm_node_execpath")?); + // THE COPY SOURCE IS EXECUTED (to read its version) and then becomes the interpreter every + // lifecycle script on the machine runs, so it passes the same scope the Python probe does: a + // dependency that lands `node.exe` on the lifecycle `PATH` must not be able to nominate + // itself as the thing nub stages and blesses. + if !probe.allows(&exec) { + return None; + } + // The FLAT Windows layout is the only one this handles, and the file name is the + // discriminator (same rule as `build_jail::node_layout`). A POSIX `bin/node` reaching here + // would stage the bin dir and miss `lib/`, so it declines instead. + if !exec + .file_name() + .is_some_and(|n| n.to_string_lossy().eq_ignore_ascii_case("node.exe")) + { + return None; + } + let source_root = exec.parent()?.to_path_buf(); + let dest = super::build_prefetch::cache_root()? + .join("jail-bin") + .join(super::build_prefetch::node_dist_key(ambient, probe)?); + + if dest.join("node.exe").is_file() { + return Some(JailBin { + exe: dest.join("node.exe"), + dir: dest, + source_root, + }); + } + populate(&source_root, &dest)?; + Some(JailBin { + exe: dest.join("node.exe"), + dir: dest, + source_root, + }) +} + +/// Publish `source` at `dest` by copying into an ACE'd staging sibling and renaming. +/// +/// The rename is what makes the hit test above (`node.exe` exists) sound: `dest` is never +/// observable half-populated, so a concurrent install either sees nothing or sees a complete +/// tree. The ace is written on the staging directory while it is still EMPTY and travels with it +/// through the rename — an explicit ace is not recomputed by a same-volume move. +fn populate(source: &Path, dest: &Path) -> Option<()> { + let parent = dest.parent()?; + std::fs::create_dir_all(parent).ok()?; + // A `dest` that exists without `node.exe` cannot be a concurrent publish (those are atomic); + // it is a broken leftover in a directory nub owns and keys by version, so it is replaced. + if dest.exists() { + std::fs::remove_dir_all(dest).ok(); + } + let staging = tempfile::TempDir::new_in(parent).ok()?; + publish_appcontainer_read(staging.path()); + copy_tree(source, staging.path(), &mut Budget::default())?; + + let staged = staging.keep(); + match std::fs::rename(&staged, dest) { + Ok(()) => Some(()), + // A concurrent install published first — its tree is a copy of the same distribution, so + // adopt it rather than fail. Anything else leaves the interpreter unstaged. + Err(_) => { + std::fs::remove_dir_all(&staged).ok(); + dest.join("node.exe").is_file().then_some(()) + } + } +} + +/// BEST-EFFORT, deliberately. A staged copy with no AAP ace still fixes the defect — nub owns the +/// directory, so the backend's own per-run leaf grant succeeds on it unprivileged. What the ace +/// buys is that the per-run grant SKIPS instead of walking the tree, which is a ~400 ms/spawn +/// saving, not correctness. +#[cfg(windows)] +fn publish_appcontainer_read(dir: &Path) { + let _ = nub_sandbox::windows_publish_appcontainer_read(dir); +} + +#[cfg(not(windows))] +fn publish_appcontainer_read(_dir: &Path) {} + +#[derive(Default)] +struct Budget { + entries: usize, + bytes: u64, +} + +/// Copy `source`'s tree into `dest`, which already exists and already carries the ace. +/// +/// Symlinks and reparse points are SKIPPED rather than followed: the Windows Node archive +/// contains none, and following one would either copy an unbounded foreign tree in or leave the +/// jail reading through a link whose target it was never granted. +fn copy_tree(source: &Path, dest: &Path, budget: &mut Budget) -> Option<()> { + for entry in std::fs::read_dir(source).ok()? { + let entry = entry.ok()?; + let kind = entry.file_type().ok()?; + if kind.is_symlink() { + continue; + } + budget.entries += 1; + if budget.entries > MAX_ENTRIES { + return None; + } + let target = dest.join(entry.file_name()); + if kind.is_dir() { + std::fs::create_dir(&target).ok()?; + copy_tree(&entry.path(), &target, budget)?; + continue; + } + budget.bytes += entry.metadata().ok()?.len(); + if budget.bytes > MAX_BYTES { + return None; + } + std::fs::copy(entry.path(), &target).ok()?; + } + Some(()) +} + +impl JailBin { + /// Point the child at the staged copy: both interpreter spellings, and `PATH`. + /// + /// `NODE` is redirected as well as `npm_node_execpath`, which changes what it means inside the + /// jail. Outside it, `NODE` is nub's own PATH shim so a build script's `$NODE child.js` + /// re-enters nub augmented — but a jailed script runs on vanilla Node by contract + /// (`NODE_COMPAT=1` is set unconditionally for exactly that reason), so the shim's only + /// remaining effect would be a re-exec hop through a binary the jail does not grant. On + /// Windows the shim is also a HARDLINK to `nub.exe`, and a hardlink shares one MFT record and + /// therefore one security descriptor — so granting it would write nub's own binary's DACL + /// (measured: an ace written on a link read back on the original). Naming the staged copy + /// directly removes the hop and the leak together. + pub(super) fn redirect_env(&self, ambient: &mut BTreeMap) { + let exe = self.exe.to_string_lossy().into_owned(); + ambient.insert("npm_node_execpath".to_string(), exe.clone()); + ambient.insert("NODE".to_string(), exe); + if let Some(path) = ambient.get("PATH") { + ambient.insert("PATH".to_string(), self.rewrite_path(path)); + } + } + + /// `` in front, and every entry inside the source distribution dropped. Split out + /// from the env write so the ordering and the removal are testable without a Node on disk. + fn rewrite_path(&self, path: &str) -> String { + let kept = std::iter::once(self.dir.clone()) + .chain(std::env::split_paths(path).filter(|dir| !dir.starts_with(&self.source_root))); + std::env::join_paths(kept) + .map(|joined| joined.to_string_lossy().into_owned()) + .unwrap_or_else(|_| path.to_string()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn bin() -> JailBin { + JailBin { + exe: PathBuf::from("/cache/jail-bin/24.18.1-x64/node.exe"), + dir: PathBuf::from("/cache/jail-bin/24.18.1-x64"), + source_root: PathBuf::from("/progfiles/nodejs"), + } + } + + /// The staged dir leads, and the source distribution is gone — including the `npm/bin` + /// subdirectory some installers add separately, which is why the filter is prefix-based + /// rather than an equality test on the root. + #[test] + fn the_staged_dir_leads_and_the_source_distribution_is_dropped() { + let sep = if cfg!(windows) { ';' } else { ':' }; + let original = + ["/progfiles/nodejs", "/usr/bin", "/progfiles/nodejs/npm/bin"].join(&sep.to_string()); + let rewritten = bin().rewrite_path(&original); + let entries: Vec = std::env::split_paths(&rewritten).collect(); + assert_eq!( + entries, + vec![ + PathBuf::from("/cache/jail-bin/24.18.1-x64"), + PathBuf::from("/usr/bin"), + ] + ); + } + + /// Both interpreter spellings move, because a lifecycle script reaches the interpreter + /// through either — and `NODE` left pointing at nub's shim would send the child through a + /// binary the jail does not grant. + #[test] + fn both_interpreter_spellings_name_the_staged_copy() { + let mut env = BTreeMap::from([ + ( + "npm_node_execpath".to_string(), + "/progfiles/nodejs/node.exe".to_string(), + ), + ( + "NODE".to_string(), + "/tmp/nub-node-shim-1-abc/node.exe".to_string(), + ), + ]); + bin().redirect_env(&mut env); + let staged = "/cache/jail-bin/24.18.1-x64/node.exe"; + assert_eq!( + env.get("npm_node_execpath").map(String::as_str), + Some(staged) + ); + assert_eq!(env.get("NODE").map(String::as_str), Some(staged)); + // No PATH in, no PATH out: the jail's env is an allowlist and inventing a key it did not + // carry would put the staged dir on a child that had no PATH at all. + assert!(!env.contains_key("PATH")); + } +} diff --git a/crates/nub-cli/src/pm_engine/mod.rs b/crates/nub-cli/src/pm_engine/mod.rs index 4fecf66fd..b8bfa761e 100644 --- a/crates/nub-cli/src/pm_engine/mod.rs +++ b/crates/nub-cli/src/pm_engine/mod.rs @@ -57,11 +57,14 @@ pub mod build_jail; // allowlist and the fail-soft contract. mod build_prefetch; mod bun_config; +// The nub-owned Node copy the Windows build jail runs on, because a leaf read grant is an ACE and +// the stock MSI installs where a standard user cannot write one. See its module doc. pub mod config_scope; mod expo_compat; pub mod identity; pub mod info_family; pub mod install_family; +mod jail_bin; pub mod log; pub mod min_release_age; pub mod output; diff --git a/crates/nub-sandbox/src/backend/mod.rs b/crates/nub-sandbox/src/backend/mod.rs index 1ee83f7fc..7f0c5f705 100644 --- a/crates/nub-sandbox/src/backend/mod.rs +++ b/crates/nub-sandbox/src/backend/mod.rs @@ -121,6 +121,8 @@ pub fn earliest_bootstrap() -> std::io::Result { #[cfg(any(target_os = "windows", test))] mod windows; #[cfg(target_os = "windows")] +pub use windows::windows_publish_appcontainer_read; +#[cfg(target_os = "windows")] #[doc(hidden)] pub use windows::{ windows_ancestor_capability_sids, windows_capability_fallbacks, windows_leaf_grant_redundant, diff --git a/crates/nub-sandbox/src/backend/windows.rs b/crates/nub-sandbox/src/backend/windows.rs index 720c04283..5e246aa02 100644 --- a/crates/nub-sandbox/src/backend/windows.rs +++ b/crates/nub-sandbox/src/backend/windows.rs @@ -879,6 +879,34 @@ pub fn windows_leaf_grant_redundant(dir: &std::path::Path) -> bool { launch::leaf_read_grant_redundant(dir) } +/// Publish `dir` to every AppContainer as read+execute, inheritably — the ONE grant an embedder +/// writes AHEAD of a launch rather than per-run, and the reason a nub-owned interpreter copy costs +/// nothing at spawn time. +/// +/// CALL THIS ON AN EMPTY DIRECTORY, THEN POPULATE IT. The ace is inheritable, so children pick it +/// up AT CREATION and there is no propagation pass; writing the same ace over an already-populated +/// tree is a walk, and the two are not close (measured on `windows-latest`: 24 ms on an empty +/// directory against 426 ms re-granting a 2,435-entry Node distribution — run 30517506683). The +/// per-launch saving is the same number again: an inheritable AAP ace is exactly what +/// [`windows_leaf_grant_redundant`] looks for, so the backend's own leaf grant on this directory +/// SKIPS, and a per-run package sid — which would have to be written every spawn — is never needed. +/// +/// The trustee is the STABLE `ALL APPLICATION PACKAGES` rather than a per-run profile sid, and that +/// is sound because a zero-capability LowBox token reads through it (measured — it is why System32 +/// is readable at all). What it costs is that the directory becomes readable to every AppContainer +/// on the machine, so an embedder may only publish a tree whose contents are already public: the +/// intended one is a copy of the user's own Node distribution, which is public bytes from +/// nodejs.org. +/// +/// Needs no elevation on any path a user owns, which is the whole point — it is the escape from +/// writing a DACL somewhere a standard user cannot (`%ProgramFiles%\nodejs`, `C:\hostedtoolcache`), +/// measured as `PrivilegeNotHeldException` there and as a clean write plus read-back under a +/// restricted token on nub's own directory. +#[cfg(target_os = "windows")] +pub fn windows_publish_appcontainer_read(dir: &std::path::Path) -> std::io::Result<()> { + launch::publish_appcontainer_read(dir) +} + // ── the FFI launcher ──────────────────────────────────────────────────────────── #[cfg(target_os = "windows")] @@ -1186,6 +1214,19 @@ pub(super) mod launch { already_granted_to_appcontainers(dir, GENERIC_READ | GENERIC_EXECUTE) } + /// See [`super::windows_publish_appcontainer_read`]. + #[doc(hidden)] + pub(super) fn publish_appcontainer_read(dir: &Path) -> io::Result<()> { + let sid = CapSid::new(ALL_APPLICATION_PACKAGES_SID)?; + set_ace( + dir, + sid.0, + GENERIC_READ | GENERIC_EXECUTE, + GRANT_ACCESS, + true, + ) + } + /// Each granted path's STRICT ancestors, deduped and ordered shallowest-first. These are /// the directories Node's `realpathSync` opens as targets on its way to a granted leaf. A /// grant that is itself an ancestor of another grant is included, and simply takes the @@ -1692,16 +1733,38 @@ pub(super) mod launch { GENERIC_READ | GENERIC_WRITE | GENERIC_EXECUTE | DELETE, ) })); + // A REFUSED **READ** GRANT IS SKIPPED, NOT FATAL — same reasoning as the + // ancestor chain in 2b, which the read grants share a failure mode with. + // Writing a DACL needs `WRITE_DAC`, and a read grant may legitimately name a + // path the user does not hold it on: a toolchain outside their profile + // (`C:\hostedtoolcache`, an all-users Python) is granted read, and taking `?` + // there aborted EVERY lifecycle script on the machine over one unreachable + // toolchain — the loudest possible failure for the mildest cause. Skipping + // cannot open the jail: a grant is a REDUCTION from the unconfined lifecycle + // spawn's complete access, so a grant not installed leaves the child with + // LESS, and the worst outcome is one package failing to find one tool. The + // interpreter no longer relies on this — nub stages a copy it owns (see + // `pm_engine::jail_bin`) — which is what makes it a genuine safety net rather + // than the mechanism. + // + // WRITE grants stay FATAL. Every one of them is nub's own private tmp or the + // package directory being built, both under the user's own tree, so a refusal + // there is not a reachable configuration — it is a broken assumption, and + // continuing would launch a build that silently cannot write its output. for (kind, dir, access) in leaves { - let installed = grant_leaf_ace(dir, ac_sid, access).map_err(|error| { - io::Error::new( - error.kind(), - format!( - "sandbox: installing {kind} grant ACE on {} failed: {error}", - dir.display() - ), - ) - })?; + let installed = match grant_leaf_ace(dir, ac_sid, access) { + Ok(installed) => installed, + Err(_) if kind == "read" => continue, + Err(error) => { + return Err(io::Error::new( + error.kind(), + format!( + "sandbox: installing {kind} grant ACE on {} failed: {error}", + dir.display() + ), + )); + } + }; if installed && !_aces.paths.contains(dir) { _aces.paths.push(dir.clone()); } diff --git a/crates/nub-sandbox/src/compiler/preset.rs b/crates/nub-sandbox/src/compiler/preset.rs index 6b08afbdf..d74120424 100644 --- a/crates/nub-sandbox/src/compiler/preset.rs +++ b/crates/nub-sandbox/src/compiler/preset.rs @@ -91,21 +91,40 @@ pub fn grant_build_jail_interpreter(name: &str, policy: &mut SandboxPolicy, ctx: /// (below) plus the resolved Python's own closure, whose derivation and bounds live with /// the embedder because it owns where each toolchain comes from. /// -/// The Node pair is its C/C++ header dir -/// (`/include/node`) and `/lib/node_modules`. node-gyp compiles an -/// addon against the headers under `npm_config_nodedir/include/node`, and `npm`/`npx`/ -/// `corepack` are each a symlink into `lib/node_modules`, so without it they dangle and -/// the standard `prebuild-install || npm run build` fallback dies at `npm: not found`. -/// nub provisions Node under its version store (`~/.cache/nub/node/`) — a path in -/// neither `$tooldirs` nor the interpreter grant (which covers only `bin/node` + the bin -/// dir). Without these grants node-gyp finds no local headers and falls back to a network -/// header download — reachable now that `nodejs.org` is allowed, but it re-fetches the -/// headers on a cold cache for every native build the jail runs, and on an offline or -/// air-gapped host the whole native-compile ecosystem fails outright. The grant is what -/// keeps the offline path working. The embedder supplies the concrete paths — it owns where nub puts Node, -/// and keeps the grant on SUBTREES rather than the bare root, which for a system Node is -/// a shared prefix. A nonexistent path (a system Node shipping no headers) yields an -/// inert allow. +/// The Node pair is the GLOBAL PACKAGE TREE and the C/C++ HEADERS, and both are spelled by +/// the embedder because BOTH DIFFER BY DISTRIBUTION SHAPE — the layouts are not variants of +/// one path, they are two different answers: +/// +/// - The global tree is `/lib/node_modules` on POSIX and `/node_modules` +/// on Windows, whose archive is FLAT (`node.exe` with `node_modules` beside it, no `bin/` +/// and no `lib/`). It is what makes `npm`/`npx`/`corepack` resolvable at all — each is a +/// symlink into that tree on POSIX, a `%~dp0`-relative `.cmd` shim on Windows — and on POSIX +/// it is genuinely load-bearing, because it sits OUTSIDE the granted bin dir (`../lib/…`): +/// without it all three dangle and the standard `prebuild-install || npm run build` fallback +/// dies at `npm: not found` (measured on `keytar`: rc 127 → rc 0 once the target is +/// readable). On the FLAT layout it is redundant rather than load-bearing, since read grants +/// are subtree grants ([`push_read_path`]) and the interpreter's own directory IS the +/// distribution root — spelled anyway, so the grant states what the jail needs instead of +/// depending on that coincidence. +/// - The headers are `/include/node` **only where the distribution ships them**, +/// which the Windows one does not — verified against `node-v24.18.1-win-x64.zip`'s central +/// directory (zero `include/`, `.h` or `.lib` entries). So `include/node` is a POSIX answer, +/// and the embedder asks the DISK rather than the platform: where the directory exists it +/// names the distribution root, and where it does not it prefetches the `-headers.tar.gz` +/// plus `node.lib` OUT of jail into a tree nub owns and names that instead. +/// +/// Either way `npm_config_nodedir` names the granted tree, which is what makes the grant +/// load-bearing rather than an optimisation. node-gyp reads `/include/node/*` and +/// `/$(Configuration)/.lib` when nodedir is set, and when it is NOT set it +/// DOWNLOADS the headers into its own `%LOCALAPPDATA%\node-gyp\Cache` / `~/.cache/node-gyp` +/// devDir (node-gyp 13 `lib/configure.js` `getNodeDir`, `lib/install.js`) — a fetch the jail's +/// deny-all egress refuses, so an ungranted nodedir is not a slow native build, it is no +/// native build at all. nub provisions Node under its version store (`~/.cache/nub/node/ +/// `), a path in neither `$tooldirs` nor the interpreter grant, so nothing else covers it. +/// +/// The embedder keeps the grant on SUBTREES rather than the bare root, which for a system Node +/// is a shared prefix carrying unrelated `etc/`/`var/`. A nonexistent path yields an inert +/// allow, which is what lets the embedder pass a speculative spelling without probing first. /// Front-inserted as base allows so the reasserted secret/`.env` floor stays authoritative; /// these paths never overlap a secret. fn grant_build_jail_extra_reads(policy: &mut SandboxPolicy, extra_reads: &[PathBuf]) { @@ -467,9 +486,10 @@ fn build_jail_net() -> Value { /// aube-process env plus the command's overlay), already reconstructed by the caller. /// `interpreter` is the closure to grant read (the provisioned Node + shim); each /// path and its bin dir become read grants. `extra_reads` are additional per-spawn read -/// subtrees the embedder derives (the provisioned Node's `include/node` headers so node-gyp -/// compiles offline, and its `lib/node_modules` so `npm`/`npx` resolve) — see -/// [`grant_build_jail_extra_reads`]. +/// subtrees the embedder derives — the headers node-gyp compiles against and the global +/// package tree `npm`/`npx` resolve through, both spelled per distribution shape (POSIX +/// `include/node` + `lib/node_modules`; Windows a prefetched header tree + a flat +/// `node_modules`) — see [`grant_build_jail_extra_reads`]. pub fn compile_build_jail( homes: Homes, package_dir: &Path, @@ -561,6 +581,34 @@ mod tests { /// declines on the denies-only static skeleton; that difference is exactly why the /// pure-allowlist invariant is asserted on both. fn production_build_jail_policy() -> SandboxPolicy { + let (interpreter, extra_reads) = POSIX_LAYOUT; + production_build_jail_policy_for(interpreter, extra_reads) + } + + /// The two DISTRIBUTION SHAPES the embedder can hand `compile_build_jail`, as + /// (interpreter, extra reads). They are not variants of one path — see + /// [`grant_build_jail_extra_reads`] — so both are exercised rather than the POSIX one + /// standing in for both, which is how the Windows spelling stayed inert unnoticed. + /// + /// Spelled POSIX-style because the compiler is OS-agnostic and the drive letter is not what + /// differs: the SHAPE is (nested `bin/` + sibling `lib/` + in-tree headers) against (flat + /// root + headers in a separately-prefetched tree). + const POSIX_LAYOUT: (&str, &[&str]) = ( + "/testhome/.cache/nub/node/v26/bin/node", + &[ + "/testhome/.cache/nub/node/v26/include/node", + "/testhome/.cache/nub/node/v26/lib/node_modules", + ], + ); + const FLAT_LAYOUT: (&str, &[&str]) = ( + "/testhome/.cache/nub/pm/jail-bin/24.18.1-x64/node.exe", + &[ + "/testhome/.cache/nub/pm/node-headers/24.18.1", + "/testhome/.cache/nub/pm/jail-bin/24.18.1-x64/node_modules", + ], + ); + + fn production_build_jail_policy_for(interpreter: &str, extra_reads: &[&str]) -> SandboxPolicy { let homes = Homes { home: PathBuf::from("/testhome"), tmp: PathBuf::from("/testtmp"), @@ -570,15 +618,51 @@ mod tests { compile_build_jail( homes, Path::new("/proj/node_modules/somepkg"), - vec![PathBuf::from("/testhome/.cache/nub/node/v26/bin/node")], - vec![PathBuf::from( - "/testhome/.cache/nub/node/v26/lib/node_modules", - )], + vec![PathBuf::from(interpreter)], + extra_reads.iter().map(PathBuf::from).collect(), BTreeMap::new(), ) .expect("build-jail compiles") } + /// Each shape's toolchain reads actually LAND — the headers node-gyp compiles against and + /// the global package tree `npm` resolves through — while a sibling of the distribution + /// stays refused. + /// + /// The refused sibling is what makes this non-vacuous: asserting only that the two trees are + /// readable would pass just as well if the grant had widened to the whole cache home. And + /// asserting it per SHAPE is the point — the Windows spellings were a POSIX path and a + /// directory that does not exist in that archive at all, which a POSIX-only fixture cannot + /// distinguish from a working grant. + #[test] + fn both_distribution_shapes_grant_their_headers_and_global_package_tree() { + for (label, (interpreter, extra_reads)) in [("posix", POSIX_LAYOUT), ("flat", FLAT_LAYOUT)] + { + let policy = production_build_jail_policy_for(interpreter, extra_reads); + let m = crate::matcher::PathMatcher::new(&policy.fs.rules); + for read in extra_reads { + let deep = Path::new(read).join("npm/bin/npm-cli.js"); + assert_eq!( + m.decide(&deep).effect, + Effect::Allow, + "{label}: {} must be readable", + deep.display() + ); + } + let sibling = Path::new(interpreter) + .parent() + .and_then(Path::parent) + .expect("the fixture interpreter has a grandparent") + .join("unrelated/secret.txt"); + assert_ne!( + m.decide(&sibling).effect, + Effect::Allow, + "{label}: the grant widened past the distribution to {}", + sibling.display() + ); + } + } + /// THE build-jail invariant: the compiled policy is a PURE ALLOWLIST — zero deny rules. /// Rationale on [`enforce_pure_allowlist`]. Asserted on BOTH entry points because they /// reach the fold differently: the static skeleton folds to denies-only so the env-deny diff --git a/crates/nub-sandbox/src/lib.rs b/crates/nub-sandbox/src/lib.rs index f34c53a7d..455cffce1 100644 --- a/crates/nub-sandbox/src/lib.rs +++ b/crates/nub-sandbox/src/lib.rs @@ -101,6 +101,8 @@ pub mod policy; pub mod preflight; pub mod proxy; +#[cfg(target_os = "windows")] +pub use backend::windows_publish_appcontainer_read; pub use backend::{ CommandArgs, CommandSpec, Degradation, Prepared, PreparedChild, PreparedSignalTarget, RuntimeCapability, StatusReport, apply, apply_with_runtime, earliest_bootstrap, From f72cdec8430cb2148aed1db85aa591062ca62d48 Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Thu, 30 Jul 2026 05:27:02 -0700 Subject: [PATCH 2/8] probe(win): measure the interpreter differential de-elevated, against a real MSI Node MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The de-elevated differential's production arm granted the PROBE BINARY as the interpreter — a file in its own fixture, which nub owns — so it has always passed and could never have seen the defect it was meant to cover. A real install grants the user's own Node, and that is the one path the ACE write can fail on. So the staging mechanism moves into the engine (backend::windows_jail_bin::stage_appcontainer_readable_copy) and the probe drives the same entry the product does, rather than a re-implementation of it. nub-cli keeps every PM decision: which interpreter, the cache key, what counts as a complete previous stage, and the env rewrite. PM-purity is unaffected — the engine function takes two paths and returns io::Result. The new group runs one lifecycle script twice under the same real compile_build_jail policy, through cmd.exe so the child's own `node` is a nested spawn from inside the AppContainer, with one variable: ambient install or nub-owned copy. The ambient arm is the control — without an arm where the defect still reproduces, a green staged arm measures nothing. The script resolves `node` bare so a PATH probe of an un-ACE'd tree is exercised too, and carries no pipe, because a piped child_process spawn hangs under an AppContainer for unrelated reasons and a hang would be indistinguishable from an unreadable interpreter. Three cells keep it honest. The child reports its own process.execPath, so a launch that succeeded while quietly running the ambient interpreter cannot read as a pass — which matters now that a refused read grant is skipped rather than fatal. Both arms report process.versions.modules, so the ABI claim is the copy compared against its own source rather than against a tabulated expectation. And the harness reports whether the SOURCE install already publishes read to AppContainers: on one that does, the ambient arm cannot fail, and the verdict declares the run VOID rather than green. The workflow installs a stock Node MSI to %ProgramFiles%\nodejs, which is the configuration that exhibits this and the one a real user has; without it the probe measures the runner's unzipped hostedtoolcache tree instead. That step needs admin, which is not a finding about nub — a user installs Node with the same consent — and the de-elevated arm afterwards holds no authority to change the DACL it left. --- .github/workflows/win-jail-interp-probe.yml | 185 +++++++++++ crates/nub-cli/src/pm_engine/jail_bin.rs | 123 ++----- crates/nub-sandbox/src/backend/mod.rs | 6 + .../src/backend/windows_jail_bin.rs | 171 ++++++++++ .../tests/windows_deelevated_jail.rs | 314 +++++++++++++++++- 5 files changed, 701 insertions(+), 98 deletions(-) create mode 100644 .github/workflows/win-jail-interp-probe.yml create mode 100644 crates/nub-sandbox/src/backend/windows_jail_bin.rs diff --git a/.github/workflows/win-jail-interp-probe.yml b/.github/workflows/win-jail-interp-probe.yml new file mode 100644 index 000000000..5468897f4 --- /dev/null +++ b/.github/workflows/win-jail-interp-probe.yml @@ -0,0 +1,185 @@ +# Branch-scoped ad-hoc probe: can the Windows build jail start a lifecycle script for a STANDARD +# USER whose Node is an all-users MSI install? No pull request required +# (.claude/skills/ci-adhoc-test/SKILL.md). CI is the only venue — AppContainer cannot be launched +# over SSH (session 0 has no window station, so every launch returns 0xC0000142), which also rules +# out the standing nub-win VM. +# +# WHAT IS MEASURED. A leaf read grant is an ACE, which needs WRITE_DAC on the target; the stock +# Node MSI installs to %ProgramFiles%\nodejs, where a standard user does not hold it. And even +# where nub can write it, CreateProcessW opens the image in the CALLER's context, so once the +# caller is itself in the AppContainer the un-ACE'd image is refused anyway. The fix is that the +# interpreter is a nub-owned COPY of the same distribution, published AppContainer-readable by +# granting the EMPTY directory and letting the copy inherit. This runs the same lifecycle script +# against both interpreters, under the same real compile_build_jail policy, on both an ELEVATED and +# a DE-ELEVATED token. +# +# THE MSI STEP IS THE POINT, and it needs admin — which is not a finding about nub, because a user +# installs Node with the same consent. What matters is the DACL the installer leaves behind, and +# that the de-elevated arm afterwards holds no authority to change it. Without this step the probe +# would measure the runner image's unzipped hostedtoolcache Node, which is a different tree with a +# different (also un-ACE'd) DACL — usable, but not the configuration a real user has. +# +# THE CONTROLS. `interp-ambient-install-cannot-run-a-lifecycle-script` is the arm where the defect +# still reproduces: without it a green staged arm measures nothing. +# `interp-staged-child-execpath-is-the-nub-owned-copy` is the attribution control — a refused read +# grant is now skipped rather than fatal, so a launch can succeed while the child quietly ran the +# ambient interpreter, and only the child's own execPath tells those apart. +# `fact:interp-source-publishes-aap` decides whether the run could exhibit the defect at all: on an +# install that already publishes read to AppContainers the ambient arm passes for a reason that has +# nothing to do with nub, which makes the differential VOID rather than green. +name: win-jail-interp-probe + +on: + push: + branches: [sandbox/win-jail-interp] + paths: + - 'crates/nub-sandbox/**' + - 'crates/nub-cli/src/pm_engine/jail_bin.rs' + - '.github/workflows/win-jail-interp-probe.yml' + workflow_dispatch: + +permissions: + contents: read + +jobs: + windows: + runs-on: windows-latest + timeout-minutes: 45 + env: + # Matches the sibling probes: crt-static makes the self-reexec probe child self-contained, so + # it starts cleanly under the LowBox token. + RUSTFLAGS: "-C target-feature=+crt-static" + steps: + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@stable + - uses: Swatinem/rust-cache@v2 + + - name: Runner baseline + shell: powershell + run: | + $id=[System.Security.Principal.WindowsIdentity]::GetCurrent() + $admin=(New-Object System.Security.Principal.WindowsPrincipal($id)).IsInRole([System.Security.Principal.WindowsBuiltinRole]::Administrator) + Write-Host "BASELINE user=$($id.Name) IsElevated=$admin os=$([System.Environment]::OSVersion.VersionString)" + + # The configuration under test. Recipe lifted from tests/win-msi-volume/msi-node-acl.ps1: the + # dist index picks the VERSION and a HEAD request decides whether the artifact exists, because + # index.json never lists win-arm64-msi for any release even where the .msi is served. + - name: Install a stock Node MSI to %ProgramFiles%\nodejs + shell: powershell + run: | + $ErrorActionPreference = 'Stop' + $arch = if ($env:PROCESSOR_ARCHITECTURE -eq 'ARM64') { 'arm64' } else { 'x64' } + $idx = Invoke-RestMethod -Uri 'https://nodejs.org/dist/index.json' -TimeoutSec 60 + $ver = $null + foreach ($r in @($idx | Where-Object { $_.lts })) { + $v = $r.version.TrimStart('v') + try { + if ((Invoke-WebRequest -Uri "https://nodejs.org/dist/v$v/node-v$v-$arch.msi" -Method Head -TimeoutSec 30).StatusCode -eq 200) { $ver = $v; break } + } catch { } + } + if (-not $ver) { throw "no LTS release serves a $arch .msi" } + $msi = Join-Path $env:TEMP "node-$ver-$arch.msi" + Invoke-WebRequest -Uri "https://nodejs.org/dist/v$ver/node-v$ver-$arch.msi" -OutFile $msi -TimeoutSec 300 + $p = Start-Process -FilePath msiexec.exe -Wait -PassThru -ArgumentList @('/i', "`"$msi`"", '/qn', '/norestart') + Write-Host "msiexec-exit=$($p.ExitCode) version=$ver" + if ($p.ExitCode -ne 0 -and $p.ExitCode -ne 3010) { throw "msiexec failed" } + $node = Join-Path $env:ProgramFiles 'nodejs\node.exe' + if (-not (Test-Path -LiteralPath $node)) { throw "no node.exe at $node" } + Write-Host "installed=$node" + + # THE FACT THE WHOLE PROBE RESTS ON. If this DACL already grants ALL APPLICATION PACKAGES, + # the ambient arm cannot fail and the differential is void — so it is dumped rather than + # assumed, and the harness reports its own read of it as `fact:interp-source-publishes-aap`. + - name: The DACL the MSI left behind + if: always() + shell: powershell + run: | + icacls "$env:ProgramFiles\nodejs" + icacls "$env:ProgramFiles\nodejs\node.exe" + + # Separate from the run so a compile error reads as a compile error rather than as a probe + # that measured nothing. + - name: Build the probe + shell: bash + run: cargo test -p nub-sandbox --test windows_deelevated_jail --no-run + + - name: Probe — paired elevated / de-elevated, ambient vs nub-owned interpreter + if: always() + timeout-minutes: 25 + shell: bash + run: | + rc=0 + timeout -k 30 1200 cargo test -p nub-sandbox --test windows_deelevated_jail > probe.log 2>&1 || rc=$? + echo "probe exit=$rc" + cat probe.log + + # The verdict is read off the LOG, never off a step's exit status: a property whose `prop:` + # line is ABSENT fails here too, so a probe that never reached an arm cannot be mistaken for + # one that passed. + - name: Verdict + if: always() + shell: bash + run: | + fail=0 + need() { + if grep -qF "$1" probe.log 2>/dev/null; then echo "OK $1" + else echo "MISS $1"; fail=1; fi + } + need_pass() { + if grep -qF "prop:$1=PASS" probe.log 2>/dev/null; then echo "PASS $1" + elif grep -qF "prop:$1=FAIL" probe.log 2>/dev/null; then echo "FAIL $1"; fail=1 + else echo "MISS $1 (never measured)"; fail=1; fi + } + [ -s probe.log ] || { echo "MISS the probe produced no output at all"; exit 1; } + + echo "---- both arms ran, and arm B was really de-elevated ----" + need 'ARM elevated=1' + grep -q 'ARM .*admin=1' probe.log || { echo "MISS no arm held administrative authority — the arms did not differ"; fail=1; } + grep -q 'ARM .*admin=0' probe.log || { echo "MISS the DE-ELEVATED arm never reported"; fail=1; } + need 'DEELEV route=' + + echo "---- the interpreter the run actually measured ----" + grep -E '^ fact:interp-(source|source-publishes-aap|stage-ms)' probe.log || { echo "MISS the interpreter facts"; fail=1; } + # VOIDS THE DIFFERENTIAL rather than merely weakening it: an already-AppContainer-readable + # source install cannot exhibit the defect, so the ambient arm's failure would have to + # come from somewhere else. + if grep -q '^ fact:interp-source-publishes-aap=true' probe.log; then + echo "VOID the source install already publishes read to AppContainers — the ambient arm is not the defect" + fail=1 + fi + grep -qE '^ fact:interp-source=.*[Pp]rogram [Ff]iles' probe.log || { echo "MISS the probe did not measure the MSI install"; fail=1; } + + echo "---- the differential (the control first: it must still reproduce) ----" + need_pass interp-ambient-install-cannot-run-a-lifecycle-script + need_pass interp-staged-copy-runs-a-lifecycle-script-to-completion + need_pass interp-staged-child-execpath-is-the-nub-owned-copy + need_pass interp-staging-succeeds + need_pass interp-staged-abi-matches-the-source + + echo "---- the cost claim: no per-spawn DACL write over the tree ----" + need_pass interp-staged-dir-publishes-read-to-appcontainers + need_pass interp-staged-deep-entry-inherited-the-ace-at-creation + + echo "---- confinement survives it ----" + need_pass interp-staged-ungranted-secret-still-refused + + echo "---- the pre-existing groups must not have regressed ----" + for p in profile-create-and-launch acl-grant-allow acl-grant-deny teardown \ + job-reap egress-deny production-jail-launch production-jail-egress; do + need_pass "$p" + done + + need 'WINDOWS BUILD JAIL HOLDS WITH NO ELEVATION' + echo "---- every fact ----" + grep -E '^ fact:' probe.log || true + echo "---- every property ----" + grep -E '^ prop:' probe.log || true + exit $fail + + - name: Upload probe log + if: always() + uses: actions/upload-artifact@v4 + with: + name: win-jail-interp-probe + path: probe.log + if-no-files-found: warn diff --git a/crates/nub-cli/src/pm_engine/jail_bin.rs b/crates/nub-cli/src/pm_engine/jail_bin.rs index 2cf9f2bac..a84311343 100644 --- a/crates/nub-cli/src/pm_engine/jail_bin.rs +++ b/crates/nub-cli/src/pm_engine/jail_bin.rs @@ -26,10 +26,13 @@ //! to load. The copy is byte-identical to the project's own distribution and keyed by the version //! it reported, which makes the constraint structural rather than a check. //! -//! GRANT FIRST, THEN POPULATE. [`nub_sandbox::windows_publish_appcontainer_read`] carries why: -//! the ace is inheritable, so writing it on the EMPTY directory has every entry inherit at -//! creation (24 ms) instead of walking a populated 2,435-entry tree (426 ms), and an inheritable -//! AAP ace is exactly what the backend's leaf grant skips on — so a lifecycle spawn pays nothing. +//! WHAT LIVES WHERE. The publishing MECHANISM — grant the empty directory, copy into it, rename +//! into place — is the engine's +//! (`nub_sandbox::backend::windows_jail_bin::stage_appcontainer_readable_copy`), because it is +//! Windows confinement plumbing with no PM knowledge in it, and because that is what lets the +//! branch-scoped Windows probe drive the same code this does rather than a re-implementation of +//! it. What stays HERE is every PM decision: which interpreter, what the cache key is, when a +//! previous stage counts as complete, and how the child's env is rewritten. //! //! WHAT THIS DOES NOT DO. It does not copy Python or the MSVC toolchain, and must not: those are //! the user's own, versions matter, and they are granted read where they already live. Nor is the @@ -61,13 +64,6 @@ pub(super) struct JailBin { source_root: PathBuf, } -/// Refuse a source tree that is not shaped like a Node distribution before spending a copy on -/// it. The real payload is 2,435 entries / ~101 MiB (measured on both Windows images), so these -/// are loose enough to never bind on a real one and tight enough that a mis-set -/// `npm_node_execpath` cannot turn an install into an unbounded copy. -const MAX_ENTRIES: usize = 20_000; -const MAX_BYTES: u64 = 512 * 1024 * 1024; - /// Stage the ambient interpreter into a nub-owned, AppContainer-readable directory, reusing a /// previous stage when one is already published. /// @@ -100,98 +96,41 @@ pub(super) fn stage( .join("jail-bin") .join(super::build_prefetch::node_dist_key(ambient, probe)?); - if dest.join("node.exe").is_file() { - return Some(JailBin { - exe: dest.join("node.exe"), - dir: dest, - source_root, - }); - } - populate(&source_root, &dest)?; - Some(JailBin { + let bin = JailBin { exe: dest.join("node.exe"), dir: dest, source_root, - }) -} - -/// Publish `source` at `dest` by copying into an ACE'd staging sibling and renaming. -/// -/// The rename is what makes the hit test above (`node.exe` exists) sound: `dest` is never -/// observable half-populated, so a concurrent install either sees nothing or sees a complete -/// tree. The ace is written on the staging directory while it is still EMPTY and travels with it -/// through the rename — an explicit ace is not recomputed by a same-volume move. -fn populate(source: &Path, dest: &Path) -> Option<()> { - let parent = dest.parent()?; - std::fs::create_dir_all(parent).ok()?; - // A `dest` that exists without `node.exe` cannot be a concurrent publish (those are atomic); - // it is a broken leftover in a directory nub owns and keys by version, so it is replaced. - if dest.exists() { - std::fs::remove_dir_all(dest).ok(); + }; + // `node.exe` IS the completeness test, and the engine's publish-by-rename is what makes it + // sound: the destination is never observable half-populated, so its presence means the whole + // distribution is there. Which file that is, is PM knowledge, so the engine asks the caller. + if bin.exe.is_file() { + return Some(bin); } - let staging = tempfile::TempDir::new_in(parent).ok()?; - publish_appcontainer_read(staging.path()); - copy_tree(source, staging.path(), &mut Budget::default())?; - - let staged = staging.keep(); - match std::fs::rename(&staged, dest) { - Ok(()) => Some(()), - // A concurrent install published first — its tree is a copy of the same distribution, so - // adopt it rather than fail. Anything else leaves the interpreter unstaged. - Err(_) => { - std::fs::remove_dir_all(&staged).ok(); - dest.join("node.exe").is_file().then_some(()) - } + // A leftover directory without `node.exe` cannot be a concurrent publish (those are atomic) — + // it is broken state in a directory nub owns and keys by version, so it is replaced. + if bin.dir.exists() { + std::fs::remove_dir_all(&bin.dir).ok(); } + if publish(&bin.source_root, &bin.dir) { + return Some(bin); + } + // A concurrent install may have published first, in which case its tree is a copy of the same + // distribution and is adopted rather than refused. + bin.exe.is_file().then_some(bin) } -/// BEST-EFFORT, deliberately. A staged copy with no AAP ace still fixes the defect — nub owns the -/// directory, so the backend's own per-run leaf grant succeeds on it unprivileged. What the ace -/// buys is that the per-run grant SKIPS instead of walking the tree, which is a ~400 ms/spawn -/// saving, not correctness. #[cfg(windows)] -fn publish_appcontainer_read(dir: &Path) { - let _ = nub_sandbox::windows_publish_appcontainer_read(dir); +fn publish(source: &Path, dest: &Path) -> bool { + nub_sandbox::backend::windows_jail_bin::stage_appcontainer_readable_copy(source, dest).is_ok() } +/// Staging exists for a Windows ACE constraint, so off Windows there is nothing to publish and +/// [`stage`] always declines. Present as a stub rather than `cfg`-ing the module out, so the env +/// rewrite and its tests compile — and run — on the dev host. #[cfg(not(windows))] -fn publish_appcontainer_read(_dir: &Path) {} - -#[derive(Default)] -struct Budget { - entries: usize, - bytes: u64, -} - -/// Copy `source`'s tree into `dest`, which already exists and already carries the ace. -/// -/// Symlinks and reparse points are SKIPPED rather than followed: the Windows Node archive -/// contains none, and following one would either copy an unbounded foreign tree in or leave the -/// jail reading through a link whose target it was never granted. -fn copy_tree(source: &Path, dest: &Path, budget: &mut Budget) -> Option<()> { - for entry in std::fs::read_dir(source).ok()? { - let entry = entry.ok()?; - let kind = entry.file_type().ok()?; - if kind.is_symlink() { - continue; - } - budget.entries += 1; - if budget.entries > MAX_ENTRIES { - return None; - } - let target = dest.join(entry.file_name()); - if kind.is_dir() { - std::fs::create_dir(&target).ok()?; - copy_tree(&entry.path(), &target, budget)?; - continue; - } - budget.bytes += entry.metadata().ok()?.len(); - if budget.bytes > MAX_BYTES { - return None; - } - std::fs::copy(entry.path(), &target).ok()?; - } - Some(()) +fn publish(_source: &Path, _dest: &Path) -> bool { + false } impl JailBin { diff --git a/crates/nub-sandbox/src/backend/mod.rs b/crates/nub-sandbox/src/backend/mod.rs index 7f0c5f705..52b9ed2da 100644 --- a/crates/nub-sandbox/src/backend/mod.rs +++ b/crates/nub-sandbox/src/backend/mod.rs @@ -129,6 +129,12 @@ pub use windows::{ windows_object_traverse_ace, }; +// Publishing a nub-owned, AppContainer-readable copy of a tool tree the jail must RUN — the +// escape from writing an ACE where a standard user cannot. Same cfg as `windows`: the copy half is +// ordinary fs work and is tested on the dev host, only the ace needs Windows. +#[cfg(any(target_os = "windows", test))] +pub mod windows_jail_bin; + // The Windows dedicated-account + WFP backend (agent-sandbox). Same cfg as `windows` so its // OS-agnostic plan derivation and mode-selection are unit-tested on the macOS dev host. #[cfg(any(target_os = "windows", test))] diff --git a/crates/nub-sandbox/src/backend/windows_jail_bin.rs b/crates/nub-sandbox/src/backend/windows_jail_bin.rs new file mode 100644 index 000000000..79da41fc2 --- /dev/null +++ b/crates/nub-sandbox/src/backend/windows_jail_bin.rs @@ -0,0 +1,171 @@ +//! Publishing a nub-owned, AppContainer-readable COPY of a tool tree the jail must run. +//! +//! WHY THE ENGINE OWNS THIS. A Windows leaf read grant is an ACE, and writing one needs +//! `WRITE_DAC` on the target — which a standard user does not hold on an all-users install +//! (`%ProgramFiles%\nodejs`, `C:\hostedtoolcache`). The build jail must hold at ZERO privilege, so +//! where the ACE cannot be written the tree has to be somewhere nub owns. And a DACL widening +//! would not have sufficed even where nub can write one: `CreateProcessW` opens the image in the +//! CALLER's context, so once the caller is itself inside the AppContainer, opening an un-ACE'd +//! image is a confined open and is REFUSED — measured against the identical command line +//! unconfined (run 30517334191, both Windows images). A copy is the only thing that answers both. +//! +//! GRANT ON THE EMPTY DIRECTORY, THEN POPULATE — the whole reason this is a function rather than +//! two calls at a call site. The ace is inheritable, so every entry picks it up AT CREATION and +//! there is no propagation pass: 24 ms writing it empty against 426 ms re-granting an +//! already-populated 2,435-entry Node distribution (measured, run 30517506683). The same number +//! is also the per-launch saving, because an inheritable `ALL APPLICATION PACKAGES` ace is exactly +//! what [`super::windows_leaf_grant_redundant`] reports on — so the backend's own leaf grant on a +//! published tree SKIPS, and a per-run package sid, which would have to be written every single +//! spawn, is never needed. +//! +//! COPY, NEVER HARD LINK. An NTFS hard link is a second directory entry on the SAME MFT record, +//! and the security descriptor lives on the record — so an ace written "on the link" is an ace on +//! the original path. Measured in both directions, including onto a protected +//! `%ProgramFiles%` file. Linking would be a grant LEAK dressed as a saving. +//! +//! WHAT IT IS NOT. It is not a way to reach a toolchain the user already has: Python and MSVC are +//! granted read where they live, which works because 43 of the 44 `C:\Program Files` children +//! already publish read to AppContainers. The Node installer is the outlier this exists for. + +use std::io; +use std::path::Path; + +/// Bound the copy. A real Node distribution is 2,435 entries / ~101 MiB (measured on both Windows +/// images), so these never bind on one — they are what stops a caller handing over a mis-resolved +/// path and turning an install into an unbounded copy. +const MAX_ENTRIES: usize = 20_000; +const MAX_BYTES: u64 = 512 * 1024 * 1024; + +/// Publish a copy of `source`'s tree at `dest`, readable and executable by every AppContainer. +/// +/// `Ok(())` means `dest` now holds a complete copy. The caller decides what "complete" means for +/// its own tree and MUST test that before calling (to reuse a previous publish) and again after an +/// `Err` (to adopt a concurrent one) — the engine has no idea what file makes a Node distribution +/// usable, and inventing a sentinel here would be the wrong crate guessing. +/// +/// ATOMIC BY RENAME. The copy lands in a staging sibling and is renamed into place, so `dest` is +/// never observable half-populated and two concurrent publishers cannot see each other's partial +/// tree. The ace is written while the staging directory is still EMPTY and travels with it: an +/// EXPLICIT ace is not recomputed by a same-volume move, and the entries' inherited copies are +/// real ace entries rather than a computation redone at the destination. +/// +/// The trustee is the STABLE `ALL APPLICATION PACKAGES`, not a per-run profile sid, which is sound +/// because a zero-capability LowBox token reads through it (it is why System32 is readable at +/// all). The cost is that `dest` becomes readable to every AppContainer on the machine, so ONLY +/// PUBLIC BYTES MAY BE PUBLISHED HERE — the intended tree is a copy of a Node distribution, which +/// is public bytes from nodejs.org. Do not hand this a tree carrying user data. +/// +/// Symlinks and reparse points in `source` are SKIPPED, not followed: the Windows Node archive has +/// none, and following one would either copy an unbounded foreign tree in or leave the jail +/// reading through a link whose target was never granted. +pub fn stage_appcontainer_readable_copy(source: &Path, dest: &Path) -> io::Result<()> { + let parent = dest.parent().ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidInput, + format!("staging destination has no parent: {}", dest.display()), + ) + })?; + std::fs::create_dir_all(parent)?; + let staging = tempfile::TempDir::new_in(parent)?; + publish_read(staging.path())?; + copy_tree(source, staging.path(), &mut Budget::default())?; + let staged = staging.keep(); + std::fs::rename(&staged, dest).inspect_err(|_| { + std::fs::remove_dir_all(&staged).ok(); + }) +} + +#[cfg(target_os = "windows")] +fn publish_read(dir: &Path) -> io::Result<()> { + super::windows::windows_publish_appcontainer_read(dir) +} + +/// The copy half is ordinary fs work, so it compiles and is tested on the dev host; only the ace +/// needs Windows. A non-Windows build therefore stages an UNPUBLISHED copy, which is why nothing +/// but the Windows backend calls this. +#[cfg(not(target_os = "windows"))] +fn publish_read(_dir: &Path) -> io::Result<()> { + Ok(()) +} + +#[derive(Default)] +struct Budget { + entries: usize, + bytes: u64, +} + +impl Budget { + fn exceeded(&self, what: &str) -> io::Error { + io::Error::other(format!( + "staging source exceeds the {what} bound ({} entries, {} bytes)", + self.entries, self.bytes + )) + } +} + +fn copy_tree(source: &Path, dest: &Path, budget: &mut Budget) -> io::Result<()> { + for entry in std::fs::read_dir(source)? { + let entry = entry?; + if entry.file_type()?.is_symlink() { + continue; + } + budget.entries += 1; + if budget.entries > MAX_ENTRIES { + return Err(budget.exceeded("entry-count")); + } + let target = dest.join(entry.file_name()); + if entry.file_type()?.is_dir() { + std::fs::create_dir(&target)?; + copy_tree(&entry.path(), &target, budget)?; + continue; + } + budget.bytes += entry.metadata()?.len(); + if budget.bytes > MAX_BYTES { + return Err(budget.exceeded("total-bytes")); + } + std::fs::copy(entry.path(), &target)?; + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The tree arrives whole and nested, and `dest` is created by the rename rather than + /// pre-existing — the property the atomicity rests on. + #[test] + fn the_published_copy_is_complete_and_nested() { + let tmp = tempfile::tempdir().unwrap(); + let source = tmp.path().join("src"); + std::fs::create_dir_all(source.join("node_modules/npm/bin")).unwrap(); + std::fs::write(source.join("node.exe"), b"MZ").unwrap(); + std::fs::write(source.join("node_modules/npm/bin/npm-cli.js"), b"//").unwrap(); + + let dest = tmp.path().join("published/24.18.1-x64"); + assert!(!dest.exists()); + stage_appcontainer_readable_copy(&source, &dest).expect("publishes"); + assert_eq!(std::fs::read(dest.join("node.exe")).unwrap(), b"MZ"); + assert!(dest.join("node_modules/npm/bin/npm-cli.js").is_file()); + } + + /// A source over the bound is refused rather than copied, and — because the staging dir is + /// dropped on the error path — leaves no partial tree at `dest` for a completeness test to + /// mistake for a publish. + #[test] + fn an_oversized_source_is_refused_and_publishes_nothing() { + let tmp = tempfile::tempdir().unwrap(); + let source = tmp.path().join("src"); + std::fs::create_dir_all(&source).unwrap(); + std::fs::write(source.join("huge.bin"), vec![0u8; 8]).unwrap(); + + let mut budget = Budget { + entries: 0, + bytes: MAX_BYTES, + }; + let dest = tmp.path().join("dest"); + std::fs::create_dir_all(&dest).unwrap(); + assert!(copy_tree(&source, &dest, &mut budget).is_err()); + assert!(!dest.join("huge.bin").exists()); + } +} diff --git a/crates/nub-sandbox/tests/windows_deelevated_jail.rs b/crates/nub-sandbox/tests/windows_deelevated_jail.rs index adbce515d..d7a4c9265 100644 --- a/crates/nub-sandbox/tests/windows_deelevated_jail.rs +++ b/crates/nub-sandbox/tests/windows_deelevated_jail.rs @@ -1023,6 +1023,10 @@ mod win { // actually clears the jail for default-on. production_jail(&mut r, &f); + // (7) …and with the REAL interpreter rather than the probe binary, which is the one + // variable (6) cannot vary and the one that carried the showstopper. + interpreter_group(&mut r, &f); + println!( "ARM done elevated={} failures={}", u8::from(elevated), @@ -1143,6 +1147,304 @@ mod win { ); } + // ── the interpreter group: can a lifecycle script run at all? ───────────────── + + /// THE SHOWSTOPPER THIS GROUP EXISTS FOR. `production_jail` above grants the PROBE BINARY as + /// the interpreter — a file in the fixture, which nub owns — so it has always passed and could + /// never have seen the real defect. A real install grants the user's own Node, and for a + /// standard user with an all-users install that is a path nub cannot write an ACE on + /// (`%ProgramFiles%\nodejs`, `C:\hostedtoolcache`) AND, independently, an image a confined + /// caller cannot open. Either one makes the jail unable to start a single lifecycle script. + /// + /// So this runs the SAME lifecycle script twice under the SAME real `compile_build_jail` + /// policy, with ONE variable: whether the interpreter is the ambient install or a nub-owned + /// copy of it. The ambient arm is the CONTROL — without an arm where the defect still + /// reproduces, a green staged arm is measuring nothing. + /// + /// It calls the same engine entry the product does + /// (`windows_jail_bin::stage_appcontainer_readable_copy`) rather than re-implementing the + /// staging, because a probe that re-implements the mechanism measures the probe. + fn interpreter_group(r: &mut Report, f: &Fixture) { + let Some(source_exe) = ambient_node() else { + for prop in INTERPRETER_PROPS { + r.record( + prop, + false, + "(no ambient node.exe found — nothing measured)", + ); + } + return; + }; + let source_root = source_exe + .parent() + .expect("node.exe has a parent") + .to_path_buf(); + println!(" fact:interp-source={}", source_exe.display()); + // Whether the SOURCE install already publishes read to AppContainers is the fact that + // decides whether this machine can exhibit the defect at all: on one that does (43 of the + // 44 `C:\Program Files` children do — `nodejs` is the outlier), the ambient arm passes for + // a reason that has nothing to do with nub, and the differential is VOID rather than green. + let source_publishes = nub_sandbox::windows_leaf_grant_redundant(&source_root); + println!(" fact:interp-source-publishes-aap={source_publishes}"); + + let (ambient_rc, ambient_log) = lifecycle_arm(f, "ambient", &source_exe, &source_root); + + // Staged under the fixture's own cache home, which is where `compile_build_jail`'s `$cache` + // anchor points — the same relationship the product's `/jail-bin/` has, without + // touching the real user cache. + let staged_dir = f.home.join("cache/nub/pm/jail-bin/probe"); + let began = Instant::now(); + let staged = nub_sandbox::backend::windows_jail_bin::stage_appcontainer_readable_copy( + &source_root, + &staged_dir, + ); + let staged_exe = staged_dir.join("node.exe"); + println!( + " fact:interp-stage-ms={} result={staged:?}", + began.elapsed().as_millis() + ); + // THE ZERO-PRIVILEGE CLAIM, and it is only a claim in the de-elevated arm — which is + // exactly why it lives in this differential rather than in a Windows unit test. + r.record( + "interp-staging-succeeds", + staged.is_ok() && staged_exe.is_file(), + &format!("({staged:?}; node.exe present {})", staged_exe.is_file()), + ); + + let (staged_rc, staged_log) = lifecycle_arm(f, "staged", &staged_exe, &staged_dir); + + // The control. `!= 0` rather than a specific code because the ambient arm can fail at + // either of two independent points — the ACE write, or the confined image open — and + // pinning one would make the property FAIL when the OTHER cause fired. + r.record( + "interp-ambient-install-cannot-run-a-lifecycle-script", + ambient_rc != 0 && !ambient_log.contains("LIFECYCLE-OK"), + &format!("(rc {ambient_rc}; log {})", one_line(&ambient_log)), + ); + r.record( + "interp-staged-copy-runs-a-lifecycle-script-to-completion", + staged_rc == 0 && staged_log.contains("LIFECYCLE-OK"), + &format!("(rc {staged_rc}; log {})", one_line(&staged_log)), + ); + // ATTRIBUTION, and it is what stops the staged arm passing for the wrong reason. A refused + // read grant is now SKIPPED rather than fatal, so a launch can succeed while the child + // quietly ran the ambient interpreter off some other PATH entry — which would look + // identical here. The child reports its own `process.execPath`, so it cannot. + r.record( + "interp-staged-child-execpath-is-the-nub-owned-copy", + staged_log + .lines() + .any(|l| l.trim().eq_ignore_ascii_case(&staged_exe.to_string_lossy())), + &format!( + "(expected {}; log {})", + staged_exe.display(), + one_line(&staged_log) + ), + ); + // THE ABI CONSTRAINT, measured rather than asserted: `prebuild-install` and + // `node-gyp-build` both default the prebuild they fetch to `process.versions.modules` of + // the RUNNING Node, so a staged interpreter reporting a different one would silently make + // every package in that family download an unloadable binary. Both arms report it, so this + // compares the copy against its own source rather than against a tabulated expectation. + let (source_abi, copy_abi) = (abi_of(&ambient_log), abi_of(&staged_log)); + r.record( + "interp-staged-abi-matches-the-source", + copy_abi.is_some() && copy_abi == source_abi, + &format!("(source {source_abi:?}, staged {copy_abi:?})"), + ); + // The cost claim: a published tree is what the backend's own leaf grant SKIPS on, so a + // lifecycle spawn writes no DACL across the ~2,400-entry tree. Asserted on a DEEP entry + // too, which is the half that proves the ace was written BEFORE the copy — an entry + // several levels down carries it only by inheritance at creation. + r.record( + "interp-staged-dir-publishes-read-to-appcontainers", + nub_sandbox::windows_leaf_grant_redundant(&staged_dir), + "(an inheritable ALL APPLICATION PACKAGES ace ⇒ the per-spawn leaf grant skips)", + ); + let deep = staged_dir.join("node_modules/npm/bin"); + r.record( + "interp-staged-deep-entry-inherited-the-ace-at-creation", + deep.is_dir() && nub_sandbox::windows_leaf_grant_redundant(&deep), + &format!("({} exists {})", deep.display(), deep.is_dir()), + ); + // Confinement survives it: the jail is not merely startable, it still withholds what it + // never granted. + let policy = build_jail_for(f, &staged_exe, &staged_dir); + let denied = code( + &policy, + f, + &f.package, + &["__sbxchild__", "read", &f.secret.to_string_lossy()], + ); + r.record( + "interp-staged-ungranted-secret-still-refused", + denied == 5 || denied == 9, + &format!("(child exit {denied}; 5/9 = denied)"), + ); + } + + /// Every property [`interpreter_group`] reports, so a machine with no Node still emits a + /// verdict for each rather than leaving the table looking complete with rows missing. + const INTERPRETER_PROPS: &[&str] = &[ + "interp-staging-succeeds", + "interp-ambient-install-cannot-run-a-lifecycle-script", + "interp-staged-copy-runs-a-lifecycle-script-to-completion", + "interp-staged-child-execpath-is-the-nub-owned-copy", + "interp-staged-abi-matches-the-source", + "interp-staged-dir-publishes-read-to-appcontainers", + "interp-staged-deep-entry-inherited-the-ace-at-creation", + "interp-staged-ungranted-secret-still-refused", + ]; + + /// The all-users MSI install first, because it is the configuration that exhibits the defect + /// and the one a real user has; the runner's unzipped `hostedtoolcache` Node otherwise, which + /// carries no AppContainer ace either. Whichever it found is reported, so a run that measured + /// an already-readable Node is legible rather than silently reassuring. + fn ambient_node() -> Option { + let msi = PathBuf::from(std::env::var("ProgramFiles").unwrap_or_default()) + .join("nodejs/node.exe"); + if msi.is_file() { + return Some(msi); + } + std::env::var_os("PATH") + .into_iter() + .flat_map(|p| std::env::split_paths(&p).collect::>()) + .map(|d| d.join("node.exe")) + .find(|c| c.is_file()) + } + + /// One arm: the real production policy for `exe`, running a real `.cmd` lifecycle script + /// through `cmd.exe` — which is how aube spawns one, and the shape that makes the child's own + /// `node` a NESTED spawn from inside the AppContainer rather than one nub performs itself. + /// + /// The script deliberately contains NO PIPE. A piped `child_process` spawn under an + /// AppContainer hangs in libuv's named-pipe retry, which is a SEPARATE defect with a separate + /// repair (the `NODE_OPTIONS` stdio shim) — including one here would make a hang + /// indistinguishable from an interpreter that could not be read. + fn lifecycle_arm(f: &Fixture, tag: &str, exe: &Path, root: &Path) -> (i32, String) { + let marker = f.package.join(format!("lc-{tag}.log")); + let script = f.package.join(format!("lc-{tag}.cmd")); + let entry = f.package.join(format!("lc-{tag}.js")); + // Reaching the distribution's BUNDLED npm tree is the exact cell an un-ACE'd install fails + // on, and requiring a file out of it is the cheapest honest way to ask for it. + std::fs::write( + &entry, + format!( + "console.log(process.execPath);\n\ + console.log('abi=' + process.versions.modules);\n\ + console.log('npm=' + require({}).version);\n", + js_string(&root.join("node_modules/npm/package.json")) + ), + ) + .expect("write the arm's entry file"); + // `node` BARE, not by absolute path: a PATH search is itself a directory probe, and a + // confined child is refused on an un-ACE'd install tree — which surfaces as `'node' is not + // recognized`, a different and earlier failure than a refused image open. Both are the + // defect; naming the interpreter absolutely would have measured only the second. + std::fs::write( + &script, + format!( + "@echo off\r\n\ + node -p \"process.version\" > \"{m}\" 2>&1\r\n\ + if errorlevel 1 exit /b 11\r\n\ + node \"{e}\" >> \"{m}\" 2>&1\r\n\ + if errorlevel 1 exit /b 12\r\n\ + echo LIFECYCLE-OK>> \"{m}\"\r\n", + m = marker.display(), + e = entry.display() + ), + ) + .expect("write the arm's script"); + + let policy = build_jail_for(f, exe, root); + let comspec = std::env::var("ComSpec").unwrap_or_else(|_| { + format!( + "{}\\System32\\cmd.exe", + std::env::var("SystemRoot").unwrap_or_else(|_| "C:\\Windows".into()) + ) + }); + let spec = CommandSpec::new(&comspec) + .arg("/c") + .arg(&script) + .cwd(&f.package); + let rc = match apply(&policy, spec) { + Ok(p) => match p.status() { + Ok(s) => s.code().unwrap_or(-1), + Err(e) => { + println!(" [{tag} status Err] {e} os={:?}", e.raw_os_error()); + -101 + } + }, + Err(d) => { + println!(" [{tag} apply Err] {d:?}"); + -100 + } + }; + let log = std::fs::read_to_string(&marker).unwrap_or_default(); + println!(" arm:{tag} rc={rc}"); + for line in log.lines() { + println!(" {tag}| {line}"); + } + (rc, log) + } + + /// The REAL production policy — `compile_build_jail`, the entry aube's lifecycle hook drives — + /// with the interpreter and its distribution's global package tree as the only variables. The + /// env mirrors what the embedder constructs: both interpreter spellings named, and `PATH` + /// carrying the distribution plus the OS floor. + fn build_jail_for(f: &Fixture, exe: &Path, root: &Path) -> SandboxPolicy { + let system32 = format!( + "{}\\System32", + std::env::var("SystemRoot").unwrap_or_else(|_| "C:\\Windows".into()) + ); + let mut ambient: BTreeMap = std::env::vars().collect(); + ambient.insert("PATH".to_string(), format!("{};{system32}", root.display())); + ambient.insert( + "npm_node_execpath".to_string(), + exe.to_string_lossy().into_owned(), + ); + ambient.insert("NODE".to_string(), exe.to_string_lossy().into_owned()); + let homes = nub_sandbox::Homes { + home: f.home.clone(), + tmp: std::env::temp_dir(), + cache: f.home.join("cache"), + project: f.project.clone(), + }; + nub_sandbox::compile_build_jail( + homes, + &f.package, + vec![exe.to_path_buf()], + vec![root.join("node_modules")], + ambient, + ) + .expect("compile_build_jail") + } + + /// A Windows path as a JS string literal — backslashes doubled, so the emitted `require()` + /// argument is the path rather than a run of escape sequences. + fn js_string(p: &Path) -> String { + format!("\"{}\"", p.to_string_lossy().replace('\\', "\\\\")) + } + + fn abi_of(log: &str) -> Option<&str> { + log.lines().find_map(|l| l.trim().strip_prefix("abi=")) + } + + /// A child log flattened onto the single `prop:` line, so a verdict carries its own evidence + /// without the reader having to correlate it against the interleaved arm output. + fn one_line(log: &str) -> String { + let flat: Vec<&str> = log + .lines() + .map(str::trim) + .filter(|l| !l.is_empty()) + .collect(); + let joined = flat.join(" / "); + match joined.char_indices().nth(240) { + Some((cut, _)) => format!("{}…", &joined[..cut]), + None => joined, + } + } + // ── the differential driver ─────────────────────────────────────────────────── struct Arm { @@ -1337,29 +1639,29 @@ mod win { println!("\n── DIFFERENTIAL ────────────────────────────────────────────"); println!("de-elevation route: {route}"); println!( - "{:<28} {:>10} {:>14}", + "{:<58} {:>10} {:>14}", "token state", "elevated", "de-elevated" ); println!( - "{:<28} {:>10} {:>14}", + "{:<58} {:>10} {:>14}", "admin authority (SCM)", elev_arm.map_or("n/a", |a| if a.admin { "YES" } else { "NO" }), if deelev.admin { "YES" } else { "NO" } ); println!( - "{:<28} {:>10} {:>14}", + "{:<58} {:>10} {:>14}", "integrity level", elev_arm.map_or("n/a".to_string(), |a| a.il.to_string()), deelev.il ); println!( - "{:<28} {:>10} {:>14}", + "{:<58} {:>10} {:>14}", "TokenIsElevated flag (stale)", elev_arm.map_or("n/a", |a| if a.elevated { "1" } else { "0" }), if deelev.elevated { "1" } else { "0" } ); println!( - "{:<28} {:>10} {:>14}", + "{:<58} {:>10} {:>14}", "property", "elevated", "de-elevated" ); for (name, ok) in &deelev.props { @@ -1368,7 +1670,7 @@ mod win { .map(|(_, v)| if *v { "PASS" } else { "FAIL" }) .unwrap_or("n/a"); println!( - "{name:<28} {e:>10} {:>14}", + "{name:<58} {e:>10} {:>14}", if *ok { "PASS" } else { "FAIL" } ); if !ok { From f86262f706c4650924ff350b1808adc7aa660793 Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Thu, 30 Jul 2026 05:43:13 -0700 Subject: [PATCH 3/8] probe(win): fix three defects run 30542604419's own output caught MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run 30542604419 measured the mechanism working — a cold copy of the real %ProgramFiles%\nodejs tree published in 13.2s, the inheritable ACE read back on both the staged dir and node_modules/npm/bin five levels down, the staged lifecycle script completing with abi=127 matching its source. Three defects were in the PROBE, and each would have kept reporting red while the fix worked. 1. The execPath attribution compared SPELLINGS. `join("cache/nub/pm/...")` keeps the forward slashes verbatim while the child prints Windows separators, so the control failed on two spellings of one path — a control that varied more than the thing under test. Compared as paths now. 2. The ambient arm demanded FAILURE in both arms, which is wrong: an elevated nub holds WRITE_DAC on %ProgramFiles% and the ambient interpreter then works, so the elevated arm was being asked to contradict itself. The property is now the DEPENDENCE that actually holds in both — the ambient install is usable exactly where nub can write its DACL — with a non-mutating WRITE_DAC probe supplying the antecedent. Non-mutating matters: writing a real ace to find out would leave a lasting AppContainer widening on the user's own Node install. 3. Absolute paths inside the script made the de-elevated arm fail with cmd's `Access is denied.` before either interpreter was reached, i.e. it measured the still-open ancestor-reachability repair rather than the interpreter. The script now opens everything relative to its cwd, and carries a breadcrumb per stage, because `rc 1` with an empty log did not distinguish "cmd could not open the script" from "bare node was unresolvable" from "the require was refused". Also recorded: the elevated arm's ambient spawn rewrites %ProgramFiles%\nodejs's DACL, and the AppContainer-readability read-back went false → TRUE across the two arms even though the per-run ace is revoked. So that fact is only trustworthy in the arm that ran first, which is a second reason the gated property keys on WRITE_DAC — a property of the token, which no earlier arm can loosen. And the MSI step ran under PowerShell 5.1, where every `Invoke-WebRequest -Method Head` against nodejs.org threw and the step died having never reached msiexec. tests/win-msi-volume runs the same recipe under pwsh, where it works. --- .github/workflows/win-jail-interp-probe.yml | 51 +++--- .../tests/windows_deelevated_jail.rs | 146 +++++++++++++++--- 2 files changed, 152 insertions(+), 45 deletions(-) diff --git a/.github/workflows/win-jail-interp-probe.yml b/.github/workflows/win-jail-interp-probe.yml index 5468897f4..ce0228c18 100644 --- a/.github/workflows/win-jail-interp-probe.yml +++ b/.github/workflows/win-jail-interp-probe.yml @@ -19,14 +19,23 @@ # would measure the runner image's unzipped hostedtoolcache Node, which is a different tree with a # different (also un-ACE'd) DACL — usable, but not the configuration a real user has. # -# THE CONTROLS. `interp-ambient-install-cannot-run-a-lifecycle-script` is the arm where the defect -# still reproduces: without it a green staged arm measures nothing. -# `interp-staged-child-execpath-is-the-nub-owned-copy` is the attribution control — a refused read -# grant is now skipped rather than fatal, so a launch can succeed while the child quietly ran the -# ambient interpreter, and only the child's own execPath tells those apart. +# THE CONTROLS. `interp-ambient-install-is-usable-only-where-nub-can-write-its-dacl` is the one the +# whole run rests on, and it is stated as a DEPENDENCE rather than a failure because that is what +# holds in both arms: an elevated nub does hold WRITE_DAC on %ProgramFiles% and the ambient +# interpreter then works, so demanding the ambient arm fail everywhere would be a self-inflicted red +# that measured nothing. `interp-staged-child-execpath-is-the-nub-owned-copy` is the attribution +# control — a refused read grant is now SKIPPED rather than fatal, so a launch can succeed while the +# child quietly ran the ambient interpreter, and only the child's own execPath tells those apart. # `fact:interp-source-publishes-aap` decides whether the run could exhibit the defect at all: on an # install that already publishes read to AppContainers the ambient arm passes for a reason that has -# nothing to do with nub, which makes the differential VOID rather than green. +# nothing to do with nub. +# +# CROSS-ARM CONTAMINATION, recorded because it bit run 30542604419: the ELEVATED arm's ambient +# lifecycle spawn rewrites %ProgramFiles%\nodejs's DACL to add its per-run grant, and the +# AppContainer-readability read-back went false → TRUE across the two arms even though the per-run +# ace is revoked. So `fact:interp-source-publishes-aap` is only trustworthy in the arm that ran +# FIRST, and the WRITE_DAC fact — a property of the token, which no earlier arm can change in the +# permissive direction — is what the gated property keys on instead. name: win-jail-interp-probe on: @@ -64,8 +73,12 @@ jobs: # The configuration under test. Recipe lifted from tests/win-msi-volume/msi-node-acl.ps1: the # dist index picks the VERSION and a HEAD request decides whether the artifact exists, because # index.json never lists win-arm64-msi for any release even where the .msi is served. + # `pwsh` (PowerShell 7), NOT `powershell` (5.1): under 5.1 every `Invoke-WebRequest -Method + # Head` against nodejs.org threw, the version loop found nothing, and the step died on + # `no LTS release serves a x64 .msi` having never reached msiexec. tests/win-msi-volume runs + # the identical recipe under pwsh, where it works. - name: Install a stock Node MSI to %ProgramFiles%\nodejs - shell: powershell + shell: pwsh run: | $ErrorActionPreference = 'Stop' $arch = if ($env:PROCESSOR_ARCHITECTURE -eq 'ARM64') { 'arm64' } else { 'x64' } @@ -139,21 +152,19 @@ jobs: need 'DEELEV route=' echo "---- the interpreter the run actually measured ----" - grep -E '^ fact:interp-(source|source-publishes-aap|stage-ms)' probe.log || { echo "MISS the interpreter facts"; fail=1; } - # VOIDS THE DIFFERENTIAL rather than merely weakening it: an already-AppContainer-readable - # source install cannot exhibit the defect, so the ambient arm's failure would have to - # come from somewhere else. - if grep -q '^ fact:interp-source-publishes-aap=true' probe.log; then - echo "VOID the source install already publishes read to AppContainers — the ambient arm is not the defect" - fail=1 - fi + grep -E '^ fact:interp-' probe.log || { echo "MISS the interpreter facts"; fail=1; } grep -qE '^ fact:interp-source=.*[Pp]rogram [Ff]iles' probe.log || { echo "MISS the probe did not measure the MSI install"; fail=1; } - - echo "---- the differential (the control first: it must still reproduce) ----" - need_pass interp-ambient-install-cannot-run-a-lifecycle-script - need_pass interp-staged-copy-runs-a-lifecycle-script-to-completion + # THE DIFFERENTIAL IS ONLY REAL IF THE TWO ARMS DIFFERED ON THE PERMISSION UNDER TEST. + # Both readings must appear across the run: an elevated arm that holds WRITE_DAC on the + # ambient install, and a de-elevated one that does not. + grep -q '^ fact:interp-source-write-dac=true' probe.log || { echo "MISS no arm held WRITE_DAC on the ambient install"; fail=1; } + grep -q '^ fact:interp-source-write-dac=false' probe.log || { echo "MISS no arm LACKED WRITE_DAC — the arms did not differ on the permission under test"; fail=1; } + + echo "---- the differential ----" + need_pass interp-ambient-install-is-usable-only-where-nub-can-write-its-dacl + need_pass interp-staged-copy-runs-a-lifecycle-script-without-write-dac need_pass interp-staged-child-execpath-is-the-nub-owned-copy - need_pass interp-staging-succeeds + need_pass interp-staging-needs-no-write-dac-anywhere-privileged need_pass interp-staged-abi-matches-the-source echo "---- the cost claim: no per-spawn DACL write over the tree ----" diff --git a/crates/nub-sandbox/tests/windows_deelevated_jail.rs b/crates/nub-sandbox/tests/windows_deelevated_jail.rs index d7a4c9265..f61bb9336 100644 --- a/crates/nub-sandbox/tests/windows_deelevated_jail.rs +++ b/crates/nub-sandbox/tests/windows_deelevated_jail.rs @@ -1187,12 +1187,20 @@ mod win { let source_publishes = nub_sandbox::windows_leaf_grant_redundant(&source_root); println!(" fact:interp-source-publishes-aap={source_publishes}"); + // THE PROXIMATE CAUSE, asked directly and WITHOUT MUTATING ANYTHING. `set_ace` needs + // `WRITE_DAC` on its target, so whether nub can grant the ambient interpreter at all is a + // property of this token against that DACL — and opening the file for `WRITE_DAC` asks + // exactly that. Writing a real ace to find out would leave a lasting widening on the + // user's own Node install, which is not a probe's business. + let write_dac = can_write_dacl(&source_exe); + println!(" fact:interp-source-write-dac={write_dac}"); + let (ambient_rc, ambient_log) = lifecycle_arm(f, "ambient", &source_exe, &source_root); // Staged under the fixture's own cache home, which is where `compile_build_jail`'s `$cache` // anchor points — the same relationship the product's `/jail-bin/` has, without // touching the real user cache. - let staged_dir = f.home.join("cache/nub/pm/jail-bin/probe"); + let staged_dir = f.home.join("cache").join("nub").join("pm").join("jail-bin"); let began = Instant::now(); let staged = nub_sandbox::backend::windows_jail_bin::stage_appcontainer_readable_copy( &source_root, @@ -1206,35 +1214,49 @@ mod win { // THE ZERO-PRIVILEGE CLAIM, and it is only a claim in the de-elevated arm — which is // exactly why it lives in this differential rather than in a Windows unit test. r.record( - "interp-staging-succeeds", + "interp-staging-needs-no-write-dac-anywhere-privileged", staged.is_ok() && staged_exe.is_file(), &format!("({staged:?}; node.exe present {})", staged_exe.is_file()), ); let (staged_rc, staged_log) = lifecycle_arm(f, "staged", &staged_exe, &staged_dir); - - // The control. `!= 0` rather than a specific code because the ambient arm can fail at - // either of two independent points — the ACE write, or the confined image open — and - // pinning one would make the property FAIL when the OTHER cause fired. + let ambient_ok = ambient_rc == 0 && ambient_log.contains("LIFECYCLE-OK"); + let staged_ok = staged_rc == 0 && staged_log.contains("LIFECYCLE-OK"); + + // THE LAW, stated so it holds in BOTH arms rather than being an expectation that + // contradicts itself across them. An earlier draft demanded the ambient arm FAIL in both, + // which is wrong and would have been a self-inflicted red: an ELEVATED nub holds + // `WRITE_DAC` on `%ProgramFiles%`, writes the ace, and the ambient interpreter works. + // What is invariant is the DEPENDENCE — the ambient install is usable exactly when nub + // could grant it — and stating it that way makes the elevated arm carry information + // instead of being the same measurement twice. r.record( - "interp-ambient-install-cannot-run-a-lifecycle-script", - ambient_rc != 0 && !ambient_log.contains("LIFECYCLE-OK"), - &format!("(rc {ambient_rc}; log {})", one_line(&ambient_log)), + "interp-ambient-install-is-usable-only-where-nub-can-write-its-dacl", + ambient_ok == write_dac, + &format!( + "(write_dac {write_dac}, lifecycle ok {ambient_ok}; rc {ambient_rc}; log {})", + one_line(&ambient_log) + ), ); + // …and the fix is that the staged copy does NOT depend on it. This is the property the + // whole change exists for, so it is gated in both arms and the de-elevated one is the + // meaningful half. r.record( - "interp-staged-copy-runs-a-lifecycle-script-to-completion", - staged_rc == 0 && staged_log.contains("LIFECYCLE-OK"), + "interp-staged-copy-runs-a-lifecycle-script-without-write-dac", + staged_ok, &format!("(rc {staged_rc}; log {})", one_line(&staged_log)), ); // ATTRIBUTION, and it is what stops the staged arm passing for the wrong reason. A refused // read grant is now SKIPPED rather than fatal, so a launch can succeed while the child // quietly ran the ambient interpreter off some other PATH entry — which would look // identical here. The child reports its own `process.execPath`, so it cannot. + // + // Compared as PATHS, not strings. The first draft compared spellings and failed on a run + // where the fix worked, because a `/`-containing `join` literal and Windows's own `\` + // spelling name the same file — a control that varied more than the thing under test. r.record( "interp-staged-child-execpath-is-the-nub-owned-copy", - staged_log - .lines() - .any(|l| l.trim().eq_ignore_ascii_case(&staged_exe.to_string_lossy())), + staged_log.lines().any(|l| same_file(l.trim(), &staged_exe)), &format!( "(expected {}; log {})", staged_exe.display(), @@ -1245,7 +1267,8 @@ mod win { // `node-gyp-build` both default the prebuild they fetch to `process.versions.modules` of // the RUNNING Node, so a staged interpreter reporting a different one would silently make // every package in that family download an unloadable binary. Both arms report it, so this - // compares the copy against its own source rather than against a tabulated expectation. + // compares the copy against its own source rather than against a tabulated expectation — + // and it can only be answered where BOTH arms produced a reading. let (source_abi, copy_abi) = (abi_of(&ambient_log), abi_of(&staged_log)); r.record( "interp-staged-abi-matches-the-source", @@ -1261,7 +1284,7 @@ mod win { nub_sandbox::windows_leaf_grant_redundant(&staged_dir), "(an inheritable ALL APPLICATION PACKAGES ace ⇒ the per-spawn leaf grant skips)", ); - let deep = staged_dir.join("node_modules/npm/bin"); + let deep = staged_dir.join("node_modules").join("npm").join("bin"); r.record( "interp-staged-deep-entry-inherited-the-ace-at-creation", deep.is_dir() && nub_sandbox::windows_leaf_grant_redundant(&deep), @@ -1283,12 +1306,56 @@ mod win { ); } + /// Whether THIS token may rewrite `path`'s DACL — the one permission [`set_ace`] needs, asked + /// by opening for it rather than by writing an ace and observing the result. Non-mutating on + /// purpose: the elevated arm WOULD succeed, and it would leave a lasting AppContainer widening + /// on the user's own Node install. + fn can_write_dacl(path: &Path) -> bool { + use windows_sys::Win32::Foundation::{CloseHandle, INVALID_HANDLE_VALUE}; + use windows_sys::Win32::Storage::FileSystem::{ + CreateFileW, FILE_ATTRIBUTE_NORMAL, FILE_SHARE_DELETE, FILE_SHARE_READ, + FILE_SHARE_WRITE, OPEN_EXISTING, + }; + const WRITE_DAC: u32 = 0x0004_0000; + let wide: Vec = path + .as_os_str() + .encode_wide() + .chain(std::iter::once(0)) + .collect(); + // SAFETY: an OPEN_EXISTING open for one access right on a NUL-terminated wide path; the + // handle is closed on the success path and nothing is written through it. + unsafe { + let h = CreateFileW( + wide.as_ptr(), + WRITE_DAC, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + std::ptr::null(), + OPEN_EXISTING, + FILE_ATTRIBUTE_NORMAL, + std::ptr::null_mut(), + ); + if h == INVALID_HANDLE_VALUE || h.is_null() { + return false; + } + CloseHandle(h); + true + } + } + + /// Whether a path the CHILD printed names the same file as one nub built. Windows accepts both + /// separators, so two spellings of one path are equal as paths and unequal as strings — and a + /// string compare here silently inverted a control (see the call site). + fn same_file(reported: &str, expected: &Path) -> bool { + let norm = |p: &str| p.replace('/', "\\").to_ascii_lowercase(); + !reported.is_empty() && norm(reported) == norm(&expected.to_string_lossy()) + } + /// Every property [`interpreter_group`] reports, so a machine with no Node still emits a /// verdict for each rather than leaving the table looking complete with rows missing. const INTERPRETER_PROPS: &[&str] = &[ - "interp-staging-succeeds", - "interp-ambient-install-cannot-run-a-lifecycle-script", - "interp-staged-copy-runs-a-lifecycle-script-to-completion", + "interp-staging-needs-no-write-dac-anywhere-privileged", + "interp-ambient-install-is-usable-only-where-nub-can-write-its-dacl", + "interp-staged-copy-runs-a-lifecycle-script-without-write-dac", "interp-staged-child-execpath-is-the-nub-owned-copy", "interp-staged-abi-matches-the-source", "interp-staged-dir-publishes-read-to-appcontainers", @@ -1337,21 +1404,35 @@ mod win { ), ) .expect("write the arm's entry file"); - // `node` BARE, not by absolute path: a PATH search is itself a directory probe, and a - // confined child is refused on an un-ACE'd install tree — which surfaces as `'node' is not + // Every path INSIDE the script is CWD-RELATIVE, and that is not tidiness. An absolute open + // from inside the jail walks the whole ancestor chain as targets, and making that chain + // reachable without elevation is a SEPARATE, still-open repair — an absolute script path + // made the de-elevated arm fail with cmd's `Access is denied.` before either interpreter + // was reached, i.e. it measured the ancestor blocker instead of the interpreter. A relative + // open resolves against the inherited cwd handle and does not. + // + // `node` is BARE, not absolute: a PATH search is itself a directory probe, and a confined + // child is refused on an un-ACE'd install tree — which surfaces as `'node' is not // recognized`, a different and earlier failure than a refused image open. Both are the // defect; naming the interpreter absolutely would have measured only the second. std::fs::write( &script, format!( + // BREADCRUMB PER STAGE, so a failure is attributable to one of them instead of + // arriving as a bare exit code. An earlier run produced `rc 1` with an EMPTY log + // and nothing distinguished "cmd could not open the script" from "`node` was + // unresolvable" from "the require was refused" — three causes with three different + // fixes. "@echo off\r\n\ - node -p \"process.version\" > \"{m}\" 2>&1\r\n\ + echo STEP-CMD-OPENED-THE-SCRIPT> \"{m}\"\r\n\ + node -p \"process.version\" >> \"{m}\" 2>&1\r\n\ if errorlevel 1 exit /b 11\r\n\ + echo STEP-BARE-NODE-RESOLVED-AND-RAN>> \"{m}\"\r\n\ node \"{e}\" >> \"{m}\" 2>&1\r\n\ if errorlevel 1 exit /b 12\r\n\ echo LIFECYCLE-OK>> \"{m}\"\r\n", - m = marker.display(), - e = entry.display() + m = leaf(&marker), + e = leaf(&entry) ), ) .expect("write the arm's script"); @@ -1365,7 +1446,7 @@ mod win { }); let spec = CommandSpec::new(&comspec) .arg("/c") - .arg(&script) + .arg(leaf(&script)) .cwd(&f.package); let rc = match apply(&policy, spec) { Ok(p) => match p.status() { @@ -1382,6 +1463,12 @@ mod win { }; let log = std::fs::read_to_string(&marker).unwrap_or_default(); println!(" arm:{tag} rc={rc}"); + if log.is_empty() { + // Nothing ran, so the grant list is the only remaining evidence about why. + for rule in &policy.fs.rules.entries { + println!(" {tag}# grant {:?} {:?}", rule.access, rule.matcher); + } + } for line in log.lines() { println!(" {tag}| {line}"); } @@ -1420,6 +1507,15 @@ mod win { .expect("compile_build_jail") } + /// A path's file name, for the cwd-relative spellings the script uses. Panics on a path with + /// no final component, which is a probe bug rather than a measurement. + fn leaf(p: &Path) -> String { + p.file_name() + .expect("an arm file has a name") + .to_string_lossy() + .into_owned() + } + /// A Windows path as a JS string literal — backslashes doubled, so the emitted `require()` /// argument is the path rather than a run of escape sequences. fn js_string(p: &Path) -> String { From 0c65a3ef94f694762999749c7f36fa96b9fb8b9f Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Thu, 30 Jul 2026 05:49:43 -0700 Subject: [PATCH 4/8] probe(win): import OsStrExt for the WRITE_DAC probe's wide path --- crates/nub-sandbox/tests/windows_deelevated_jail.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/nub-sandbox/tests/windows_deelevated_jail.rs b/crates/nub-sandbox/tests/windows_deelevated_jail.rs index f61bb9336..f07b93451 100644 --- a/crates/nub-sandbox/tests/windows_deelevated_jail.rs +++ b/crates/nub-sandbox/tests/windows_deelevated_jail.rs @@ -1311,6 +1311,7 @@ mod win { /// purpose: the elevated arm WOULD succeed, and it would leave a lasting AppContainer widening /// on the user's own Node install. fn can_write_dacl(path: &Path) -> bool { + use std::os::windows::ffi::OsStrExt; use windows_sys::Win32::Foundation::{CloseHandle, INVALID_HANDLE_VALUE}; use windows_sys::Win32::Storage::FileSystem::{ CreateFileW, FILE_ATTRIBUTE_NORMAL, FILE_SHARE_DELETE, FILE_SHARE_READ, From 5d6c4c2b73c2823910997bddba922118de32cc3a Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Thu, 30 Jul 2026 05:57:52 -0700 Subject: [PATCH 5/8] probe(win): take the realpath blocker out of the interpreter measurement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run 30544198501's breadcrumbs did their job. Both arms reached STEP-BARE-NODE-RESOLVED-AND-RAN and printed v24.18.1, so bare `node` resolved off the sanitized PATH and the interpreter was readable — then both died identically at `EPERM: operation not permitted, lstat 'C:\'` inside resolveMainPath. That is the still-open ancestor-reachability repair, orthogonal to the interpreter, and it was masking the thing under test in both arms at once. An entry FILE is what drags it in: resolveMainPath realpath's every prefix of the path from the volume root. `node -e` reaches the same reads with no main-path resolution, which the same run corroborates — `node -p` was unaffected while `node ` died, the shape the repairs probe already documented. The npm-tree require moves into the -e program, single-quoted, because the whole program is one cmd argument delimited by `"`. The ambient property also has to admit a second way an install can be readable. That run read the source install's AppContainer-readability FALSE in the elevated arm and TRUE in the de-elevated one, on one machine. The elevated arm's own grant on that path is the suspect, but its teardown revokes a different sid than this read-back matches, so the mechanism is UNEXPLAINED and is not asserted — the tests/win-msi-volume lane already owns that question. What follows regardless is that an install the OS already publishes is readable with no grant, so the law is now `usable iff (nub can write its DACL or it is already published)`, with both inputs read before the arm runs. The reading is also taken again at the END, so a run where an arm changed it is visible rather than inferred. --- .../tests/windows_deelevated_jail.rs | 62 +++++++++++++------ 1 file changed, 43 insertions(+), 19 deletions(-) diff --git a/crates/nub-sandbox/tests/windows_deelevated_jail.rs b/crates/nub-sandbox/tests/windows_deelevated_jail.rs index f07b93451..b1194e4c1 100644 --- a/crates/nub-sandbox/tests/windows_deelevated_jail.rs +++ b/crates/nub-sandbox/tests/windows_deelevated_jail.rs @@ -1230,14 +1230,30 @@ mod win { // What is invariant is the DEPENDENCE — the ambient install is usable exactly when nub // could grant it — and stating it that way makes the elevated arm carry information // instead of being the same measurement twice. + // `source_publishes` is the second disjunct and it is not hypothetical: run 30544198501 + // read this FALSE in the elevated arm and TRUE in the de-elevated one, on the same machine, + // for reasons not yet established (the elevated arm's own grant on that path is the + // suspect, but its teardown revokes a DIFFERENT sid than the one this matches, so the + // mechanism is UNEXPLAINED and is not asserted here). Whatever the cause, an install the OS + // already publishes is readable without nub granting anything, so the law has to admit it — + // and both inputs are read BEFORE the arm runs. + let readable_without_a_grant = write_dac || source_publishes; r.record( "interp-ambient-install-is-usable-only-where-nub-can-write-its-dacl", - ambient_ok == write_dac, + ambient_ok == readable_without_a_grant, &format!( - "(write_dac {write_dac}, lifecycle ok {ambient_ok}; rc {ambient_rc}; log {})", + "(write_dac {write_dac}, already-published {source_publishes}, lifecycle ok \ + {ambient_ok}; rc {ambient_rc}; log {})", one_line(&ambient_log) ), ); + // Re-read at the END, so a run where an arm CHANGED the ambient install's + // AppContainer-readability is visible rather than inferred. This is the reading the next + // probe of that question starts from. + println!( + " fact:interp-source-publishes-aap-after={}", + nub_sandbox::windows_leaf_grant_redundant(&source_root) + ); // …and the fix is that the staged copy does NOT depend on it. This is the property the // whole change exists for, so it is gated in both arms and the de-elevated one is the // meaningful half. @@ -1392,19 +1408,23 @@ mod win { fn lifecycle_arm(f: &Fixture, tag: &str, exe: &Path, root: &Path) -> (i32, String) { let marker = f.package.join(format!("lc-{tag}.log")); let script = f.package.join(format!("lc-{tag}.cmd")); - let entry = f.package.join(format!("lc-{tag}.js")); + // `node -e`, NOT `node `. An ENTRY FILE goes through `resolveMainPath`, which + // realpath's every prefix of the path starting at the volume root, and de-elevated that + // dies `EPERM: operation not permitted, lstat 'C:\'` — the still-open + // ancestor-reachability repair, which is orthogonal to the interpreter and was masking it + // in BOTH arms identically (run 30544198501, rc 12 with the stack frame to prove it). + // `-e` reaches the same reads with no main-path resolution: the run before showed + // `node -p` unaffected while `node ` died, which is the documented shape. + // // Reaching the distribution's BUNDLED npm tree is the exact cell an un-ACE'd install fails - // on, and requiring a file out of it is the cheapest honest way to ask for it. - std::fs::write( - &entry, - format!( - "console.log(process.execPath);\n\ - console.log('abi=' + process.versions.modules);\n\ - console.log('npm=' + require({}).version);\n", - js_string(&root.join("node_modules/npm/package.json")) - ), - ) - .expect("write the arm's entry file"); + // on, and requiring a file out of it is the cheapest honest way to ask for it. Only single + // quotes appear inside, because cmd gives the whole `-e` program to `"`. + let program = format!( + "console.log(process.execPath);\ + console.log('abi='+process.versions.modules);\ + console.log('npm='+require({}).version)", + js_string(&root.join("node_modules/npm/package.json")) + ); // Every path INSIDE the script is CWD-RELATIVE, and that is not tidiness. An absolute open // from inside the jail walks the whole ancestor chain as targets, and making that chain // reachable without elevation is a SEPARATE, still-open repair — an absolute script path @@ -1429,11 +1449,11 @@ mod win { node -p \"process.version\" >> \"{m}\" 2>&1\r\n\ if errorlevel 1 exit /b 11\r\n\ echo STEP-BARE-NODE-RESOLVED-AND-RAN>> \"{m}\"\r\n\ - node \"{e}\" >> \"{m}\" 2>&1\r\n\ + node -e \"{e}\" >> \"{m}\" 2>&1\r\n\ if errorlevel 1 exit /b 12\r\n\ echo LIFECYCLE-OK>> \"{m}\"\r\n", m = leaf(&marker), - e = leaf(&entry) + e = program ), ) .expect("write the arm's script"); @@ -1517,10 +1537,14 @@ mod win { .into_owned() } - /// A Windows path as a JS string literal — backslashes doubled, so the emitted `require()` - /// argument is the path rather than a run of escape sequences. + /// A Windows path as a SINGLE-QUOTED JS string literal, backslashes doubled. + /// + /// Single quotes are not a style choice: the whole `-e` program is one cmd argument delimited by + /// `"`, so a double-quoted JS literal inside it would terminate that argument and hand cmd a + /// path as a second token. Doubling the separators is what keeps `C:\Program Files` a path + /// rather than a run of escape sequences. fn js_string(p: &Path) -> String { - format!("\"{}\"", p.to_string_lossy().replace('\\', "\\\\")) + format!("'{}'", p.to_string_lossy().replace('\\', "\\\\")) } fn abi_of(log: &str) -> Option<&str> { From 0c9bd32578e684c153fc35c513351bb62adf6bb9 Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Thu, 30 Jul 2026 06:05:21 -0700 Subject: [PATCH 6/8] =?UTF-8?q?probe(win):=20the=20bundled-npm=20read=20is?= =?UTF-8?q?=20a=20fact,=20not=20a=20gate=20=E2=80=94=20it=20rides=20the=20?= =?UTF-8?q?ancestor=20repair?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run 30544794220 settled where the remaining failure lives. De-elevated, the staged copy's own contract all passes: staging succeeded with no WRITE_DAC anywhere, the child's process.execPath IS the nub-owned copy, its ABI matches the source, and the inheritable ACE reads back on both the staged dir and node_modules/npm/bin. What fails is the last step of the script body, in BOTH interpreter arms identically and at the same frame — a CJS require of an ABSOLUTE path realpath's every prefix from the volume root (Module._findPath → toRealPath) and gets `EPERM … lstat 'C:\'`. That is the ancestor-reachability repair, not the interpreter. Gating on it made this group red for someone else's open work while reporting nothing about its own subject, so the npm-tree read now runs after the verdict line and its outcome is recorded as fact:interp--npm-tree-read. The gated body is now exactly the interpreter's contract: cmd opens the script, bare `node` resolves off the sanitized PATH and runs, and it reports which binary it is. The consequence to keep in view: the bundled-npm read — the cell 5j measured failing — is verified ELEVATED only. It cannot be verified de-elevated until the ancestor repair lands, and no amount of interpreter work will change that. --- .github/workflows/win-jail-interp-probe.yml | 8 ++++++ .../tests/windows_deelevated_jail.rs | 28 ++++++++++++++----- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/.github/workflows/win-jail-interp-probe.yml b/.github/workflows/win-jail-interp-probe.yml index ce0228c18..906fbcd7d 100644 --- a/.github/workflows/win-jail-interp-probe.yml +++ b/.github/workflows/win-jail-interp-probe.yml @@ -174,6 +174,14 @@ jobs: echo "---- confinement survives it ----" need_pass interp-staged-ungranted-secret-still-refused + # RECORDED, NOT GATED. Reading the distribution's bundled npm tree is the cell 5j found + # failing, but a CJS require of an absolute path realpath's from the volume root and dies + # de-elevated on the still-open ancestor repair — in BOTH interpreter arms identically, so + # gating on it would report someone else's open work as this group's failure. Absence is + # still an error: a fact that was never measured tells us nothing. + echo "---- the bundled npm tree (a fact: it rides the ancestor repair, not this one) ----" + grep -E '^ fact:interp-(ambient|staged)-npm-tree-read=' probe.log || { echo "MISS the npm-tree facts"; fail=1; } + echo "---- the pre-existing groups must not have regressed ----" for p in profile-create-and-launch acl-grant-allow acl-grant-deny teardown \ job-reap egress-deny production-jail-launch production-jail-egress; do diff --git a/crates/nub-sandbox/tests/windows_deelevated_jail.rs b/crates/nub-sandbox/tests/windows_deelevated_jail.rs index b1194e4c1..31958ca49 100644 --- a/crates/nub-sandbox/tests/windows_deelevated_jail.rs +++ b/crates/nub-sandbox/tests/windows_deelevated_jail.rs @@ -1419,10 +1419,17 @@ mod win { // Reaching the distribution's BUNDLED npm tree is the exact cell an un-ACE'd install fails // on, and requiring a file out of it is the cheapest honest way to ask for it. Only single // quotes appear inside, because cmd gives the whole `-e` program to `"`. - let program = format!( - "console.log(process.execPath);\ - console.log('abi='+process.versions.modules);\ - console.log('npm='+require({}).version)", + let identity = "console.log(process.execPath);console.log('abi='+process.versions.modules)"; + // READING THE BUNDLED npm TREE IS RECORDED, NOT GATED, and that is a scoping decision with + // a measurement behind it. It is the cell §5j found failing, so it belongs here — but a CJS + // `require` of an ABSOLUTE path realpath's every prefix from the volume root + // (`Module._findPath` → `toRealPath`), which de-elevated dies `EPERM … lstat 'C:\'` in BOTH + // interpreter arms identically (run 30544794220). That is the ancestor-reachability repair, + // not the interpreter, and gating on it would make this group red for someone else's open + // work while reporting nothing about its own subject. So it runs LAST, after the verdict + // line, and its outcome is a fact. + let npm_read = format!( + "console.log('npm='+require({}).version)", js_string(&root.join("node_modules/npm/package.json")) ); // Every path INSIDE the script is CWD-RELATIVE, and that is not tidiness. An absolute open @@ -1449,11 +1456,14 @@ mod win { node -p \"process.version\" >> \"{m}\" 2>&1\r\n\ if errorlevel 1 exit /b 11\r\n\ echo STEP-BARE-NODE-RESOLVED-AND-RAN>> \"{m}\"\r\n\ - node -e \"{e}\" >> \"{m}\" 2>&1\r\n\ + node -e \"{i}\" >> \"{m}\" 2>&1\r\n\ if errorlevel 1 exit /b 12\r\n\ - echo LIFECYCLE-OK>> \"{m}\"\r\n", + echo LIFECYCLE-OK>> \"{m}\"\r\n\ + node -e \"{n}\" >> \"{m}\" 2>&1\r\n\ + if errorlevel 1 echo NPM-TREE-READ-FAILED>> \"{m}\"\r\n", m = leaf(&marker), - e = program + i = identity, + n = npm_read ), ) .expect("write the arm's script"); @@ -1483,6 +1493,10 @@ mod win { } }; let log = std::fs::read_to_string(&marker).unwrap_or_default(); + println!( + " fact:interp-{tag}-npm-tree-read={}", + if log.contains("npm=") { "ok" } else { "failed" } + ); println!(" arm:{tag} rc={rc}"); if log.is_empty() { // Nothing ran, so the grant list is the only remaining evidence about why. From 5c7d08e55ee271a2474d6c8bcb9fecd2167f928a Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Thu, 30 Jul 2026 06:20:13 -0700 Subject: [PATCH 7/8] probe(win): settle whether cmd.exe is broken confined, or was aborted by its own grant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A sibling lane measured cmd.exe de-elevated as 19-of-19 cells ABSENT — exit 1 at 100 ms, empty transcript, `Access is denied.` — and read it as cmd being unusable under confinement. This run's staged arm ran a real .cmd postinstall to completion de-elevated, so both cannot be simply true. The suspect is structural and is in this diff. `resolve_program` auto-grants the program FILE ITSELF into read_grants (windows.rs, "Auto-grant read+execute on the program FILE ITSELF ... so the LowBox child can exec"). So launching %SystemRoot%\System32\cmd.exe attempts an ACE on it, and de-elevated a standard user holds no WRITE_DAC there. Under the fail-closed `?` this replaced, that refusal aborted the launch — which produces exactly exit-1-with-an-empty- transcript and is indistinguishable from cmd misbehaving under confinement. So the loop gets a seam, mirroring NUB_SANDBOX_WIN_NO_ANCESTOR_REPAIR, and one arm of the same run restores the old behaviour on the same fixture, same policy, same script, same token. It can only ever make the jail stricter, so it is not a lever anything can be widened with. The script body also grows the battery it was missing. Completing a postinstall proves cmd can execute a script; it does not prove `if exist`, `cd`, `dir`, `for`, `set`/expansion and where.exe behave, which is what the sibling's cells covered. Nine named cells now, reported per arm as a LIST rather than a count — the difference between "cmd is broken confined" and "one builtin is". Both properties are stated as the dependence that holds in BOTH arms rather than "fail-closed always fails": elevated, the program grant legitimately succeeds and there is nothing to abort. That is the same correction the ambient property already needed. --- .github/workflows/win-jail-interp-probe.yml | 11 ++ crates/nub-sandbox/src/backend/windows.rs | 11 +- .../tests/windows_deelevated_jail.rs | 120 +++++++++++++++++- 3 files changed, 135 insertions(+), 7 deletions(-) diff --git a/.github/workflows/win-jail-interp-probe.yml b/.github/workflows/win-jail-interp-probe.yml index 906fbcd7d..3d6861fb2 100644 --- a/.github/workflows/win-jail-interp-probe.yml +++ b/.github/workflows/win-jail-interp-probe.yml @@ -182,6 +182,17 @@ jobs: echo "---- the bundled npm tree (a fact: it rides the ancestor repair, not this one) ----" grep -E '^ fact:interp-(ambient|staged)-npm-tree-read=' probe.log || { echo "MISS the npm-tree facts"; fail=1; } + # SETTLES A SIBLING LANE'S VERDICT that cmd.exe cannot run confined de-elevated. One + # variable: the leaf-read-grant loop's fail-closed-vs-fail-soft behaviour, same fixture, + # same policy, same script, same token. `resolve_program` auto-grants the program FILE, so + # a System32 program's own grant is attempted and refused de-elevated — under `?` that + # aborted the launch, which looks exactly like cmd misbehaving. + echo "---- cmd.exe: fail-closed vs fail-soft, one variable ----" + need_pass interp-cmd-under-fail-closed-aborts-only-when-its-own-grant-is-unwritable + need_pass interp-cmd-under-fail-soft-runs-the-whole-battery + grep -E '^ fact:interp-cmd-program-grant-writable=' probe.log || { echo "MISS the program-grant writability fact"; fail=1; } + grep -E '^ fact:interp-.*-cmd-cells=' probe.log || { echo "MISS the cmd cell tallies"; fail=1; } + echo "---- the pre-existing groups must not have regressed ----" for p in profile-create-and-launch acl-grant-allow acl-grant-deny teardown \ job-reap egress-deny production-jail-launch production-jail-egress; do diff --git a/crates/nub-sandbox/src/backend/windows.rs b/crates/nub-sandbox/src/backend/windows.rs index 5e246aa02..2bf08793d 100644 --- a/crates/nub-sandbox/src/backend/windows.rs +++ b/crates/nub-sandbox/src/backend/windows.rs @@ -1751,10 +1751,19 @@ pub(super) mod launch { // package directory being built, both under the user's own tree, so a refusal // there is not a reachable configuration — it is a broken assumption, and // continuing would launch a build that silently cannot write its output. + // THE SEAM, and it exists for one reason: the fail-closed behaviour this replaced is + // the prime suspect for a sibling lane's finding that `cmd.exe` cannot run confined at + // all de-elevated. `resolve_program` auto-grants the program FILE (above), so a + // System32 program's own leaf grant is attempted, and de-elevated it is refused — which + // under `?` aborted the launch and is indistinguishable from cmd misbehaving. Without an + // arm that restores the old behaviour on the same fixture, neither reading can be told + // from the other. It can only ever make the jail STRICTER, so it is not a lever + // anything can be widened with. + let fail_closed = std::env::var_os("NUB_SANDBOX_WIN_FAIL_CLOSED_READ_GRANTS").is_some(); for (kind, dir, access) in leaves { let installed = match grant_leaf_ace(dir, ac_sid, access) { Ok(installed) => installed, - Err(_) if kind == "read" => continue, + Err(_) if kind == "read" && !fail_closed => continue, Err(error) => { return Err(io::Error::new( error.kind(), diff --git a/crates/nub-sandbox/tests/windows_deelevated_jail.rs b/crates/nub-sandbox/tests/windows_deelevated_jail.rs index 31958ca49..446188e8a 100644 --- a/crates/nub-sandbox/tests/windows_deelevated_jail.rs +++ b/crates/nub-sandbox/tests/windows_deelevated_jail.rs @@ -1320,6 +1320,67 @@ mod win { denied == 5 || denied == 9, &format!("(child exit {denied}; 5/9 = denied)"), ); + + // ── the fail-closed differential ────────────────────────────────────────────── + // + // A sibling lane measured `cmd.exe` de-elevated as 19-of-19 cells ABSENT — exit 1 at + // 100 ms, empty transcript, `Access is denied.` — and read it as cmd being unusable under + // confinement, which would make a different lifecycle shell necessary rather than merely + // better. This arm settles it, because the leaf-read-grant loop is the ONE thing that + // changed and it is exactly the suspect: `resolve_program` auto-grants the program FILE, so + // launching `%SystemRoot%\System32\cmd.exe` attempts an ACE on it, and de-elevated a + // standard user holds no `WRITE_DAC` there. Under `?` that aborted the launch — which + // produces precisely that shape and is indistinguishable from cmd misbehaving. + // + // ONE VARIABLE: the same fixture, the same policy, the same script, the same token; only + // the seam differs. Reported as a fact in the elevated arm (where the grant SUCCEEDS, so + // there is nothing to abort and the arms cannot differ) and GATED de-elevated. + println!( + " fact:interp-cmd-program-grant-writable={}", + can_write_dacl(Path::new(&comspec_path())) + ); + let (fc_rc, fc_log) = with_fail_closed_read_grants(|| { + lifecycle_arm(f, "staged-failclosed", &staged_exe, &staged_dir) + }); + let fc_cells = CMD_CELLS.iter().filter(|c| fc_log.contains(**c)).count(); + let soft_cells = CMD_CELLS + .iter() + .filter(|c| staged_log.contains(**c)) + .count(); + // The law that holds in BOTH arms: fail-closed loses the launch exactly when the program's + // own grant cannot be written. Stated as the dependence rather than "fail-closed always + // fails", because elevated it legitimately succeeds — the same mistake the ambient property + // already had to be rewritten to avoid. + let program_writable = can_write_dacl(Path::new(&comspec_path())); + r.record( + "interp-cmd-under-fail-closed-aborts-only-when-its-own-grant-is-unwritable", + (fc_cells > 0) == program_writable, + &format!( + "(program grant writable {program_writable}; fail-closed cells {fc_cells}/{}, rc \ + {fc_rc}; fail-soft cells {soft_cells}/{})", + CMD_CELLS.len(), + CMD_CELLS.len() + ), + ); + // …and the consequence the sibling verdict turns on: under fail-SOFT, cmd runs the whole + // battery. If this ever fails while the fail-closed arm also produces nothing, cmd really is + // broken confined and the sibling reading stands. + r.record( + "interp-cmd-under-fail-soft-runs-the-whole-battery", + soft_cells == CMD_CELLS.len(), + &format!("({soft_cells}/{} cells)", CMD_CELLS.len()), + ); + } + + /// The command interpreter the arms launch, resolved the way [`lifecycle_arm`] resolves it so + /// the grant-writability fact is about the same file. + fn comspec_path() -> String { + std::env::var("ComSpec").unwrap_or_else(|_| { + format!( + "{}\\System32\\cmd.exe", + std::env::var("SystemRoot").unwrap_or_else(|_| "C:\\Windows".into()) + ) + }) } /// Whether THIS token may rewrite `path`'s DACL — the one permission [`set_ace`] needs, asked @@ -1378,6 +1439,8 @@ mod win { "interp-staged-dir-publishes-read-to-appcontainers", "interp-staged-deep-entry-inherited-the-ace-at-creation", "interp-staged-ungranted-secret-still-refused", + "interp-cmd-under-fail-closed-aborts-only-when-its-own-grant-is-unwritable", + "interp-cmd-under-fail-soft-runs-the-whole-battery", ]; /// The all-users MSI install first, because it is the configuration that exhibits the defect @@ -1459,6 +1522,16 @@ mod win { node -e \"{i}\" >> \"{m}\" 2>&1\r\n\ if errorlevel 1 exit /b 12\r\n\ echo LIFECYCLE-OK>> \"{m}\"\r\n\ + if exist \"{m}\" echo CELL-IF-EXIST>> \"{m}\"\r\n\ + cd>> \"{m}\" 2>&1\r\n\ + if not errorlevel 1 echo CELL-CD>> \"{m}\"\r\n\ + dir /b>> \"{m}\" 2>&1\r\n\ + if not errorlevel 1 echo CELL-DIR>> \"{m}\"\r\n\ + for %%%%V in (1) do echo CELL-FOR>> \"{m}\"\r\n\ + set NUBCELL=1\r\n\ + if \"%%NUBCELL%%\"==\"1\" echo CELL-SET-AND-EXPAND>> \"{m}\"\r\n\ + where.exe node>> \"{m}\" 2>&1\r\n\ + if not errorlevel 1 echo CELL-WHERE>> \"{m}\"\r\n\ node -e \"{n}\" >> \"{m}\" 2>&1\r\n\ if errorlevel 1 echo NPM-TREE-READ-FAILED>> \"{m}\"\r\n", m = leaf(&marker), @@ -1469,12 +1542,7 @@ mod win { .expect("write the arm's script"); let policy = build_jail_for(f, exe, root); - let comspec = std::env::var("ComSpec").unwrap_or_else(|_| { - format!( - "{}\\System32\\cmd.exe", - std::env::var("SystemRoot").unwrap_or_else(|_| "C:\\Windows".into()) - ) - }); + let comspec = comspec_path(); let spec = CommandSpec::new(&comspec) .arg("/c") .arg(leaf(&script)) @@ -1493,6 +1561,17 @@ mod win { } }; let log = std::fs::read_to_string(&marker).unwrap_or_default(); + let cells: Vec<&str> = CMD_CELLS + .iter() + .copied() + .filter(|c| log.contains(c)) + .collect(); + println!( + " fact:interp-{tag}-cmd-cells={}/{} [{}]", + cells.len(), + CMD_CELLS.len(), + cells.join(" ") + ); println!( " fact:interp-{tag}-npm-tree-read={}", if log.contains("npm=") { "ok" } else { "failed" } @@ -1551,6 +1630,35 @@ mod win { .into_owned() } + /// The cmd-interpreter behaviours the script body exercises beyond "a script ran": the + /// builtins and the PATH search a real postinstall depends on. Named so a partial result is a + /// list rather than a count — which is the difference between "cmd is broken confined" and "one + /// builtin is". + const CMD_CELLS: &[&str] = &[ + "STEP-CMD-OPENED-THE-SCRIPT", + "STEP-BARE-NODE-RESOLVED-AND-RAN", + "LIFECYCLE-OK", + "CELL-IF-EXIST", + "CELL-CD", + "CELL-DIR", + "CELL-FOR", + "CELL-SET-AND-EXPAND", + "CELL-WHERE", + ]; + + /// Run `body` with the leaf-read-grant loop restored to its FAIL-CLOSED behaviour, so one arm + /// of the same run carries the code a sibling lane measured `cmd.exe` under. Mirrors + /// `windows_jail_repairs`'s `without_ancestor_repair`. + fn with_fail_closed_read_grants(body: impl FnOnce() -> T) -> T { + const KEY: &str = "NUB_SANDBOX_WIN_FAIL_CLOSED_READ_GRANTS"; + // SAFETY: the arm is single-threaded at this point (every launch in this group is + // sequential), and the variable is removed before returning on both paths. + unsafe { std::env::set_var(KEY, "1") }; + let out = body(); + unsafe { std::env::remove_var(KEY) }; + out + } + /// A Windows path as a SINGLE-QUOTED JS string literal, backslashes doubled. /// /// Single quotes are not a style choice: the whole `-e` program is one cmd argument delimited by From 1991efae41bce0f8351967951a663abda69effb3 Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Thu, 30 Jul 2026 06:27:38 -0700 Subject: [PATCH 8/8] =?UTF-8?q?probe(win):=20the=20cmd=20verdict=20was=20a?= =?UTF-8?q?=20fail-closed=20artifact=20=E2=80=94=20and=20two=20of=20my=20o?= =?UTF-8?q?wn=20cells=20were=20a=20bug?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run 30546452759 settled it, de-elevated, one variable: fail-SOFT 5/9 cells, rc 0 fail-CLOSED 0/9 cells, rc -101 [staged-failclosed status Err] sandbox: installing read grant ACE on C:\Windows\system32\cmd.exe failed: Access is denied. (os error 5) That string is verbatim what a sibling lane read as cmd.exe being unusable under confinement, and the path is the PROGRAM FILE `resolve_program` auto-grants. So the 19-of-19-absent cmd result was the launch aborting on cmd's own grant, not cmd misbehaving. The sibling's own busybox row is the same mechanism from the other side: it is staged into the fixture, so its grant reads `writable` where `cmd-exe` reads `refused:Some(5)` — a nub-owned program is grantable, which is exactly what this PR does for the interpreter. Not overstated, because the sibling's data does not fully fit: pwsh ran confined de-elevated (`code=Some(0) after_ms=802`) and powershell.exe started, both with equally-refused grants. So "fail-closed aborted everything refused" is NOT a universal account. The cmd case is what is established, causally. Two corrections to this probe, both mine: The property keyed on `can_write_dacl`, which is an UNSOUND proxy — it returned false in the ELEVATED arm on System32\cmd.exe while the real grant there plainly succeeded, because an elevated token's DACL-write authority does not come from the file's DACL and an access-checked open cannot see it. It now keys on the mechanism: launch a trivial `exit /b 0` under fail-closed and read the launcher's own error, which names the path it refused. The proxy stays as a labelled fact. And CELL-FOR and CELL-SET-AND-EXPAND never had a chance: I over-escaped `%` in the Rust literal (`%%%%V` reaches the .cmd as `%%%%V`, not `%%V`), so two "cmd cannot do this" readings were my own quoting. Fixed. The tally is a fact rather than a gate, because de-elevated `dir /b` IS genuinely refused in the package dir while it works elevated — a real residual belonging to the ancestor/traverse work. What is gated is the part read as zero: cmd starts, opens its script, resolves `node` off the sanitized PATH, and reaches its verdict. --- .github/workflows/win-jail-interp-probe.yml | 12 ++- .../tests/windows_deelevated_jail.rs | 93 ++++++++++++++----- 2 files changed, 80 insertions(+), 25 deletions(-) diff --git a/.github/workflows/win-jail-interp-probe.yml b/.github/workflows/win-jail-interp-probe.yml index 3d6861fb2..b086f77f4 100644 --- a/.github/workflows/win-jail-interp-probe.yml +++ b/.github/workflows/win-jail-interp-probe.yml @@ -188,9 +188,15 @@ jobs: # a System32 program's own grant is attempted and refused de-elevated — under `?` that # aborted the launch, which looks exactly like cmd misbehaving. echo "---- cmd.exe: fail-closed vs fail-soft, one variable ----" - need_pass interp-cmd-under-fail-closed-aborts-only-when-its-own-grant-is-unwritable - need_pass interp-cmd-under-fail-soft-runs-the-whole-battery - grep -E '^ fact:interp-cmd-program-grant-writable=' probe.log || { echo "MISS the program-grant writability fact"; fail=1; } + need_pass interp-cmd-under-fail-closed-aborts-iff-its-own-program-grant-is-refused + need_pass interp-cmd-under-fail-soft-reaches-step-cmd-opened-the-script + need_pass interp-cmd-under-fail-soft-reaches-step-bare-node-resolved-and-ran + need_pass interp-cmd-under-fail-soft-reaches-lifecycle-ok + grep -E '^ fact:interp-cmd-program-grant-write-dac-open=' probe.log || { echo "MISS the program-grant fact"; fail=1; } + # The cell TALLIES are facts, not gates. De-elevated `dir /b` is refused in the package dir + # while it works elevated — a real residual that belongs to the ancestor/traverse work, not + # to the interpreter — so a full 9/9 is not the bar here. What IS gated is the part the + # sibling lane read as zero. grep -E '^ fact:interp-.*-cmd-cells=' probe.log || { echo "MISS the cmd cell tallies"; fail=1; } echo "---- the pre-existing groups must not have regressed ----" diff --git a/crates/nub-sandbox/tests/windows_deelevated_jail.rs b/crates/nub-sandbox/tests/windows_deelevated_jail.rs index 446188e8a..69bec0ffc 100644 --- a/crates/nub-sandbox/tests/windows_deelevated_jail.rs +++ b/crates/nub-sandbox/tests/windows_deelevated_jail.rs @@ -1335,8 +1335,15 @@ mod win { // ONE VARIABLE: the same fixture, the same policy, the same script, the same token; only // the seam differs. Reported as a fact in the elevated arm (where the grant SUCCEEDS, so // there is nothing to abort and the arms cannot differ) and GATED de-elevated. + // `can_write_dacl` is a FACT here and deliberately NOT what the property keys on. It is an + // UNSOUND proxy for whether `set_ace` succeeds, measured: it returned false in the ELEVATED + // arm on `System32\cmd.exe` while the real grant there plainly succeeded — that arm produced + // cells under fail-closed and never aborted. An elevated token's DACL-write authority does + // not come from the file's DACL, so an access-checked open cannot see it. Kept because it is + // still right de-elevated and it is the reading a future reader reaches for; labelled so + // nobody keys a verdict on it again. println!( - " fact:interp-cmd-program-grant-writable={}", + " fact:interp-cmd-program-grant-write-dac-open={} (UNSOUND above medium IL)", can_write_dacl(Path::new(&comspec_path())) ); let (fc_rc, fc_log) = with_fail_closed_read_grants(|| { @@ -1347,33 +1354,73 @@ mod win { .iter() .filter(|c| staged_log.contains(**c)) .count(); - // The law that holds in BOTH arms: fail-closed loses the launch exactly when the program's - // own grant cannot be written. Stated as the dependence rather than "fail-closed always - // fails", because elevated it legitimately succeeds — the same mistake the ambient property - // already had to be rewritten to avoid. - let program_writable = can_write_dacl(Path::new(&comspec_path())); + // THE PROPERTY, keyed on the MECHANISM rather than a proxy for it: fail-closed loses the + // launch exactly when the leaf grant it aborts on is the one that was refused, which the + // launcher names in its OWN error. Elevated that grant succeeds and the launch survives; + // de-elevated it is refused and the launch dies with + // `installing read grant ACE on C:\Windows\system32\cmd.exe failed` — verbatim the string a + // sibling lane read as cmd being unusable under confinement. One law, both arms. + let aborted = fc_rc == -101 && fc_cells == 0; + let grant_refused = !program_grant_landed(f, &staged_exe, &staged_dir); r.record( - "interp-cmd-under-fail-closed-aborts-only-when-its-own-grant-is-unwritable", - (fc_cells > 0) == program_writable, + "interp-cmd-under-fail-closed-aborts-iff-its-own-program-grant-is-refused", + aborted == grant_refused, &format!( - "(program grant writable {program_writable}; fail-closed cells {fc_cells}/{}, rc \ + "(program grant refused {grant_refused}; fail-closed cells {fc_cells}/{}, rc \ {fc_rc}; fail-soft cells {soft_cells}/{})", CMD_CELLS.len(), CMD_CELLS.len() ), ); - // …and the consequence the sibling verdict turns on: under fail-SOFT, cmd runs the whole - // battery. If this ever fails while the fail-closed arm also produces nothing, cmd really is - // broken confined and the sibling reading stands. - r.record( - "interp-cmd-under-fail-soft-runs-the-whole-battery", - soft_cells == CMD_CELLS.len(), - &format!("({soft_cells}/{} cells)", CMD_CELLS.len()), - ); + // The consequence the sibling verdict turns on, and deliberately NOT "the whole battery": + // de-elevated `dir /b` returns `Access is denied.` in the package dir even though it works + // elevated, which is a real residual belonging to the ancestor/traverse work rather than to + // the interpreter. What this asserts is the part the sibling read as ZERO — cmd starts, opens + // its script, resolves `node` off the sanitized PATH, and reaches its verdict line. The full + // tally stays a fact so a regression in the rest is still visible. + for cell in [ + "STEP-CMD-OPENED-THE-SCRIPT", + "STEP-BARE-NODE-RESOLVED-AND-RAN", + "LIFECYCLE-OK", + ] { + r.record( + &format!( + "interp-cmd-under-fail-soft-reaches-{}", + cell.to_ascii_lowercase() + ), + staged_log.contains(cell), + &format!("({soft_cells}/{} cells total)", CMD_CELLS.len()), + ); + } + } + + /// Whether the launcher could install the leaf grant on the PROGRAM file — asked by launching a + /// trivial `exit /b 0` under fail-closed and reading the launcher's OWN error, which names the + /// path it refused. That is the mechanism itself; the cheaper stand-in for it was wrong above the + /// medium integrity level (see the call site), which is why this pays for a launch. + fn program_grant_landed(f: &Fixture, exe: &Path, root: &Path) -> bool { + let policy = build_jail_for(f, exe, root); + let spec = CommandSpec::new(&comspec_path()) + .arg("/c") + .arg("exit /b 0") + .cwd(&f.package); + with_fail_closed_read_grants(|| match apply(&policy, spec) { + Ok(prepared) => match prepared.status() { + Ok(_) => true, + Err(e) => { + println!(" [program-grant probe] {e}"); + !e.to_string().contains("installing read grant ACE") + } + }, + Err(d) => { + println!(" [program-grant probe apply Err] {d:?}"); + false + } + }) } /// The command interpreter the arms launch, resolved the way [`lifecycle_arm`] resolves it so - /// the grant-writability fact is about the same file. + /// the grant fact is about the same file. fn comspec_path() -> String { std::env::var("ComSpec").unwrap_or_else(|_| { format!( @@ -1439,8 +1486,10 @@ mod win { "interp-staged-dir-publishes-read-to-appcontainers", "interp-staged-deep-entry-inherited-the-ace-at-creation", "interp-staged-ungranted-secret-still-refused", - "interp-cmd-under-fail-closed-aborts-only-when-its-own-grant-is-unwritable", - "interp-cmd-under-fail-soft-runs-the-whole-battery", + "interp-cmd-under-fail-closed-aborts-iff-its-own-program-grant-is-refused", + "interp-cmd-under-fail-soft-reaches-step-cmd-opened-the-script", + "interp-cmd-under-fail-soft-reaches-step-bare-node-resolved-and-ran", + "interp-cmd-under-fail-soft-reaches-lifecycle-ok", ]; /// The all-users MSI install first, because it is the configuration that exhibits the defect @@ -1527,9 +1576,9 @@ mod win { if not errorlevel 1 echo CELL-CD>> \"{m}\"\r\n\ dir /b>> \"{m}\" 2>&1\r\n\ if not errorlevel 1 echo CELL-DIR>> \"{m}\"\r\n\ - for %%%%V in (1) do echo CELL-FOR>> \"{m}\"\r\n\ + for %%V in (1) do echo CELL-FOR>> \"{m}\"\r\n\ set NUBCELL=1\r\n\ - if \"%%NUBCELL%%\"==\"1\" echo CELL-SET-AND-EXPAND>> \"{m}\"\r\n\ + if \"%NUBCELL%\"==\"1\" echo CELL-SET-AND-EXPAND>> \"{m}\"\r\n\ where.exe node>> \"{m}\" 2>&1\r\n\ if not errorlevel 1 echo CELL-WHERE>> \"{m}\"\r\n\ node -e \"{n}\" >> \"{m}\" 2>&1\r\n\