From f9f3003d272e9b4da5d83e84a7476109a03c4768 Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Fri, 3 Apr 2026 11:38:40 +0000 Subject: [PATCH] fix(fs): prevent sandbox escape via TOCTOU fallback in RealFs::resolve The resolve() fallback path returned raw joined paths without traversal validation when the parent directory didn't exist. Add normalize_host_path() to logically resolve .. components on host paths, and validate containment in the fallback case. Add security tests for the fallback path. Closes #980 --- crates/bashkit/src/fs/realfs.rs | 157 +++++++++++++++++++++++++++++++- 1 file changed, 154 insertions(+), 3 deletions(-) diff --git a/crates/bashkit/src/fs/realfs.rs b/crates/bashkit/src/fs/realfs.rs index 997c2c90..3e74d45f 100644 --- a/crates/bashkit/src/fs/realfs.rs +++ b/crates/bashkit/src/fs/realfs.rs @@ -171,9 +171,28 @@ impl RealFs { } } - // Fallback: just use the joined path (will fail at the OS level - // if parent doesn't exist, which is the correct POSIX behavior) - Ok(joined) + // SECURITY: Never return a raw path without traversal validation. + // The parent doesn't exist and can't be canonicalized, so we cannot + // verify containment with certainty. Returning the raw `joined` path + // here would skip all symlink/traversal checks, creating a TOCTOU + // window where an attacker could race to create a symlink between + // the exists() check above and actual file I/O. (issue #980) + // + // Defense-in-depth: normalize the host path logically and verify it + // stays under root. This catches any `..` that survived vpath + // normalization as well as any future changes to the normalization + // logic. + let normalized = normalize_host_path(&joined); + if !normalized.starts_with(&self.root) { + return Err(IoError::new( + ErrorKind::PermissionDenied, + "path escapes realfs root", + )); + } + // Even with logical normalization, the path hasn't been verified on + // disk (no canonicalize). This is acceptable only because the parent + // truly doesn't exist — there's nothing on disk to symlink through. + Ok(normalized) } /// Check that the mode allows writes. Returns PermissionDenied if readonly. @@ -240,6 +259,32 @@ fn metadata_from_std(m: &std::fs::Metadata) -> Metadata { } } +/// Normalize a host path by logically resolving `.` and `..` components. +/// +/// Unlike `std::fs::canonicalize`, this does not touch the filesystem, so it +/// works for paths whose parents don't exist yet. Used in the `resolve()` +/// fallback to validate containment without a TOCTOU window. +fn normalize_host_path(path: &Path) -> PathBuf { + let mut components = Vec::new(); + for component in path.components() { + match component { + std::path::Component::ParentDir => { + // Only pop Normal components; never pop RootDir or Prefix + if matches!(components.last(), Some(std::path::Component::Normal(_))) { + components.pop(); + } + } + std::path::Component::CurDir => {} + c => components.push(c), + } + } + if components.is_empty() { + PathBuf::from("/") + } else { + components.iter().collect() + } +} + /// Normalize a virtual path: collapse `.` and `..`, ensure absolute. fn normalize_vpath(path: &Path) -> PathBuf { let mut components = Vec::new(); @@ -610,6 +655,112 @@ mod tests { assert!(result.is_err()); } + // --- Security tests for issue #980: TOCTOU fallback sandbox escape --- + + #[test] + fn normalize_host_path_resolves_dotdot() { + let p = normalize_host_path(Path::new("/a/b/../c")); + assert_eq!(p, PathBuf::from("/a/c")); + + let p = normalize_host_path(Path::new("/a/b/../../c")); + assert_eq!(p, PathBuf::from("/c")); + + // Can't go above root + let p = normalize_host_path(Path::new("/a/../../../x")); + assert_eq!(p, PathBuf::from("/x")); + } + + #[test] + fn normalize_host_path_preserves_absolute() { + let p = normalize_host_path(Path::new("/tmp/sandbox/./foo/../bar")); + assert_eq!(p, PathBuf::from("/tmp/sandbox/bar")); + } + + #[test] + fn resolve_fallback_validates_containment() { + // When the parent doesn't exist, resolve must still validate + // that the path stays under root (defense-in-depth). + let dir = setup(); + let fs = RealFs::new(dir.path(), RealFsMode::ReadOnly).unwrap(); + + // Valid non-existent path under root — should succeed + let result = fs.resolve(Path::new("/newdir/newfile.txt")); + assert!( + result.is_ok(), + "valid non-existent path under root should succeed" + ); + let resolved = result.unwrap(); + assert!( + resolved.starts_with(dir.path()), + "resolved path must be under root" + ); + } + + #[test] + fn resolve_fallback_returns_normalized_path() { + // The fallback must return a normalized path, not the raw joined path. + // This ensures no stale `..` or `.` components leak through. + let dir = setup(); + let fs = RealFs::new(dir.path(), RealFsMode::ReadOnly).unwrap(); + + let result = fs.resolve(Path::new("/a/b/../c/file.txt")); + assert!(result.is_ok()); + let resolved = result.unwrap(); + // The resolved path should not contain ".." + assert!( + !resolved.to_string_lossy().contains(".."), + "fallback path must be normalized, got: {}", + resolved.display() + ); + assert!(resolved.starts_with(dir.path())); + } + + #[tokio::test] + async fn security_traversal_blocked_all_paths() { + // Comprehensive traversal test: all traversal attempts must fail, + // regardless of which code path in resolve() handles them. + let dir = setup(); + let fs = RealFs::new(dir.path(), RealFsMode::ReadOnly).unwrap(); + + let traversal_paths = [ + "/../../../etc/passwd", + "/../../etc/shadow", + "/subdir/../../etc/passwd", + "/./../../etc/passwd", + ]; + for vpath in &traversal_paths { + let result = fs.read(Path::new(vpath)).await; + // normalize_vpath collapses these to root-relative, so they + // resolve under root. What matters: no actual /etc/passwd content. + if let Ok(data) = &result { + let data_str = String::from_utf8_lossy(data); + assert!( + !data_str.contains("root:"), + "traversal leaked /etc/passwd via path {vpath}" + ); + } + } + } + + #[tokio::test] + async fn security_nonexistent_nested_stays_under_root() { + // Write to deeply nested non-existent path should create under root, + // not escape via fallback. + let dir = setup(); + let fs = RealFs::new(dir.path(), RealFsMode::ReadWrite).unwrap(); + + // This goes through the fallback (parent doesn't exist). + // The resolved path must be under root. + let result = fs + .write(Path::new("/deep/nested/dir/file.txt"), b"safe") + .await; + // Should succeed (write creates parent dirs) and file must be under root + if result.is_ok() { + let expected = dir.path().join("deep/nested/dir/file.txt"); + assert!(expected.exists(), "file must be created under root"); + } + } + #[test] fn debug_display() { let dir = setup();