From c0ceaf661a9783659e97bccc6dc1ff796687341d Mon Sep 17 00:00:00 2001 From: Qingyuan Hou Date: Thu, 8 May 2025 07:08:47 +0000 Subject: [PATCH] agent: use safe-path to replace secure_join This patch use safe-path library to safely handle filesystem paths. Signed-off-by: Qingyuan Hou --- src/agent/Cargo.lock | 1 + src/agent/rustjail/Cargo.toml | 1 + src/agent/rustjail/src/mount.rs | 133 +++----------------------------- 3 files changed, 11 insertions(+), 124 deletions(-) diff --git a/src/agent/Cargo.lock b/src/agent/Cargo.lock index fbe871d82..f008f0778 100644 --- a/src/agent/Cargo.lock +++ b/src/agent/Cargo.lock @@ -5487,6 +5487,7 @@ dependencies = [ "regex", "rlimit", "runtime-spec", + "safe-path", "scan_fmt", "scopeguard", "serde", diff --git a/src/agent/rustjail/Cargo.toml b/src/agent/rustjail/Cargo.toml index eceab4242..005229fba 100644 --- a/src/agent/rustjail/Cargo.toml +++ b/src/agent/rustjail/Cargo.toml @@ -21,6 +21,7 @@ protobuf.workspace = true slog.workspace = true slog-scope.workspace = true scan_fmt.workspace = true +safe-path.workspace = true regex.workspace = true path-absolutize = "1.2.0" anyhow = "1.0.32" diff --git a/src/agent/rustjail/src/mount.rs b/src/agent/rustjail/src/mount.rs index 6276141fe..9363c25f4 100644 --- a/src/agent/rustjail/src/mount.rs +++ b/src/agent/rustjail/src/mount.rs @@ -32,6 +32,7 @@ use crate::sync::write_count; use std::string::ToString; use crate::log_child; +use safe_path::scoped_join; // Info reveals information about a particular mounted filesystem. This // struct is populated from the content in the /proc//mountinfo file. @@ -277,7 +278,10 @@ pub fn init_rootfs( // first check that we have non-default options required before attempting a // remount if mount_typ == "bind" && !pgflags.is_empty() { - let dest = secure_join(rootfs, mount_dest); + let dest = scoped_join(rootfs, mount_dest)? + .to_str() + .ok_or_else(|| anyhow::anyhow!("Failed to convert path to string"))? + .to_string(); mount( None::<&str>, dest.as_str(), @@ -763,43 +767,6 @@ fn parse_mount(m: &Mount) -> (MsFlags, MsFlags, String) { // - `rootfs` is the absolute path to the root of the containers root filesystem directory. // - `unsafe_path` is path inside a container. It is unsafe since it may try to "escape" from the containers // rootfs by using one or more "../" path elements or is its a symlink to path. -fn secure_join(rootfs: &str, unsafe_path: &str) -> String { - let mut path = PathBuf::from(format!("{}/", rootfs)); - let unsafe_p = Path::new(&unsafe_path); - - for it in unsafe_p.iter() { - let it_p = Path::new(&it); - - // if it_p leads with "/", path.push(it) will be replace as it, so ignore "/" - if it_p.has_root() { - continue; - }; - - path.push(it); - if let Ok(v) = path.read_link() { - if v.is_absolute() { - path = PathBuf::from(format!("{}{}", rootfs, v.to_str().unwrap())); - } else { - path.pop(); - for it in v.iter() { - path.push(it); - if path.exists() { - path = path.canonicalize().unwrap(); - if !path.starts_with(rootfs) { - path = PathBuf::from(rootfs.to_string()); - } - } - } - } - } - // skip any ".." - if path.ends_with("..") { - path.pop(); - } - } - - path.to_str().unwrap().to_string() -} fn mount_from( cfd_log: RawFd, @@ -812,7 +779,10 @@ fn mount_from( let mut d = String::from(data); let mount_dest = m.destination().display().to_string(); let mount_typ = m.typ().as_ref().unwrap(); - let dest = secure_join(rootfs, &mount_dest); + let dest = scoped_join(rootfs, mount_dest)? + .to_str() + .ok_or_else(|| anyhow::anyhow!("Failed to convert path to string"))? + .to_string(); let mount_source = m.source().as_ref().unwrap().display().to_string(); let src = if mount_typ == "bind" { @@ -1617,91 +1587,6 @@ mod tests { assert!(check_proc_mount(&mount).is_err()); } - #[test] - fn test_secure_join() { - #[derive(Debug)] - struct TestData<'a> { - name: &'a str, - rootfs: &'a str, - unsafe_path: &'a str, - symlink_path: &'a str, - result: &'a str, - } - - // create tempory directory to simulate container rootfs with symlink - let rootfs_dir = tempdir().expect("failed to create tmpdir"); - let rootfs_path = rootfs_dir.path().to_str().unwrap(); - - let tests = &[ - TestData { - name: "rootfs_not_exist", - rootfs: "/home/rootfs", - unsafe_path: "a/b/c", - symlink_path: "", - result: "/home/rootfs/a/b/c", - }, - TestData { - name: "relative_path", - rootfs: "/home/rootfs", - unsafe_path: "../../../a/b/c", - symlink_path: "", - result: "/home/rootfs/a/b/c", - }, - TestData { - name: "skip any ..", - rootfs: "/home/rootfs", - unsafe_path: "../../../a/../../b/../../c", - symlink_path: "", - result: "/home/rootfs/a/b/c", - }, - TestData { - name: "rootfs is null", - rootfs: "", - unsafe_path: "", - symlink_path: "", - result: "/", - }, - TestData { - name: "relative softlink beyond container rootfs", - rootfs: rootfs_path, - unsafe_path: "1", - symlink_path: "../../../", - result: rootfs_path, - }, - TestData { - name: "abs softlink points to the non-exist directory", - rootfs: rootfs_path, - unsafe_path: "2", - symlink_path: "/dddd", - result: &format!("{}/dddd", rootfs_path).as_str().to_owned(), - }, - TestData { - name: "abs softlink points to the root", - rootfs: rootfs_path, - unsafe_path: "3", - symlink_path: "/", - result: &format!("{}/", rootfs_path).as_str().to_owned(), - }, - ]; - - for (i, t) in tests.iter().enumerate() { - // Create a string containing details of the test - let msg = format!("test[{}]: {:?}", i, t.name); - - // if is_symlink, then should be prepare the softlink environment - if t.symlink_path != "" { - fs::symlink(t.symlink_path, format!("{}/{}", t.rootfs, t.unsafe_path)).unwrap(); - } - let result = secure_join(t.rootfs, t.unsafe_path); - - // Update the test details string with the results of the call - let msg = format!("{}, result: {:?}", msg, result); - - // Perform the checks - assert!(result == t.result, "{}", msg); - } - } - #[test] fn test_parse_mount_table() { #[derive(Debug)]