fix(agent): prevent dangling-symlink write escape from the sandbox
validate_path_within_cwd used exists() to decide whether a path was new. exists() follows symlinks, so a *dangling* symlink (target missing) reported false and fell into the not-yet-existing branch, which only bound-checked the nearest existing ancestor — never the symlink itself. An approved `ln -s /etc/cron.d/pwn evil` inside the sandbox would therefore pass validation, and a subsequent Write to `evil` would follow the link and create a file outside the sandbox. In AcceptEdits mode this chained cleanly: approve one innocuous-looking symlink (Execute), then writes auto-allow. Resolve every path component with lstat (symlink_metadata, which does not follow links) instead of exists(): - Final component: if it is a symlink (dangling or live), resolve its real target via read_link + canonicalize-nearest-ancestor and reject unless the target stays within the sandbox. - Ancestor walk: an intermediate dangling symlink directory is likewise resolved and bound-checked rather than skipped. Live symlinks were already caught by canonicalize; this closes the dangling case. It does not close the check-vs-open TOCTOU (the open happens inside the SDK tool and would need O_NOFOLLOW/openat2 there); tracked for phase 2 along with OS-level bash isolation. Covered by tests/agent_sandbox.rs (dangling symlink to absolute and relative targets, write tool through a dangling link, write through an intermediate dangling symlink dir, all rejected; dangling link pointing inside and ordinary new files still allowed) plus mirrored in-crate unit tests.
This commit is contained in:
@@ -51,6 +51,15 @@ fn sanitize_session_id(session_id: &str) -> String {
|
||||
/// not-yet-existing paths the nearest existing ancestor is canonicalized and
|
||||
/// checked, so writes to new files inside the sandbox are permitted while writes
|
||||
/// that would escape it are rejected.
|
||||
///
|
||||
/// Symlinks are handled explicitly with `lstat` (not `exists()`, which follows
|
||||
/// links): any component that is a symlink — including a *dangling* one whose
|
||||
/// target does not exist yet — has its target resolved and is rejected if it
|
||||
/// points outside the sandbox. This stops an approved `ln -s /outside x` from
|
||||
/// being written through to escape (a write to `x` would otherwise follow the
|
||||
/// link). Note this closes the escape, not the check-vs-open TOCTOU race; the
|
||||
/// actual open happens inside the tool and would need `O_NOFOLLOW`/`openat2`
|
||||
/// there to be fully atomic.
|
||||
pub fn validate_path_within_cwd(path: &str, cwd: &Path) -> Result<PathBuf, String> {
|
||||
let cwd_canonical = cwd
|
||||
.canonicalize()
|
||||
@@ -63,6 +72,22 @@ pub fn validate_path_within_cwd(path: &str, cwd: &Path) -> Result<PathBuf, Strin
|
||||
cwd_canonical.join(target)
|
||||
};
|
||||
|
||||
// Final component: if it is a symlink (dangling or not), trust only its
|
||||
// target. lstat does not follow the link, so a dangling symlink is detected
|
||||
// here instead of slipping into the not-yet-existing branch below.
|
||||
if let Ok(meta) = std::fs::symlink_metadata(&resolved) {
|
||||
if meta.file_type().is_symlink() {
|
||||
let tgt = symlink_real_target(&resolved)?;
|
||||
return if tgt.starts_with(&cwd_canonical) {
|
||||
Ok(resolved)
|
||||
} else {
|
||||
Err(format!(
|
||||
"path '{path}' is a symlink that escapes the session sandbox"
|
||||
))
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
if resolved.exists() {
|
||||
let canonical = resolved
|
||||
.canonicalize()
|
||||
@@ -76,10 +101,25 @@ pub fn validate_path_within_cwd(path: &str, cwd: &Path) -> Result<PathBuf, Strin
|
||||
};
|
||||
}
|
||||
|
||||
// Not-yet-existing path: walk up to the nearest existing ancestor.
|
||||
// Not-yet-existing path: walk up to the nearest existing ancestor. Use
|
||||
// lstat so an intermediate *dangling* symlink directory is caught (an
|
||||
// existing symlink ancestor with a live target is already caught by
|
||||
// canonicalize, but a dangling one would otherwise be skipped over).
|
||||
let mut ancestor = resolved.parent();
|
||||
while let Some(dir) = ancestor {
|
||||
if dir.exists() {
|
||||
match std::fs::symlink_metadata(dir) {
|
||||
Ok(meta) if meta.file_type().is_symlink() => {
|
||||
let tgt = symlink_real_target(dir)?;
|
||||
return if tgt.starts_with(&cwd_canonical) {
|
||||
Ok(resolved)
|
||||
} else {
|
||||
Err(format!(
|
||||
"path '{path}' traverses a symlink that escapes the session sandbox"
|
||||
))
|
||||
};
|
||||
}
|
||||
Ok(_) => {
|
||||
// Existing real directory/file: canonicalize and bound-check.
|
||||
let anc_canonical = dir
|
||||
.canonicalize()
|
||||
.map_err(|e| format!("cannot resolve ancestor of '{path}': {e}"))?;
|
||||
@@ -89,11 +129,54 @@ pub fn validate_path_within_cwd(path: &str, cwd: &Path) -> Result<PathBuf, Strin
|
||||
Err(format!("path '{path}' is outside the session sandbox"))
|
||||
};
|
||||
}
|
||||
// Component does not exist in any form yet: keep walking up.
|
||||
Err(_) => {}
|
||||
}
|
||||
ancestor = dir.parent();
|
||||
}
|
||||
Err(format!("cannot verify path '{path}' against the session sandbox"))
|
||||
}
|
||||
|
||||
/// Resolve where a (possibly dangling) symlink points, as a real absolute path,
|
||||
/// without requiring the target to exist: read the link, make it absolute
|
||||
/// relative to the link's parent, then canonicalize its nearest existing
|
||||
/// ancestor and re-append the missing remainder.
|
||||
fn symlink_real_target(link: &Path) -> Result<PathBuf, String> {
|
||||
let raw = std::fs::read_link(link)
|
||||
.map_err(|e| format!("cannot read symlink '{}': {e}", link.display()))?;
|
||||
let abs = if raw.is_absolute() {
|
||||
raw
|
||||
} else {
|
||||
link.parent().unwrap_or_else(|| Path::new("/")).join(raw)
|
||||
};
|
||||
real_location(&abs)
|
||||
.ok_or_else(|| format!("cannot resolve symlink target '{}'", abs.display()))
|
||||
}
|
||||
|
||||
/// Real absolute location of a path that may not fully exist: canonicalize the
|
||||
/// nearest existing ancestor (resolving any symlinks/`..` in the existing part)
|
||||
/// and re-append the non-existing remainder lexically.
|
||||
fn real_location(p: &Path) -> Option<PathBuf> {
|
||||
if let Ok(c) = p.canonicalize() {
|
||||
return Some(c);
|
||||
}
|
||||
let mut remainder: Vec<std::ffi::OsString> = Vec::new();
|
||||
let mut cur = p.to_path_buf();
|
||||
loop {
|
||||
let parent = cur.parent()?;
|
||||
if let Some(name) = cur.file_name() {
|
||||
remainder.push(name.to_os_string());
|
||||
}
|
||||
if let Ok(mut base) = parent.canonicalize() {
|
||||
while let Some(name) = remainder.pop() {
|
||||
base.push(name);
|
||||
}
|
||||
return Some(base);
|
||||
}
|
||||
cur = parent.to_path_buf();
|
||||
}
|
||||
}
|
||||
|
||||
/// Check whether a tool's path-bearing arguments stay within the sandbox.
|
||||
///
|
||||
/// Returns `Ok(())` when the tool either takes no path arguments or all of its
|
||||
@@ -184,4 +267,60 @@ mod tests {
|
||||
let input = json!({"command": "ls -la"});
|
||||
assert!(check_tool_paths("bash", &input, &sb).is_ok());
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn rejects_dangling_symlink_escape() {
|
||||
// Models an approved `ln -s /etc/cron.d/pwn evil` inside the sandbox,
|
||||
// whose target does not exist yet. A later Write to `evil` must NOT be
|
||||
// allowed to follow the link out of the sandbox.
|
||||
let sb = tmp_sandbox();
|
||||
std::os::unix::fs::symlink("/etc/cron.d/pwn_dangling", sb.join("evil")).unwrap();
|
||||
let res = validate_path_within_cwd("evil", &sb);
|
||||
assert!(res.is_err(), "dangling symlink escape must be rejected: {res:?}");
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn rejects_dangling_symlink_to_absolute_outside() {
|
||||
let sb = tmp_sandbox();
|
||||
std::os::unix::fs::symlink("/tmp/definitely_outside_xyz", sb.join("link")).unwrap();
|
||||
assert!(validate_path_within_cwd("link", &sb).is_err());
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn rejects_relative_dangling_symlink_escape() {
|
||||
// `evil -> ../../escape` (target missing) must be rejected too.
|
||||
let sb = tmp_sandbox();
|
||||
std::os::unix::fs::symlink("../../escape_target", sb.join("evil")).unwrap();
|
||||
assert!(validate_path_within_cwd("evil", &sb).is_err());
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn allows_dangling_symlink_pointing_inside() {
|
||||
// A symlink whose (missing) target is still inside the sandbox is fine.
|
||||
let sb = tmp_sandbox();
|
||||
std::os::unix::fs::symlink(sb.join("inside_target"), sb.join("ok_link")).unwrap();
|
||||
assert!(validate_path_within_cwd("ok_link", &sb).is_ok());
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn rejects_write_through_intermediate_dangling_symlink_dir() {
|
||||
// `evildir -> /etc` (dangling), then write to `evildir/pwn`.
|
||||
let sb = tmp_sandbox();
|
||||
std::os::unix::fs::symlink("/etc/nonexistent_dir_xyz", sb.join("evildir")).unwrap();
|
||||
assert!(validate_path_within_cwd("evildir/pwn", &sb).is_err());
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn check_tool_paths_blocks_dangling_symlink_write() {
|
||||
let sb = tmp_sandbox();
|
||||
std::os::unix::fs::symlink("/etc/cron.d/pwn2", sb.join("evil")).unwrap();
|
||||
let bad = json!({"path": "evil", "content": "* * * * * root sh"});
|
||||
assert!(check_tool_paths("write", &bad, &sb).is_err());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,65 @@
|
||||
//! Integration test for the dangling-symlink write-escape fix.
|
||||
//!
|
||||
//! Runs against the library's public API so it compiles/executes independently
|
||||
//! of unrelated in-crate test fixtures. Reproduces the concrete escape an
|
||||
//! approved `ln -s` could otherwise open: a symlink whose target does not exist
|
||||
//! yet (so `exists()` is false and `canonicalize()` fails) must still be
|
||||
//! rejected when it points outside the per-session sandbox, instead of being
|
||||
//! treated as a brand-new in-sandbox path that a Write would follow out.
|
||||
|
||||
#![cfg(unix)]
|
||||
|
||||
use boosapi::services::agent_workbench::sandbox::{check_tool_paths, validate_path_within_cwd};
|
||||
use serde_json::json;
|
||||
use std::os::unix::fs::symlink;
|
||||
use std::path::PathBuf;
|
||||
|
||||
fn tmp_sandbox() -> PathBuf {
|
||||
let base = std::env::temp_dir().join(format!("agent_it_sb_{}", uuid::Uuid::new_v4().simple()));
|
||||
std::fs::create_dir_all(&base).unwrap();
|
||||
base.canonicalize().unwrap()
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn dangling_symlink_to_absolute_target_is_rejected() {
|
||||
let sb = tmp_sandbox();
|
||||
symlink("/etc/cron.d/pwn_it", sb.join("evil")).unwrap();
|
||||
let res = validate_path_within_cwd("evil", &sb);
|
||||
assert!(res.is_err(), "expected rejection, got {res:?}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn dangling_relative_symlink_escape_is_rejected() {
|
||||
let sb = tmp_sandbox();
|
||||
symlink("../../escape_it", sb.join("evil")).unwrap();
|
||||
assert!(validate_path_within_cwd("evil", &sb).is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn write_tool_through_dangling_symlink_is_blocked() {
|
||||
let sb = tmp_sandbox();
|
||||
symlink("/etc/cron.d/pwn_it2", sb.join("evil")).unwrap();
|
||||
let input = json!({ "path": "evil", "content": "* * * * * root sh -c id" });
|
||||
assert!(check_tool_paths("write", &input, &sb).is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn write_through_intermediate_dangling_symlink_dir_is_blocked() {
|
||||
let sb = tmp_sandbox();
|
||||
symlink("/etc/nonexistent_it_dir", sb.join("evildir")).unwrap();
|
||||
let input = json!({ "path": "evildir/pwn", "content": "x" });
|
||||
assert!(check_tool_paths("write", &input, &sb).is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn dangling_symlink_pointing_inside_sandbox_is_allowed() {
|
||||
let sb = tmp_sandbox();
|
||||
symlink(sb.join("future_file"), sb.join("ok_link")).unwrap();
|
||||
assert!(validate_path_within_cwd("ok_link", &sb).is_ok());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn ordinary_new_file_still_allowed() {
|
||||
let sb = tmp_sandbox();
|
||||
assert!(validate_path_within_cwd("notes/today.md", &sb).is_ok());
|
||||
}
|
||||
Reference in New Issue
Block a user