From bdcd745cf5694f5a96c81d0f16200cbd4b7780ce Mon Sep 17 00:00:00 2001 From: Shivansh Vij Date: Thu, 30 Apr 2026 20:47:21 +0000 Subject: [PATCH] fix: drop unsafe host env vars when envp == NULL Signed-off-by: Shivansh Vij --- include/libkrun.h | 12 +- src/libkrun/src/lib.rs | 138 ++++++++++++++++++-- tests/runner/src/main.rs | 2 +- tests/test_cases/src/common.rs | 17 ++- tests/test_cases/src/lib.rs | 4 + tests/test_cases/src/test_exec_null_envp.rs | 37 ++++++ 6 files changed, 193 insertions(+), 17 deletions(-) create mode 100644 tests/test_cases/src/test_exec_null_envp.rs diff --git a/include/libkrun.h b/include/libkrun.h index b8f8008a5..7f5d96125 100644 --- a/include/libkrun.h +++ b/include/libkrun.h @@ -794,8 +794,10 @@ int32_t krun_set_workdir(uint32_t ctx_id, * "exec_path" - the path to the executable, relative to the root configured with "krun_set_root". * "argv" - an array of string pointers to be passed as arguments. * "envp" - an array of string pointers to be injected as environment variables into the - * context of the executable. If NULL, it will auto-generate an array collecting the - * the variables currently present in the environment. + * context of the executable. If NULL, it inherits the host process's + * environment on a best-effort basis, where variables whose key or value contains + * whitespace, control characters, or non-ASCII bytes are skipped, and the + * inherited set is truncated to fit the kernel command line. * * Returns: * Zero on success or a negative error number on failure. @@ -849,8 +851,10 @@ int32_t krun_set_kernel(uint32_t ctx_id, * Arguments: * "ctx_id" - the configuration context ID. * "envp" - an array of string pointers to be injected as environment variables into the - * context of the executable. If NULL, it will auto-generate an array collecting the - * the variables currently present in the environment. + * context of the executable. If NULL, it inherits the host process's + * environment on a best-effort basis, where variables whose key or value contains + * whitespace, control characters, or non-ASCII bytes are skipped, and the + * inherited set is truncated to fit the kernel command line. * * Returns: * Zero on success or a negative error number on failure. diff --git a/src/libkrun/src/lib.rs b/src/libkrun/src/lib.rs index 8acf6d205..32b2c4e3a 100644 --- a/src/libkrun/src/lib.rs +++ b/src/libkrun/src/lib.rs @@ -1303,6 +1303,45 @@ pub unsafe extern "C" fn krun_set_workdir(ctx_id: u32, c_workdir_path: *const c_ KRUN_SUCCESS } +/// Soft cap on the size of the env section inherited from the host process when +/// the caller passes `envp = NULL` to `krun_set_exec`/`krun_set_env`. Linux +/// genreally accepts only the first 2048 bytes so setting this value to 1024 leaves +/// room for default options (KRUN_INIT, KRUN_RLIMITS, etc.). +const MAX_INHERITED_ENV_BYTES: usize = 1024; + +/// Best-effort inheritance of the host process's environment for the guest's +/// kernel cmdline. Vars whose key or value contain anything outside printable +/// ASCII (excluding space) are dropped, since the Linux kernel cmdline parser +/// tokenizes at whitespace and `Cmdline::valid_str` rejects non-printable +/// chars. Each var is also dropped individually if appending it would push +/// the total over `MAX_INHERITED_ENV_BYTES`, so a single oversized var (e.g. +/// LS_COLORS) doesn't take out smaller vars iterated after it. +fn inherit_host_env() -> String { + serialize_env(env::vars()) +} + +fn serialize_env>(vars: I) -> String { + let mut buf = String::new(); + for (k, v) in vars { + if k.is_empty() + || k.contains('=') + || !k.chars().all(|c| c.is_ascii_graphic()) + { + continue; + } + if !v.chars().all(|c| c.is_ascii_graphic()) { + continue; + } + + let entry = format!(" {k}={v}"); + if buf.len() + entry.len() > MAX_INHERITED_ENV_BYTES { + continue; + } + buf.push_str(&entry); + } + buf +} + unsafe fn collapse_str_array(array: &[*const c_char]) -> Result { let mut strvec = Vec::new(); @@ -1318,7 +1357,6 @@ unsafe fn collapse_str_array(array: &[*const c_char]) -> Result i32 { @@ -1390,9 +1425,7 @@ pub unsafe extern "C" fn krun_set_env(ctx_id: u32, c_envp: *const *const c_char) } } } else { - env::vars() - .map(|(key, value)| format!(" {key}=\"{value}\"")) - .collect() + inherit_host_env() }; match CTX_MAP.lock().unwrap().entry(ctx_id) { @@ -2786,3 +2819,92 @@ fn krun_start_enter_nitro(ctx_id: u32) -> i32 { } } } + +#[cfg(test)] +mod tests { + use super::*; + + fn vars(pairs: &[(&str, &str)]) -> Vec<(String, String)> { + pairs.iter().map(|(k, v)| (k.to_string(), v.to_string())).collect() + } + + #[test] + fn serialize_env_passes_clean_vars() { + let out = serialize_env(vars(&[("HOME", "/root"), ("PATH", "/usr/bin")])); + assert_eq!(out, " HOME=/root PATH=/usr/bin"); + } + + #[test] + fn serialize_env_drops_value_with_space() { + let out = serialize_env(vars(&[("OLDPWD", "/some path"), ("HOME", "/root")])); + assert_eq!(out, " HOME=/root"); + } + + #[test] + fn serialize_env_drops_key_with_space() { + let out = serialize_env(vars(&[("BAD KEY", "v"), ("HOME", "/root")])); + assert_eq!(out, " HOME=/root"); + } + + #[test] + fn serialize_env_drops_key_containing_equals() { + let out = serialize_env(vars(&[("K=Y", "v"), ("HOME", "/root")])); + assert_eq!(out, " HOME=/root"); + } + + #[test] + fn serialize_env_drops_non_ascii() { + let out = serialize_env(vars(&[("LANG", "en_US.üTF8"), ("HOME", "/root")])); + assert_eq!(out, " HOME=/root"); + } + + #[test] + fn serialize_env_drops_control_chars() { + let out = serialize_env(vars(&[("X", "a\tb"), ("HOME", "/root")])); + assert_eq!(out, " HOME=/root"); + } + + #[test] + fn serialize_env_truncates_at_pair_boundary() { + // Build enough fixed-shape entries to exceed MAX_INHERITED_ENV_BYTES. + // Each entry is " K{i:08}=" (10 chars) + 100-char value = 110 bytes. + const KEY_WIDTH: usize = 8; + const VAL_WIDTH: usize = 100; + let entry_bytes = 1 + 1 + KEY_WIDTH + 1 + VAL_WIDTH; + let n = MAX_INHERITED_ENV_BYTES / entry_bytes + 2; + let value = "v".repeat(VAL_WIDTH); + let pairs: Vec<(String, String)> = (0..n) + .map(|i| (format!("K{i:0width$}", width = KEY_WIDTH), value.clone())) + .collect(); + let out = serialize_env(pairs); + assert!(out.len() <= MAX_INHERITED_ENV_BYTES); + assert!( + out.len() > MAX_INHERITED_ENV_BYTES - entry_bytes, + "expected near-full buffer, got {} of {}", + out.len(), + MAX_INHERITED_ENV_BYTES + ); + // Every entry must be a complete KEY=VALUE pair — no half-written keys + // or values from a mid-pair truncation. + for entry in out.split(' ').filter(|s| !s.is_empty()) { + let mut parts = entry.splitn(2, '='); + let k = parts.next().unwrap(); + let v = parts.next().expect("entry must contain '='"); + assert_eq!(k.len(), 1 + KEY_WIDTH, "key truncated: {k:?}"); + assert_eq!(v.len(), VAL_WIDTH, "value truncated: {v:?}"); + } + } + + #[test] + fn serialize_env_skips_oversize_but_keeps_smaller_following_vars() { + // An oversize var must be dropped individually — not stop processing + // — so smaller vars that come after it still land in the output. + let oversize_value = "v".repeat(MAX_INHERITED_ENV_BYTES + 1); + let pairs = vec![ + ("BIG".to_string(), oversize_value), + ("HOME".to_string(), "/root".to_string()), + ]; + let out = serialize_env(pairs); + assert_eq!(out, " HOME=/root"); + } +} diff --git a/tests/runner/src/main.rs b/tests/runner/src/main.rs index f45611f3e..9a4a122df 100644 --- a/tests/runner/src/main.rs +++ b/tests/runner/src/main.rs @@ -375,7 +375,7 @@ fn run_tests( .count(); let num_fail = results .iter() - .filter(|r| matches!(r.outcome, TestOutcome::Fail(_))) + .filter(|r| matches!(r.outcome, TestOutcome::Fail(_) | TestOutcome::Timeout)) .count(); let num_skip = results .iter() diff --git a/tests/test_cases/src/common.rs b/tests/test_cases/src/common.rs index ff0c155b3..8300cd810 100644 --- a/tests/test_cases/src/common.rs +++ b/tests/test_cases/src/common.rs @@ -1,7 +1,7 @@ //! Common utilities used by multiple test use anyhow::Context; -use std::ffi::CString; +use std::ffi::{c_char, CString}; use std::fs; use std::fs::create_dir; use std::os::unix::ffi::OsStrExt; @@ -35,6 +35,17 @@ pub fn setup_rootfs(test_setup: &TestSetup) -> anyhow::Result { /// Sets up the root filesystem, copies the guest agent into it, and enters the VM. pub fn setup_fs_and_enter(ctx: u32, test_setup: TestSetup) -> anyhow::Result<()> { + let envp = [null()]; + setup_fs_and_enter_with_envp(ctx, test_setup, envp.as_ptr()) +} + +/// Like `setup_fs_and_enter`, but takes a raw envp pointer so tests can +/// exercise NULL / non-standard envp values against `krun_set_exec`. +pub fn setup_fs_and_enter_with_envp( + ctx: u32, + test_setup: TestSetup, + envp: *const *const c_char, +) -> anyhow::Result<()> { let root_dir = setup_rootfs(&test_setup)?; let path_str = CString::new(root_dir.as_os_str().as_bytes()).context("CString::new")?; @@ -43,13 +54,11 @@ pub fn setup_fs_and_enter(ctx: u32, test_setup: TestSetup) -> anyhow::Result<()> krun_call!(krun_set_workdir(ctx, c"/".as_ptr()))?; let test_case_cstr = CString::new(test_setup.test_case).context("CString::new")?; let argv = [test_case_cstr.as_ptr(), null()]; - //let envp = [c"RUST_BACKTRACE=1".as_ptr(), null()]; - let envp = [null()]; krun_call!(krun_set_exec( ctx, c"/guest-agent".as_ptr(), argv.as_ptr(), - envp.as_ptr(), + envp, ))?; krun_call!(krun_start_enter(ctx))?; } diff --git a/tests/test_cases/src/lib.rs b/tests/test_cases/src/lib.rs index 992d67a82..84891fe2e 100644 --- a/tests/test_cases/src/lib.rs +++ b/tests/test_cases/src/lib.rs @@ -22,6 +22,9 @@ use test_multiport_console::TestMultiportConsole; mod test_virtiofs_root_ro; use test_virtiofs_root_ro::TestVirtiofsRootRo; +mod test_exec_null_envp; +use test_exec_null_envp::TestExecNullEnvp; + pub enum TestOutcome { Pass, Fail(String), @@ -78,6 +81,7 @@ pub fn test_cases() -> Vec { TestCase::new("net-vmnet-helper", Box::new(TestNet::new_vmnet_helper())), TestCase::new("multiport-console", Box::new(TestMultiportConsole)), TestCase::new("virtiofs-root-ro", Box::new(TestVirtiofsRootRo)), + TestCase::new("exec-null-envp", Box::new(TestExecNullEnvp)), TestCase::new("perf-net-passt-tx", Box::new(TestNetPerf::new_passt_tx())), TestCase::new("perf-net-passt-rx", Box::new(TestNetPerf::new_passt_rx())), TestCase::new("perf-net-tap-tx", Box::new(TestNetPerf::new_tap_tx())), diff --git a/tests/test_cases/src/test_exec_null_envp.rs b/tests/test_cases/src/test_exec_null_envp.rs new file mode 100644 index 000000000..a0303558b --- /dev/null +++ b/tests/test_cases/src/test_exec_null_envp.rs @@ -0,0 +1,37 @@ +use macros::{guest, host}; + +pub struct TestExecNullEnvp; + +#[host] +mod host { + use super::*; + + use crate::common::setup_fs_and_enter_with_envp; + use crate::{krun_call, krun_call_u32}; + use crate::{Test, TestSetup}; + use krun_sys::*; + + impl Test for TestExecNullEnvp { + fn start_vm(self: Box, test_setup: TestSetup) -> anyhow::Result<()> { + unsafe { + krun_call!(krun_set_log_level(KRUN_LOG_LEVEL_TRACE))?; + let ctx = krun_call_u32!(krun_create_ctx())?; + krun_call!(krun_set_vm_config(ctx, 1, 256))?; + setup_fs_and_enter_with_envp(ctx, test_setup, std::ptr::null())?; + } + Ok(()) + } + } +} + +#[guest] +mod guest { + use super::*; + use crate::Test; + + impl Test for TestExecNullEnvp { + fn in_guest(self: Box) { + println!("OK"); + } + } +}