Merge pull request #1216 from houstar/2.0-dev

agent: add secure_join to prevent softlink escape

src/agent/rustjail/src/mount.rs

@@ -235,7 +235,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>)?;
                     }
                 }
@@ -677,6 +677,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,
@@ -686,7 +732,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())?;
@@ -970,6 +1016,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;
 
@@ -999,7 +1049,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 {
@@ -1010,8 +1060,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 _ = remove_dir_all(rootfs.path().join("dev"));
-        let _ = fs::create_dir(rootfs.path().join("dev"));
+        let _ = create_dir(rootfs.path().join("dev"));
 
         // Adding bad mount point to spec.mounts
         spec.mounts.push(oci::Mount {
@@ -1029,8 +1079,8 @@ mod tests {
             ret
         );
         spec.mounts.pop();
-        let _ = fs::remove_dir_all(rootfs.path().join("dev"));
+        let _ = remove_dir_all(rootfs.path().join("dev"));
-        let _ = fs::create_dir(rootfs.path().join("dev"));
+        let _ = create_dir(rootfs.path().join("dev"));
 
         // mounting a cgroup
         spec.mounts.push(oci::Mount {
@@ -1043,8 +1093,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 _ = remove_dir_all(rootfs.path().join("dev"));
-        let _ = fs::create_dir(rootfs.path().join("dev"));
+        let _ = create_dir(rootfs.path().join("dev"));
 
         // mounting /dev
         spec.mounts.push(oci::Mount {
@@ -1081,11 +1131,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(
@@ -1233,4 +1283,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);
+        }
+    }
 }