From 8366bb08b124109939a121f5b26138d64d69178f Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Wed, 11 Nov 2026 15:48:49 +0530 Subject: [PATCH] Close the bwrap symlink follow-ups from the #99 review Four of the six follow-ups in #104: - reject_symlinked_grants resolved a grant only below a prefix that names a writable root, so a host alias pointing into a directory inside the root (/aliases/v -> /vendor, with vendor replaced by a planted link) was never checked. It now resolves each grant one component at a time, following every link, and refuses one that passes through a link living inside a writable root or grant. A link cycle is refused after 40 hops instead of being followed. - bwrap's base_dir compared an uncanonicalized $XDG_RUNTIME_DIR or TMPDIR against canonical grants, so a runtime directory reached through a symlink into a grant was accepted. Candidates are canonicalized before the check, and the canonical path is returned. - The module's planted-link test asserts INVALID_FILE_PATH instead of just is_err(). - The module and jail READMEs say that a grant through a link inside a writable root is refused, host-created links (pnpm, monorepos) included. The bus doc and the 1.20 spec no longer say a missing deny_read path is ignored: since #107 bwrap refuses one inside a mount. Not in this change: fd-bound grants via openat2/--ro-bind-fd (item 1) and the known gaps in item 6 (seccomp, resource limits, late deny_read). Refs #104 --- crates/tinybox-bus/src/jail.rs | 7 +- crates/tinybox-jail/README.md | 4 +- crates/tinybox-jail/src/bwrap/launcher.rs | 5 +- .../tinybox-jail/src/bwrap/launcher_tests.rs | 30 ++++- crates/tinybox-jail/src/jail.rs | 121 +++++++++++------- crates/tinybox-jail/src/jail_tests.rs | 49 +++++++ .../src/tinybus_module/README.md | 7 +- .../src/tinybus_module/native_jail_tests.rs | 14 +- docs/specs/contract-1-20.md | 5 +- 9 files changed, 186 insertions(+), 56 deletions(-) diff --git a/crates/tinybox-bus/src/jail.rs b/crates/tinybox-bus/src/jail.rs index c4d686a8..58d7ead0 100644 --- a/crates/tinybox-bus/src/jail.rs +++ b/crates/tinybox-bus/src/jail.rs @@ -29,8 +29,11 @@ pub struct NativeJailConfig { pub require_process_isolation: bool, /// Absolute paths the child may neither read nor write, even where the /// root or a grant would otherwise reach them (credential stores, for a - /// provider that reads allow-by-default). Paths that do not exist are - /// ignored. Added in contract 1.20; empty by default. + /// provider that reads allow-by-default). A path that does not exist yet is + /// kept and resolved again just before spawn, except that the bubblewrap + /// backend refuses a missing denial inside one of the jail's mounts: it + /// would have to create the path to mask it. Added in contract 1.20; empty + /// by default. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub deny_read: Vec, /// Optional hard resource limits; omitted for compatibility with older requests. diff --git a/crates/tinybox-jail/README.md b/crates/tinybox-jail/README.md index 879d4f5e..85d1a966 100644 --- a/crates/tinybox-jail/README.md +++ b/crates/tinybox-jail/README.md @@ -26,7 +26,9 @@ spawns, never the core process itself. Seatbelt on macOS, otherwise the `unsupported` backend. - Spawn a `std::process::Command` inside the jail, canonicalizing `root` (and the read-only and read/write paths) first so backends never see `..` or symlink - trickery. + trickery. Before canonicalizing, a grant whose resolution passes through a + symlink inside the root or a writable grant is refused (`InvalidInput`), + host-created links included, since the jailed program could replace one. - Provide a persistent registry to manage many jailed workspaces side by side, each with a stable id, label, directory, and metadata, indexed in a JSON file. diff --git a/crates/tinybox-jail/src/bwrap/launcher.rs b/crates/tinybox-jail/src/bwrap/launcher.rs index ca5a4680..2d56330f 100644 --- a/crates/tinybox-jail/src/bwrap/launcher.rs +++ b/crates/tinybox-jail/src/bwrap/launcher.rs @@ -75,7 +75,9 @@ pub(super) fn pick_shell(exists: impl Fn(&str) -> bool) -> &'static str { /// Where the args file may live: `$XDG_RUNTIME_DIR` (a private per-user /// directory) when set, else the temp directory. A candidate that lies under /// any of the jail's mounts is refused, since the jailed program could then -/// read or replace the file. +/// read or replace the file. Candidates are canonicalized before that check, +/// as the grants are, so a runtime directory reached through a symlink into a +/// grant is caught; the canonical path is what is returned. /// /// # Errors /// `PermissionDenied` when no candidate is outside the jail's grants. @@ -87,6 +89,7 @@ pub(super) fn base_dir( xdg_runtime_dir .into_iter() .chain(std::iter::once(temp_dir)) + .filter_map(|dir| dir.canonicalize().ok()) .find(|dir| dir.is_dir() && !grants.iter().any(|grant| dir.starts_with(grant))) .ok_or_else(|| { io::Error::new( diff --git a/crates/tinybox-jail/src/bwrap/launcher_tests.rs b/crates/tinybox-jail/src/bwrap/launcher_tests.rs index 06374151..2eddea54 100644 --- a/crates/tinybox-jail/src/bwrap/launcher_tests.rs +++ b/crates/tinybox-jail/src/bwrap/launcher_tests.rs @@ -158,14 +158,19 @@ fn the_args_directory_prefers_xdg_runtime_dir_but_never_one_inside_a_grant() -> let xdg = tempfile::tempdir()?; let temp = tempfile::tempdir()?; let chosen = base_dir(&[], Some(xdg.path().into()), temp.path().into())?; - assert_eq!(chosen, xdg.path()); + assert_eq!(chosen, xdg.path().canonicalize()?); // XDG dir inside a grant: fall back to the temp dir. - let chosen = base_dir(&[xdg.path()], Some(xdg.path().into()), temp.path().into())?; - assert_eq!(chosen, temp.path()); + let granted = xdg.path().canonicalize()?; + let chosen = base_dir( + &[granted.as_path()], + Some(xdg.path().into()), + temp.path().into(), + )?; + assert_eq!(chosen, temp.path().canonicalize()?); // Unset or missing XDG dir: temp dir. let missing = xdg.path().join("missing"); let chosen = base_dir(&[], Some(missing), temp.path().into())?; - assert_eq!(chosen, temp.path()); + assert_eq!(chosen, temp.path().canonicalize()?); // Everything inside a grant: refuse. let kind = base_dir(&[temp.path()], None, temp.path().join("sub")) .err() @@ -173,3 +178,20 @@ fn the_args_directory_prefers_xdg_runtime_dir_but_never_one_inside_a_grant() -> assert_eq!(kind, Some(io::ErrorKind::PermissionDenied)); Ok(()) } + +#[test] +fn an_args_directory_reached_through_a_symlink_into_a_grant_is_refused() -> io::Result<()> { + let grant = tempfile::tempdir()?; + let runtime = grant.path().join("runtime"); + std::fs::create_dir(&runtime)?; + let holder = tempfile::tempdir()?; + let xdg = holder.path().join("xdg"); + std::os::unix::fs::symlink(&runtime, &xdg)?; + let temp = tempfile::tempdir()?; + let granted = grant.path().canonicalize()?; + + let chosen = base_dir(&[granted.as_path()], Some(xdg), temp.path().into())?; + + assert_eq!(chosen, temp.path().canonicalize()?); + Ok(()) +} diff --git a/crates/tinybox-jail/src/jail.rs b/crates/tinybox-jail/src/jail.rs index 3b9aa4dc..dcb642aa 100644 --- a/crates/tinybox-jail/src/jail.rs +++ b/crates/tinybox-jail/src/jail.rs @@ -249,58 +249,93 @@ impl Jail { /// spells it (the path itself, its canonical form, or any alias that resolves /// to it). /// +/// The grant is resolved one component at a time, following every link, and +/// refused when any link on the way lives inside a writable ancestor. That also +/// catches a host alias that points *into* a writable root +/// (`/aliases/v -> /vendor`, with `vendor` replaced by a planted link), +/// which no prefix of the grant's own spelling would reveal. +/// /// # Errors /// `InvalidInput` naming the symlink. pub fn reject_symlinked_grants(writable: &[&Path], grants: &[&Path]) -> std::io::Result<()> { - for ancestor in writable { - let canonical = ancestor.canonicalize().ok(); - for grant in grants { - if grant == ancestor { - continue; - } - // The shortest prefix of the grant that names the ancestor. - let mut prefixes: Vec<&Path> = grant.ancestors().collect(); - prefixes.reverse(); - let anchor = prefixes.into_iter().find(|prefix| { - *prefix == *ancestor - || canonical - .as_ref() - .is_some_and(|c| prefix.canonicalize().is_ok_and(|p| p == *c)) - }); - if let Some(anchor) = anchor { - reject_symlink_below(anchor, grant)?; - } - } + let writable: Vec = writable + .iter() + .filter_map(|ancestor| ancestor.canonicalize().ok()) + .collect(); + for grant in grants { + reject_link_inside(grant, &writable)?; } Ok(()) } -/// Errors when `path` lies inside `ancestor` (lexically) and any component -/// below `ancestor` is a symlink. A path equal to `ancestor` passes. -/// -/// # Errors -/// `InvalidInput` naming the symlink. -pub(crate) fn reject_symlink_below(ancestor: &Path, path: &Path) -> std::io::Result<()> { - let Ok(rest) = path.strip_prefix(ancestor) else { +/// The most links one grant may pass through, as the kernel's `ELOOP` limit. +const MAX_LINK_HOPS: usize = 40; + +/// Resolves `grant` component by component and refuses it when a link it +/// passes through sits strictly inside one of the canonical `writable` paths. +/// A missing component ends the walk, as the lexical check did. +fn reject_link_inside(grant: &Path, writable: &[PathBuf]) -> std::io::Result<()> { + use std::path::Component; + + if !grant.is_absolute() { return Ok(()); - }; - let mut current = ancestor.to_path_buf(); - for component in rest.components() { - current.push(component); - match std::fs::symlink_metadata(¤t) { - Ok(meta) if meta.file_type().is_symlink() => { - return Err(std::io::Error::new( - std::io::ErrorKind::InvalidInput, - format!( - "grant {} reaches {} through a symlink inside writable {}", - path.display(), - current.display(), - ancestor.display() - ), - )); + } + let mut resolved = PathBuf::new(); + let mut pending: Vec = Vec::new(); + // Components are consumed from the back, so the queue is built reversed. + for component in grant.components().rev() { + pending.push(component.as_os_str().to_os_string()); + } + let mut hops = 0; + while let Some(next) = pending.pop() { + let step = Path::new(&next); + match step.components().next() { + Some(Component::Prefix(_) | Component::RootDir) => { + resolved.push(step); + continue; + } + Some(Component::CurDir) | None => continue, + Some(Component::ParentDir) => { + resolved.pop(); + continue; } - Ok(_) => {} - Err(_) => break, + Some(Component::Normal(_)) => {} + } + let candidate = resolved.join(step); + let Ok(meta) = std::fs::symlink_metadata(&candidate) else { + break; + }; + if !meta.file_type().is_symlink() { + resolved = candidate; + continue; + } + if let Some(ancestor) = writable + .iter() + .find(|ancestor| candidate.starts_with(ancestor) && candidate != **ancestor) + { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!( + "grant {} reaches {} through a symlink inside writable {}", + grant.display(), + candidate.display(), + ancestor.display() + ), + )); + } + hops += 1; + if hops > MAX_LINK_HOPS { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!("grant {} passes through too many symlinks", grant.display()), + )); + } + let target = std::fs::read_link(&candidate)?; + if target.is_absolute() { + resolved = PathBuf::new(); + } + for component in target.components().rev() { + pending.push(component.as_os_str().to_os_string()); } } Ok(()) diff --git a/crates/tinybox-jail/src/jail_tests.rs b/crates/tinybox-jail/src/jail_tests.rs index f46d08f4..9483993e 100644 --- a/crates/tinybox-jail/src/jail_tests.rs +++ b/crates/tinybox-jail/src/jail_tests.rs @@ -309,6 +309,55 @@ fn the_check_sees_a_planted_link_however_the_root_is_spelled() -> std::io::Resul Ok(()) } +#[cfg(unix)] +#[test] +fn a_host_alias_into_a_writable_root_cannot_reach_a_planted_link() -> std::io::Result<()> { + let root = tempfile::tempdir()?; + let secret = tempfile::tempdir()?; + // The jailed program replaced `vendor` with a link to a secret. + std::os::unix::fs::symlink(secret.path(), root.path().join("vendor"))?; + // The host aliases a directory inside the root, not the root itself, so + // no prefix of the alias names the root. + let holder = tempfile::tempdir()?; + let alias = holder.path().join("v"); + std::os::unix::fs::symlink(root.path().join("vendor"), &alias)?; + + for grant in [alias.clone(), alias.join("file")] { + let err = crate::reject_symlinked_grants(&[root.path()], &[grant.as_path()]) + .err() + .map(|e| e.kind()); + assert_eq!(err, Some(std::io::ErrorKind::InvalidInput), "{grant:?}"); + } + Ok(()) +} + +#[cfg(unix)] +#[test] +fn a_host_alias_into_a_real_directory_of_a_writable_root_is_allowed() -> std::io::Result<()> { + let root = tempfile::tempdir()?; + std::fs::create_dir(root.path().join("vendor"))?; + let holder = tempfile::tempdir()?; + let alias = holder.path().join("v"); + std::os::unix::fs::symlink(root.path().join("vendor"), &alias)?; + + crate::reject_symlinked_grants(&[root.path()], &[alias.as_path()]) +} + +#[cfg(unix)] +#[test] +fn a_grant_through_a_link_cycle_is_refused_rather_than_followed_forever() -> std::io::Result<()> { + let root = tempfile::tempdir()?; + let holder = tempfile::tempdir()?; + std::os::unix::fs::symlink(holder.path().join("b"), holder.path().join("a"))?; + std::os::unix::fs::symlink(holder.path().join("a"), holder.path().join("b"))?; + + let err = crate::reject_symlinked_grants(&[root.path()], &[holder.path().join("a").as_path()]) + .err() + .map(|e| e.kind()); + assert_eq!(err, Some(std::io::ErrorKind::InvalidInput)); + Ok(()) +} + #[test] fn nested_denial_refuses_only_paths_deeper_than_a_direct_child() { let writable = Path::new("/work"); diff --git a/crates/tinybox-module/src/tinybus_module/README.md b/crates/tinybox-module/src/tinybus_module/README.md index aa2def9e..6548d797 100644 --- a/crates/tinybox-module/src/tinybus_module/README.md +++ b/crates/tinybox-module/src/tinybus_module/README.md @@ -272,7 +272,12 @@ requiring the process-isolation security floor sets `require_process_isolation`. The host retains approval policy and chooses the credential paths; it sends them as `deny_read` (1.20). The module retains prospective paths and resolves them against current filesystem aliases both at preflight and immediately before -spawn. It refuses a denial covering the root or a writable grant. Seatbelt can +spawn. It refuses a denial covering the root or a writable grant. A grant is +refused as an invalid path when resolving it passes through a symlink that lives +inside the root or a writable grant, because the jailed program could replace +that link. That includes host-created links: a pnpm or monorepo workspace whose +packages are linked inside the root cannot be granted through those links, so +grant their real paths instead. Seatbelt can carve a denial that is a direct child of a writable grant (or of the root or `/private/tmp`), and refuses a deeper one, which a rename of the directory in between would move out of its path-based rule; allow-list backends refuse any diff --git a/crates/tinybox-module/src/tinybus_module/native_jail_tests.rs b/crates/tinybox-module/src/tinybus_module/native_jail_tests.rs index ebbab801..42f70d51 100644 --- a/crates/tinybox-module/src/tinybus_module/native_jail_tests.rs +++ b/crates/tinybox-module/src/tinybus_module/native_jail_tests.rs @@ -1515,6 +1515,14 @@ fn process_isolation_is_decided_per_jail_so_shared_network_is_refused() Ok(()) } +/// A planted-link refusal is an invalid path, not an unavailable backend. +#[cfg(unix)] +fn assert_refused_as_an_invalid_path(result: &Result) { + // `None` means a grant through a planted link was admitted. + let refused_as = result.as_ref().err().map(tinybus::Error::wire_name); + assert_eq!(refused_as, Some(tinybox_bus::INVALID_FILE_PATH)); +} + #[cfg(unix)] #[test] fn a_symlink_planted_in_the_writable_root_cannot_redirect_a_grant() @@ -1536,13 +1544,13 @@ fn a_symlink_planted_in_the_writable_root_cannot_redirect_a_grant() read_only .read_only .push(planted.to_string_lossy().into_owned()); - assert!(prepare(root.path(), &read_only, &command, &backend).is_err()); + assert_refused_as_an_invalid_path(&prepare(root.path(), &read_only, &command, &backend)); let mut read_write = config(); read_write .read_write .push(planted.to_string_lossy().into_owned()); - assert!(prepare(root.path(), &read_write, &command, &backend).is_err()); + assert_refused_as_an_invalid_path(&prepare(root.path(), &read_write, &command, &backend)); // Also when the root is spelled through an alias of its real path. let alias_holder = tempfile::tempdir()?; @@ -1552,7 +1560,7 @@ fn a_symlink_planted_in_the_writable_root_cannot_redirect_a_grant() aliased .read_only .push(planted.to_string_lossy().into_owned()); - assert!(prepare(&alias, &aliased, &command, &backend).is_err()); + assert_refused_as_an_invalid_path(&prepare(&alias, &aliased, &command, &backend)); // A symlink outside every writable ancestor is still a fine grant. let holder = tempfile::tempdir()?; diff --git a/docs/specs/contract-1-20.md b/docs/specs/contract-1-20.md index fff4b26d..136aa56a 100644 --- a/docs/specs/contract-1-20.md +++ b/docs/specs/contract-1-20.md @@ -18,7 +18,10 @@ at least 20, which `tinybox_bus::is_compatible` already enforces. - Validated like grants (absolute, no `..`, no NUL, bounded length, counted against `MAX_NATIVE_JAIL_GRANTS`). Missing paths are retained so a store - created after preflight remains denied. Existing aliases and missing paths + created after preflight remains denied. (Since #107, the bubblewrap backend + instead refuses a missing denial that falls inside one of the jail's bind + mounts, because masking it would create it on the host.) Existing aliases and + missing paths beneath existing symlinks resolve to canonical targets; denials are resolved and checked again immediately before spawn. - Refused (`INVALID_FILE_PATH`) when a denial is the root, an ancestor of it, or