diff --git a/sdk/node/src/policy.ts b/sdk/node/src/policy.ts index bdbcb3cfa..8a9f2c3cc 100644 --- a/sdk/node/src/policy.ts +++ b/sdk/node/src/policy.ts @@ -7,7 +7,6 @@ */ import * as fs from 'fs'; -import * as os from 'os'; import * as path from 'path'; import { execSync } from 'child_process'; import { randomBytes } from 'crypto'; @@ -54,7 +53,7 @@ interface KnownEnvVar { /** Split a path list using the platform-appropriate separator. */ function splitPathList(value: string): string[] { - const separator = os.platform() === 'win32' ? ';' : ':'; + const separator = process.platform === 'win32' ? ';' : ':'; return value.split(separator).filter(p => p.length > 0); } @@ -119,20 +118,72 @@ function getWindowsDirectory(): string { return process.env['WINDIR'] || process.env['windir'] || 'C:\\Windows'; } +/** + * Strip a Windows verbatim / device-namespace prefix (`\\?\`, `\\?\UNC\`, + * `\\.\`), rewriting `\\?\UNC\server\share` back to `\\server\share`. + */ +function stripVerbatimPrefix(dirPath: string): string { + const upper = dirPath.toUpperCase(); + if (upper.startsWith('\\\\?\\UNC\\')) { + return '\\\\' + dirPath.slice('\\\\?\\UNC\\'.length); + } + if (upper.startsWith('\\\\?\\') || upper.startsWith('\\\\.\\')) { + return dirPath.slice('\\\\?\\'.length); + } + return dirPath; +} + +/** Whether a resolved path is a filesystem root (`C:\`, `\\server\share\`, `/`). */ +function isRootPath(resolved: string, pathApi: path.PlatformPath): boolean { + return pathApi.parse(resolved).root === resolved; +} + /** * Returns `true` if the path resides under system-critical locations. - * On Windows: under %WINDIR%. On Linux: /bin, /sbin, /boot, /proc, /sys, /dev, etc. + * On Windows: a volume/share root or under %WINDIR%. On Linux: `/`, /bin, + * /sbin, /boot, /proc, /sys, /dev, etc. + * + * `pathApi` and `isWindows` are parameters rather than reads of the ambient + * `path` module and `process.platform` so that Windows semantics — drive and + * UNC roots, drive-relative paths, verbatim and device namespaces — can be + * exercised from a POSIX host. Mocking `process.platform` alone cannot reach + * those branches, because the imported `path` module stays POSIX. + * + * @internal Exported for tests; not part of the SDK's public surface. */ -function isSystemCriticalPath(dirPath: string): boolean { - if (os.platform() === 'win32') { +export function isSystemCriticalPathWith( + dirPath: string, + pathApi: path.PlatformPath, + isWindows: boolean, +): boolean { + const resolved = pathApi.resolve(dirPath); + // A volume or share root (`C:\`, `\\server\share`, `/`) is never a + // legitimate tool directory, and granting one exposes every file on the + // volume. `path.parse().root` equals the path itself only for a root. + if (isRootPath(resolved, pathApi)) { + return true; + } + if (isWindows) { + // Strip a verbatim (`\\?\`, `\\?\UNC\`) or device-namespace (`\\.\`) + // prefix so a path supplied in that form still matches the plain + // comparisons below, as the Rust mirror does. The root test above runs + // on the un-stripped path first, so stripping can never turn an + // absolute root into a cwd-relative path that escapes it. + const unwrapped = stripVerbatimPrefix(resolved); + if (unwrapped !== resolved && isRootPath(pathApi.resolve(unwrapped), pathApi)) { + return true; + } const winDir = getWindowsDirectory().toLowerCase(); - const normalized = path.resolve(dirPath).toLowerCase(); + const normalized = unwrapped.toLowerCase(); return normalized === winDir || normalized.startsWith(winDir + '\\'); } // Linux: protect critical system paths - const normalized = path.resolve(dirPath); const criticalPaths = ['/bin', '/sbin', '/usr/bin', '/usr/sbin', '/boot', '/proc', '/sys', '/dev']; - return criticalPaths.some(cp => normalized === cp || normalized.startsWith(cp + '/')); + return criticalPaths.some(cp => resolved === cp || resolved.startsWith(cp + '/')); +} + +function isSystemCriticalPath(dirPath: string): boolean { + return isSystemCriticalPathWith(dirPath, path, process.platform === 'win32'); } /** @@ -141,7 +192,7 @@ function isSystemCriticalPath(dirPath: string): boolean { * Only applicable on Windows. */ function hasAllApplicationPackagesAccess(dirPath: string): boolean { - if (os.platform() !== 'win32') { + if (process.platform !== 'win32') { return false; // Only applicable on Windows } try { @@ -171,7 +222,7 @@ function directoryExists(dirPath: string): boolean { * case-sensitive on other platforms. Paths are resolved to absolute form. */ function deduplicatePaths(paths: string[]): string[] { - const isWindows = os.platform() === 'win32'; + const isWindows = process.platform === 'win32'; const seen = new Set(); const result: string[] = []; for (const p of paths) { @@ -194,10 +245,20 @@ function deduplicatePaths(paths: string[]): string[] { * the supplied PATH directories for a `pwsh.exe` binary. * * When PowerShell is found, return a policy fragment with: - * - `C:\` in `readonlyPaths` — pwsh.exe enumerates the drive root on startup. + * - `$PSHOME` in `readonlyPaths` — the directory holding `pwsh.exe`, its + * bundled modules and `powershell.config.json`. Additional module trees + * reach the policy through `PSModulePath`, which + * {@link getAvailableToolsPolicy} already discovers. * - The PSReadLine history directory in `readwritePaths` so the PSReadLine * module can persist command history. * + * It deliberately does **not** grant the system-drive root. `pwsh.exe` does + * stat `C:\` on startup, but that only needs metadata rights on the root + * directory itself — which `wxc-host-prep prepare-system-drive` grants + * host-wide with non-inheriting ACEs (see `docs/host-prep.md`). A `C:\` entry + * in `readonlyPaths` instead hands the sandbox read access to every file on + * the volume. + * * On non-Windows platforms or when pwsh.exe is not found on PATH, returns an * empty policy. * @@ -208,11 +269,11 @@ function getPowerShellPolicy( pathDirs: string[], env: { [key: string]: string | undefined }, ): FilesystemPolicyResult { - if (os.platform() !== 'win32') { + if (process.platform !== 'win32') { return { readonlyPaths: [], readwritePaths: [] }; } - const pwshFound = pathDirs.some(dir => { + const psHome = pathDirs.find(dir => { try { return fs.existsSync(path.join(dir, 'pwsh.exe')); } catch { @@ -220,15 +281,11 @@ function getPowerShellPolicy( } }); - if (!pwshFound) { + if (!psHome) { return { readonlyPaths: [], readwritePaths: [] }; } - const systemDrive = process.env["SystemDrive"] || 'C:'; - const systemRoot = systemDrive + "\\"; - const readonlyPaths: string[] = [systemRoot]; const readwritePaths: string[] = []; - const userProfile = env['USERPROFILE']; if (userProfile) { const psReadLineDir = path.join( @@ -237,7 +294,7 @@ function getPowerShellPolicy( readwritePaths.push(psReadLineDir); } - return { readonlyPaths, readwritePaths }; + return { readonlyPaths: [psHome], readwritePaths }; } // --------------------------------------------------------------------------- @@ -249,18 +306,19 @@ function getPowerShellPolicy( * policy paths. * * Reads the `PATH` variable and a set of well-known tool / SDK environment - * variables, enumerates the directories they reference, then applies filters: + * variables, adds the PowerShell paths when `pwsh.exe` is found on `PATH`, + * then applies filters to every discovered directory: * * 1. Directories that do not exist on disk are removed. - * 2. System-critical directories (under `%WINDIR%`) are removed. + * 2. System-critical directories (filesystem roots, and anything under + * `%WINDIR%`) are removed. * 3. When `options.containerType` is `'processcontainer'`, directories whose ACLs * already grant access to `ALL_APPLICATION_PACKAGES` are removed because * AppContainer processes can see them without explicit brokering. * - * Additionally, if PowerShell (`pwsh.exe`) is found on PATH, the drive root - * (`C:\`) is added to `readonlyPaths` and the PSReadLine history directory - * is added to `readwritePaths` so that interactive PowerShell sessions work - * correctly inside the container. + * When PowerShell is found, `$PSHOME` is added to `readonlyPaths` and the + * PSReadLine history directory is added to `readwritePaths` so that + * interactive PowerShell sessions work correctly inside the container. * * @param env - Environment variable map. Defaults to `process.env`. * @param options - Filtering options. @@ -286,6 +344,11 @@ export function getAvailableToolsPolicy( } } + // Merged before the filter below so the PowerShell grant is held to the + // same bar as every other discovered directory. + const pwshPolicy = getPowerShellPolicy(pathDirs, environment); + collected.push(...pwshPolicy.readonlyPaths); + const unique = deduplicatePaths(collected); // Filter out non-existent paths, system-critical paths, and (optionally) @@ -303,12 +366,15 @@ export function getAvailableToolsPolicy( return true; }); - // Merge PowerShell-specific paths when pwsh.exe is available - const pwshPolicy = getPowerShellPolicy(pathDirs, environment); - return { - readonlyPaths: deduplicatePaths([...filtered, ...pwshPolicy.readonlyPaths]), - readwritePaths: deduplicatePaths([...pwshPolicy.readwritePaths]), + readonlyPaths: filtered, + // Write grants get the system-critical check too — a `USERPROFILE` + // under `%WINDIR%` (the SYSTEM account's `config\systemprofile`, say) + // would otherwise yield a read-write grant inside a protected system + // directory. They are deliberately *not* existence-filtered: + // PowerShell creates the PSReadLine history directory on first use. + readwritePaths: deduplicatePaths(pwshPolicy.readwritePaths) + .filter(dirPath => !isSystemCriticalPath(dirPath)), }; } @@ -323,7 +389,7 @@ export function getAvailableToolsPolicy( export function getUserProfilePolicy(): FilesystemPolicyResult { const readonlyPaths: string[] = []; - if (os.platform() === 'win32') { + if (process.platform === 'win32') { /* TODO: Need to think through the implications of granting access to folders within APPDATA versus LOCALAPPDATA. const appData = process.env['APPDATA']; @@ -389,7 +455,7 @@ export function getTemporaryFilesPolicy( const environment = env ?? process.env; // On Linux, prefer TMPDIR; on Windows, prefer TEMP/TMP - const tempRoot = os.platform() === 'win32' + const tempRoot = process.platform === 'win32' ? (environment['TEMP'] || environment['TMP']) : (environment['TMPDIR'] || '/tmp'); diff --git a/sdk/node/tests/unit/policy.test.ts b/sdk/node/tests/unit/policy.test.ts index 7432d996b..a052528ab 100644 --- a/sdk/node/tests/unit/policy.test.ts +++ b/sdk/node/tests/unit/policy.test.ts @@ -3,17 +3,11 @@ import { describe, it, afterEach } from 'node:test'; import assert from 'node:assert'; -import { getAvailableToolsPolicy } from '../../src/policy.js'; +import { getAvailableToolsPolicy, isSystemCriticalPathWith } from '../../src/policy.js'; import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; -// TODO: Investigate why Object.defineProperty(process, 'platform', ...) does not -// take effect on Linux ADO pipeline runners (Node.js v20.19.x). These tests mock -// process.platform to 'win32' and must be skipped on Linux until the root cause -// is understood. -const isLinux = process.platform === 'linux'; - describe('getAvailableToolsPolicy - PowerShell discovery', () => { let originalPlatform: PropertyDescriptor | undefined; let tmpDir: string | undefined; @@ -46,32 +40,36 @@ describe('getAvailableToolsPolicy - PowerShell discovery', () => { } }); - it('should add system root to readonlyPaths when pwsh.exe is on PATH', { skip: isLinux }, () => { + it('should add $PSHOME (not the drive root) to readonlyPaths when pwsh.exe is on PATH', () => { mockWindows(); const pwshDir = createFakePwshDir(); const env = { PATH: pwshDir, USERPROFILE: 'C:\\Users\\TestUser' }; const result = getAvailableToolsPolicy(env); assert.ok( - result.readonlyPaths.some(p => /^[a-z]:\\$/i.test(p)), - 'System root (e.g. C:\\) should be in readonlyPaths when pwsh.exe is on PATH', + result.readonlyPaths.some(p => p.toLowerCase() === path.resolve(pwshDir).toLowerCase()), + '$PSHOME should be in readonlyPaths when pwsh.exe is on PATH', + ); + assert.ok( + !result.readonlyPaths.some(p => /^[a-z]:\\$/i.test(p)), + 'The drive root must never be granted — it exposes the whole volume', ); }); - it('should add PSReadLine dir to readwritePaths when pwsh.exe is on PATH', { skip: isLinux }, () => { + it('should add PSReadLine dir to readwritePaths when pwsh.exe is on PATH', () => { mockWindows(); const pwshDir = createFakePwshDir(); const env = { PATH: pwshDir, USERPROFILE: 'C:\\Users\\TestUser' }; const result = getAvailableToolsPolicy(env); - const expected = path.join( + const expected = path.resolve(path.join( 'C:\\Users\\TestUser', 'AppData', 'Roaming', 'Microsoft', 'Windows', 'PowerShell', 'PSReadLine', - ); + )); assert.ok( result.readwritePaths.some(p => p.toLowerCase() === expected.toLowerCase()), 'PSReadLine directory should be in readwritePaths', ); }); - it('should not add PowerShell paths when pwsh.exe is not on PATH', { skip: isLinux }, () => { + it('should not add PowerShell paths when pwsh.exe is not on PATH', () => { mockWindows(); const env = { PATH: 'C:\\Windows\\System32', USERPROFILE: 'C:\\Users\\TestUser' }; const result = getAvailableToolsPolicy(env); @@ -84,7 +82,7 @@ describe('getAvailableToolsPolicy - PowerShell discovery', () => { ); }); - it('should return empty policy on non-Windows even when pwsh.exe is on PATH', { skip: isLinux }, () => { + it('should return empty policy on non-Windows even when pwsh.exe is on PATH', () => { mockLinux(); const pwshDir = createFakePwshDir(); const env = { PATH: pwshDir, USERPROFILE: 'C:\\Users\\TestUser' }; @@ -98,17 +96,116 @@ describe('getAvailableToolsPolicy - PowerShell discovery', () => { ); }); - it('should not add PSReadLine path when USERPROFILE is not set', { skip: isLinux }, () => { + it('should not add PSReadLine path when USERPROFILE is not set', () => { mockWindows(); const pwshDir = createFakePwshDir(); const env = { PATH: pwshDir }; const result = getAvailableToolsPolicy(env); assert.ok( - result.readonlyPaths.some(p => /^[a-z]:\\$/i.test(p)), - 'System root should still be in readonlyPaths', + result.readonlyPaths.some(p => p.toLowerCase() === path.resolve(pwshDir).toLowerCase()), + '$PSHOME should still be in readonlyPaths', ); assert.strictEqual(result.readwritePaths.length, 0, 'readwritePaths should be empty without USERPROFILE', ); }); + + it('should never grant a filesystem root discovered on PATH', () => { + const root = path.parse(process.cwd()).root; + const result = getAvailableToolsPolicy({ PATH: root }); + assert.deepStrictEqual(result.readonlyPaths, [], + 'A filesystem root must never be granted — it exposes the whole volume', + ); + }); + + // The PSReadLine write grant is derived from USERPROFILE, which can + // legitimately sit under %WINDIR% — the SYSTEM account's profile is + // C:\Windows\System32\config\systemprofile. Only a real Windows host can + // exercise this: mocking `process.platform` leaves the imported `path` + // module POSIX, so the backslash-based %WINDIR% comparison never matches. + // The equivalent semantics are covered on every host by the + // 'Windows path semantics' suite below. + it('should not grant write access under %WINDIR%', { skip: process.platform !== 'win32' }, () => { + const pwshDir = createFakePwshDir(); + const env = { + PATH: pwshDir, + USERPROFILE: path.join(process.env['WINDIR'] || 'C:\\Windows', 'System32', 'config', 'systemprofile'), + }; + const result = getAvailableToolsPolicy(env); + assert.deepStrictEqual(result.readwritePaths, [], + 'No write grant may land under %WINDIR%', + ); + }); +}); + +describe('isSystemCriticalPath - Windows path semantics', () => { + // The imported `path` module is POSIX on Linux/macOS, so mocking + // `process.platform` alone cannot reach the Windows branches. Injecting + // `path.win32` exercises them on every host. + const isCritical = (p: string) => isSystemCriticalPathWith(p, path.win32, true); + const originalWinDir = process.env['WINDIR']; + + afterEach(() => { + if (originalWinDir === undefined) { + delete process.env['WINDIR']; + } else { + process.env['WINDIR'] = originalWinDir; + } + }); + + it('should reject drive and UNC share roots', () => { + process.env['WINDIR'] = 'C:\\Windows'; + for (const root of ['C:\\', 'C:/', 'D:\\', '\\\\server\\share', '\\\\server\\share\\']) { + assert.strictEqual(isCritical(root), true, `${root} must be system-critical`); + } + }); + + it('should reject roots expressed in the verbatim or device namespace', () => { + process.env['WINDIR'] = 'C:\\Windows'; + for (const root of [ + '\\\\?\\C:\\', + '\\\\?\\C:', // drive-relative: must not resolve against the cwd + '\\\\.\\C:', + '\\\\?\\Volume{9f1b2c3d-0000-0000-0000-000000000000}\\', + '\\\\?\\UNC\\server\\share', + '\\\\?\\unc\\server\\share\\', + ]) { + assert.strictEqual(isCritical(root), true, `${root} must be system-critical`); + } + }); + + it('should reject %WINDIR% however it is spelled', () => { + process.env['WINDIR'] = 'C:\\Windows'; + for (const dir of [ + 'C:\\Windows', + 'c:\\windows\\system32', + 'C:\\Windows\\..\\Windows\\System32', + '\\\\?\\C:\\Windows\\System32', + '\\\\.\\C:\\Windows', + // The SYSTEM account's USERPROFILE — the PSReadLine write grant + // derived from it must be rejected. + 'C:\\Windows\\System32\\config\\systemprofile\\AppData\\Roaming\\Microsoft\\Windows\\PowerShell\\PSReadLine', + ]) { + assert.strictEqual(isCritical(dir), true, `${dir} must be system-critical`); + } + }); + + it('should allow ordinary tool directories', () => { + process.env['WINDIR'] = 'C:\\Windows'; + for (const dir of [ + 'C:\\Program Files\\PowerShell\\7', + 'C:\\tools', + 'C:\\WindowsApps\\vendor', // prefix of %WINDIR% but not under it + '\\\\server\\share\\tools', + '\\\\?\\UNC\\server\\share\\tools', + ]) { + assert.strictEqual(isCritical(dir), false, `${dir} must not be system-critical`); + } + }); + + it('should reject the POSIX root on non-Windows', () => { + assert.strictEqual(isSystemCriticalPathWith('/', path.posix, false), true); + assert.strictEqual(isSystemCriticalPathWith('/usr/bin', path.posix, false), true); + assert.strictEqual(isSystemCriticalPathWith('/opt/tools', path.posix, false), false); + }); }); diff --git a/src/backends/appcontainer/common/src/fallback_detector.rs b/src/backends/appcontainer/common/src/fallback_detector.rs index 8747bb43e..8d3494523 100644 --- a/src/backends/appcontainer/common/src/fallback_detector.rs +++ b/src/backends/appcontainer/common/src/fallback_detector.rs @@ -371,7 +371,7 @@ fn system_drive_root() -> std::path::PathBuf { /// Read-only and best-effort: a failed DACL read for any SID yields /// `false`, so the caller surfaces the recommendation rather than /// suppressing it on incomplete information. -fn system_drive_prepared() -> bool { +pub(crate) fn system_drive_prepared() -> bool { let root = system_drive_root(); HOST_PREP_AC_SIDS.iter().all( |sid| match wxc_common::filesystem_dacl::scan_explicit_aces_for_sid(&root, sid) { diff --git a/src/backends/appcontainer/common/src/launch_diagnostics.rs b/src/backends/appcontainer/common/src/launch_diagnostics.rs index 3ca301eb0..6db6bfc21 100644 --- a/src/backends/appcontainer/common/src/launch_diagnostics.rs +++ b/src/backends/appcontainer/common/src/launch_diagnostics.rs @@ -143,15 +143,20 @@ fn check_exe_heuristics( }); } - if missing_root_readonly(exe_path, readonly_paths) { + if missing_root_metadata_access(exe_path, readonly_paths, || { + crate::fallback_detector::system_drive_prepared() + }) { let root = drive_root(exe_path); return Some(LaunchDiagnostic { kind: "missing_filesystem_access", message: format!( - "pwsh.exe versions before 7.7 require read-only access to the \ - root drive ({root}) to start. The current sandbox policy does \ - not grant this access. Add \"{root}\" to `readonlyPaths` in your \ - sandbox policy, or upgrade to pwsh 7.7+." + "pwsh.exe versions before 7.7 require read access to the root \ + drive ({root}) to start, and this host has not been prepared \ + to grant it. Run `wxc-host-prep prepare-system-drive` — it \ + adds metadata-only, non-inheriting ACEs for the AppContainer \ + SIDs on {root} — or upgrade to pwsh 7.7+. Do not add \ + \"{root}\" to `readonlyPaths`: that grant is recursive and \ + would expose every file on the volume to the sandbox." ), }); } @@ -284,7 +289,25 @@ fn is_powershell(exe_path: &Path) -> bool { filename == "pwsh.exe" || filename == "powershell.exe" } -fn missing_root_readonly(exe_path: &Path, readonly_paths: &[String]) -> bool { +/// Whether a failing `pwsh.exe` launch plausibly lacks the root-drive +/// metadata access it needs to start. +/// +/// The sandbox can obtain that access two ways: a (dangerously recursive) +/// root entry in `readonlyPaths`, or the narrow metadata-only ACEs that +/// `wxc-host-prep prepare-system-drive` stamps on the root. Since the policy +/// discovery helpers deliberately never grant a filesystem root, the host-prep +/// state is normally the only signal — without checking it, every ordinary +/// nonzero `pwsh.exe` exit (a script error, say) would be misreported as +/// missing filesystem access. +/// +/// `drive_prepared` is a closure so the host-prep DACL probe is skipped +/// entirely for non-PowerShell exes and for policies that already grant the +/// root, and so tests can supply a deterministic host state. +fn missing_root_metadata_access( + exe_path: &Path, + readonly_paths: &[String], + drive_prepared: impl FnOnce() -> bool, +) -> bool { let filename = exe_path .file_name() .unwrap_or_default() @@ -294,9 +317,10 @@ fn missing_root_readonly(exe_path: &Path, readonly_paths: &[String]) -> bool { return false; } let root = drive_root(exe_path); - !readonly_paths + let policy_grants_root = readonly_paths .iter() - .any(|p| p.eq_ignore_ascii_case(&root) || p == "\\") + .any(|p| p.eq_ignore_ascii_case(&root) || p == "\\"); + !policy_grants_root && !drive_prepared() } fn drive_root(exe_path: &Path) -> String { @@ -399,11 +423,30 @@ mod tests { } #[test] - fn missing_root_readonly_from_exit() { - let diag = - diagnose_process_exit(r#""C:\Program Files\PowerShell\7\pwsh.exe""#, &[], &[], 1); - assert!(diag.is_some()); - assert_eq!(diag.unwrap().kind, "missing_filesystem_access"); + fn missing_root_metadata_access_only_when_host_unprepared() { + let pwsh = Path::new(r"C:\Program Files\PowerShell\7\pwsh.exe"); + + // Unprepared host, no root grant: the diagnostic is warranted. + assert!(missing_root_metadata_access(pwsh, &[], || false)); + + // Prepared host: the metadata ACEs already grant what pwsh needs, so + // a nonzero exit must NOT be blamed on filesystem access. + assert!(!missing_root_metadata_access(pwsh, &[], || true)); + + // An explicit (over-broad) root grant also satisfies pwsh, and must + // short-circuit before the host probe runs. + assert!(!missing_root_metadata_access( + pwsh, + &["C:\\".to_string()], + || { panic!("host-prep probe must not run when the policy already grants the root") } + )); + + // Non-PowerShell exes never reach the probe. + assert!(!missing_root_metadata_access( + Path::new(r"C:\tools\node.exe"), + &[], + || { panic!("host-prep probe must not run for non-PowerShell executables") } + )); } #[test] diff --git a/src/core/mxc_engine/src/policy.rs b/src/core/mxc_engine/src/policy.rs index 0b1cb9a97..6e73a7027 100644 --- a/src/core/mxc_engine/src/policy.rs +++ b/src/core/mxc_engine/src/policy.rs @@ -14,7 +14,7 @@ use std::borrow::Cow; use std::collections::HashSet; -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; use wxc_common::logger::{Logger, Mode}; use wxc_common::models::ExecutionRequest; @@ -99,21 +99,15 @@ fn join_str(base: &str, segments: &[&str]) -> String { } /// Resolve a path to absolute, lexically-normalized form — the equivalent of -/// the SDK's `path.resolve`. Purely lexical (no filesystem access, no symlink -/// resolution): a relative path is joined with the cwd, then `.`/`..` segments -/// are collapsed. Crucially it does *not* canonicalize, so on Windows it keeps -/// the plain `C:\...` form (no `\\?\` verbatim prefix) — otherwise +/// the SDK's `path.resolve`. It does not touch the filesystem or resolve +/// symlinks: [`std::path::absolute`] makes the path absolute using +/// platform rules (on Windows via `GetFullPathNameW`, so drive-relative +/// `C:foo` resolves against that drive's current directory), then `.`/`..` +/// segments are collapsed. Crucially it does *not* canonicalize, so on Windows +/// it keeps the plain `C:\...` form (no `\\?\` verbatim prefix) — otherwise /// [`is_system_critical_path`]'s `C:\Windows` prefix check would never match. fn resolve_path(p: &str) -> String { - let path = Path::new(p); - let absolute = if path.is_absolute() { - path.to_path_buf() - } else { - match std::env::current_dir() { - Ok(cwd) => cwd.join(path), - Err(_) => path.to_path_buf(), - } - }; + let absolute = std::path::absolute(p).unwrap_or_else(|_| PathBuf::from(p)); normalize_lexically(&absolute) .to_string_lossy() .into_owned() @@ -122,7 +116,6 @@ fn resolve_path(p: &str) -> String { /// Collapse `.`/`..` segments without touching the filesystem, preserving the /// path prefix/root (the well-known lexical-normalize pattern). fn normalize_lexically(path: &Path) -> PathBuf { - use std::path::Component; let mut components = path.components().peekable(); let mut out = if let Some(c @ Component::Prefix(..)) = components.peek().copied() { components.next(); @@ -174,6 +167,15 @@ fn deduplicate_paths(paths: &[String]) -> Vec { /// Whether `dir` is under a system-critical location that must not be exposed. fn is_system_critical_path(dir: &str) -> bool { let normalized = resolve_path(dir); + // A volume or share root (`C:\`, `\\server\share`, `/`) is never a + // legitimate tool directory, and granting one exposes every file on the + // volume. A root is the only absolute path with no `Normal` component. + if !Path::new(&normalized) + .components() + .any(|c| matches!(c, Component::Normal(_))) + { + return true; + } if is_windows() { // A set-but-empty `WINDIR` must not disable the filter: treat empty as // unset and fall back (the same `WINDIR` handling `powershell_policy` @@ -184,12 +186,14 @@ fn is_system_critical_path(dir: &str) -> bool { .filter(|s| !s.is_empty()) .unwrap_or_else(|| "C:\\Windows".to_string()) .to_lowercase(); - // Strip a verbatim (`\\?\`, `\\?\UNC\`) prefix so a path supplied in - // that form still matches the plain `C:\Windows` comparison. + // Strip a verbatim (`\\?\`, `\\?\UNC\`) or device-namespace (`\\.\`) + // prefix so a path supplied in that form still matches the plain + // `C:\Windows` comparison. let n = normalized.to_lowercase(); let n = n .strip_prefix(r"\\?\unc\") .or_else(|| n.strip_prefix(r"\\?\")) + .or_else(|| n.strip_prefix(r"\\.\")) .unwrap_or(&n); return n == win_dir || n.starts_with(&format!("{win_dir}\\")); } @@ -233,13 +237,21 @@ fn env_or_process(env: Option<&[(String, String)]>) -> Cow<'_, [(String, String) } /// PowerShell-specific policy: when `pwsh.exe` is found on `path_dirs` -/// (Windows only), grant the system-drive root (`C:\`) read-only — `pwsh.exe` -/// enumerates the drive root on startup — plus the PSReadLine history directory -/// read-write so the module can persist command history. +/// (Windows only), grant `$PSHOME` — the directory holding `pwsh.exe`, its +/// bundled modules and `powershell.config.json` — read-only, plus the +/// PSReadLine history directory read-write so the module can persist command +/// history. Additional module trees reach the policy through `PSModulePath`, +/// which [`available_tools_policy`] already discovers. +/// +/// It deliberately does **not** grant the system-drive root. `pwsh.exe` does +/// stat `C:\` on startup, but that only needs metadata rights on the root +/// directory itself — which `wxc-host-prep prepare-system-drive` grants +/// host-wide with non-inheriting ACEs (see `docs/host-prep.md`). A `C:\` entry +/// in `readonly_paths` instead hands the sandbox read access to every file on +/// the volume. /// -/// Mirrors the SDK's `getPowerShellPolicy`. The system drive is read from the -/// process environment (`SystemDrive`, defaulting to `C:`); the user-scoped -/// `USERPROFILE` comes from the passed-in `env`. +/// Mirrors the SDK's `getPowerShellPolicy`. The user-scoped `USERPROFILE` +/// comes from the passed-in `env`. /// /// On non-Windows, or when `pwsh.exe` is not on `path_dirs`, returns an empty /// policy. @@ -248,18 +260,12 @@ fn powershell_policy(path_dirs: &[String], env: &[(String, String)]) -> Filesyst return FilesystemPolicyResult::default(); } - let pwsh_found = path_dirs + let Some(ps_home) = path_dirs .iter() - .any(|dir| Path::new(dir).join("pwsh.exe").exists()); - if !pwsh_found { + .find(|dir| Path::new(dir).join("pwsh.exe").exists()) + else { return FilesystemPolicyResult::default(); - } - - let system_drive = std::env::var("SystemDrive") - .ok() - .filter(|s| !s.is_empty()) - .unwrap_or_else(|| "C:".to_string()); - let readonly_paths = vec![format!("{system_drive}\\")]; + }; let mut readwrite_paths: Vec = Vec::new(); if let Some(user_profile) = env_get(env, "USERPROFILE") { @@ -278,7 +284,7 @@ fn powershell_policy(path_dirs: &[String], env: &[(String, String)]) -> Filesyst } FilesystemPolicyResult { - readonly_paths, + readonly_paths: vec![ps_home.clone()], readwrite_paths, } } @@ -286,9 +292,10 @@ fn powershell_policy(path_dirs: &[String], env: &[(String, String)]) -> Filesyst /// Discover tool and SDK directories from `env` (defaults to the process /// environment) as read-only policy paths. /// -/// Reads `PATH` plus a registry of well-known tool/SDK variables, then filters -/// out non-existent and system-critical directories, and adds PowerShell paths -/// when `pwsh.exe` is on `PATH`. The Rust port of `getAvailableToolsPolicy`. +/// Reads `PATH` plus a registry of well-known tool/SDK variables, and the +/// PowerShell paths when `pwsh.exe` is on `PATH`, then filters out +/// non-existent and system-critical directories (including filesystem roots). +/// The Rust port of `getAvailableToolsPolicy`. /// (The SDK's `processcontainer` AAP-ACL filter is Windows-runtime-specific and /// is applied server-side; it is not replicated here.) pub fn available_tools_policy(env: Option<&[(String, String)]>) -> FilesystemPolicyResult { @@ -313,19 +320,29 @@ pub fn available_tools_policy(env: Option<&[(String, String)]>) -> FilesystemPol } } - let filtered: Vec = deduplicate_paths(&collected) + // Merged before the filter below so the PowerShell grant is held to the + // same "exists and is not system-critical" bar as every other directory. + let pwsh = powershell_policy(&path_dirs, env); + collected.extend(pwsh.readonly_paths); + + let readonly_paths: Vec = deduplicate_paths(&collected) .into_iter() .filter(|dir| directory_exists(dir) && !is_system_critical_path(dir)) .collect(); - let pwsh = powershell_policy(&path_dirs, env); - - let mut readonly = filtered; - readonly.extend(pwsh.readonly_paths); + // Write grants get the system-critical check too — a `USERPROFILE` under + // `%WINDIR%` (the SYSTEM account's `config\systemprofile`, say) would + // otherwise yield a read-write grant inside a protected system directory. + // They are deliberately *not* existence-filtered: PowerShell creates the + // PSReadLine history directory on first use. + let readwrite_paths: Vec = deduplicate_paths(&pwsh.readwrite_paths) + .into_iter() + .filter(|dir| !is_system_critical_path(dir)) + .collect(); FilesystemPolicyResult { - readonly_paths: deduplicate_paths(&readonly), - readwrite_paths: deduplicate_paths(&pwsh.readwrite_paths), + readonly_paths, + readwrite_paths, } } @@ -1156,7 +1173,7 @@ mod tests { #[cfg(target_os = "windows")] #[test] - fn powershell_policy_grants_system_drive_root() { + fn powershell_policy_grants_pshome_not_the_drive_root() { use super::powershell_policy; use std::fs; use std::path::PathBuf; @@ -1181,16 +1198,12 @@ mod tests { // Clean up before asserting so a failing assertion still leaves nothing. let _ = fs::remove_dir_all(&ps_home); - // The system-drive root (e.g. `C:\`) is granted read-only — pwsh - // enumerates the drive root on startup (mirrors `getPowerShellPolicy`). - // A bare drive root normalizes to a 2-char `X:` after trimming separators. - assert!( - result.readonly_paths.iter().any(|p| { - let trimmed = p.trim_end_matches(['\\', '/']); - trimmed.len() == 2 && trimmed.ends_with(':') - }), - "expected system-drive root in readonly paths: {:?}", - result.readonly_paths + // Only `$PSHOME` is granted — never a bare drive root, which would + // expose every file on the volume. + assert_eq!( + result.readonly_paths, + vec![ps_home_str], + "expected only $PSHOME in readonly paths" ); // PSReadLine command history stays read-write. assert!( @@ -1203,6 +1216,84 @@ mod tests { ); } + #[test] + fn filesystem_roots_are_system_critical() { + use super::is_system_critical_path; + + if cfg!(target_os = "windows") { + assert!(is_system_critical_path("C:\\")); + assert!(is_system_critical_path("C:/")); + assert!(is_system_critical_path("C:\\tools\\..")); + assert!(!is_system_critical_path("C:\\tools")); + } else { + assert!(is_system_critical_path("/")); + assert!(is_system_critical_path("/opt/..")); + assert!(!is_system_critical_path("/opt/tools")); + } + } + + #[test] + fn tool_paths_never_grant_a_filesystem_root() { + use super::available_tools_policy; + + // A root on `PATH` must not survive discovery: granting it would hand + // the sandbox read access to the whole volume. + let root = if cfg!(target_os = "windows") { + "C:\\" + } else { + "/" + }; + let env = vec![("PATH".to_string(), root.to_string())]; + let result = available_tools_policy(Some(&env)); + + assert!( + result.readonly_paths.is_empty(), + "a filesystem root must never be granted: {:?}", + result.readonly_paths + ); + } + + /// The PSReadLine write grant is derived from `USERPROFILE`, which can + /// legitimately sit under `%WINDIR%` — the SYSTEM account's profile is + /// `C:\Windows\System32\config\systemprofile`. A read-write grant there + /// would breach the system-critical invariant, so it must be filtered. + #[cfg(target_os = "windows")] + #[test] + fn tool_paths_never_grant_write_access_under_windir() { + use super::available_tools_policy; + use std::fs; + + let unique = format!( + "mxc_pwsh_rw_test_{}_{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + ); + let ps_home = std::env::temp_dir().join(unique); + fs::create_dir_all(&ps_home).expect("create temp $PSHOME"); + fs::write(ps_home.join("pwsh.exe"), b"").expect("create fake pwsh.exe"); + + let win_dir = std::env::var("WINDIR").unwrap_or_else(|_| "C:\\Windows".to_string()); + let env = vec![ + ("PATH".to_string(), ps_home.to_string_lossy().into_owned()), + ( + "USERPROFILE".to_string(), + format!("{win_dir}\\System32\\config\\systemprofile"), + ), + ]; + let result = available_tools_policy(Some(&env)); + + let _ = fs::remove_dir_all(&ps_home); + + assert!( + result.readwrite_paths.is_empty(), + "no write grant may land under %WINDIR%: {:?}", + result.readwrite_paths + ); + } + use super::{ build_request, CaptureDenialsMode, CaptureDenialsSection, NetworkSection, SandboxPolicy, }; diff --git a/tests/configs/pwsh_setlocation.json b/tests/configs/pwsh_setlocation.json index a4b86ffdb..4aafe7987 100644 --- a/tests/configs/pwsh_setlocation.json +++ b/tests/configs/pwsh_setlocation.json @@ -11,9 +11,6 @@ "C:\\Program Files\\PowerShell\\7", "C:\\temp", "C:\\Users" - ], - "readonlyPaths": [ - "C:\\" ] }, "ui": { diff --git a/tests/examples/08_pwsh.json b/tests/examples/08_pwsh.json index 5e6a1a239..c9d0a709b 100644 --- a/tests/examples/08_pwsh.json +++ b/tests/examples/08_pwsh.json @@ -6,14 +6,11 @@ "commandLine": "pwsh.exe -nop -nol -c \"Set-PSReadLineOption -HistorySaveStyle SaveNothing; Set-Location c:\\temp\"; Get-Location" }, "filesystem": { - "readwritePaths": [ - "C:\\Program Files\\PowerShell\\7", - "C:\\temp", - "C:\\Users", - "C:\\Users\\stscha\\AppData\\Roaming\\Microsoft\\Windows\\PowerShell\\PSReadLine" - ], "readonlyPaths": [ - "C:\\" + "C:\\Program Files\\PowerShell\\7" + ], + "readwritePaths": [ + "C:\\temp" ] } } \ No newline at end of file