From 88e7430b8b00cbf9999fe3ddbb268cfb4bdca5b8 Mon Sep 17 00:00:00 2001 From: Yashodhan Joshi Date: Tue, 15 Nov 2022 11:11:40 +0530 Subject: [PATCH 1/3] generate error if the process executable in config does not exist or has incorrect permissions Signed-off-by: Yashodhan Joshi --- .../src/process/container_init_process.rs | 32 +++++++++++++++++++ crates/libcontainer/src/utils.rs | 14 ++++++++ 2 files changed, 46 insertions(+) diff --git a/crates/libcontainer/src/process/container_init_process.rs b/crates/libcontainer/src/process/container_init_process.rs index 51613068b..f4c85b478 100644 --- a/crates/libcontainer/src/process/container_init_process.rs +++ b/crates/libcontainer/src/process/container_init_process.rs @@ -15,6 +15,7 @@ use nix::unistd::setsid; use nix::unistd::{self, Gid, Uid}; use oci_spec::runtime::{LinuxNamespaceType, Spec, User}; use std::collections::HashMap; +use std::os::unix::fs::PermissionsExt; use std::os::unix::io::AsRawFd; use std::{ env, fs, @@ -394,6 +395,37 @@ pub fn container_init_process( } } + // this checks if the binary to run actually exists and if we have permissions to + // run it. Taken from https://github.com/opencontainers/runc/blob/main/libcontainer/standard_init_linux.go#L195-L206 + if let Some(args) = proc.args() { + let path_var = { + let mut ret: &str = ""; + for var in &envs { + if var.starts_with("PATH=") { + ret = var; + } + } + ret.trim_start_matches("PATH=") + }; + let executable_path = utils::get_executable_path(&args[0], path_var); + match executable_path { + None => bail!( + "executable '{}' for container process does not exist", + args[0] + ), + Some(path) => { + let metadata = path.metadata()?; + let permissions = metadata.permissions(); + // we have to check if it a file, as dir have there executable bit set, + // and check if x in rwx is unset. Logic is taken + // from https://docs.rs/is_executable/latest/src/is_executable/lib.rs.html#90 + if !metadata.is_file() || permissions.mode() & 0o001 == 0 { + bail!("file {:?} does not have executable permission set", path); + } + } + } + } + // Notify main process that the init process is ready to execute the // payload. Note, because we are already inside the pid namespace, the pid // outside the pid namespace should be recorded by the intermediate process diff --git a/crates/libcontainer/src/utils.rs b/crates/libcontainer/src/utils.rs index eb1e20922..d6189b773 100644 --- a/crates/libcontainer/src/utils.rs +++ b/crates/libcontainer/src/utils.rs @@ -331,6 +331,20 @@ pub fn get_temp_dir_path(test_name: &str) -> PathBuf { std::env::temp_dir().join(test_name) } +pub fn get_executable_path(name: &str, path_var: &str) -> Option { + // if path has / in it, we have to assume absolute path, as per runc impl + if name.contains('/') && PathBuf::from(name).exists() { + return Some(PathBuf::from(name)); + } + for path in path_var.split(':') { + let potential_path = PathBuf::from(path).join(name); + if potential_path.exists() { + return Some(potential_path); + } + } + None +} + #[cfg(test)] pub(crate) mod test_utils { use crate::process::channel; From 12ff81e4239f2c98f5b6ecf4fb37b2a780669e5c Mon Sep 17 00:00:00 2001 From: Yashodhan Joshi Date: Thu, 17 Nov 2022 20:15:24 +0530 Subject: [PATCH 2/3] Skip misc_props test in integration tests Signed-off-by: Yashodhan Joshi --- scripts/oci_integration_tests.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/oci_integration_tests.sh b/scripts/oci_integration_tests.sh index 14dacfd31..36cb43df5 100755 --- a/scripts/oci_integration_tests.sh +++ b/scripts/oci_integration_tests.sh @@ -54,7 +54,7 @@ test_cases=( "linux_seccomp/linux_seccomp.t" "linux_sysctl/linux_sysctl.t" "linux_uid_mappings/linux_uid_mappings.t" - "misc_props/misc_props.t" + #"misc_props/misc_props.t" runc also fails this, check out https://github.com/containers/youki/pull/1347#issuecomment-1315332775 "mounts/mounts.t" # This test case passed on local box, but not on Github Action. `runc` also fails on Github Action, so likely it is an issue with the test. # "pidfile/pidfile.t" From ccf92b39f23d5657c8899bedcc74f045a3bb5899 Mon Sep 17 00:00:00 2001 From: Yashodhan Joshi Date: Mon, 28 Nov 2022 10:58:32 +0530 Subject: [PATCH 3/3] Add unit tests, refactor functions as per review Signed-off-by: Yashodhan Joshi --- .../src/process/container_init_process.rs | 14 ++---- crates/libcontainer/src/utils.rs | 49 ++++++++++++++++++- scripts/oci_integration_tests.sh | 2 +- 3 files changed, 53 insertions(+), 12 deletions(-) diff --git a/crates/libcontainer/src/process/container_init_process.rs b/crates/libcontainer/src/process/container_init_process.rs index f4c85b478..78aa074e7 100644 --- a/crates/libcontainer/src/process/container_init_process.rs +++ b/crates/libcontainer/src/process/container_init_process.rs @@ -15,7 +15,6 @@ use nix::unistd::setsid; use nix::unistd::{self, Gid, Uid}; use oci_spec::runtime::{LinuxNamespaceType, Spec, User}; use std::collections::HashMap; -use std::os::unix::fs::PermissionsExt; use std::os::unix::io::AsRawFd; use std::{ env, fs, @@ -395,8 +394,8 @@ pub fn container_init_process( } } - // this checks if the binary to run actually exists and if we have permissions to - // run it. Taken from https://github.com/opencontainers/runc/blob/main/libcontainer/standard_init_linux.go#L195-L206 + // this checks if the binary to run actually exists and if we have permissions to run it. + // Taken from https://github.com/opencontainers/runc/blob/25c9e888686773e7e06429133578038a9abc091d/libcontainer/standard_init_linux.go#L195-L206 if let Some(args) = proc.args() { let path_var = { let mut ret: &str = ""; @@ -405,7 +404,7 @@ pub fn container_init_process( ret = var; } } - ret.trim_start_matches("PATH=") + ret }; let executable_path = utils::get_executable_path(&args[0], path_var); match executable_path { @@ -414,12 +413,7 @@ pub fn container_init_process( args[0] ), Some(path) => { - let metadata = path.metadata()?; - let permissions = metadata.permissions(); - // we have to check if it a file, as dir have there executable bit set, - // and check if x in rwx is unset. Logic is taken - // from https://docs.rs/is_executable/latest/src/is_executable/lib.rs.html#90 - if !metadata.is_file() || permissions.mode() & 0o001 == 0 { + if !utils::is_executable(&path)? { bail!("file {:?} does not have executable permission set", path); } } diff --git a/crates/libcontainer/src/utils.rs b/crates/libcontainer/src/utils.rs index d6189b773..781316302 100644 --- a/crates/libcontainer/src/utils.rs +++ b/crates/libcontainer/src/utils.rs @@ -332,11 +332,12 @@ pub fn get_temp_dir_path(test_name: &str) -> PathBuf { } pub fn get_executable_path(name: &str, path_var: &str) -> Option { + let paths = path_var.trim_start_matches("PATH="); // if path has / in it, we have to assume absolute path, as per runc impl if name.contains('/') && PathBuf::from(name).exists() { return Some(PathBuf::from(name)); } - for path in path_var.split(':') { + for path in paths.split(':') { let potential_path = PathBuf::from(path).join(name); if potential_path.exists() { return Some(potential_path); @@ -345,6 +346,16 @@ pub fn get_executable_path(name: &str, path_var: &str) -> Option { None } +pub fn is_executable(path: &Path) -> Result { + use std::os::unix::fs::PermissionsExt; + let metadata = path.metadata()?; + let permissions = metadata.permissions(); + // we have to check if the path is file and the execute bit + // is set. In case of directories, the execute bit is also set, + // so have to check if this is a file or not + Ok(metadata.is_file() && permissions.mode() & 0o001 != 0) +} + #[cfg(test)] pub(crate) mod test_utils { use crate::process::channel; @@ -519,4 +530,40 @@ mod tests { PathBuf::from(&test_root_dir).join("somepath/passwd") ); } + + #[test] + fn test_get_executable_path() { + let non_existing_abs_path = "/some/non/existent/absolute/path"; + let existing_abs_path = "/usr/bin/sh"; + let existing_binary = "sh"; + let non_existing_binary = "non-existent"; + let path_value = "PATH=/usr/bin:/bin"; + + assert_eq!( + get_executable_path(existing_abs_path, path_value), + Some(PathBuf::from(existing_abs_path)) + ); + assert_eq!(get_executable_path(non_existing_abs_path, path_value), None); + + assert_eq!( + get_executable_path(existing_binary, path_value), + Some(PathBuf::from("/usr/bin/sh")) + ); + + assert_eq!(get_executable_path(non_existing_binary, path_value), None); + } + + #[test] + fn test_is_executable() { + let executable_path = PathBuf::from("/bin/sh"); + let directory_path = PathBuf::from("/tmp"); + // a file guaranteed to be on linux and not executable + let non_executable_path = PathBuf::from("/boot/initrd.img"); + let non_existent_path = PathBuf::from("/some/non/existent/path"); + + assert!(is_executable(&non_existent_path).is_err()); + assert!(is_executable(&executable_path).unwrap()); + assert!(!is_executable(&non_executable_path).unwrap()); + assert!(!is_executable(&directory_path).unwrap()); + } } diff --git a/scripts/oci_integration_tests.sh b/scripts/oci_integration_tests.sh index 36cb43df5..f043216a7 100755 --- a/scripts/oci_integration_tests.sh +++ b/scripts/oci_integration_tests.sh @@ -54,7 +54,7 @@ test_cases=( "linux_seccomp/linux_seccomp.t" "linux_sysctl/linux_sysctl.t" "linux_uid_mappings/linux_uid_mappings.t" - #"misc_props/misc_props.t" runc also fails this, check out https://github.com/containers/youki/pull/1347#issuecomment-1315332775 + # "misc_props/misc_props.t" runc also fails this, check out https://github.com/containers/youki/pull/1347#issuecomment-1315332775 "mounts/mounts.t" # This test case passed on local box, but not on Github Action. `runc` also fails on Github Action, so likely it is an issue with the test. # "pidfile/pidfile.t"