diff --git a/src/agent/protocols/build.rs b/src/agent/protocols/build.rs index fcc443447..616e63c6a 100644 --- a/src/agent/protocols/build.rs +++ b/src/agent/protocols/build.rs @@ -52,4 +52,3 @@ fn replace_text_in_file(file_name: &str, from: &str, to: &str) -> Result<(), std Ok(()) } - diff --git a/src/agent/rustjail/src/mount.rs b/src/agent/rustjail/src/mount.rs index 5f4c9e26d..b7de5ee52 100644 --- a/src/agent/rustjail/src/mount.rs +++ b/src/agent/rustjail/src/mount.rs @@ -232,7 +232,7 @@ pub fn init_rootfs( if m.r#type == "bind" { for o in &m.options { if let Some(fl) = PROPAGATION.get(o.as_str()) { - let dest = format!("{}{}", &rootfs, &m.destination); + let dest = secure_join(rootfs, &m.destination); mount(None::<&str>, dest.as_str(), None::<&str>, *fl, None::<&str>)?; } } @@ -675,6 +675,52 @@ fn parse_mount(m: &Mount) -> (MsFlags, String) { (flags, data.join(",")) } +// This function constructs a canonicalized path by combining the `rootfs` and `unsafe_path` elements. +// The resulting path is guaranteed to be ("below" / "in a directory under") the `rootfs` directory. +// +// Parameters: +// +// - `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().to_string())); + } 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, m: &Mount, @@ -684,7 +730,7 @@ fn mount_from( _label: &str, ) -> Result<()> { let d = String::from(data); - let dest = format!("{}{}", rootfs, &m.destination); + let dest = secure_join(rootfs, &m.destination); let src = if m.r#type.as_str() == "bind" { let src = fs::canonicalize(m.source.as_str())?; @@ -968,6 +1014,10 @@ fn readonly_path(path: &str) -> Result<()> { mod tests { use super::*; use crate::skip_if_not_root; + use std::fs::create_dir; + use std::fs::create_dir_all; + use std::fs::remove_dir_all; + use std::os::unix::fs; use std::os::unix::io::AsRawFd; use tempfile::tempdir; @@ -997,7 +1047,7 @@ mod tests { ); let rootfs = tempdir().unwrap(); - let ret = fs::create_dir(rootfs.path().join("dev")); + let ret = create_dir(rootfs.path().join("dev")); assert!(ret.is_ok(), "Got: {:?}", ret); spec.root = Some(oci::Root { @@ -1008,8 +1058,8 @@ mod tests { // there is no spec.mounts, but should pass let ret = init_rootfs(stdout_fd, &spec, &cpath, &mounts, true); assert!(ret.is_ok(), "Should pass. Got: {:?}", ret); - let _ = fs::remove_dir_all(rootfs.path().join("dev")); - let _ = fs::create_dir(rootfs.path().join("dev")); + let _ = remove_dir_all(rootfs.path().join("dev")); + let _ = create_dir(rootfs.path().join("dev")); // Adding bad mount point to spec.mounts spec.mounts.push(oci::Mount { @@ -1027,8 +1077,8 @@ mod tests { ret ); spec.mounts.pop(); - let _ = fs::remove_dir_all(rootfs.path().join("dev")); - let _ = fs::create_dir(rootfs.path().join("dev")); + let _ = remove_dir_all(rootfs.path().join("dev")); + let _ = create_dir(rootfs.path().join("dev")); // mounting a cgroup spec.mounts.push(oci::Mount { @@ -1041,8 +1091,8 @@ mod tests { let ret = init_rootfs(stdout_fd, &spec, &cpath, &mounts, true); assert!(ret.is_ok(), "Should pass. Got: {:?}", ret); spec.mounts.pop(); - let _ = fs::remove_dir_all(rootfs.path().join("dev")); - let _ = fs::create_dir(rootfs.path().join("dev")); + let _ = remove_dir_all(rootfs.path().join("dev")); + let _ = create_dir(rootfs.path().join("dev")); // mounting /dev spec.mounts.push(oci::Mount { @@ -1079,11 +1129,11 @@ mod tests { cgroup_mounts.insert("cpu".to_string(), "cpu".to_string()); cgroup_mounts.insert("memory".to_string(), "memory".to_string()); - let ret = fs::create_dir_all(tempdir.path().join("cgroups")); + let ret = create_dir_all(tempdir.path().join("cgroups")); assert!(ret.is_ok(), "Should pass. Got {:?}", ret); - let ret = fs::create_dir_all(tempdir.path().join("cpu")); + let ret = create_dir_all(tempdir.path().join("cpu")); assert!(ret.is_ok(), "Should pass. Got {:?}", ret); - let ret = fs::create_dir_all(tempdir.path().join("memory")); + let ret = create_dir_all(tempdir.path().join("memory")); assert!(ret.is_ok(), "Should pass. Got {:?}", ret); let ret = mount_cgroups( @@ -1231,4 +1281,89 @@ 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); + + // 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); + } + } }