From 64847600977f338e3609a7626a1f26ac40be3832 Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Wed, 4 Nov 2026 12:36:18 -0800 Subject: [PATCH 1/2] fix(sandbox): validate filesystem policy anchors Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com> --- crates/openshell-sandbox/README.md | 7 + .../src/sandbox/linux/landlock.rs | 381 +++++++++++++++--- .../openshell-supervisor-process/src/ssh.rs | 116 +++++- .../src/ssh/exec_input_tests.rs | 14 +- docs/how-it-works/policies/schema.mdx | 9 +- e2e/rust/tests/landlock.rs | 76 ++++ skills/generate-sandbox-policy/SKILL.md | 4 + 7 files changed, 539 insertions(+), 68 deletions(-) diff --git a/crates/openshell-sandbox/README.md b/crates/openshell-sandbox/README.md index 72d01b73c7..e365721b8e 100644 --- a/crates/openshell-sandbox/README.md +++ b/crates/openshell-sandbox/README.md @@ -10,6 +10,13 @@ the lower-level mechanisms used by that orchestration and the boundary server. Unsafe kernel ABI, process-launch, and descriptor-ownership operations stay in this runtime crate behind safe APIs where their contracts can be enforced. +Landlock policy anchors are resolved relative to open directory descriptors. +Symlinks are followed only through ancestors that the workload neither owns nor +can write, preserving system aliases while rejecting mutable workspace links. +The rule uses the resolved descriptor directly. Unsafe resolution and an empty +effective allowlist fail the launch independently of Landlock compatibility; +they cannot trigger the best-effort fallback to baseline-only restrictions. + The network broker retains source socket descriptors only while preparing a workload endpoint. A successful connection or `listen` releases that reference; the kernel then tracks the lifetime through workload-owned descriptors. Native diff --git a/crates/openshell-sandbox/src/sandbox/linux/landlock.rs b/crates/openshell-sandbox/src/sandbox/linux/landlock.rs index 25b505f389..b695ccfc82 100644 --- a/crates/openshell-sandbox/src/sandbox/linux/landlock.rs +++ b/crates/openshell-sandbox/src/sandbox/linux/landlock.rs @@ -3,13 +3,19 @@ //! Landlock filesystem sandboxing. +#[cfg(test)] +use landlock::PathFd; use landlock::{ - ABI, Access, AccessFs, BitFlags, CompatLevel, Compatible, PathBeneath, PathFd, PathFdError, - Ruleset, RulesetAttr, RulesetCreatedAttr, + ABI, Access, AccessFs, BitFlags, CompatLevel, Compatible, PathBeneath, Ruleset, RulesetAttr, + RulesetCreatedAttr, }; use miette::{IntoDiagnostic, Result}; use openshell_core::policy::{LandlockCompatibility, SandboxPolicy}; +use std::collections::VecDeque; +use std::ffi::OsString; use std::os::fd::{AsFd, OwnedFd}; +use std::os::unix::ffi::OsStringExt; +use std::path::Component; use std::path::{Path, PathBuf}; use tracing::debug; @@ -318,6 +324,19 @@ fn prepare_with_path_open_mode( .build() ); + // Path admission is independent of kernel compatibility. In particular, an + // unsafe policy anchor must never reach the best-effort ruleset fallback. + let read_only = open_policy_paths(&read_only, compatibility, path_open_mode)?; + let read_write = open_policy_paths(&read_write, compatibility, path_open_mode)?; + let rules_applied = read_only.len() + read_write.len(); + if rules_applied == 0 { + return Err(miette::miette!( + "Landlock ruleset has zero valid paths — all {} path(s) failed to open. \ + Refusing to continue without filesystem restrictions.", + total_paths, + )); + } + let result: Result = (|| { let access_all = AccessFs::from_all(abi); let access_read = AccessFs::from_read(abi); @@ -329,36 +348,20 @@ fn prepare_with_path_open_mode( .into_diagnostic()?; let mut ruleset = ruleset.create().into_diagnostic()?; - let mut rules_applied: usize = 0; - - for path in &read_only { - if let Some(path_fd) = try_open_path(path, compatibility, path_open_mode)? { - let allowed_access = access_for_path_fd(&path_fd, access_read, abi)?; - debug!(path = %path.display(), "Landlock allow read-only"); - ruleset = ruleset - .add_rule(PathBeneath::new(path_fd, allowed_access)) - .into_diagnostic()?; - rules_applied += 1; - } - } - - for path in &read_write { - if let Some(path_fd) = try_open_path(path, compatibility, path_open_mode)? { - let allowed_access = access_for_path_fd(&path_fd, access_all, abi)?; - debug!(path = %path.display(), "Landlock allow read-write"); - ruleset = ruleset - .add_rule(PathBeneath::new(path_fd, allowed_access)) - .into_diagnostic()?; - rules_applied += 1; - } + for (path, path_fd) in read_only { + let allowed_access = access_for_path_fd(&path_fd, access_read, abi)?; + debug!(path = %path.display(), "Landlock allow read-only"); + ruleset = ruleset + .add_rule(PathBeneath::new(path_fd, allowed_access)) + .into_diagnostic()?; } - if rules_applied == 0 { - return Err(miette::miette!( - "Landlock ruleset has zero valid paths — all {} path(s) failed to open. \ - Refusing to apply an empty ruleset that would block all filesystem access.", - total_paths, - )); + for (path, path_fd) in read_write { + let allowed_access = access_for_path_fd(&path_fd, access_all, abi)?; + debug!(path = %path.display(), "Landlock allow read-write"); + ruleset = ruleset + .add_rule(PathBeneath::new(path_fd, allowed_access)) + .into_diagnostic()?; } let skipped = total_paths - rules_applied; @@ -469,11 +472,119 @@ fn access_for_path_fd( }) } +/// A path-opening failure that must not be confused with compatibility errors. +#[derive(Debug)] +enum PolicyPathError { + Io(std::io::Error), + UnsafeSymlink, +} + +impl From for PolicyPathError { + fn from(error: rustix::io::Errno) -> Self { + Self::Io(error.into()) + } +} + +/// Whether a directory's entries can be changed by this workload identity. +/// Ownership counts even without current write permission: owners can chmod. +/// An unrecognized access-check failure is conservatively treated as mutable. +fn workload_can_modify_directory(directory: &OwnedFd) -> Result { + use rustix::fs::{Access, AtFlags, accessat, fstat}; + let stat = fstat(directory)?; + if stat.st_uid == rustix::process::geteuid().as_raw() { + return Ok(true); + } + // A read-only image mount is also immutable to the capability-free + // workload. Unknown failures remain untrusted rather than granting access. + Ok(!matches!( + accessat(directory, ".", Access::WRITE_OK, AtFlags::EACCESS), + Err(rustix::io::Errno::ACCESS | rustix::io::Errno::ROFS) + )) +} + +fn path_components(path: &Path) -> VecDeque { + path.components() + .filter_map(|component| match component { + Component::RootDir | Component::CurDir => None, + Component::ParentDir => Some(OsString::from("..")), + Component::Normal(name) => Some(name.to_owned()), + Component::Prefix(_) => unreachable!("Linux paths have no prefix"), + }) + .collect() +} + +/// Resolve policy anchors through pinned descriptors, following only symlinks +/// beneath directories the workload cannot modify. This preserves immutable +/// image aliases while refusing mutable aliases at every path component. +fn open_policy_path(path: &Path) -> Result { + use rustix::fs::{FileType, Mode, OFlags, fstat, open, openat, readlinkat}; + if !path.is_absolute() { + return Err(PolicyPathError::Io(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "Landlock policy paths must be absolute", + ))); + } + let flags = OFlags::PATH | OFlags::NOFOLLOW | OFlags::CLOEXEC; + let mut directory = open("/", flags | OFlags::DIRECTORY, Mode::empty())?; + let mut mutable_ancestor = workload_can_modify_directory(&directory)?; + let mut pending = path_components(path); + let mut symlinks = 0; + while let Some(component) = pending.pop_front() { + let fd = openat(&directory, &component, flags, Mode::empty())?; + match FileType::from_raw_mode(fstat(&fd)?.st_mode) { + FileType::Symlink => { + // A non-writable immediate parent is insufficient: an ancestor + // may allow renaming that parent or replacing its entire tree. + if mutable_ancestor { + return Err(PolicyPathError::UnsafeSymlink); + } + symlinks += 1; + if symlinks > 40 { + return Err(rustix::io::Errno::LOOP.into()); + } + // Empty-path readlinkat reads the opened link, not a possibly + // replaced directory entry. Magic links are never opened. + let target = PathBuf::from(OsString::from_vec( + readlinkat(&fd, "", Vec::new())?.into_bytes(), + )); + if target.is_absolute() { + directory = open("/", flags | OFlags::DIRECTORY, Mode::empty())?; + mutable_ancestor |= workload_can_modify_directory(&directory)?; + } + let mut target_components = path_components(&target); + target_components.append(&mut pending); + pending = target_components; + } + FileType::Directory => { + mutable_ancestor |= workload_can_modify_directory(&fd)?; + directory = fd; + } + _ if pending.is_empty() => return Ok(fd), + _ => return Err(rustix::io::Errno::NOTDIR.into()), + } + } + Ok(directory) +} + +fn open_policy_paths( + paths: &[PathBuf], + compatibility: &LandlockCompatibility, + path_open_mode: PathOpenMode, +) -> Result> { + let mut opened = Vec::new(); + for path in paths { + if let Some(fd) = try_open_path(path, compatibility, path_open_mode)? { + opened.push((path.clone(), fd)); + } + } + Ok(opened) +} + /// Attempt to open a path for Landlock rule creation. /// -/// In `BestEffort` mode, inaccessible paths (missing, permission denied, symlink -/// loops, etc.) are skipped with a warning and `Ok(None)` is returned so the -/// caller can continue building the ruleset from the remaining valid paths. +/// Mutable symlink traversal is always fatal. In `BestEffort` mode, ordinary +/// inaccessible paths are skipped with `Ok(None)` so the caller can continue +/// building the ruleset from the remaining valid paths. /// /// In `HardRequirement` mode, any failure is fatal — the caller propagates the /// error, which ultimately aborts sandbox startup. @@ -481,16 +592,16 @@ fn try_open_path( path: &Path, compatibility: &LandlockCompatibility, path_open_mode: PathOpenMode, -) -> Result> { - match PathFd::new(path) { +) -> Result> { + match open_policy_path(path) { Ok(fd) => Ok(Some(fd)), - Err(err) => { - let reason = classify_path_fd_error(&err); - let is_not_found = matches!( - &err, - PathFdError::OpenCall { source, .. } - if source.kind() == std::io::ErrorKind::NotFound - ); + Err(PolicyPathError::UnsafeSymlink) => Err(miette::miette!( + "Landlock policy path traverses a symlink beneath a workload-modifiable directory: {}", + path.display() + )), + Err(PolicyPathError::Io(err)) => { + let reason = classify_io_error(&err); + let is_not_found = err.kind() == std::io::ErrorKind::NotFound; if matches!(path_open_mode, PathOpenMode::CurrentUser) { if is_not_found { debug!( @@ -556,19 +667,6 @@ fn try_open_path( } } -/// Classify a [`PathFdError`] into a human-readable reason. -/// -/// `PathFd::new()` wraps `open(path, O_PATH | O_CLOEXEC)` which can fail for -/// several reasons beyond simple non-existence. The `PathFdError::OpenCall` -/// variant wraps the underlying `std::io::Error`. -fn classify_path_fd_error(err: &PathFdError) -> &'static str { - match err { - PathFdError::OpenCall { source, .. } => classify_io_error(source), - // PathFdError is #[non_exhaustive], handle future variants gracefully. - _ => "unexpected error", - } -} - /// Classify a `std::io::Error` into a human-readable reason string. fn classify_io_error(err: &std::io::Error) -> &'static str { match err.kind() { @@ -884,6 +982,171 @@ mod tests { assert!(result.unwrap().is_none()); } + #[test] + fn policy_path_rejects_mutable_symlink_in_every_compatibility_mode() { + let root = tempfile::tempdir().unwrap(); + let link = root.path().join("workspace"); + std::os::unix::fs::symlink("/", &link).unwrap(); + for compatibility in [ + LandlockCompatibility::BestEffort, + LandlockCompatibility::HardRequirement, + ] { + for mode in [PathOpenMode::CurrentUser, PathOpenMode::Privileged] { + let error = try_open_path(&link, &compatibility, mode).unwrap_err(); + assert!(error.to_string().contains("workload-modifiable")); + } + } + } + + #[test] + fn policy_path_rejects_mutable_ancestor_symlink() { + let root = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + std::fs::create_dir(outside.path().join("nested")).unwrap(); + let link = root.path().join("workspace"); + std::os::unix::fs::symlink(outside.path(), &link).unwrap(); + assert!(matches!( + open_policy_path(&link.join("nested")), + Err(PolicyPathError::UnsafeSymlink) + )); + } + + #[test] + fn policy_path_owned_readonly_directory_is_mutable() { + use std::os::unix::fs::PermissionsExt; + let root = tempfile::tempdir().unwrap(); + std::fs::set_permissions(root.path(), std::fs::Permissions::from_mode(0o555)).unwrap(); + let fd = rustix::fs::open( + root.path(), + rustix::fs::OFlags::PATH | rustix::fs::OFlags::DIRECTORY, + rustix::fs::Mode::empty(), + ) + .unwrap(); + assert!(workload_can_modify_directory(&fd).unwrap()); + // Restore permissions for TempDir cleanup. + std::fs::set_permissions(root.path(), std::fs::Permissions::from_mode(0o700)).unwrap(); + } + + #[test] + fn policy_path_descriptor_stays_on_admitted_inode_after_replacement() { + let root = tempfile::tempdir().unwrap(); + let workspace = root.path().join("workspace"); + std::fs::create_dir(&workspace).unwrap(); + let fd = open_policy_path(&workspace).unwrap(); + let admitted = rustix::fs::fstat(&fd).unwrap(); + std::fs::remove_dir(&workspace).unwrap(); + std::os::unix::fs::symlink("/", &workspace).unwrap(); + let retained = rustix::fs::fstat(&fd).unwrap(); + assert_eq!( + (retained.st_dev, retained.st_ino), + (admitted.st_dev, admitted.st_ino) + ); + assert!(matches!( + open_policy_path(&workspace), + Err(PolicyPathError::UnsafeSymlink) + )); + } + + #[test] + fn policy_path_rejects_relative_paths() { + assert!(matches!( + open_policy_path(Path::new("workspace")), + Err(PolicyPathError::Io(_)) + )); + } + + #[test] + fn policy_path_trusted_image_aliases_and_mutable_ancestry() { + use std::os::unix::fs::PermissionsExt; + use std::os::unix::process::CommandExt; + // Exercise the actual DAC check as an unprivileged workload. Root owns + // every containing system directory and is not an admitted workload UID. + if rustix::process::geteuid().is_root() { + let fixture = tempfile::tempdir().unwrap(); + std::fs::set_permissions(fixture.path(), std::fs::Permissions::from_mode(0o777)) + .unwrap(); + let immutable = fixture.path().join("immutable"); + std::fs::create_dir(&immutable).unwrap(); + std::os::unix::fs::symlink("/", immutable.join("alias")).unwrap(); + std::fs::set_permissions(&immutable, std::fs::Permissions::from_mode(0o555)).unwrap(); + let mut command = std::process::Command::new(std::env::current_exe().unwrap()); + command.args([ + "--exact", + "sandbox::linux::landlock::tests::policy_path_trusted_image_aliases_and_mutable_ancestry", + "--nocapture", + ]).env("OPENSHELL_LANDLOCK_MUTABLE_ANCESTRY", immutable.join("alias")); + // SAFETY: these credential operations execute only in the forked + // test child before exec and do not change the test runner identity. + #[allow(unsafe_code)] + unsafe { + command.pre_exec(|| { + if libc::setgroups(0, std::ptr::null()) != 0 + || libc::setgid(65534) != 0 + || libc::setuid(65534) != 0 + { + return Err(std::io::Error::last_os_error()); + } + Ok(()) + }); + } + let status = command.status().unwrap(); + std::fs::set_permissions(&immutable, std::fs::Permissions::from_mode(0o755)).unwrap(); + assert!(status.success()); + return; + } + for alias in ["/bin", "/lib"] { + if std::fs::symlink_metadata(alias).is_ok() { + open_policy_path(Path::new(alias)).unwrap(); + } + } + if let Some(path) = std::env::var_os("OPENSHELL_LANDLOCK_MUTABLE_ANCESTRY") { + assert!(matches!( + open_policy_path(Path::new(&path)), + Err(PolicyPathError::UnsafeSymlink) + )); + } + } + + #[test] + fn prepare_mutable_policy_anchor_never_uses_best_effort_fallback() { + if !matches!(probe_availability(), LandlockAvailability::Available { .. }) { + return; + } + let root = tempfile::tempdir().unwrap(); + let link = root.path().join("workspace"); + std::os::unix::fs::symlink("/", &link).unwrap(); + for compatibility in [ + LandlockCompatibility::BestEffort, + LandlockCompatibility::HardRequirement, + ] { + for mode in [PathOpenMode::CurrentUser, PathOpenMode::Privileged] { + let mut policy = + hard_requirement_policy(vec![root.path().to_path_buf()], vec![link.clone()]); + policy.landlock.compatibility = compatibility.clone(); + let error = prepare_with_path_open_mode(&policy, None, mode) + .err() + .expect("unsafe anchor must abort preparation"); + assert!(error.to_string().contains("workload-modifiable")); + } + } + } + + #[test] + fn prepare_zero_usable_paths_never_uses_best_effort_fallback() { + if !matches!(probe_availability(), LandlockAvailability::Available { .. }) { + return; + } + let mut policy = hard_requirement_policy( + vec![], + vec![PathBuf::from("/nonexistent/openshell/test/path")], + ); + policy.landlock.compatibility = LandlockCompatibility::BestEffort; + let error = prepare_current_user(&policy, None) + .err() + .expect("empty allowlist must fail closed"); + assert!(error.to_string().contains("zero valid paths")); + } + #[test] fn classify_not_found() { let err = std::io::Error::from_raw_os_error(libc::ENOENT); @@ -920,14 +1183,6 @@ mod tests { assert_eq!(classify_io_error(&err), "unexpected error"); } - #[test] - fn classify_path_fd_error_extracts_io_error() { - // Use PathFd::new on a non-existent path to get a real PathFdError - // (the OpenCall variant is #[non_exhaustive] and can't be constructed directly). - let err = PathFd::new("/nonexistent/openshell/classify/test").unwrap_err(); - assert_eq!(classify_path_fd_error(&err), "path does not exist"); - } - #[test] fn probe_availability_returns_a_result() { // The probe should not panic regardless of whether Landlock is available. diff --git a/crates/openshell-supervisor-process/src/ssh.rs b/crates/openshell-supervisor-process/src/ssh.rs index 63ae30fbcc..50d1a333e1 100644 --- a/crates/openshell-supervisor-process/src/ssh.rs +++ b/crates/openshell-supervisor-process/src/ssh.rs @@ -701,11 +701,15 @@ impl russh::server::Handler for SshHandler { // endings. Forcing a PTY here caused CRLF translation which made // VS Code misdetect the platform as Windows (and then try to run // `powershell`). + let terminal = self + .channels + .get(&channel) + .is_some_and(|state| state.pty_request.is_some()); match self.start_shell(channel, session.handle(), None).await { Ok(()) => session.channel_success(channel)?, Err(error) => { warn!(%error, ?channel, "failed to start boundary shell"); - session.channel_failure(channel)?; + self.finish_failed_start(channel, session, &error, terminal)?; } } Ok(()) @@ -730,6 +734,10 @@ impl russh::server::Handler for SshHandler { session.channel_success(channel)?; return Ok(()); } + let terminal = self + .channels + .get(&channel) + .is_some_and(|state| state.pty_request.is_some()); match self .start_shell(channel, session.handle(), Some(command)) .await @@ -737,7 +745,7 @@ impl russh::server::Handler for SshHandler { Ok(()) => session.channel_success(channel)?, Err(error) => { warn!(%error, ?channel, "failed to start boundary exec"); - session.channel_failure(channel)?; + self.finish_failed_start(channel, session, &error, terminal)?; } } Ok(()) @@ -1237,6 +1245,39 @@ impl SshHandler { Ok(()) } + /// Complete an accepted command request whose boundary process cannot start. + /// Request failure alone leaves clients waiting for command output or exit. + fn finish_failed_start( + &mut self, + channel: ChannelId, + session: &mut Session, + error: &anyhow::Error, + terminal: bool, + ) -> anyhow::Result<()> { + if let Some(mut state) = self.channels.remove(&channel) { + state.input_task.take(); + // A PTY resize can fail after spawning. Release that process too. + if let Some(process) = state.process.take() { + tokio::spawn(async move { + if let Err(error) = process.terminate().await { + warn!(%error, "failed to terminate exec after startup failure"); + } + }); + } + } + let line_ending = if terminal { "\r\n" } else { "\n" }; + session.channel_success(channel)?; + session.extended_data( + channel, + 1, + format!("openshell: cannot start command: {error}{line_ending}").into_bytes(), + )?; + session.exit_status_request(channel, 1)?; + session.eof(channel)?; + session.close(channel)?; + Ok(()) + } + async fn start_shell( &mut self, channel: ChannelId, @@ -1969,6 +2010,77 @@ mod tests { )); } + async fn assert_boundary_start_failure(shell: bool, terminal: bool) { + let client = authenticated_test_client().await; + let mut channel = client.channel_open_session().await.unwrap(); + if terminal { + channel + .request_pty(true, "xterm", 80, 24, 0, 0, &[]) + .await + .unwrap(); + assert!(matches!( + next_main_event(&mut channel).await, + russh::ChannelMsg::Success + )); + } + if shell { + channel.request_shell(true).await.unwrap(); + } else { + channel.exec(true, "true").await.unwrap(); + } + assert!(matches!( + next_main_event(&mut channel).await, + russh::ChannelMsg::Success + )); + match next_main_event(&mut channel).await { + russh::ChannelMsg::ExtendedData { data, ext: 1 } => { + let message = String::from_utf8_lossy(&data); + assert!( + message.starts_with("openshell: cannot start command: "), + "{message}" + ); + assert!( + message.contains("exec is not used by direct-tcpip tests"), + "{message}" + ); + assert!(message.ends_with(if terminal { "\r\n" } else { "\n" })); + } + event => panic!("expected startup diagnostic on stderr, got {event:?}"), + } + assert!(matches!( + next_main_event(&mut channel).await, + russh::ChannelMsg::ExitStatus { exit_status: 1 } + )); + assert!(matches!( + next_main_event(&mut channel).await, + russh::ChannelMsg::Eof + )); + assert!(matches!( + next_main_event(&mut channel).await, + russh::ChannelMsg::Close + )); + assert!( + tokio::time::timeout(Duration::from_secs(5), channel.wait()) + .await + .expect("failed command channel must close") + .is_none() + ); + } + + #[tokio::test] + async fn boundary_exec_start_failure_reports_error_and_closes_channel() { + for terminal in [false, true] { + assert_boundary_start_failure(false, terminal).await; + } + } + + #[tokio::test] + async fn boundary_shell_start_failure_reports_error_and_closes_channel() { + for terminal in [false, true] { + assert_boundary_start_failure(true, terminal).await; + } + } + #[tokio::test] async fn main_attachment_missing_session_reports_terminal_error() { assert_main_rejection(None, false, "openshell: cannot connect to the canonical main process: no main session is available\n").await; diff --git a/crates/openshell-supervisor-process/src/ssh/exec_input_tests.rs b/crates/openshell-supervisor-process/src/ssh/exec_input_tests.rs index 35c16cf75d..221005d4d6 100644 --- a/crates/openshell-supervisor-process/src/ssh/exec_input_tests.rs +++ b/crates/openshell-supervisor-process/src/ssh/exec_input_tests.rs @@ -197,7 +197,19 @@ async fn exec_start_failure_keeps_ssh_session_usable() { let mut rejected = client.channel_open_session().await.unwrap(); rejected.exec(true, "first").await.unwrap(); - assert!(matches!(rejected.wait().await, Some(ChannelMsg::Failure))); + assert!(matches!(rejected.wait().await, Some(ChannelMsg::Success))); + match rejected.wait().await { + Some(ChannelMsg::ExtendedData { data, ext: 1 }) => { + assert!(String::from_utf8_lossy(&data).contains("test boundary admission failure")); + } + event => panic!("expected boundary failure diagnostic, got {event:?}"), + } + assert!(matches!( + rejected.wait().await, + Some(ChannelMsg::ExitStatus { exit_status: 1 }) + )); + assert!(matches!(rejected.wait().await, Some(ChannelMsg::Eof))); + assert!(matches!(rejected.wait().await, Some(ChannelMsg::Close))); let mut channel = start_exec(&client).await; control.stdout.shutdown().await.unwrap(); diff --git a/docs/how-it-works/policies/schema.mdx b/docs/how-it-works/policies/schema.mdx index 693efb209c..8daa5ba9c9 100644 --- a/docs/how-it-works/policies/schema.mdx +++ b/docs/how-it-works/policies/schema.mdx @@ -55,6 +55,11 @@ These defaults and the filesystem path restrictions below also apply when the su Each path must be absolute, must not contain `..`, and must not exceed 4096 bytes. `read_write` cannot contain `/`. A policy can list at most 256 paths. +Policy paths can follow system symlinks only when the workload cannot modify +any containing directory. A symlink beneath a directory the workload owns or +can write causes launch to fail, including in `best_effort` mode. List the +target directory directly for links inside a writable workspace. + ```yaml showLineNumbers={false} filesystem_policy: include_workdir: true @@ -69,8 +74,8 @@ filesystem_policy: | `compatibility` | string | `best_effort` | `best_effort` or `hard_requirement` | Both values skip individual paths that are missing or that the workload cannot -open. They differ when none of the listed paths can be applied, or when Landlock -fails to enforce the policy's rules: +open. If none of the listed paths can be applied, launch fails in either mode. +They differ when Landlock fails to build or enforce the policy's rules: | Value | Behavior | |---|---| diff --git a/e2e/rust/tests/landlock.rs b/e2e/rust/tests/landlock.rs index ae1e16601f..cfbdcd5846 100644 --- a/e2e/rust/tests/landlock.rs +++ b/e2e/rust/tests/landlock.rs @@ -6,6 +6,7 @@ //! End-to-end coverage for Landlock filesystem enforcement. use std::io::Write; +use std::time::Duration; use openshell_e2e::harness::sandbox::SandboxGuard; use tempfile::NamedTempFile; @@ -13,6 +14,10 @@ use tempfile::NamedTempFile; const SUCCESS_MARKER: &str = "landlock-hard-requirement-ok"; fn write_hard_requirement_policy() -> Result { + write_policy("hard_requirement", "/sandbox") +} + +fn write_policy(compatibility: &str, extra_path: &str) -> Result { let mut file = NamedTempFile::new().map_err(|error| format!("create temporary policy: {error}"))?; let policy = r#"version: 1 @@ -45,6 +50,15 @@ network_policies: - path: "/**" "#; + let policy = policy + .replace( + "compatibility: hard_requirement", + &format!("compatibility: {compatibility}"), + ) + .replace( + " - /sandbox\n", + &format!(" - /sandbox\n - {extra_path}\n"), + ); file.write_all(policy.as_bytes()) .map_err(|error| format!("write temporary policy: {error}"))?; file.flush() @@ -72,3 +86,65 @@ async fn hard_requirement_accepts_enriched_device_path() { assert!(sandbox.create_output.contains(SUCCESS_MARKER)); sandbox.cleanup().await; } + +async fn check_policy_path_replacement(compatibility: &str, replace_ancestor: bool) { + let policy = write_policy(compatibility, "/sandbox/policy-parent/leaf") + .expect("write filesystem policy"); + let policy_path = policy.path().to_string_lossy().into_owned(); + let mut sandbox = SandboxGuard::create(&["--policy", &policy_path]) + .await + .expect("create sandbox with filesystem policy"); + + sandbox + .exec(&["sh", "-c", "mkdir -p /sandbox/policy-parent/leaf"]) + .await + .expect("create declared directory under writable workspace"); + sandbox + .exec(&[ + "sh", + "-c", + "echo allowed > /sandbox/policy-parent/leaf/check", + ]) + .await + .expect("normal directory should remain usable on the next exec"); + + let script = if replace_ancestor { + "mv /sandbox/policy-parent /sandbox/original-parent; ln -s /var/tmp /sandbox/policy-parent" + } else { + "mv /sandbox/policy-parent/leaf /sandbox/original-leaf; ln -s /var/tmp /sandbox/policy-parent/leaf" + }; + sandbox + .exec(&["sh", "-c", script]) + .await + .expect("replace workspace directory"); + let result = tokio::time::timeout(Duration::from_secs(20), sandbox.exec(&["true"])).await; + // Delete through the control plane even when further workload launches fail. + sandbox.cleanup().await; + let error = result + .expect("rejected launch must close its SSH channel promptly") + .expect_err("mutable policy symlink must prevent the next launch"); + assert!( + error.to_lowercase().contains("symlink"), + "unexpected launch error: {error}" + ); +} + +#[tokio::test] +async fn best_effort_rejects_replaced_policy_path() { + check_policy_path_replacement("best_effort", false).await; +} + +#[tokio::test] +async fn hard_requirement_rejects_replaced_policy_path() { + check_policy_path_replacement("hard_requirement", false).await; +} + +#[tokio::test] +async fn best_effort_rejects_replaced_policy_ancestor() { + check_policy_path_replacement("best_effort", true).await; +} + +#[tokio::test] +async fn hard_requirement_rejects_replaced_policy_ancestor() { + check_policy_path_replacement("hard_requirement", true).await; +} diff --git a/skills/generate-sandbox-policy/SKILL.md b/skills/generate-sandbox-policy/SKILL.md index 80690c45d1..9dcb5d9d8d 100644 --- a/skills/generate-sandbox-policy/SKILL.md +++ b/skills/generate-sandbox-policy/SKILL.md @@ -519,6 +519,10 @@ network_policies: ``` The `filesystem_policy` and `landlock` sections above are sensible defaults. +Use actual target paths for symlinks inside writable workspaces. Policy paths +that traverse symlinks beneath workload-owned or writable directories fail +launch even in `best_effort` mode; system aliases through protected directories +remain supported. A policy with no usable filesystem paths also fails launch. Process identity is omitted so the selected compute driver can choose it. For Docker and Podman, each omitted identity field falls back to the image's OCI `USER`. Tell the user these are defaults and may need adjustment for their From a597c947f40e52bc6a704b60c1df07e4036667a3 Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Fri, 6 Nov 2026 07:20:56 -0800 Subject: [PATCH 2/2] fix(ssh): keep startup failures from stalling sessions Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com> --- .../openshell-supervisor-process/src/ssh.rs | 109 +++++++++++++++--- .../src/ssh/exec_input_tests.rs | 81 +++++++++++++ 2 files changed, 171 insertions(+), 19 deletions(-) diff --git a/crates/openshell-supervisor-process/src/ssh.rs b/crates/openshell-supervisor-process/src/ssh.rs index 50d1a333e1..795838da17 100644 --- a/crates/openshell-supervisor-process/src/ssh.rs +++ b/crates/openshell-supervisor-process/src/ssh.rs @@ -46,6 +46,7 @@ const MAIN_DETACH_KEY: u8 = 0x11; const MAIN_DETACH_EOF: u8 = 0x04; const SSH_KEEPALIVE_INTERVAL: Duration = Duration::from_secs(15); const SSH_PEER_TIMEOUT: Duration = Duration::from_mins(1); +const STARTUP_DIAGNOSTIC_TIMEOUT: Duration = Duration::from_millis(50); mod peer_stream; @@ -392,6 +393,7 @@ async fn handle_connection( #[allow(clippy::struct_excessive_bools)] #[derive(Default)] struct ChannelState { + output: Option>, upload: Option, command_started: bool, input_sender: Option, @@ -501,7 +503,15 @@ impl russh::server::Handler for SshHandler { reply: ChannelOpenHandle, _session: &mut Session, ) -> Result<(), Self::Error> { - self.channels.insert(channel.id(), ChannelState::default()); + let id = channel.id(); + let (_, output) = channel.split(); + self.channels.insert( + id, + ChannelState { + output: Some(output), + ..Default::default() + }, + ); reply.accept().await; Ok(()) } @@ -687,11 +697,9 @@ impl russh::server::Handler for SshHandler { channel: ChannelId, session: &mut Session, ) -> Result<(), Self::Error> { - if self - .channels - .get(&channel) - .is_some_and(|state| state.upload.is_some() || state.command_started) - { + if self.channels.get(&channel).is_some_and(|state| { + state.upload.is_some() || state.command_started || state.main_attached + }) { session.channel_failure(channel)?; return Ok(()); } @@ -721,11 +729,9 @@ impl russh::server::Handler for SshHandler { data: &[u8], session: &mut Session, ) -> Result<(), Self::Error> { - if self - .channels - .get(&channel) - .is_some_and(|state| state.upload.is_some() || state.command_started) - { + if self.channels.get(&channel).is_some_and(|state| { + state.upload.is_some() || state.command_started || state.main_attached + }) { session.channel_failure(channel)?; return Ok(()); } @@ -1254,7 +1260,9 @@ impl SshHandler { error: &anyhow::Error, terminal: bool, ) -> anyhow::Result<()> { + let mut output = None; if let Some(mut state) = self.channels.remove(&channel) { + output = state.output.take(); state.input_task.take(); // A PTY resize can fail after spawning. Release that process too. if let Some(process) = state.process.take() { @@ -1267,14 +1275,29 @@ impl SshHandler { } let line_ending = if terminal { "\r\n" } else { "\n" }; session.channel_success(channel)?; - session.extended_data( - channel, - 1, - format!("openshell: cannot start command: {error}{line_ending}").into_bytes(), - )?; - session.exit_status_request(channel, 1)?; - session.eof(channel)?; - session.close(channel)?; + if let Some(output) = output { + let diagnostic = + format!("openshell: cannot start command: {error}{line_ending}").into_bytes(); + tokio::spawn(async move { + // Reserve client credit before queueing bytes. Session output + // would buffer stderr without credit and block closure and + // Handle messages for every channel on this connection. + let _ = tokio::time::timeout( + STARTUP_DIAGNOSTIC_TIMEOUT, + output.extended_data_bytes(1, diagnostic), + ) + .await; + // Diagnostics may be partial or absent; terminal messages + // must still be sent when the peer withholds output credit. + let _ = output.exit_status(1).await; + let _ = output.eof().await; + let _ = output.close().await; + }); + } else { + session.exit_status_request(channel, 1)?; + session.eof(channel)?; + session.close(channel)?; + } Ok(()) } @@ -2081,6 +2104,54 @@ mod tests { } } + #[tokio::test] + async fn main_attachment_rejects_extra_commands_without_losing_input_lease() { + for shell in [false, true] { + tokio::time::timeout(Duration::from_secs(5), async { + let (main, mut input) = MainSession::inert_with_input(); + let client = main_test_client(Some(main.clone())).await; + let mut attached = client.channel_open_session().await.unwrap(); + attached + .request_subsystem(true, "openshell-main") + .await + .unwrap(); + assert!(matches!( + next_main_event(&mut attached).await, + russh::ChannelMsg::Success + )); + if shell { + attached.request_shell(true).await.unwrap(); + } else { + attached.exec(true, "extra").await.unwrap(); + } + assert!(matches!( + next_main_event(&mut attached).await, + russh::ChannelMsg::Failure + )); + attached.data(&b"still attached"[..]).await.unwrap(); + assert_eq!(input.recv().await.unwrap(), b"still attached"); + attached.close().await.unwrap(); + // Processing a new channel follows the previous close on the + // wire, so the old owner's lease must already be released. + let mut fresh = client.channel_open_session().await.unwrap(); + fresh + .request_subsystem(true, "openshell-main") + .await + .unwrap(); + assert!(matches!( + next_main_event(&mut fresh).await, + russh::ChannelMsg::Success + )); + fresh.data(&b"new owner"[..]).await.unwrap(); + assert_eq!(input.recv().await.unwrap(), b"new owner"); + assert!(!main.finished()); + fresh.close().await.unwrap(); + }) + .await + .expect("extra command stranded main attachment or input lease"); + } + } + #[tokio::test] async fn main_attachment_missing_session_reports_terminal_error() { assert_main_rejection(None, false, "openshell: cannot connect to the canonical main process: no main session is available\n").await; diff --git a/crates/openshell-supervisor-process/src/ssh/exec_input_tests.rs b/crates/openshell-supervisor-process/src/ssh/exec_input_tests.rs index 221005d4d6..bc3621b93f 100644 --- a/crates/openshell-supervisor-process/src/ssh/exec_input_tests.rs +++ b/crates/openshell-supervisor-process/src/ssh/exec_input_tests.rs @@ -230,6 +230,87 @@ async fn exec_start_failure_keeps_ssh_session_usable() { .expect("a rejected exec closed the SSH session"); } +#[tokio::test] +async fn startup_failure_backpressure_does_not_block_sibling_completion() { + for window_size in [0, 8, 65_536] { + for shell in [false, true] { + tokio::time::timeout(DEADLINE, async { + let (session, mut control) = exec_session(); + let client = test_client( + None, + rpc_test::exec(Arc::new(FailThenExec { + failed: AtomicBool::new(false), + session: Mutex::new(Some(session)), + })), + russh::client::Config { + window_size, + ..Default::default() + }, + ) + .await; + let mut rejected = client.channel_open_session().await.unwrap(); + if shell { + rejected.request_shell(true).await.unwrap(); + } else { + rejected.exec(true, "rejected").await.unwrap(); + } + assert!(matches!(rejected.wait().await, Some(ChannelMsg::Success))); + let mut sibling = start_exec(&client).await; + control.stdout.shutdown().await.unwrap(); + control.stderr.shutdown().await.unwrap(); + control + .process + .status + .send_replace(Some(BoundaryExitStatus::Exited(0))); + // Withhold reads on the failed channel until its sibling has + // finished: a small receive window must not stall the session. + let mut sibling_exited = false; + while let Some(event) = sibling.wait().await { + match event { + ChannelMsg::ExitStatus { exit_status: 0 } => sibling_exited = true, + ChannelMsg::Close => break, + _ => {} + } + } + assert!(sibling_exited, "sibling must report its exit status"); + let mut exited = false; + let mut eof = false; + let mut closed = false; + let mut diagnostic = Vec::new(); + while let Some(event) = rejected.wait().await { + match event { + ChannelMsg::ExtendedData { data, ext: 1 } => { + diagnostic.extend_from_slice(&data); + } + ChannelMsg::ExitStatus { exit_status: 1 } => exited = true, + ChannelMsg::Eof => eof = true, + ChannelMsg::Close => { + closed = true; + break; + } + event => panic!("unexpected failure event: {event:?}"), + } + } + assert!( + exited && eof && closed, + "failure must finish independently of stderr" + ); + if window_size == 0 { + assert!(diagnostic.is_empty()); + } + if window_size == 65_536 { + assert!( + String::from_utf8_lossy(&diagnostic) + .contains("test boundary admission failure") + ); + } + }) + .await + .expect("startup diagnostic blocked channel closure or sibling completion"); + } + } +} + #[tokio::test] async fn full_input_allows_output_signals_and_channel_close() { tokio::time::timeout(DEADLINE, async {