Youki Symlink Following Vulnerability

@@ -9,6 +9,8 @@ pub mod device;
 pub use device::Device;
 
 pub(super) mod mount;
+pub use mount::Mount;
+
 pub(super) mod symlink;
 
 pub mod utils;

@@ -1,10 +1,12 @@
 use std::fs::{canonicalize, create_dir_all, OpenOptions};
-use std::mem;
+use std::io::ErrorKind;
+use std::os::unix::fs::MetadataExt;
 use std::os::unix::io::AsRawFd;
 use std::path::{Path, PathBuf};
 use std::time::Duration;
 #[cfg(feature = "v1")]
 use std::{borrow::Cow, collections::HashMap};
+use std::{fs, mem};
 
 use libcgroups::common::CgroupSetup::{Hybrid, Legacy, Unified};
 #[cfg(feature = "v1")]
@@ -14,6 +16,7 @@ use nix::errno::Errno;
 use nix::fcntl::OFlag;
 use nix::mount::MsFlags;
 use nix::sys::stat::Mode;
+use nix::sys::statfs::{statfs, PROC_SUPER_MAGIC};
 use nix::NixPath;
 use oci_spec::runtime::{Mount as SpecMount, MountBuilder as SpecMountBuilder};
 use procfs::process::{MountInfo, MountOptFields, Process};
@@ -119,6 +122,46 @@ impl Mount {
                     }
                 }
             }
+            // procfs and sysfs are special because we need to ensure they are actually
+            // mounted on a specific path in a container without any funny business.
+            // Ref: https://github.com/opencontainers/runc/security/advisories/GHSA-fh74-hm69-rqjw
+            Some(typ @ ("proc" | "sysfs")) => {
+                let dest_path = options
+                    .root
+                    .join_safely(Path::new(mount.destination()).normalize())
+                    .map_err(|err| {
+                        tracing::error!(
+                            "could not join rootfs path with mount destination {:?}: {}",
+                            mount.destination(),
+                            err
+                        );
+                        MountError::Other(err.into())
+                    })?;
+
+                match fs::symlink_metadata(&dest_path) {
+                    Ok(m) if !m.is_dir() => {
+                        return Err(MountError::Other(
+                            format!("filesystem {} must be mounted on ordinary directory", typ)
+                                .into(),
+                        ));
+                    }
+                    Err(e) if e.kind() != ErrorKind::NotFound => {
+                        return Err(MountError::Other(
+                            format!("symlink_metadata failed for {}: {}", dest_path.display(), e)
+                                .into(),
+                        ));
+                    }
+                    _ => {}
+                }
+
+                self.check_proc_mount(options.root, mount)?;
+
+                self.mount_into_container(mount, options.root, &mount_option_config, options.label)
+                    .map_err(|err| {
+                        tracing::error!("failed to mount {:?}: {}", mount, err);
+                        err
+                    })?;
+            }
             _ => {
                 if *mount.destination() == PathBuf::from("/dev") {
                     mount_option_config.flags &= !MsFlags::MS_RDONLY;
@@ -630,6 +673,139 @@ impl Mount {
 
         Ok(())
     }
+
+    /// check_proc_mount checks to ensure that the mount destination is not over the top of /proc.
+    /// dest is required to be an abs path and have any symlinks resolved before calling this function.
+    /// # Example  (a valid case where `/proc` is mounted with `proc` type.)
+    ///
+    /// ```
+    /// use std::path::PathBuf;
+    /// use oci_spec::runtime::MountBuilder as SpecMountBuilder;
+    /// use libcontainer::rootfs::Mount;
+    ///
+    /// let mounter = Mount::new();
+    ///
+    /// let rootfs = PathBuf::from("/var/lib/my-runtime/containers/abcd1234/rootfs");
+    /// let destination = PathBuf::from("/proc");
+    /// let source = PathBuf::from("proc");
+    /// let typ = "proc";
+    ///
+    /// let mount = SpecMountBuilder::default()
+    ///     .destination(destination)
+    ///     .typ(typ)
+    ///     .source(source)
+    ///     .build()
+    ///     .expect("failed to build SpecMount");
+    ///
+    /// assert!(mounter.check_proc_mount(rootfs.as_path(), &mount).is_ok());
+    /// ```
+    /// # Example (bind mount to `/proc` that should fail)
+    /// ```
+    /// use std::path::PathBuf;
+    /// use oci_spec::runtime::MountBuilder as SpecMountBuilder;
+    /// use libcontainer::rootfs::Mount;
+    ///
+    /// let mounter = Mount::new();
+    ///
+    /// let rootfs = PathBuf::from("/var/lib/my-runtime/containers/abcd1234/rootfs");
+    /// let destination = PathBuf::from("/proc");
+    /// let source = PathBuf::from("/tmp");
+    /// let typ = "bind";
+    ///
+    /// let mount = SpecMountBuilder::default()
+    ///     .destination(destination)
+    ///     .typ(typ)
+    ///     .source(source)
+    ///     .build()
+    ///     .expect("failed to build SpecMount");
+    ///
+    /// assert!(mounter.check_proc_mount(rootfs.as_path(), &mount).is_err());
+    /// ```
+    pub fn check_proc_mount(&self, rootfs: &Path, mount: &SpecMount) -> Result<()> {
+        const PROC_ROOT_INO: u64 = 1;
+        const VALID_PROC_MOUNTS: &[&str] = &[
+            "/proc/cpuinfo",
+            "/proc/diskstats",
+            "/proc/meminfo",
+            "/proc/stat",
+            "/proc/swaps",
+            "/proc/uptime",
+            "/proc/loadavg",
+            "/proc/slabinfo",
+            "/proc/sys/kernel/ns_last_pid",
+            "/proc/sys/crypto/fips_enabled",
+        ];
+
+        let dest = mount.destination();
+
+        let container_proc_path = rootfs.join("proc");
+        let dest_path = rootfs.join_safely(dest).map_err(|err| {
+            tracing::error!(
+                "could not join rootfs path with mount destination {:?}: {}",
+                dest,
+                err
+            );
+            MountError::Other(err.into())
+        })?;
+
+        // If path is Ok, it means dest_path is under /proc.
+        // - Ok(p) with p.is_empty(): mount target is exactly /proc.
+        //   In this case, check if the mount source is procfs.
+        // - Ok(p) with !p.is_empty(): mount target is under /proc.
+        //   Only allow if it matches a specific whitelist of proc entries.
+        // - Err: not under /proc, so no further checks are needed
+        let path = dest_path.strip_prefix(&container_proc_path);
+
+        match path {
+            Err(_) => Ok(()),
+            Ok(p) if p.as_os_str().is_empty() => {
+                if mount.typ().as_deref() == Some("proc") {
+                    return Ok(());
+                }
+
+                if mount.typ().as_deref() == Some("bind") {
+                    if let Some(source) = mount.source() {
+                        let stat = statfs(source).map_err(MountError::from)?;
+                        if stat.filesystem_type() == PROC_SUPER_MAGIC {
+                            let meta = fs::metadata(source).map_err(MountError::from)?;
+                            // Follow the behavior of runc's checkProcMount function.
+                            if meta.ino() != PROC_ROOT_INO {
+                                tracing::warn!(
+                                    "bind-mount {} (source {:?}) is of type procfs but not the root (inode {}). \
+                                    Future versions may reject this.",
+                                    dest.display(),
+                                    mount.source(),
+                                    meta.ino()
+                                );
+                            }
+                            return Ok(());
+                        }
+                    }
+                }
+
+                Err(MountError::Custom(format!(
+                    "{} cannot be mounted because it is not type proc",
+                    dest.display()
+                )))
+            }
+            Ok(_) => {
+                // Here dest is definitely under /proc. Do not allow those,
+                // except for a few specific entries emulated by lxcfs.
+                let is_allowed = VALID_PROC_MOUNTS.iter().any(|allowed_path| {
+                    let container_allowed_path = rootfs.join(allowed_path.trim_start_matches('/'));
+                    dest_path == container_allowed_path
+                });
+
+                if is_allowed {
+                    Ok(())
+                } else {
+                    Err(MountError::Other(
+                        format!("{} is not a valid mount under /proc", dest.display()).into(),
+                    ))
+                }
+            }
+        }
+    }
 }
 
 /// Find parent mount of rootfs in given mount infos
@@ -652,6 +828,7 @@ pub fn find_parent_mount(
 mod tests {
     #[cfg(feature = "v1")]
     use std::fs;
+    use std::os::unix::fs::symlink;
 
     use anyhow::{Context, Ok, Result};
 
@@ -1167,4 +1344,123 @@ mod tests {
         let res = find_parent_mount(Path::new("/path/to/rootfs"), mount_infos);
         assert!(res.is_err());
     }
+
+    #[test]
+    fn test_check_proc_mount_proc_ok() -> Result<()> {
+        let rootfs = tempfile::tempdir()?;
+        let mounter = Mount::new();
+
+        let mount = SpecMountBuilder::default()
+            .destination(PathBuf::from("/proc"))
+            .typ("proc".to_string())
+            .source(PathBuf::from("proc"))
+            .build()?;
+
+        assert!(mounter.check_proc_mount(rootfs.path(), &mount).is_ok());
+        Ok(())
+    }
+
+    #[test]
+    fn test_check_proc_mount_allowed_subpath() -> Result<()> {
+        let rootfs = tempfile::tempdir()?;
+        let uptime = rootfs.path().join("proc/uptime");
+        std::fs::create_dir_all(uptime.parent().unwrap())?;
+
+        let mounter = Mount::new();
+        let mount = SpecMountBuilder::default()
+            .destination(PathBuf::from("/proc/uptime"))
+            .typ("bind".to_string())
+            .source(uptime)
+            .build()?;
+
+        assert!(mounter.check_proc_mount(rootfs.path(), &mount).is_ok());
+        Ok(())
+    }
+
+    #[test]
+    fn test_check_proc_mount_denied_subpath() -> Result<()> {
+        let rootfs = tempfile::tempdir()?;
+        let custom = rootfs.path().join("proc/custom");
+        std::fs::create_dir_all(custom.parent().unwrap())?;
+
+        let mounter = Mount::new();
+        let mount = SpecMountBuilder::default()
+            .destination(PathBuf::from("/proc/custom"))
+            .typ("bind".to_string())
+            .source(custom)
+            .build()?;
+
+        assert!(mounter.check_proc_mount(rootfs.path(), &mount).is_err());
+        Ok(())
+    }
+
+    #[test]
+    fn setup_mount_proc_fails_if_destination_is_symlink() -> Result<()> {
+        let tmp = tempfile::tempdir()?;
+        let rootfs = tmp.path();
+
+        let symlink_path = rootfs.join("symlink");
+        fs::create_dir_all(&symlink_path)?;
+        let proc_path = rootfs.join("proc");
+
+        symlink(&symlink_path, &proc_path)?;
+
+        let mount = SpecMountBuilder::default()
+            .destination(PathBuf::from("/proc"))
+            .typ("proc")
+            .source(proc_path)
+            .build()?;
+
+        let options = MountOptions {
+            root: rootfs,
+            label: None,
+            cgroup_ns: true,
+        };
+
+        let m = Mount::new();
+
+        let res = m.setup_mount(&mount, &options);
+
+        // proc destination symlink should be rejected
+        assert!(res.is_err());
+        let err = format!("{:?}", res.err().unwrap());
+        assert!(err.contains("must be mounted on ordinary directory"));
+
+        Ok(())
+    }
+
+    #[test]
+    fn setup_mount_sys_fails_if_destination_is_symlink() -> Result<()> {
+        let tmp = tempfile::tempdir()?;
+        let rootfs = tmp.path();
+
+        let symlink_path = rootfs.join("symlink");
+        fs::create_dir_all(&symlink_path)?;
+        let sys_path = rootfs.join("sys");
+
+        symlink(&symlink_path, &sys_path)?;
+
+        let mount = SpecMountBuilder::default()
+            .destination(PathBuf::from("/sys"))
+            .typ("sysfs")
+            .source(sys_path)
+            .build()?;
+
+        let options = MountOptions {
+            root: rootfs,
+            label: None,
+            cgroup_ns: true,
+        };
+
+        let m = Mount::new();
+
+        let res = m.setup_mount(&mount, &options);
+
+        // sys destination symlink should be rejected
+        assert!(res.is_err());
+        let err = format!("{:?}", res.err().unwrap());
+        assert!(err.contains("must be mounted on ordinary directory"));
+
+        Ok(())
+    }
 }

@@ -32,6 +32,7 @@ use crate::tests::process_oom_score_adj::get_process_oom_score_adj_test;
 use crate::tests::process_rlimits::get_process_rlimits_test;
 use crate::tests::process_rlimits_fail::get_process_rlimits_fail_test;
 use crate::tests::process_user::get_process_user_test;
+use crate::tests::prohibit_symlink::get_prohibit_symlink_test;
 use crate::tests::readonly_paths::get_ro_paths_test;
 use crate::tests::root_readonly_true::get_root_readonly_test;
 use crate::tests::rootfs_propagation::get_rootfs_propagation_test;
@@ -144,6 +145,7 @@ fn main() -> Result<()> {
     let process_capabilities_fail = get_process_capabilities_fail_test();
     let uid_mappings = get_uid_mappings_test();
     let exec_cpu_affinity = get_exec_cpu_affinity_test();
+    let prohibit_symlink = get_prohibit_symlink_test();
 
     tm.add_test_group(Box::new(cl));
     tm.add_test_group(Box::new(cc));
@@ -183,6 +185,7 @@ fn main() -> Result<()> {
     tm.add_test_group(Box::new(process_capabilities_fail));
     tm.add_test_group(Box::new(uid_mappings));
     tm.add_test_group(Box::new(exec_cpu_affinity));
+    tm.add_test_group(Box::new(prohibit_symlink));
 
     tm.add_test_group(Box::new(io_priority_test));
     tm.add_cleanup(Box::new(cgroups::cleanup_v1));

@@ -22,6 +22,7 @@ pub mod process_oom_score_adj;
 pub mod process_rlimits;
 pub mod process_rlimits_fail;
 pub mod process_user;
+pub mod prohibit_symlink;
 pub mod readonly_paths;
 pub mod root_readonly_true;
 pub mod rootfs_propagation;

@@ -0,0 +1,3 @@
+mod prohibit_symlink_test;
+
+pub use prohibit_symlink_test::get_prohibit_symlink_test;

@@ -0,0 +1,91 @@
+use std::fs;
+use std::os::unix::fs::symlink;
+
+use anyhow::{anyhow, Context, Ok, Result};
+use oci_spec::runtime::{ProcessBuilder, Spec, SpecBuilder};
+use test_framework::{test_result, Test, TestGroup, TestResult};
+
+use crate::utils::test_inside_container;
+use crate::utils::test_utils::CreateOptions;
+
+fn create_spec() -> Result<Spec> {
+    let process = ProcessBuilder::default()
+        .args(vec!["sleep".to_string(), "3000".to_string()])
+        .build()
+        .expect("error in creating process config");
+
+    let spec = SpecBuilder::default()
+        .process(process)
+        .build()
+        .context("failed to build spec")?;
+
+    Ok(spec)
+}
+
+fn prohibit_symlink_test(path: String) -> TestResult {
+    let spec = test_result!(create_spec());
+    let result = test_inside_container(&spec, &CreateOptions::default(), &|bundle| {
+        let symlink_path = bundle.join(path.clone());
+        fs::create_dir_all(&symlink_path)?;
+
+        let link = bundle.join(path.clone());
+
+        // delete existing directory or file
+        if link.exists() {
+            let md = fs::symlink_metadata(&link)?;
+            if md.file_type().is_dir() {
+                fs::remove_dir_all(&link)?;
+            } else {
+                fs::remove_file(&link)?;
+            }
+        }
+
+        // create symbolic link
+        symlink(&symlink_path, &link)?;
+        Ok(())
+    });
+
+    match result {
+        TestResult::Failed(e) => {
+            let err_str = format!("{:?}", e);
+            if err_str.contains("must be mounted on ordinary directory") {
+                TestResult::Passed
+            } else {
+                TestResult::Failed(anyhow!(
+                    "unexpected error (expected substring not found): {err_str}"
+                ))
+            }
+        }
+        TestResult::Skipped => TestResult::Failed(anyhow!("test was skipped unexpectedly.")),
+        TestResult::Passed => {
+            TestResult::Failed(anyhow!("container creation succeeded unexpectedly."))
+        }
+    }
+}
+
+fn prohibit_symlink_proc_test() -> TestResult {
+    prohibit_symlink_test("proc".to_string())
+}
+
+fn prohibit_symlink_sys_test() -> TestResult {
+    prohibit_symlink_test("sys".to_string())
+}
+
+pub fn get_prohibit_symlink_test() -> TestGroup {
+    let mut prohibit_symlink_test_group = TestGroup::new("prohibit_symlink");
+
+    let prohibit_symlink_proc_test = Test::new(
+        "prohibit_symlink_proc_test",
+        Box::new(prohibit_symlink_proc_test),
+    );
+    let prohibit_symlink_sys_test = Test::new(
+        "prohibit_symlink_sys_test",
+        Box::new(prohibit_symlink_sys_test),
+    );
+    prohibit_symlink_test_group.add(vec![
+        Box::new(prohibit_symlink_proc_test),
+        Box::new(prohibit_symlink_sys_test),
+    ]);
+
+    prohibit_symlink_test_group
+}