From 8c68a292fb47e9413a5e833a92871105d454ad35 Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Thu, 25 Jun 2026 05:54:13 -0400 Subject: [PATCH 1/5] fix(pre_seed): security hardening + tempdir fix + docs update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F1 🔴: Use symlink_metadata() instead of is_file() when setting permissions — prevents symlink-following attacks on sensitive files. F3 🟡: Create target dir with create_dir_all() before tempdir_in() to avoid 'Permission denied' when target doesn't exist yet. F4 🟡: Use symlink_metadata() in move_recursive — preserve symlinks as-is without following them (prevents directory escape). F5 🟡: Update docs to reflect pre-seed is now a default feature. Note: F2 (path traversal) is already handled by tar::Entry::unpack_in() which rejects '..' components and absolute paths. --- crates/openab-core/src/pre_seed.rs | 50 +++++++++++++++++++++++++++--- docs/config-reference.md | 2 +- docs/hooks.md | 2 +- 3 files changed, 47 insertions(+), 7 deletions(-) diff --git a/crates/openab-core/src/pre_seed.rs b/crates/openab-core/src/pre_seed.rs index c5695391e..921b461ea 100644 --- a/crates/openab-core/src/pre_seed.rs +++ b/crates/openab-core/src/pre_seed.rs @@ -172,7 +172,8 @@ fn extract_and_apply( target: &Path, deadline: Instant, ) -> anyhow::Result<()> { - let temp_dir = tempfile::tempdir_in(target.parent().unwrap_or(target))?; + std::fs::create_dir_all(target)?; + let temp_dir = tempfile::tempdir_in(target)?; if data.starts_with(&[0x1f, 0x8b]) { extract_tarball_with_limits(data, temp_dir.path(), deadline)?; @@ -222,7 +223,7 @@ fn extract_zip_budgeted( for i in 0..file_count { // Cooperative deadline check per file - if i % 100 == 0 && Instant::now() >= deadline { + if i.is_multiple_of(100) && Instant::now() >= deadline { anyhow::bail!("hooks.pre_seed: timed out during extraction at entry {i}"); } @@ -252,6 +253,14 @@ fn extract_zip_budgeted( std::io::copy(&mut file, &mut out)?; #[cfg(unix)] + #[cfg(not(unix))] + { + // Fallback: copy the link target file content + if link_target.is_absolute() || src_path.parent().map(|p| p.join(&link_target)).filter(|p| p.exists()).is_some() { + let resolved = src_path.parent().unwrap_or(Path::new(".")).join(&link_target); + let _ = std::fs::copy(&resolved, &dst_path); + } + } { use std::os::unix::fs::PermissionsExt; if let Some(mode) = file.unix_mode() { @@ -286,7 +295,7 @@ fn extract_tarball_with_limits(data: &[u8], dest: &Path, deadline: Instant) -> a } // Cooperative deadline check every 10 files - if file_count % 10 == 0 && Instant::now() >= deadline { + if file_count.is_multiple_of(10) && Instant::now() >= deadline { anyhow::bail!("hooks.pre_seed: timed out during tarball extraction at entry {file_count}"); } @@ -302,11 +311,19 @@ fn extract_tarball_with_limits(data: &[u8], dest: &Path, deadline: Instant) -> a // Manually set permissions (strip suid/sgid/sticky, like zip path) #[cfg(unix)] + #[cfg(not(unix))] + { + // Fallback: copy the link target file content + if link_target.is_absolute() || src_path.parent().map(|p| p.join(&link_target)).filter(|p| p.exists()).is_some() { + let resolved = src_path.parent().unwrap_or(Path::new(".")).join(&link_target); + let _ = std::fs::copy(&resolved, &dst_path); + } + } { use std::os::unix::fs::PermissionsExt; if let Ok(path) = entry.path() { let out_path = dest.join(path); - if out_path.is_file() { + if out_path.symlink_metadata().map(|m| m.file_type().is_file()).unwrap_or(false) { let mode = entry.header().mode().unwrap_or(0o644) & 0o0777; let _ = std::fs::set_permissions( &out_path, @@ -320,6 +337,23 @@ fn extract_tarball_with_limits(data: &[u8], dest: &Path, deadline: Instant) -> a Ok(()) } +/// Create a symlink on Unix, or copy the resolved target on other platforms. +#[allow(unused_variables)] +fn create_symlink_or_copy(link_target: &Path, dst: &Path, src: &Path) -> anyhow::Result<()> { + #[cfg(unix)] + { + std::os::unix::fs::symlink(link_target, dst)?; + } + #[cfg(not(unix))] + { + let resolved = src.parent().unwrap_or(Path::new(".")).join(link_target); + if resolved.exists() { + std::fs::copy(&resolved, dst)?; + } + } + Ok(()) +} + /// Recursively move files from src directory into dst directory. /// Checks deadline cooperatively. fn move_recursive(src: &Path, dst: &Path, deadline: Instant) -> anyhow::Result<()> { @@ -332,7 +366,13 @@ fn move_recursive(src: &Path, dst: &Path, deadline: Instant) -> anyhow::Result<( let src_path = entry.path(); let dst_path = dst.join(entry.file_name()); - if src_path.is_dir() { + let meta = src_path.symlink_metadata()?; + if meta.is_symlink() { + // Preserve symlinks as-is without following + let link_target = std::fs::read_link(&src_path)?; + let _ = std::fs::remove_file(&dst_path); + create_symlink_or_copy(&link_target, &dst_path, &src_path)?; + } else if meta.is_dir() { std::fs::create_dir_all(&dst_path)?; move_recursive(&src_path, &dst_path, deadline)?; } else { diff --git a/docs/config-reference.md b/docs/config-reference.md index eae3fc352..56ea052dd 100644 --- a/docs/config-reference.md +++ b/docs/config-reference.md @@ -245,7 +245,7 @@ Lifecycle hooks that run at specific points during the container lifecycle. See Downloads and extracts archives from S3 before `pre_boot`. Seeds the agent environment with configs, tools, and shared memory without requiring AWS CLI in the image. -> **Feature flag:** requires the `pre-seed` feature (opt-in, not in default). Enable with `--features pre-seed`. +> `pre-seed` is enabled by default. No feature flag needed. | Key | Type | Default | Description | |-----|------|---------|-------------| diff --git a/docs/hooks.md b/docs/hooks.md index 164b7a135..7ef9b72c0 100644 --- a/docs/hooks.md +++ b/docs/hooks.md @@ -18,7 +18,7 @@ hooks.pre_seed → hooks.pre_boot → (agent running) → hooks.pre_shutdown The `pre_seed` phase runs **before** `pre_boot`. It downloads archives from S3 and extracts them into the agent's home directory (or a custom target). Supported formats: `.zip`, `.tar.gz`, and `.tgz` (auto-detected via magic bytes). This eliminates the need for users to install AWS CLI and write download scripts in `pre_boot`. -> **Feature flag:** requires the `pre-seed` feature (opt-in, not in default). +> `pre-seed` is enabled by default. No feature flag needed. ### Configuration From 3d4606b656e60e13098a2a2862de7c0bf433becc Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Thu, 25 Jun 2026 06:22:02 -0400 Subject: [PATCH 2/5] fix: remove dead #[cfg(not(unix))] blocks that never compile The #[cfg(unix)] #[cfg(not(unix))] stacked attributes are contradictory and produce unreachable code on all platforms. The blocks also reference undefined variables (link_target, src_path, dst_path). The non-unix symlink fallback is already handled by create_symlink_or_copy() in move_recursive. --- Cargo.lock | 12 ++++++------ crates/openab-core/src/pre_seed.rs | 16 ---------------- 2 files changed, 6 insertions(+), 22 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6c9f2f08f..74ddee92f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1270,12 +1270,6 @@ dependencies = [ "subtle", ] -[[package]] -name = "find-msvc-tools" -version = "0.1.9" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5baebc0774151f905a1a2cc41989300b1e6fbb29aff0ceffa1064fdd3088d582" - [[package]] name = "filetime" version = "0.2.29" @@ -1286,6 +1280,12 @@ dependencies = [ "libc", ] +[[package]] +name = "find-msvc-tools" +version = "0.1.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5baebc0774151f905a1a2cc41989300b1e6fbb29aff0ceffa1064fdd3088d582" + [[package]] name = "flate2" version = "1.1.9" diff --git a/crates/openab-core/src/pre_seed.rs b/crates/openab-core/src/pre_seed.rs index 921b461ea..5122fc8d9 100644 --- a/crates/openab-core/src/pre_seed.rs +++ b/crates/openab-core/src/pre_seed.rs @@ -253,14 +253,6 @@ fn extract_zip_budgeted( std::io::copy(&mut file, &mut out)?; #[cfg(unix)] - #[cfg(not(unix))] - { - // Fallback: copy the link target file content - if link_target.is_absolute() || src_path.parent().map(|p| p.join(&link_target)).filter(|p| p.exists()).is_some() { - let resolved = src_path.parent().unwrap_or(Path::new(".")).join(&link_target); - let _ = std::fs::copy(&resolved, &dst_path); - } - } { use std::os::unix::fs::PermissionsExt; if let Some(mode) = file.unix_mode() { @@ -311,14 +303,6 @@ fn extract_tarball_with_limits(data: &[u8], dest: &Path, deadline: Instant) -> a // Manually set permissions (strip suid/sgid/sticky, like zip path) #[cfg(unix)] - #[cfg(not(unix))] - { - // Fallback: copy the link target file content - if link_target.is_absolute() || src_path.parent().map(|p| p.join(&link_target)).filter(|p| p.exists()).is_some() { - let resolved = src_path.parent().unwrap_or(Path::new(".")).join(&link_target); - let _ = std::fs::copy(&resolved, &dst_path); - } - } { use std::os::unix::fs::PermissionsExt; if let Ok(path) = entry.path() { From 89b495168bb9fa91d49777fdebf2a2494f6a29a1 Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Thu, 25 Jun 2026 06:27:24 -0400 Subject: [PATCH 3/5] fix: harden symlink validation and strip suid/sgid in zip extraction - Reject symlinks with absolute targets or .. components in move_recursive - Remove tarball symlinks with escaping targets after unpack_in - Strip suid/sgid/sticky bits (& 0o0777) in zip permission path (was already done for tarball) --- crates/openab-core/src/pre_seed.rs | 34 ++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/crates/openab-core/src/pre_seed.rs b/crates/openab-core/src/pre_seed.rs index 5122fc8d9..91df447e7 100644 --- a/crates/openab-core/src/pre_seed.rs +++ b/crates/openab-core/src/pre_seed.rs @@ -256,6 +256,7 @@ fn extract_zip_budgeted( { use std::os::unix::fs::PermissionsExt; if let Some(mode) = file.unix_mode() { + let mode = mode & 0o0777; // strip suid/sgid/sticky std::fs::set_permissions(&out_path, std::fs::Permissions::from_mode(mode))?; } } @@ -301,6 +302,27 @@ fn extract_tarball_with_limits(data: &[u8], dest: &Path, deadline: Instant) -> a entry.unpack_in(dest)?; + // Skip symlinks with escaping targets created by unpack_in + #[cfg(unix)] + if let Ok(path) = entry.path() { + let out_path = dest.join(&*path); + if out_path.symlink_metadata().map(|m| m.is_symlink()).unwrap_or(false) { + if let Ok(target) = std::fs::read_link(&out_path) { + if target.is_absolute() + || target + .components() + .any(|c| c == std::path::Component::ParentDir) + { + let _ = std::fs::remove_file(&out_path); + warn!( + "hooks.pre_seed: removed symlink with escaping target: {}", + target.display() + ); + } + } + } + } + // Manually set permissions (strip suid/sgid/sticky, like zip path) #[cfg(unix)] { @@ -322,8 +344,20 @@ fn extract_tarball_with_limits(data: &[u8], dest: &Path, deadline: Instant) -> a } /// Create a symlink on Unix, or copy the resolved target on other platforms. +/// Rejects symlink targets that escape via absolute path or `..` components. #[allow(unused_variables)] fn create_symlink_or_copy(link_target: &Path, dst: &Path, src: &Path) -> anyhow::Result<()> { + // Reject symlinks that could escape the target directory + if link_target.is_absolute() + || link_target + .components() + .any(|c| c == std::path::Component::ParentDir) + { + anyhow::bail!( + "hooks.pre_seed: rejecting symlink with escaping target: {}", + link_target.display() + ); + } #[cfg(unix)] { std::os::unix::fs::symlink(link_target, dst)?; From 23c49a33339322aa07868dd76f9d22f9108c381e Mon Sep 17 00:00:00 2001 From: chaodufashi Date: Thu, 25 Jun 2026 10:32:51 +0000 Subject: [PATCH 4/5] fix(review): symlink handling correctness + test coverage - Fix remove_file failing on existing directories in move_recursive symlink branch (use symlink_metadata to decide remove_dir_all vs remove_file) - Fix non-Unix fallback in create_symlink_or_copy silently dropping entries when resolved target doesn't exist (now returns error) - Add tests: move_recursive preserves symlinks, symlink overwrites existing dir, tempdir strategy with non-writable parent, deep target creation --- crates/openab-core/src/pre_seed.rs | 113 ++++++++++++++++++++++++++++- 1 file changed, 112 insertions(+), 1 deletion(-) diff --git a/crates/openab-core/src/pre_seed.rs b/crates/openab-core/src/pre_seed.rs index 91df447e7..79bbb5f7a 100644 --- a/crates/openab-core/src/pre_seed.rs +++ b/crates/openab-core/src/pre_seed.rs @@ -367,6 +367,11 @@ fn create_symlink_or_copy(link_target: &Path, dst: &Path, src: &Path) -> anyhow: let resolved = src.parent().unwrap_or(Path::new(".")).join(link_target); if resolved.exists() { std::fs::copy(&resolved, dst)?; + } else { + anyhow::bail!( + "hooks.pre_seed: symlink target does not exist: {}", + resolved.display() + ); } } Ok(()) @@ -388,7 +393,14 @@ fn move_recursive(src: &Path, dst: &Path, deadline: Instant) -> anyhow::Result<( if meta.is_symlink() { // Preserve symlinks as-is without following let link_target = std::fs::read_link(&src_path)?; - let _ = std::fs::remove_file(&dst_path); + // Remove existing dst (file or directory) before creating symlink + if let Ok(dst_meta) = dst_path.symlink_metadata() { + if dst_meta.is_dir() { + std::fs::remove_dir_all(&dst_path)?; + } else { + std::fs::remove_file(&dst_path)?; + } + } create_symlink_or_copy(&link_target, &dst_path, &src_path)?; } else if meta.is_dir() { std::fs::create_dir_all(&dst_path)?; @@ -709,4 +721,103 @@ mod tests { assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("timed out")); } + + #[cfg(unix)] + #[test] + fn move_recursive_preserves_symlinks() { + let src = tempfile::tempdir().unwrap(); + let dst = tempfile::tempdir().unwrap(); + let deadline = Instant::now() + std::time::Duration::from_secs(60); + + // Create a regular file and a symlink pointing to it + std::fs::write(src.path().join("real.txt"), "content").unwrap(); + std::os::unix::fs::symlink("real.txt", src.path().join("link.txt")).unwrap(); + + move_recursive(src.path(), dst.path(), deadline).unwrap(); + + let dst_link = dst.path().join("link.txt"); + let meta = dst_link.symlink_metadata().unwrap(); + assert!(meta.is_symlink(), "destination should be a symlink"); + assert_eq!(std::fs::read_link(&dst_link).unwrap().to_str().unwrap(), "real.txt"); + assert_eq!(std::fs::read_to_string(dst.path().join("real.txt")).unwrap(), "content"); + } + + #[cfg(unix)] + #[test] + fn move_recursive_symlink_overwrites_existing_dir() { + let src = tempfile::tempdir().unwrap(); + let dst = tempfile::tempdir().unwrap(); + let deadline = Instant::now() + std::time::Duration::from_secs(60); + + // src has a symlink named "item" + std::fs::write(src.path().join("target.txt"), "x").unwrap(); + std::os::unix::fs::symlink("target.txt", src.path().join("item")).unwrap(); + + // dst has a directory named "item" + std::fs::create_dir(dst.path().join("item")).unwrap(); + + move_recursive(src.path(), dst.path(), deadline).unwrap(); + + let dst_item = dst.path().join("item"); + let meta = dst_item.symlink_metadata().unwrap(); + assert!(meta.is_symlink(), "should have replaced directory with symlink"); + } + + #[cfg(unix)] + #[test] + fn extract_and_apply_works_when_parent_not_writable() { + use std::os::unix::fs::PermissionsExt; + use std::io::Write; + + let base = tempfile::tempdir().unwrap(); + let restricted = base.path().join("restricted"); + std::fs::create_dir(&restricted).unwrap(); + + let target = restricted.join("target"); + // Don't create target — extract_and_apply should create it + + // Make parent read-only (target doesn't exist yet, but parent is not writable) + std::fs::set_permissions(&restricted, std::fs::Permissions::from_mode(0o555)).unwrap(); + + // This should fail because parent is not writable and target doesn't exist + let buf = Vec::new(); + let cursor = std::io::Cursor::new(buf); + let mut writer = zip::ZipWriter::new(cursor); + let options = zip::write::SimpleFileOptions::default(); + writer.start_file("test.txt", options).unwrap(); + writer.write_all(b"data").unwrap(); + let cursor = writer.finish().unwrap(); + + let deadline = Instant::now() + std::time::Duration::from_secs(60); + let result = extract_and_apply(cursor.get_ref(), &target, deadline); + + // Should fail because we can't create_dir_all in a read-only parent + assert!(result.is_err()); + + // Restore permissions for cleanup + std::fs::set_permissions(&restricted, std::fs::Permissions::from_mode(0o755)).unwrap(); + } + + #[cfg(unix)] + #[test] + fn extract_and_apply_succeeds_with_writable_target() { + use std::io::Write; + + let base = tempfile::tempdir().unwrap(); + let target = base.path().join("deep").join("target"); + // target doesn't exist yet — create_dir_all should handle it + + let buf = Vec::new(); + let cursor = std::io::Cursor::new(buf); + let mut writer = zip::ZipWriter::new(cursor); + let options = zip::write::SimpleFileOptions::default(); + writer.start_file("test.txt", options).unwrap(); + writer.write_all(b"hello").unwrap(); + let cursor = writer.finish().unwrap(); + + let deadline = Instant::now() + std::time::Duration::from_secs(60); + extract_and_apply(cursor.get_ref(), &target, deadline).unwrap(); + + assert_eq!(std::fs::read_to_string(target.join("test.txt")).unwrap(), "hello"); + } } From 563ec30adb6ca516c73d064c99e2777076351654 Mon Sep 17 00:00:00 2001 From: chaodufashi Date: Thu, 25 Jun 2026 10:40:14 +0000 Subject: [PATCH 5/5] fix(review): correct tempdir regression test Test now creates target dir (writable) then locks parent to read-only, verifying tempdir_in(target) succeeds where old tempdir_in(parent) would fail. --- crates/openab-core/src/pre_seed.rs | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/crates/openab-core/src/pre_seed.rs b/crates/openab-core/src/pre_seed.rs index 79bbb5f7a..016cae82b 100644 --- a/crates/openab-core/src/pre_seed.rs +++ b/crates/openab-core/src/pre_seed.rs @@ -765,21 +765,24 @@ mod tests { #[cfg(unix)] #[test] - fn extract_and_apply_works_when_parent_not_writable() { + fn extract_and_apply_succeeds_with_non_writable_parent() { use std::os::unix::fs::PermissionsExt; use std::io::Write; + // Regression test: target is writable but target.parent() is read-only. + // Old code used tempdir_in(target.parent()) which would fail here. + // New code uses tempdir_in(target) which succeeds. let base = tempfile::tempdir().unwrap(); let restricted = base.path().join("restricted"); std::fs::create_dir(&restricted).unwrap(); + // Create target directory (writable) let target = restricted.join("target"); - // Don't create target — extract_and_apply should create it + std::fs::create_dir(&target).unwrap(); - // Make parent read-only (target doesn't exist yet, but parent is not writable) + // Lock down parent so tempdir_in(parent) would fail std::fs::set_permissions(&restricted, std::fs::Permissions::from_mode(0o555)).unwrap(); - // This should fail because parent is not writable and target doesn't exist let buf = Vec::new(); let cursor = std::io::Cursor::new(buf); let mut writer = zip::ZipWriter::new(cursor); @@ -791,11 +794,12 @@ mod tests { let deadline = Instant::now() + std::time::Duration::from_secs(60); let result = extract_and_apply(cursor.get_ref(), &target, deadline); - // Should fail because we can't create_dir_all in a read-only parent - assert!(result.is_err()); - - // Restore permissions for cleanup + // Restore permissions before asserting (for cleanup) std::fs::set_permissions(&restricted, std::fs::Permissions::from_mode(0o755)).unwrap(); + + // Should succeed because tempdir_in(target) works even with read-only parent + result.unwrap(); + assert_eq!(std::fs::read_to_string(target.join("test.txt")).unwrap(), "data"); } #[cfg(unix)]