Skip to content

fix: drop unsafe host env vars when envp is NULL #661

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ShivanshVij wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: drop unsafe host env vars when envp is NULL #661

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions include/libkrun.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
138 changes: 130 additions & 8 deletions src/libkrun/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines +1307 to +1308
Copy link
Copy Markdown
Collaborator

@dorindabassey dorindabassey May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here it's not Linux that has a 2048 limit - the limit depends on the architecture

Copy link
Copy Markdown
Collaborator

@mtjhrc mtjhrc May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that is misleading, even if we keep a single limit (not platform specific), this comments needs to be updated.

/// room for default options (KRUN_INIT, KRUN_RLIMITS, etc.).
const MAX_INHERITED_ENV_BYTES: usize = 1024;
Copy link
Copy Markdown
Collaborator

@dorindabassey dorindabassey May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

about this constant, cmdline size limits vary for different archs, for example riscv64 max size is 1024 bytes, aarch64 is 2048 and so on, should MAX_INHERITED_ENV_BYTES be architecture-aware? Something like this if you're trying to leave room for default options:

Suggested change
const MAX_INHERITED_ENV_BYTES: usize = 1024;
#[cfg(target_arch = "riscv64")]
const MAX_INHERITED_ENV_BYTES: usize = 256;
#[cfg(target_arch = "aarch64")]
const MAX_INHERITED_ENV_BYTES: usize = 1024;
#[cfg(target_arch = "x86_64")]
const MAX_INHERITED_ENV_BYTES: usize = 8192;

Or is the 1024 limit intentional?

nit: s/genreally/generally

Copy link
Copy Markdown
Collaborator

@mtjhrc mtjhrc May 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dorindabassey Keep in mind the MAX_INHERITED_ENV_BYTES has to be smaller than the kernel cmdline size though (because there are also other arguments to the kernel unrelated to this...). Where did you get these values btw?

Also TBH, I wouldn't worry too much about this, we can never do this fully correctly (as the comment this PR added mentions it's best effort). In the long term we should stop using this to pass the arguments. That said its possible to "optimize" this to pick a better value. I'm just not sure It's good use of time to worry about this, this argument passing mechanism is inherently broken.


/// 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())
}
Comment on lines +1319 to +1321
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using std::env::vars() can cause a panic if any environment variable contains invalid Unicode. In a library like libkrun, it is safer to use std::env::vars_os() and skip variables that cannot be converted to UTF-8 strings, ensuring the host process remains stable even if the environment is malformed.

fn inherit_host_env() -> String {
    serialize_env(env::vars_os().filter_map(|(k, v)| {
        match (k.into_string(), v.into_string()) {
            (Ok(k), Ok(v)) => Some((k, v)),
            _ => None,
        }
    }))
}


fn serialize_env<I: IntoIterator<Item = (String, String)>>(vars: I) -> String {
let mut buf = String::new();
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since the maximum size of the environment string is capped at MAX_INHERITED_ENV_BYTES, initializing the buffer with this capacity avoids unnecessary reallocations as variables are appended.

Suggested change
let mut buf = String::new();
let mut buf = String::with_capacity(MAX_INHERITED_ENV_BYTES);

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<String, std::str::Utf8Error> {
let mut strvec = Vec::new();

Expand All @@ -1318,7 +1357,6 @@ unsafe fn collapse_str_array(array: &[*const c_char]) -> Result<String, std::str
Ok(strvec.join(" "))
}

#[allow(clippy::format_collect)]
#[allow(clippy::missing_safety_doc)]
#[no_mangle]
pub unsafe extern "C" fn krun_set_exec(
Expand Down Expand Up @@ -1358,9 +1396,7 @@ pub unsafe extern "C" fn krun_set_exec(
}
}
} else {
env::vars()
.map(|(key, value)| format!(" {key}=\"{value}\""))
.collect()
inherit_host_env()
};

match CTX_MAP.lock().unwrap().entry(ctx_id) {
Expand All @@ -1376,7 +1412,6 @@ pub unsafe extern "C" fn krun_set_exec(
KRUN_SUCCESS
}

#[allow(clippy::format_collect)]
#[allow(clippy::missing_safety_doc)]
#[no_mangle]
pub unsafe extern "C" fn krun_set_env(ctx_id: u32, c_envp: *const *const c_char) -> i32 {
Expand All @@ -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) {
Expand Down Expand Up @@ -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");
}
}
2 changes: 1 addition & 1 deletion tests/runner/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
17 changes: 13 additions & 4 deletions tests/test_cases/src/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -35,6 +35,17 @@ pub fn setup_rootfs(test_setup: &TestSetup) -> anyhow::Result<PathBuf> {

/// 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")?;
Expand All @@ -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))?;
}
Expand Down
4 changes: 4 additions & 0 deletions tests/test_cases/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -78,6 +81,7 @@ pub fn test_cases() -> Vec<TestCase> {
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())),
Expand Down
37 changes: 37 additions & 0 deletions tests/test_cases/src/test_exec_null_envp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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<Self>, 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<Self>) {
println!("OK");
}
}
}
Loading