From fa581d334f5adb99e562788abfab302920181fac Mon Sep 17 00:00:00 2001 From: Qingyuan Hou Date: Sat, 26 Dec 2020 20:59:42 +0800 Subject: [PATCH] agent: add secure_join to prevent softlink escape This patch fixed the security issue if the container images has unsafe symlink to the container rootfs and hackers can be exploit this symlink to hack the guest system. e.g. make directory or files on guest. CVE-2015-3629 Fixes: #1219 Signed-off-by: Qingyuan Hou --- src/agent/protocols/build.rs | 1 - src/agent/rustjail/src/mount.rs | 159 +++++++++++++++++++++++++++++--- 2 files changed, 147 insertions(+), 13 deletions(-) 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); + } + } }