Use safe_path crate instead of our original secure_join (#1911)

Our secure_join had a bug and did not work perfectly with K8s. It did not take into account the case where the symbolic destination is an absolute path. Thus there are many cases where secure_join should be considered; it would be more worthwhile to use safe_path, which kata-container makes, and mature this one. Signed-off-by: utam0k <[email protected]>
youki-dev · May 13, 2023 · 91b476a · 91b476a
1 parent dcc13ff
commit 91b476a
Show file tree

Hide file tree

Showing 6 changed files with 21 additions and 160 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/crates/libcontainer/Cargo.toml b/crates/libcontainer/Cargo.toml
@@ -45,6 +45,7 @@ clone3 = "0.2.3"
 regex = "1.7.3"
 thiserror = "1.0.24"
 tracing = { version = "0.1.37", features = ["attributes"]}
+safe-path = "0.1.0"
 
 [dev-dependencies]
 oci-spec = { version = "^0.6.0", features = ["proptests", "runtime"] }

diff --git a/crates/libcontainer/src/rootfs/device.rs b/crates/libcontainer/src/rootfs/device.rs
@@ -1,6 +1,6 @@
 use super::utils::to_sflag;
 use crate::syscall::{syscall::create_syscall, Syscall};
-use crate::utils::{self, PathBufExt};
+use crate::utils::PathBufExt;
 use anyhow::{bail, Context, Result};
 use nix::{
     fcntl::{open, OFlag},
@@ -109,7 +109,7 @@ fn create_container_dev_path(rootfs: &Path, dev: &LinuxDevice) -> Result<PathBuf
         .path()
         .as_relative()
         .with_context(|| format!("could not convert {:?} to relative path", dev.path()))?;
-    let full_container_path = utils::secure_join(rootfs, relative_dev_path)
+    let full_container_path = safe_path::scoped_join(rootfs, relative_dev_path)
         .with_context(|| format!("could not join {:?} with {:?}", rootfs, dev.path()))?;
 
     crate::utils::create_dir_all(

diff --git a/crates/libcontainer/src/rootfs/mount.rs b/crates/libcontainer/src/rootfs/mount.rs
@@ -3,7 +3,6 @@ use super::symlink::Symlink;
 use super::utils::{find_parent_mount, parse_mount, MountOptionConfig};
 use crate::{
     syscall::{linux, syscall::create_syscall, Syscall, SyscallError},
-    utils,
     utils::PathBufExt,
 };
 #[cfg(feature = "v2")]
@@ -15,6 +14,7 @@ use libcgroups::common::DEFAULT_CGROUP_ROOT;
 use nix::{dir::Dir, errno::Errno, fcntl::OFlag, mount::MsFlags, sys::stat::Mode};
 use oci_spec::runtime::{Mount as SpecMount, MountBuilder as SpecMountBuilder};
 use procfs::process::{MountInfo, MountOptFields, Process};
+use safe_path;
 use std::fs::{canonicalize, create_dir_all, OpenOptions};
 use std::mem;
 use std::os::unix::io::AsRawFd;
@@ -380,7 +380,7 @@ impl Mount {
             }
         }
 
-        let dest_for_host = utils::secure_join(rootfs, m.destination())
+        let dest_for_host = safe_path::scoped_join(rootfs, m.destination())
             .with_context(|| format!("failed to join {:?} with {:?}", rootfs, m.destination()))?;
 
         let dest = Path::new(&dest_for_host);

diff --git a/crates/libcontainer/src/utils.rs b/crates/libcontainer/src/utils.rs
@@ -1,18 +1,18 @@
 //! Utility functionality
 
-use nix::sys::stat::Mode;
-use nix::sys::statfs;
-use nix::unistd;
-use nix::unistd::{Uid, User};
 use std::collections::HashMap;
 use std::ffi::CString;
 use std::fs::{self, DirBuilder, File};
-use std::io::ErrorKind;
 use std::os::linux::fs::MetadataExt;
 use std::os::unix::fs::DirBuilderExt;
 use std::os::unix::prelude::{AsRawFd, OsStrExt};
 use std::path::{Component, Path, PathBuf};
 
+use nix::sys::stat::Mode;
+use nix::sys::statfs;
+use nix::unistd;
+use nix::unistd::{Uid, User};
+
 #[derive(Debug, thiserror::Error)]
 pub enum PathBufExtError {
     #[error("relative path cannot be converted to the path in the container")]
@@ -318,85 +318,6 @@ pub fn ensure_procfs(path: &Path) -> Result<(), EnsureProcfsError> {
     Ok(())
 }
 
-#[derive(Debug, thiserror::Error)]
-pub enum SecureJoinError {
-    #[error("dereference too many symlinks, may be infinite loop")]
-    TooManySymlinks,
-    #[error("unable to obtain symlink metadata")]
-    SymlinkMetadata {
-        source: std::io::Error,
-        path: PathBuf,
-    },
-    #[error("failed to read link")]
-    ReadLink {
-        source: std::io::Error,
-        path: PathBuf,
-    },
-}
-
-pub fn secure_join<P: Into<PathBuf>>(
-    rootfs: P,
-    unsafe_path: P,
-) -> Result<PathBuf, SecureJoinError> {
-    let mut rootfs = rootfs.into();
-    let mut path = unsafe_path.into();
-    let mut clean_path = PathBuf::new();
-
-    let mut part = path.iter();
-    let mut i = 0;
-
-    loop {
-        if i > 255 {
-            return Err(SecureJoinError::TooManySymlinks);
-        }
-
-        let part_path = match part.next() {
-            None => break,
-            Some(part) => PathBuf::from(part),
-        };
-
-        if !part_path.is_absolute() {
-            if part_path.starts_with("..") {
-                clean_path.pop();
-            } else {
-                // check if symlink then dereference
-                let curr_path = PathBuf::from(&rootfs).join(&clean_path).join(&part_path);
-                let metadata = match curr_path.symlink_metadata() {
-                    Ok(metadata) => Some(metadata),
-                    Err(err) if err.kind() == ErrorKind::NotFound => None,
-                    Err(err) => {
-                        return Err(SecureJoinError::SymlinkMetadata {
-                            source: err,
-                            path: curr_path,
-                        })
-                    }
-                };
-
-                if let Some(metadata) = metadata {
-                    if metadata.file_type().is_symlink() {
-                        let link_path =
-                            fs::read_link(&curr_path).map_err(|err| SecureJoinError::ReadLink {
-                                source: err,
-                                path: curr_path.to_owned(),
-                            })?;
-                        path = link_path.join(part.as_path());
-                        part = path.iter();
-
-                        // increase after dereference symlink
-                        i += 1;
-                        continue;
-                    }
-                }
-
-                clean_path.push(&part_path);
-            }
-        }
-    }
-
-    rootfs.push(clean_path);
-    Ok(rootfs)
-}
-
 pub fn get_executable_path(name: &str, path_var: &str) -> Option<PathBuf> {
     let paths = path_var.trim_start_matches("PATH=");
     // if path has / in it, we have to assume absolute path, as per runc impl
@@ -512,6 +433,7 @@ mod tests {
             PathBuf::from("/youki")
         );
     }
+
     #[test]
     fn test_parse_env() -> Result<()> {
         let key = "key".to_string();
@@ -527,77 +449,6 @@ mod tests {
 
         Ok(())
     }
-    #[test]
-    fn test_secure_join() {
-        assert_eq!(
-            secure_join(Path::new("/tmp/rootfs"), Path::new("path")).unwrap(),
-            PathBuf::from("/tmp/rootfs/path")
-        );
-        assert_eq!(
-            secure_join(Path::new("/tmp/rootfs"), Path::new("more/path")).unwrap(),
-            PathBuf::from("/tmp/rootfs/more/path")
-        );
-        assert_eq!(
-            secure_join(Path::new("/tmp/rootfs"), Path::new("/absolute/path")).unwrap(),
-            PathBuf::from("/tmp/rootfs/absolute/path")
-        );
-        assert_eq!(
-            secure_join(
-                Path::new("/tmp/rootfs"),
-                Path::new("/path/with/../parent/./sample")
-            )
-            .unwrap(),
-            PathBuf::from("/tmp/rootfs/path/parent/sample")
-        );
-        assert_eq!(
-            secure_join(Path::new("/tmp/rootfs"), Path::new("/../../../../tmp")).unwrap(),
-            PathBuf::from("/tmp/rootfs/tmp")
-        );
-        assert_eq!(
-            secure_join(Path::new("/tmp/rootfs"), Path::new("./../../../../var/log")).unwrap(),
-            PathBuf::from("/tmp/rootfs/var/log")
-        );
-        assert_eq!(
-            secure_join(
-                Path::new("/tmp/rootfs"),
-                Path::new("../../../../etc/passwd")
-            )
-            .unwrap(),
-            PathBuf::from("/tmp/rootfs/etc/passwd")
-        );
-    }
-    #[test]
-    fn test_secure_join_symlink() {
-        use std::os::unix::fs::symlink;
-
-        let tmp = tempfile::tempdir().expect("create temp directory for test");
-        let test_root_dir = tmp.path();
-
-        symlink("somepath", PathBuf::from(&test_root_dir).join("etc")).unwrap();
-        symlink(
-            "../../../../../../../../../../../../../etc",
-            PathBuf::from(&test_root_dir).join("longbacklink"),
-        )
-        .unwrap();
-        symlink(
-            "/../../../../../../../../../../../../../etc/passwd",
-            PathBuf::from(&test_root_dir).join("absolutelink"),
-        )
-        .unwrap();
-
-        assert_eq!(
-            secure_join(test_root_dir, PathBuf::from("etc").as_path()).unwrap(),
-            PathBuf::from(&test_root_dir).join("somepath")
-        );
-        assert_eq!(
-            secure_join(test_root_dir, PathBuf::from("longbacklink").as_path()).unwrap(),
-            PathBuf::from(&test_root_dir).join("somepath")
-        );
-        assert_eq!(
-            secure_join(test_root_dir, PathBuf::from("absolutelink").as_path()).unwrap(),
-            PathBuf::from(&test_root_dir).join("somepath/passwd")
-        );
-    }
 
     #[test]
     fn test_get_executable_path() {

diff --git a/tests/k8s/deploy.yaml b/tests/k8s/deploy.yaml
@@ -24,4 +24,3 @@ spec:
         image: nginx:1.16.1
         ports:
         - containerPort: 80
-      automountServiceAccountToken: false