fix(fs): handle symlinks in overlay rename and copy (#1014)

## Summary

- `OverlayFs::rename` and `copy` used `read_file()` which intentionally
doesn't follow symlinks (TM-ESC-002), causing silent failures when
operating on symlinks. Now detects symlinks via `stat()` and preserves
them using `read_link()` + `symlink()`.
- Same fix applied to `MountableFs` cross-mount rename/copy paths.
- Added TM-ESC-016 to threat model spec and public docs.

## Test plan

- [x] `overlay_rename_preserves_symlink` — rename symlink in upper layer
- [x] `overlay_rename_symlink_from_lower_layer` — rename symlink from
lower to upper (CoW)
- [x] `overlay_copy_preserves_symlink` — copy preserves symlink type
- [x] `symlink_rename_cannot_escape_mount_via_read` — read_file on
renamed symlink still blocked
- [x] `bash_mv_symlink_in_overlay` — end-to-end mv + readlink in bash
session
- [x] Full test suite passes, clippy clean

Author: chaliy

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:**
 

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
+            }
         }
     }
 

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
     }
 

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<dyn FileSystem>;
+    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<dyn FileSystem>));
+
+    // 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<dyn FileSystem>;
+    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<dyn FileSystem>));
+
+    // 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<dyn FileSystem>);
+    mountable
+        .mount("/mnt/a", mount_a as Arc<dyn FileSystem>)
+        .unwrap();
+    mountable
+        .mount("/mnt/b", mount_b as Arc<dyn FileSystem>)
+        .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<dyn FileSystem>);
+    mountable
+        .mount("/mnt/a", mount_a as Arc<dyn FileSystem>)
+        .unwrap();
+    mountable
+        .mount("/mnt/b", mount_b as Arc<dyn FileSystem>)
+        .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<dyn FileSystem>;
+    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");
+}

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 |