From 9bea3a42a7eb3bbfcb9a0287415ef61d509f7eb7 Mon Sep 17 00:00:00 2001 From: bin Date: Wed, 10 Nov 2021 20:03:26 +0800 Subject: [PATCH 1/4] agent: check environment variables if empty or invalid Invalid environment variable key/value will cause set_env panic. Refer: https://doc.rust-lang.org/std/env/fn.set_var.html#panics Fixes: #3006 Signed-off-by: bin --- src/agent/rustjail/src/container.rs | 76 +++++++++++++++++++++++++++-- 1 file changed, 72 insertions(+), 4 deletions(-) diff --git a/src/agent/rustjail/src/container.rs b/src/agent/rustjail/src/container.rs index d716c5344d..9eb27d4fb9 100644 --- a/src/agent/rustjail/src/container.rs +++ b/src/agent/rustjail/src/container.rs @@ -636,11 +636,10 @@ fn do_init_child(cwfd: RawFd) -> Result<()> { // setup the envs for e in env.iter() { - let v: Vec<&str> = e.splitn(2, '=').collect(); - if v.len() != 2 { - continue; + match valid_env(e) { + Some((key, value)) => env::set_var(key, value), + None => log_child!(cfd_log, "invalid env key-value: {:?}", e), } - env::set_var(v[0], v[1]); } // set the "HOME" env getting from "/etc/passwd", if @@ -1565,6 +1564,30 @@ async fn execute_hook(logger: &Logger, h: &Hook, st: &OCIState) -> Result<()> { } } +// valid environment variables according to https://doc.rust-lang.org/std/env/fn.set_var.html#panics +fn valid_env(e: &str) -> Option<(&str, &str)> { + // wherther key or value will contain NULL char. + if e.as_bytes().contains(&b'\0') { + return None; + } + + let v: Vec<&str> = e.splitn(2, '=').collect(); + + // key can't hold an `equal` sign, but value can + if v.len() != 2 { + return None; + } + + let (key, value) = (v[0].trim(), v[1].trim()); + + // key can't be empty + if key.is_empty() { + return None; + } + + Some((key, value)) +} + #[cfg(test)] mod tests { use super::*; @@ -1988,4 +2011,49 @@ mod tests { let ret = do_init_child(std::io::stdin().as_raw_fd()); assert!(ret.is_err(), "Expecting Err, Got {:?}", ret); } + + #[test] + fn test_valid_env() { + let env = valid_env("a=b=c"); + assert_eq!(Some(("a", "b=c")), env); + + let env = valid_env("a=b"); + assert_eq!(Some(("a", "b")), env); + let env = valid_env("a =b"); + assert_eq!(Some(("a", "b")), env); + + let env = valid_env(" a =b"); + assert_eq!(Some(("a", "b")), env); + + let env = valid_env("a= b"); + assert_eq!(Some(("a", "b")), env); + + let env = valid_env("a=b "); + assert_eq!(Some(("a", "b")), env); + let env = valid_env("a=b c "); + assert_eq!(Some(("a", "b c")), env); + + let env = valid_env("=b"); + assert_eq!(None, env); + + let env = valid_env("a="); + assert_eq!(Some(("a", "")), env); + + let env = valid_env("a=="); + assert_eq!(Some(("a", "=")), env); + + let env = valid_env("a"); + assert_eq!(None, env); + + let invalid_str = vec![97, b'\0', 98]; + let invalid_string = std::str::from_utf8(&invalid_str).unwrap(); + + let invalid_env = format!("{}=value", invalid_string); + let env = valid_env(&invalid_env); + assert_eq!(None, env); + + let invalid_env = format!("key={}", invalid_string); + let env = valid_env(&invalid_env); + assert_eq!(None, env); + } } From 9c1953641bad5e0ae2af2d1dad921eb9b9a1900f Mon Sep 17 00:00:00 2001 From: Binbin Zhang Date: Fri, 24 Dec 2021 15:58:53 +0800 Subject: [PATCH 2/4] agent: Fix execute_hook() args error 1. The hook.args[0] is the hook binary name which shouldn't be included in the Command.args. 2. Add new unit tests Fixes: #2610 Signed-off-by: Binbin Zhang Signed-off-by: bin --- src/agent/rustjail/src/container.rs | 72 +++++++++++++++++++++++++---- 1 file changed, 63 insertions(+), 9 deletions(-) diff --git a/src/agent/rustjail/src/container.rs b/src/agent/rustjail/src/container.rs index 9eb27d4fb9..e7f360a4f6 100644 --- a/src/agent/rustjail/src/container.rs +++ b/src/agent/rustjail/src/container.rs @@ -1478,7 +1478,12 @@ async fn execute_hook(logger: &Logger, h: &Hook, st: &OCIState) -> Result<()> { return Err(anyhow!(nix::Error::from_errno(Errno::EINVAL))); } - let args = h.args.clone(); + let mut args = h.args.clone(); + // the hook.args[0] is the hook binary name which shouldn't be included + // in the Command.args + if args.len() > 1 { + args.remove(0); + } let env: HashMap = h .env .iter() @@ -1525,7 +1530,7 @@ async fn execute_hook(logger: &Logger, h: &Hook, st: &OCIState) -> Result<()> { // Close stdin so that hook program could receive EOF child.stdin.take(); - // read something from stdout for debug + // read something from stdout and stderr for debug let mut out = String::new(); child .stdout @@ -1536,6 +1541,16 @@ async fn execute_hook(logger: &Logger, h: &Hook, st: &OCIState) -> Result<()> { .unwrap(); info!(logger, "child stdout: {}", out.as_str()); + let mut err = String::new(); + child + .stderr + .as_mut() + .unwrap() + .read_to_string(&mut err) + .await + .unwrap(); + info!(logger, "child stderr: {}", err.as_str()); + match child.wait().await { Ok(exit) => { let code = exit @@ -1543,7 +1558,10 @@ async fn execute_hook(logger: &Logger, h: &Hook, st: &OCIState) -> Result<()> { .ok_or_else(|| anyhow!("hook exit status has no status code"))?; if code != 0 { - error!(logger, "hook {} exit status is {}", &path, code); + error!( + logger, + "hook {} exit status is {}, error message is {}", &path, code, err + ); return Err(anyhow!(nix::Error::from_errno(Errno::UnknownErrno))); } @@ -1620,13 +1638,44 @@ mod tests { #[tokio::test] async fn test_execute_hook() { - let xargs = which("xargs").await; + let temp_file = "/tmp/test_execute_hook"; + + let touch = which("touch").await; + + defer!(fs::remove_file(temp_file).unwrap();); execute_hook( &slog_scope::logger(), &Hook { - path: xargs, - args: vec![], + path: touch, + args: vec!["touch".to_string(), temp_file.to_string()], + env: vec![], + timeout: Some(10), + }, + &OCIState { + version: "1.2.3".to_string(), + id: "321".to_string(), + status: ContainerState::Running, + pid: 2, + bundle: "".to_string(), + annotations: Default::default(), + }, + ) + .await + .unwrap(); + + assert_eq!(Path::new(&temp_file).exists(), true); + } + + #[tokio::test] + async fn test_execute_hook_with_error() { + let ls = which("ls").await; + + let res = execute_hook( + &slog_scope::logger(), + &Hook { + path: ls, + args: vec!["ls".to_string(), "/tmp/not-exist".to_string()], env: vec![], timeout: None, }, @@ -1639,8 +1688,13 @@ mod tests { annotations: Default::default(), }, ) - .await - .unwrap() + .await; + + let expected_err = nix::Error::from_errno(Errno::UnknownErrno); + assert_eq!( + res.unwrap_err().downcast::().unwrap(), + expected_err + ); } #[tokio::test] @@ -1651,7 +1705,7 @@ mod tests { &slog_scope::logger(), &Hook { path: sleep, - args: vec!["2".to_string()], + args: vec!["sleep".to_string(), "2".to_string()], env: vec![], timeout: Some(1), }, From 9b34cf46da5f86fdc4fb64fcf7a6d6bc33821a14 Mon Sep 17 00:00:00 2001 From: bin Date: Tue, 15 Feb 2022 11:51:23 +0800 Subject: [PATCH 3/4] agent: valid envs for hooks Envs contain null-byte will cause running hooks to panic, this commit will filter envs and only pass valid envs to hooks. Fixes: #3667 Signed-off-by: bin --- src/agent/rustjail/src/container.rs | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/src/agent/rustjail/src/container.rs b/src/agent/rustjail/src/container.rs index e7f360a4f6..10b31dfef1 100644 --- a/src/agent/rustjail/src/container.rs +++ b/src/agent/rustjail/src/container.rs @@ -1484,14 +1484,9 @@ async fn execute_hook(logger: &Logger, h: &Hook, st: &OCIState) -> Result<()> { if args.len() > 1 { args.remove(0); } - let env: HashMap = h - .env - .iter() - .map(|e| { - let v: Vec<&str> = e.split('=').collect(); - (v[0].to_string(), v[1].to_string()) - }) - .collect(); + + // all invalid envs will be ommit, only valid envs will be passed to hook. + let env: HashMap<&str, &str> = h.env.iter().filter_map(|e| valid_env(e)).collect(); // Avoid the exit signal to be reaped by the global reaper. let _wait_locker = WAIT_PID_LOCKER.lock().await; @@ -1502,8 +1497,7 @@ async fn execute_hook(logger: &Logger, h: &Hook, st: &OCIState) -> Result<()> { .stdin(Stdio::piped()) .stdout(Stdio::piped()) .stderr(Stdio::piped()) - .spawn() - .unwrap(); + .spawn()?; // default timeout 10s let mut timeout: u64 = 10; @@ -1643,13 +1637,16 @@ mod tests { let touch = which("touch").await; defer!(fs::remove_file(temp_file).unwrap();); + let invalid_str = vec![97, b'\0', 98]; + let invalid_string = std::str::from_utf8(&invalid_str).unwrap(); + let invalid_env = format!("{}=value", invalid_string); execute_hook( &slog_scope::logger(), &Hook { path: touch, args: vec!["touch".to_string(), temp_file.to_string()], - env: vec![], + env: vec![invalid_env], timeout: Some(10), }, &OCIState { From 7af719e47c90bbb0531ffb0016c38f67af54e07c Mon Sep 17 00:00:00 2001 From: bin Date: Tue, 15 Feb 2022 13:27:14 +0800 Subject: [PATCH 4/4] agent: handle hook process result Current hook process is handled by just calling unwrap() on it, sometime it will cause panic. By handling all Result type and check the error can avoid panic. Fixes: #3649 Signed-off-by: bin --- src/agent/rustjail/src/container.rs | 58 +++++++++++++++-------------- 1 file changed, 30 insertions(+), 28 deletions(-) diff --git a/src/agent/rustjail/src/container.rs b/src/agent/rustjail/src/container.rs index 10b31dfef1..c3df3c1c2c 100644 --- a/src/agent/rustjail/src/container.rs +++ b/src/agent/rustjail/src/container.rs @@ -1485,7 +1485,7 @@ async fn execute_hook(logger: &Logger, h: &Hook, st: &OCIState) -> Result<()> { args.remove(0); } - // all invalid envs will be ommit, only valid envs will be passed to hook. + // all invalid envs will be omitted, only valid envs will be passed to hook. let env: HashMap<&str, &str> = h.env.iter().filter_map(|e| valid_env(e)).collect(); // Avoid the exit signal to be reaped by the global reaper. @@ -1513,37 +1513,39 @@ async fn execute_hook(logger: &Logger, h: &Hook, st: &OCIState) -> Result<()> { let path = h.path.clone(); let join_handle = tokio::spawn(async move { - child - .stdin - .as_mut() - .unwrap() - .write_all(state.as_bytes()) - .await - .unwrap(); - - // Close stdin so that hook program could receive EOF - child.stdin.take(); + if let Some(mut stdin) = child.stdin.take() { + match stdin.write_all(state.as_bytes()).await { + Ok(_) => {} + Err(e) => { + info!(logger, "write to child stdin failed: {:?}", e); + } + } + } // read something from stdout and stderr for debug - let mut out = String::new(); - child - .stdout - .as_mut() - .unwrap() - .read_to_string(&mut out) - .await - .unwrap(); - info!(logger, "child stdout: {}", out.as_str()); + if let Some(stdout) = child.stdout.as_mut() { + let mut out = String::new(); + match stdout.read_to_string(&mut out).await { + Ok(_) => { + info!(logger, "child stdout: {}", out.as_str()); + } + Err(e) => { + info!(logger, "read from child stdout failed: {:?}", e); + } + } + } let mut err = String::new(); - child - .stderr - .as_mut() - .unwrap() - .read_to_string(&mut err) - .await - .unwrap(); - info!(logger, "child stderr: {}", err.as_str()); + if let Some(stderr) = child.stderr.as_mut() { + match stderr.read_to_string(&mut err).await { + Ok(_) => { + info!(logger, "child stderr: {}", err.as_str()); + } + Err(e) => { + info!(logger, "read from child stderr failed: {:?}", e); + } + } + } match child.wait().await { Ok(exit) => {