From 700dc2254dedeb69e86c72e1812d53d4338c8b5b Mon Sep 17 00:00:00 2001 From: Vantz Stockwell Date: Thu, 11 Jun 2026 11:57:08 -0400 Subject: [PATCH] =?UTF-8?q?fix(host-agent):=20SECURITY=20=E2=80=94=20file?= =?UTF-8?q?=20manager=20copy/list=20no=20longer=20follow=20symlinks=20out?= =?UTF-8?q?=20of=20the=20jail?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Automated security review (HIGH) caught a jail-escape my own review missed: copy_recursive used fs::metadata (follows symlinks). A symlink inside the jail pointing to e.g. /etc, then a 'copy' of its parent dir, would dereference it and pull external content INTO the jail where it could be read — a read-escape exfiltration. jail() validates only the top-level src/dest; the recursive walk reintroduced the escape. Fix: copy_recursive uses symlink_metadata and refuses any symlink ('symlinks are not followed across the jail boundary'). list() likewise switched to symlink_metadata so it reports the link, never the dereferenced target's size/type (info leak). Two regression tests added: copy-symlink-exfil (asserts no external content lands inside) and list-no-deref. 44/44 tests green. Rolled forward to alpha.4 (vulnerable alpha.3 superseded). Co-Authored-By: Claude Fable 5 --- corrosion-host-agent/Cargo.lock | 2 +- corrosion-host-agent/Cargo.toml | 2 +- corrosion-host-agent/src/filemanager.rs | 23 ++++++++-- corrosion-host-agent/tests/filemanager.rs | 56 +++++++++++++++++++++++ 4 files changed, 78 insertions(+), 5 deletions(-) diff --git a/corrosion-host-agent/Cargo.lock b/corrosion-host-agent/Cargo.lock index 417eabf..8841a96 100644 --- a/corrosion-host-agent/Cargo.lock +++ b/corrosion-host-agent/Cargo.lock @@ -264,7 +264,7 @@ checksum = "773648b94d0e5d620f64f280777445740e61fe701025087ec8b57f45c791888b" [[package]] name = "corrosion-host-agent" -version = "2.0.0-alpha.3" +version = "2.0.0-alpha.4" dependencies = [ "anyhow", "async-nats", diff --git a/corrosion-host-agent/Cargo.toml b/corrosion-host-agent/Cargo.toml index 3b01a86..b24cfc4 100644 --- a/corrosion-host-agent/Cargo.toml +++ b/corrosion-host-agent/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "corrosion-host-agent" -version = "2.0.0-alpha.3" +version = "2.0.0-alpha.4" edition = "2021" description = "Corrosion Host Agent — multi-game ops runtime for self-hosted game servers" license = "UNLICENSED" diff --git a/corrosion-host-agent/src/filemanager.rs b/corrosion-host-agent/src/filemanager.rs index 01e308e..3d46bab 100644 --- a/corrosion-host-agent/src/filemanager.rs +++ b/corrosion-host-agent/src/filemanager.rs @@ -198,7 +198,11 @@ pub fn list(root: &Path, rel: &str) -> anyhow::Result> { let mut entries: Vec = Vec::new(); for item in rd { let item = item.with_context(|| format!("reading directory entry in '{}'", abs.display()))?; - let meta = item.metadata().with_context(|| format!("stat '{}'", item.path().display()))?; + // symlink_metadata (lstat): report the link itself, never the target — + // following it would leak the size/type/existence of files outside the + // jail. A symlink lists as a zero-ish-size non-dir entry. + let meta = fs::symlink_metadata(item.path()) + .with_context(|| format!("stat '{}'", item.path().display()))?; let name = item.file_name().to_string_lossy().into_owned(); let is_dir = meta.is_dir(); @@ -367,11 +371,24 @@ pub fn copy(root: &Path, src: &str, dest: &str) -> anyhow::Result<()> { .with_context(|| format!("copy '{}' -> '{}'", src_abs.display(), dest_abs.display())) } -/// Recursive copy helper (mirrors Go's `copyRecursive`). +/// Recursive copy helper. +/// +/// SECURITY: uses `symlink_metadata` (does NOT follow symlinks) and refuses to +/// copy any symlink. `jail()` only validates the top-level src/dest; a symlink +/// *inside* a copied directory that points outside the jail would, if followed, +/// pull external content (e.g. `/etc`) into the jail where it could then be +/// read — a jail-escape exfiltration. Refusing symlinks closes that path. fn copy_recursive(src: &Path, dest: &Path) -> anyhow::Result<()> { - let meta = fs::metadata(src) + let meta = fs::symlink_metadata(src) .with_context(|| format!("stat source '{}'", src.display()))?; + if meta.file_type().is_symlink() { + bail!( + "refusing to copy symlink '{}' — symlinks are not followed across the jail boundary", + src.display() + ); + } + if meta.is_dir() { fs::create_dir_all(dest) .with_context(|| format!("create_dir_all '{}'", dest.display()))?; diff --git a/corrosion-host-agent/tests/filemanager.rs b/corrosion-host-agent/tests/filemanager.rs index 189ecf3..e072b76 100644 --- a/corrosion-host-agent/tests/filemanager.rs +++ b/corrosion-host-agent/tests/filemanager.rs @@ -347,6 +347,62 @@ fn jail_rejects_chained_symlink_escape() { ); } +/// SECURITY REGRESSION: copying a directory that contains a symlink pointing +/// OUTSIDE the jail must NOT dereference it and pull external content inside. +/// jail() validates only the top-level src/dest; the recursive copy must +/// refuse symlinks itself or it becomes a read-escape exfiltration path. +#[cfg(unix)] +#[test] +fn copy_refuses_to_follow_symlink_out_of_jail() { + let dir = tempdir(); + let root = dir.path(); + let outside = tempdir(); + std::fs::write(outside.path().join("secret.txt"), "TOP SECRET") + .expect("write external secret"); + + // A directory inside the jail containing a symlink to the outside dir. + std::fs::create_dir(root.join("src")).expect("mkdir src"); + std::os::unix::fs::symlink(outside.path(), root.join("src").join("escape")) + .expect("plant symlink to outside"); + + // Attempt to copy src -> dest (both inside the jail). + let err = filemanager::copy(root, "src", "dest") + .expect_err("copy must refuse the embedded symlink"); + assert!( + format!("{err:#}").contains("symlink"), + "error should name the refused symlink, got: {err:#}" + ); + + // The external secret must NOT have landed inside the jail. + assert!( + !root.join("dest").join("escape").join("secret.txt").exists(), + "external content leaked into the jail via symlink-following copy", + ); +} + +/// `list` must report a symlink as the link itself, never the dereferenced +/// target — otherwise it leaks the size/type of files outside the jail. +#[cfg(unix)] +#[test] +fn list_does_not_dereference_symlink_metadata() { + let dir = tempdir(); + let root = dir.path(); + std::os::unix::fs::symlink(Path::new("/etc/passwd"), root.join("leak")) + .expect("plant symlink"); + + let entries = filemanager::list(root, "").expect("list root"); + let leak = entries.iter().find(|e| e.name == "leak").expect("symlink listed"); + // /etc/passwd is a regular file; if we followed the link, is_dir would + // reflect the target. We must report the link, which is not a directory, + // and must NOT expose the target's byte size. + assert!(!leak.is_dir, "symlink must not be reported as a directory"); + let target_size = std::fs::metadata("/etc/passwd").map(|m| m.len()).unwrap_or(0); + assert!( + leak.size != target_size || target_size == 0, + "list leaked the symlink target's size ({target_size} bytes)" + ); +} + // --------------------------------------------------------------------------- // Dispatch layer tests // ---------------------------------------------------------------------------