From bf26d3f9d7ce2b0fc6b21fdfafb31c7cf4b02bcb Mon Sep 17 00:00:00 2001 From: yihuaf Date: Sun, 13 Aug 2023 12:43:03 -0700 Subject: [PATCH] move the validation logic into executor To allow more flexibility for the executor, we move the validate logic into the executor. The validate runs in the `create` step before workloads are executed. Instead of implementing the validation in the `exec`, to maintain backward competiability, we have to introduce an extra step. The exec is too late to fail if the spec is not validated. Signed-off-by: yihuaf --- .../src/process/container_init_process.rs | 108 +------------- crates/libcontainer/src/workload/default.rs | 132 ++++++++++++++++-- crates/libcontainer/src/workload/mod.rs | 2 + crates/youki/src/workload/executor.rs | 23 +++ crates/youki/src/workload/wasmedge.rs | 8 ++ crates/youki/src/workload/wasmer.rs | 8 ++ crates/youki/src/workload/wasmtime.rs | 8 ++ 7 files changed, 174 insertions(+), 115 deletions(-) diff --git a/crates/libcontainer/src/process/container_init_process.rs b/crates/libcontainer/src/process/container_init_process.rs index 5b6045be51..cf16a6de36 100644 --- a/crates/libcontainer/src/process/container_init_process.rs +++ b/crates/libcontainer/src/process/container_init_process.rs @@ -76,72 +76,6 @@ pub enum InitProcessError { type Result = std::result::Result; -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 -} - -fn is_executable(path: &Path) -> std::result::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) -} - -// 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 -fn verify_binary(args: &[String], envs: &[String]) -> Result<()> { - let path_vars: Vec<&String> = envs.iter().filter(|&e| e.starts_with("PATH=")).collect(); - if path_vars.is_empty() { - tracing::error!("PATH environment variable is not set"); - return Err(InitProcessError::InvalidExecutable(args[0].clone())); - } - let path_var = path_vars[0].trim_start_matches("PATH="); - match get_executable_path(&args[0], path_var) { - None => { - tracing::error!( - "executable {} for container process not found in PATH", - args[0] - ); - return Err(InitProcessError::InvalidExecutable(args[0].clone())); - } - Some(path) => match is_executable(&path) { - Ok(true) => { - tracing::debug!("found executable {:?}", path); - } - Ok(false) => { - tracing::error!( - "executable {:?} does not have the correct permission set", - path - ); - return Err(InitProcessError::InvalidExecutable(args[0].clone())); - } - Err(err) => { - tracing::error!( - "failed to check permissions for executable {:?}: {}", - path, - err - ); - return Err(InitProcessError::Io(err)); - } - }, - } - Ok(()) -} - fn sysctl(kernel_params: &HashMap) -> Result<()> { let sys = PathBuf::from("/proc/sys"); for (kernel_param, value) in kernel_params { @@ -637,9 +571,7 @@ pub fn container_init_process( tracing::warn!("seccomp not available, unable to set seccomp privileges!") } - if let Some(args) = proc.args() { - verify_binary(args, &envs)?; - } + args.executor.validate(spec)?; // Notify main process that the init process is ready to execute the // payload. Note, because we are already inside the pid namespace, the pid @@ -1091,44 +1023,6 @@ mod tests { assert_eq!(0, got.len()); } - #[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 = "/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 tmp = tempfile::tempdir().expect("create temp directory for test"); - let executable_path = PathBuf::from("/bin/sh"); - let directory_path = tmp.path(); - let non_executable_path = directory_path.join("non_executable_file"); - let non_existent_path = PathBuf::from("/some/non/existent/path"); - - std::fs::File::create(non_executable_path.as_path()).unwrap(); - - 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()); - } - #[test] fn test_set_io_priority() { let test_command = TestHelperSyscall::default(); diff --git a/crates/libcontainer/src/workload/default.rs b/crates/libcontainer/src/workload/default.rs index 4f008aa025..c278e832a9 100644 --- a/crates/libcontainer/src/workload/default.rs +++ b/crates/libcontainer/src/workload/default.rs @@ -1,9 +1,12 @@ -use std::ffi::CString; +use std::{ + ffi::CString, + path::{Path, PathBuf}, +}; use nix::unistd; use oci_spec::runtime::Spec; -use super::{Executor, ExecutorError, EMPTY}; +use super::{Executor, ExecutorError}; #[derive(Clone)] pub struct DefaultExecutor {} @@ -15,12 +18,10 @@ impl Executor for DefaultExecutor { .process() .as_ref() .and_then(|p| p.args().as_ref()) - .unwrap_or(&EMPTY); - - if args.is_empty() { - tracing::error!("no arguments provided to execute"); - Err(ExecutorError::InvalidArg)?; - } + .ok_or_else(|| { + tracing::error!("no arguments provided to execute"); + ExecutorError::InvalidArg + })?; let executable = args[0].as_str(); let cstring_path = CString::new(executable.as_bytes()).map_err(|err| { @@ -40,8 +41,123 @@ impl Executor for DefaultExecutor { // payload through execvp, so it should never reach here. unreachable!(); } + + fn validate(&self, spec: &Spec) -> Result<(), ExecutorError> { + let proc = spec + .process() + .as_ref() + .ok_or_else(|| ExecutorError::InvalidArg)?; + + if let Some(args) = proc.args() { + let envs: Vec = proc.env().as_ref().unwrap_or(&vec![]).clone(); + let path_vars: Vec<&String> = envs.iter().filter(|&e| e.starts_with("PATH=")).collect(); + if path_vars.is_empty() { + tracing::error!("PATH environment variable is not set"); + Err(ExecutorError::InvalidArg)?; + } + let path_var = path_vars[0].trim_start_matches("PATH="); + match get_executable_path(&args[0], path_var) { + None => { + tracing::error!( + "executable {} for container process not found in PATH", + args[0] + ); + Err(ExecutorError::InvalidArg)?; + } + Some(path) => match is_executable(&path) { + Ok(true) => { + tracing::debug!("found executable {:?}", path); + } + Ok(false) => { + tracing::error!( + "executable {:?} does not have the correct permission set", + path + ); + Err(ExecutorError::InvalidArg)?; + } + Err(err) => { + tracing::error!( + "failed to check permissions for executable {:?}: {}", + path, + err + ); + Err(ExecutorError::InvalidArg)?; + } + }, + } + } + + Ok(()) + } } pub fn get_executor() -> Box { Box::new(DefaultExecutor {}) } + +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 +} + +fn is_executable(path: &Path) -> std::result::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)] +mod tests { + use super::*; + + #[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 = "/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 tmp = tempfile::tempdir().expect("create temp directory for test"); + let executable_path = PathBuf::from("/bin/sh"); + let directory_path = tmp.path(); + let non_executable_path = directory_path.join("non_executable_file"); + let non_existent_path = PathBuf::from("/some/non/existent/path"); + + std::fs::File::create(non_executable_path.as_path()).unwrap(); + + 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/crates/libcontainer/src/workload/mod.rs b/crates/libcontainer/src/workload/mod.rs index 1d9b83aa25..2e522ed2f7 100644 --- a/crates/libcontainer/src/workload/mod.rs +++ b/crates/libcontainer/src/workload/mod.rs @@ -46,6 +46,8 @@ pub trait CloneBoxExecutor { pub trait Executor: CloneBoxExecutor { /// Executes the workload fn exec(&self, spec: &Spec) -> Result<(), ExecutorError>; + + fn validate(&self, spec: &Spec) -> Result<(), ExecutorError>; } impl CloneBoxExecutor for T diff --git a/crates/youki/src/workload/executor.rs b/crates/youki/src/workload/executor.rs index f68b9c15ed..28505f414e 100644 --- a/crates/youki/src/workload/executor.rs +++ b/crates/youki/src/workload/executor.rs @@ -29,6 +29,29 @@ impl Executor for DefaultExecutor { // container workloads. libcontainer::workload::default::get_executor().exec(spec) } + + fn validate(&self, spec: &Spec) -> Result<(), ExecutorError> { + #[cfg(feature = "wasm-wasmer")] + match super::wasmer::get_executor().validate(spec) { + Ok(_) => return Ok(()), + Err(ExecutorError::CantHandle(_)) => (), + Err(err) => return Err(err), + } + #[cfg(feature = "wasm-wasmedge")] + match super::wasmedge::get_executor().validate(spec) { + Ok(_) => return Ok(()), + Err(ExecutorError::CantHandle(_)) => (), + Err(err) => return Err(err), + } + #[cfg(feature = "wasm-wasmtime")] + match super::wasmtime::get_executor().validate(spec) { + Ok(_) => return Ok(()), + Err(ExecutorError::CantHandle(_)) => (), + Err(err) => return Err(err), + } + + libcontainer::workload::default::get_executor().validate(spec) + } } pub fn default_executor() -> DefaultExecutor { diff --git a/crates/youki/src/workload/wasmedge.rs b/crates/youki/src/workload/wasmedge.rs index 7ac6ef6c49..473d4e8492 100644 --- a/crates/youki/src/workload/wasmedge.rs +++ b/crates/youki/src/workload/wasmedge.rs @@ -62,6 +62,14 @@ impl Executor for WasmedgeExecutor { Ok(()) } + + fn validate(&self, spec: &Spec) -> Result<(), ExecutorError> { + if !can_handle(spec) { + return Err(ExecutorError::CantHandle(EXECUTOR_NAME)); + } + + Ok(()) + } } pub fn get_executor() -> WasmedgeExecutor { diff --git a/crates/youki/src/workload/wasmer.rs b/crates/youki/src/workload/wasmer.rs index 95d98c6ddc..f0415403f1 100644 --- a/crates/youki/src/workload/wasmer.rs +++ b/crates/youki/src/workload/wasmer.rs @@ -80,6 +80,14 @@ impl Executor for WasmerExecutor { Ok(()) } + + fn validate(&self, spec: &Spec) -> Result<(), ExecutorError> { + if !can_handle(spec) { + return Err(ExecutorError::CantHandle(EXECUTOR_NAME)); + } + + Ok(()) + } } pub fn get_executor() -> WasmerExecutor { diff --git a/crates/youki/src/workload/wasmtime.rs b/crates/youki/src/workload/wasmtime.rs index 58f977b21a..9402bb47de 100644 --- a/crates/youki/src/workload/wasmtime.rs +++ b/crates/youki/src/workload/wasmtime.rs @@ -90,6 +90,14 @@ impl Executor for WasmtimeExecutor { .call(&mut store, &[], &mut []) .map_err(|err| ExecutorError::Execution(err.into())) } + + fn validate(&self, spec: &Spec) -> Result<(), ExecutorError> { + if !can_handle(spec) { + return Err(ExecutorError::CantHandle(EXECUTOR_NAME)); + } + + Ok(()) + } } pub fn get_executor() -> WasmtimeExecutor {