diff --git a/crates/bashkit/docs/threat-model.md b/crates/bashkit/docs/threat-model.md index f23fc267..2837f287 100644 --- a/crates/bashkit/docs/threat-model.md +++ b/crates/bashkit/docs/threat-model.md @@ -152,6 +152,7 @@ Scripts may attempt to break out of the sandbox to access the host system. | VFS limit bypass (TM-ESC-012) | `add_file()` skips limits | Restrict API visibility | **OPEN** | | OverlayFs upper() exposed (TM-ESC-013) | `upper()` returns unlimited FS | Restrict visibility | **OPEN** | | Custom builtins lost (TM-ESC-014) | `std::mem::take` empties builtins | Arc-cloned builtins | **FIXED** | +| Symlink overlay rename (TM-ESC-016) | `ln -s /etc/passwd x; mv x y` | Overlay rename/copy preserve symlinks | **FIXED** | **Process Escape:** diff --git a/crates/bashkit/src/fs/mountable.rs b/crates/bashkit/src/fs/mountable.rs index a1eabd01..8363ddc6 100644 --- a/crates/bashkit/src/fs/mountable.rs +++ b/crates/bashkit/src/fs/mountable.rs @@ -429,10 +429,18 @@ impl FileSystem for MountableFs { if Arc::ptr_eq(&from_fs, &to_fs) { from_fs.rename(&from_resolved, &to_resolved).await } else { - // Cross-mount rename: copy then delete - let content = from_fs.read_file(&from_resolved).await?; - to_fs.write_file(&to_resolved, &content).await?; - from_fs.remove(&from_resolved, false).await + // Cross-mount rename: handle symlinks specially since read_file + // intentionally doesn't follow them (THREAT[TM-ESC-002]). + let meta = from_fs.stat(&from_resolved).await?; + if meta.file_type == FileType::Symlink { + let target = from_fs.read_link(&from_resolved).await?; + to_fs.symlink(&target, &to_resolved).await?; + from_fs.remove(&from_resolved, false).await + } else { + let content = from_fs.read_file(&from_resolved).await?; + to_fs.write_file(&to_resolved, &content).await?; + from_fs.remove(&from_resolved, false).await + } } } @@ -445,9 +453,15 @@ impl FileSystem for MountableFs { if Arc::ptr_eq(&from_fs, &to_fs) { from_fs.copy(&from_resolved, &to_resolved).await } else { - // Cross-mount copy - let content = from_fs.read_file(&from_resolved).await?; - to_fs.write_file(&to_resolved, &content).await + // Cross-mount copy: handle symlinks specially (THREAT[TM-ESC-002]). + let meta = from_fs.stat(&from_resolved).await?; + if meta.file_type == FileType::Symlink { + let target = from_fs.read_link(&from_resolved).await?; + to_fs.symlink(&target, &to_resolved).await + } else { + let content = from_fs.read_file(&from_resolved).await?; + to_fs.write_file(&to_resolved, &content).await + } } } diff --git a/crates/bashkit/src/fs/overlay.rs b/crates/bashkit/src/fs/overlay.rs index 1cc2ce65..218d1ffe 100644 --- a/crates/bashkit/src/fs/overlay.rs +++ b/crates/bashkit/src/fs/overlay.rs @@ -679,13 +679,22 @@ impl FileSystem for OverlayFs { let from = Self::normalize_path(from); let to = Self::normalize_path(to); - // Read from source (checking both layers) - let content = self.read_file(&from).await?; + // THREAT[TM-ESC-002]: Check if source is a symlink first. + // Symlinks must be moved as symlinks, not dereferenced via read_file + // (which would fail since InMemoryFs intentionally doesn't follow them). + let meta = self.stat(&from).await?; + if meta.file_type == FileType::Symlink { + let target = self.read_link(&from).await?; + self.check_write_limits(0)?; + self.remove_whiteout(&to); + self.upper.symlink(&target, &to).await?; + self.remove(&from, false).await?; + return Ok(()); + } - // Write to destination in upper + // Regular file: read content and write to new location + let content = self.read_file(&from).await?; self.write_file(&to, &content).await?; - - // Delete source (will add whiteout if needed) self.remove(&from, false).await?; Ok(()) @@ -702,10 +711,17 @@ impl FileSystem for OverlayFs { let from = Self::normalize_path(from); let to = Self::normalize_path(to); - // Read from source (checking both layers) - let content = self.read_file(&from).await?; + // THREAT[TM-ESC-002]: Copy symlinks as symlinks, not via read_file. + let meta = self.stat(&from).await?; + if meta.file_type == FileType::Symlink { + let target = self.read_link(&from).await?; + self.check_write_limits(0)?; + self.remove_whiteout(&to); + return self.upper.symlink(&target, &to).await; + } - // Write to destination in upper + // Regular file + let content = self.read_file(&from).await?; self.write_file(&to, &content).await } diff --git a/crates/bashkit/tests/symlink_overlay_security_tests.rs b/crates/bashkit/tests/symlink_overlay_security_tests.rs new file mode 100644 index 00000000..798587fe --- /dev/null +++ b/crates/bashkit/tests/symlink_overlay_security_tests.rs @@ -0,0 +1,228 @@ +//! Security tests for symlink handling in overlay mode. +//! +//! THREAT[TM-ESC-002]: Validates that symlinks cannot be used to escape +//! mount boundaries, especially after rename/move operations. + +use bashkit::{Bash, FileSystem, InMemoryFs, MountableFs, OverlayFs}; +use std::path::Path; +use std::sync::Arc; + +/// Renaming a symlink in overlay mode must preserve it as a symlink +/// (not silently fail or convert to a regular file). +#[tokio::test] +async fn overlay_rename_preserves_symlink() { + let lower = Arc::new(InMemoryFs::new()) as Arc; + let overlay = Arc::new(OverlayFs::new(lower)); + + // Create a file and a symlink to it + overlay + .write_file(Path::new("/target.txt"), b"hello") + .await + .unwrap(); + overlay + .symlink(Path::new("/target.txt"), Path::new("/link1")) + .await + .unwrap(); + + // Rename the symlink + overlay + .rename(Path::new("/link1"), Path::new("/link2")) + .await + .unwrap(); + + // The renamed entry should still be a symlink + let target = overlay.read_link(Path::new("/link2")).await.unwrap(); + assert_eq!(target, Path::new("/target.txt")); + + // Original should be gone + assert!(!overlay.exists(Path::new("/link1")).await.unwrap()); +} + +/// Renaming a symlink from the lower layer into the upper layer must +/// preserve it as a symlink in the upper layer. +#[tokio::test] +async fn overlay_rename_symlink_from_lower_layer() { + let lower = Arc::new(InMemoryFs::new()); + lower + .write_file(Path::new("/data.txt"), b"secret") + .await + .unwrap(); + lower + .symlink(Path::new("/data.txt"), Path::new("/link_lower")) + .await + .unwrap(); + + let overlay = Arc::new(OverlayFs::new(lower.clone() as Arc)); + + // Rename symlink from lower to a new name (goes to upper) + overlay + .rename(Path::new("/link_lower"), Path::new("/link_upper")) + .await + .unwrap(); + + // Should be a symlink in the overlay + let target = overlay.read_link(Path::new("/link_upper")).await.unwrap(); + assert_eq!(target, Path::new("/data.txt")); +} + +/// Copying a symlink in overlay mode should copy it as a symlink. +#[tokio::test] +async fn overlay_copy_preserves_symlink() { + let lower = Arc::new(InMemoryFs::new()) as Arc; + let overlay = Arc::new(OverlayFs::new(lower)); + + overlay + .write_file(Path::new("/target.txt"), b"data") + .await + .unwrap(); + overlay + .symlink(Path::new("/target.txt"), Path::new("/link")) + .await + .unwrap(); + + overlay + .copy(Path::new("/link"), Path::new("/link_copy")) + .await + .unwrap(); + + // Copy should be a symlink with same target + let target = overlay.read_link(Path::new("/link_copy")).await.unwrap(); + assert_eq!(target, Path::new("/target.txt")); + + // Original still exists + let orig_target = overlay.read_link(Path::new("/link")).await.unwrap(); + assert_eq!(orig_target, Path::new("/target.txt")); +} + +/// A symlink pointing outside a mount should not allow reading the external +/// file through cat after being moved within the mount. +#[tokio::test] +async fn symlink_rename_cannot_escape_mount_via_read() { + // Lower layer has /etc/passwd (simulating files outside the sandbox scope) + let lower = Arc::new(InMemoryFs::new()); + lower.mkdir(Path::new("/etc"), false).await.unwrap(); + lower + .write_file(Path::new("/etc/passwd"), b"root:x:0:0") + .await + .unwrap(); + lower.mkdir(Path::new("/sandbox"), false).await.unwrap(); + + let overlay = Arc::new(OverlayFs::new(lower as Arc)); + + // Create a symlink inside the overlay pointing to /etc/passwd + overlay + .symlink(Path::new("/etc/passwd"), Path::new("/sandbox/evil")) + .await + .unwrap(); + + // Rename symlink within the sandbox + overlay + .rename(Path::new("/sandbox/evil"), Path::new("/sandbox/moved")) + .await + .unwrap(); + + // The renamed symlink should exist and point to /etc/passwd + let target = overlay + .read_link(Path::new("/sandbox/moved")) + .await + .unwrap(); + assert_eq!(target, Path::new("/etc/passwd")); + + // But reading through read_file must NOT follow the symlink and return content + // (symlinks are intentionally not followed — TM-ESC-002) + let result = overlay.read_file(Path::new("/sandbox/moved")).await; + assert!(result.is_err(), "read_file on symlink must not follow it"); +} + +/// Cross-mount rename of a symlink in MountableFs must preserve the symlink. +#[tokio::test] +async fn mountable_cross_mount_rename_preserves_symlink() { + let root = Arc::new(InMemoryFs::new()); + let mount_a = Arc::new(InMemoryFs::new()); + let mount_b = Arc::new(InMemoryFs::new()); + + // Create a symlink in mount_a + mount_a + .symlink(Path::new("/target.txt"), Path::new("/link")) + .await + .unwrap(); + + let mountable = MountableFs::new(root as Arc); + mountable + .mount("/mnt/a", mount_a as Arc) + .unwrap(); + mountable + .mount("/mnt/b", mount_b as Arc) + .unwrap(); + + // Cross-mount rename: /mnt/a/link -> /mnt/b/link + mountable + .rename(Path::new("/mnt/a/link"), Path::new("/mnt/b/link")) + .await + .unwrap(); + + // Should be a symlink in mount_b + let target = mountable.read_link(Path::new("/mnt/b/link")).await.unwrap(); + assert_eq!(target, Path::new("/target.txt")); + + // Source should be gone + assert!(!mountable.exists(Path::new("/mnt/a/link")).await.unwrap()); +} + +/// Cross-mount copy of a symlink in MountableFs must preserve the symlink. +#[tokio::test] +async fn mountable_cross_mount_copy_preserves_symlink() { + let root = Arc::new(InMemoryFs::new()); + let mount_a = Arc::new(InMemoryFs::new()); + let mount_b = Arc::new(InMemoryFs::new()); + + mount_a + .symlink(Path::new("/target.txt"), Path::new("/link")) + .await + .unwrap(); + + let mountable = MountableFs::new(root as Arc); + mountable + .mount("/mnt/a", mount_a as Arc) + .unwrap(); + mountable + .mount("/mnt/b", mount_b as Arc) + .unwrap(); + + // Cross-mount copy + mountable + .copy(Path::new("/mnt/a/link"), Path::new("/mnt/b/link")) + .await + .unwrap(); + + // Both should be symlinks + let target_a = mountable.read_link(Path::new("/mnt/a/link")).await.unwrap(); + let target_b = mountable.read_link(Path::new("/mnt/b/link")).await.unwrap(); + assert_eq!(target_a, Path::new("/target.txt")); + assert_eq!(target_b, Path::new("/target.txt")); +} + +/// mv of a symlink in a bash session should work and preserve the symlink. +#[tokio::test] +async fn bash_mv_symlink_in_overlay() { + let lower = Arc::new(InMemoryFs::new()) as Arc; + let overlay = Arc::new(OverlayFs::new(lower)); + + overlay + .write_file(Path::new("/tmp/target.txt"), b"content") + .await + .unwrap(); + overlay.mkdir(Path::new("/tmp"), true).await.unwrap_or(()); // may already exist + + let mut bash = Bash::builder().fs(overlay.clone()).build(); + + bash.exec("ln -s /tmp/target.txt /tmp/mylink") + .await + .unwrap(); + let result = bash.exec("mv /tmp/mylink /tmp/renamed_link").await.unwrap(); + assert_eq!(result.exit_code, 0, "mv should succeed: {}", result.stderr); + + // readlink should show the original target + let result = bash.exec("readlink /tmp/renamed_link").await.unwrap(); + assert_eq!(result.stdout.trim(), "/tmp/target.txt"); +} diff --git a/specs/006-threat-model.md b/specs/006-threat-model.md index f22190f8..c2c623df 100644 --- a/specs/006-threat-model.md +++ b/specs/006-threat-model.md @@ -320,6 +320,7 @@ max_parser_operations: 100_000, // Parser fuel (TM-DOS-024) | TM-ESC-002 | Symlink escape | `ln -s /etc/passwd /tmp/x` | Symlinks not followed | **MITIGATED** | | TM-ESC-003 | Real FS access | Direct syscalls | No real FS by default | **MITIGATED** | | TM-ESC-004 | Mount escape | Mount real paths | MountableFs controlled | **MITIGATED** | +| TM-ESC-016 | Symlink escape via overlay rename | `ln -s /etc/passwd x; mv x y` | Overlay rename/copy preserve symlinks as symlinks | **FIXED** | **Current Risk**: MEDIUM - Two open escape vectors (TM-ESC-012, TM-ESC-013) need remediation @@ -342,6 +343,13 @@ or return a view that enforces the overlay's limits. **TM-ESC-014**: Fixed. `BashTool::create_bash()` now clones `Arc`-wrapped builtins instead of taking ownership via `std::mem::take`. Custom builtins persist across multiple calls. See `tool.rs:659-662`. +**TM-ESC-016**: Fixed. `OverlayFs::rename` and `copy` previously used `read_file()` + `write_file()` +which silently failed on symlinks (since `read_file` intentionally doesn't follow symlinks per +TM-ESC-002). A symlink could not be renamed at all, but the underlying design gap meant any future +relaxation of symlink-following policy could expose the target. Fix: `rename`/`copy` now detect +symlinks via `stat()` and use `read_link()` + `symlink()` to preserve them. Same fix applied to +`MountableFs` cross-mount operations. See `fs/overlay.rs` and `fs/mountable.rs`. + #### 2.2 Process Escape | ID | Threat | Attack Vector | Mitigation | Status |