fix: return result.json and stdout results from sandboxed containers (#10460)

* fix: return result.json and stdout results from sandboxed containers

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: capture unmasked stdout-only last line, validate container result.json

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: reject image WorkingDir that escapes the container root

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: verify the whole result mount destination against the extracted rootfs

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: name the right skip reason and gate the symlink test to unix

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ruben Fiszel
2026-08-03 12:35:12 +02:00
committed by GitHub
co-authored by Claude Opus 5
parent 45b5c7a0c0
commit d5095515ed
5 changed files with 307 additions and 26 deletions
@@ -46,6 +46,17 @@ gidmap {
# and avoid it. Generated from the extracted rootfs.
{ROOTFS_MOUNTS}
# `./result.json` (relative to the image's WorkingDir) bound to a host file outside the
# image rootfs, so the JSON result the container writes flows back to the job without the
# worker ever reading a path the image controls.
#
# Placed immediately after the rootfs binds and BEFORE every other mount: nsjail resolves
# a mount destination against its temporary root before pivot_root, so whatever is mounted
# earlier can steer where this lands. Keeping only the rootfs ahead of it means the image
# is the single influence on that resolution and `result_mount_dst` verifies the whole
# path against the extracted rootfs on the host. Empty when it can't be verified.
{RESULT_MOUNT}
# Pseudo-filesystems the image expects. /tmp honors the same instance settings as
# every other nsjail job (nsjail_tmp_backing tmpfs/disk, nsjail_tmpfs_size_mb);
# /dev gets the standard nodes; /proc comes from mount_proc (the jail's own pid ns).
+216 -4
View File
@@ -32,8 +32,9 @@ use windmill_queue::{append_logs, CanceledBy, MiniPulledJob};
use crate::{
common::{
build_args_map, get_reserved_variables, raw_to_string, resolve_nsjail_timeout,
resolve_nsjail_tmp_mount_block, start_child_process, OccupancyMetrics, DEV_CONF_NSJAIL,
build_args_map, get_reserved_variables, raw_to_string, read_and_check_file,
resolve_nsjail_timeout, resolve_nsjail_tmp_mount_block, start_child_process,
OccupancyMetrics, DEV_CONF_NSJAIL,
},
get_proxy_envs_for_lang,
handle_child::handle_child,
@@ -169,6 +170,10 @@ struct OciConfig {
/// directives and break out of the sandbox. Every byte is emitted as a printable
/// ASCII char or a valid protobuf escape (`\"`, `\\`, `\n`/`\r`/`\t`, or 3-digit
/// octal `\NNN` for control/non-ASCII bytes), so the result always parses.
///
/// `{` and `}` are escaped too: the profile is rendered by substituting `{PLACEHOLDER}`
/// tokens one after another, so a value that still contained braces could smuggle in a
/// later placeholder's expansion — and `{ENVARS}` expands to unquoted directives.
fn proto_str(s: &str) -> String {
let mut out = String::with_capacity(s.len() + 2);
out.push('"');
@@ -179,6 +184,7 @@ fn proto_str(s: &str) -> String {
b'\n' => out.push_str("\\n"),
b'\r' => out.push_str("\\r"),
b'\t' => out.push_str("\\t"),
b'{' | b'}' => out.push_str(&format!("\\{b:03o}")),
0x20..=0x7e => out.push(b as char),
_ => out.push_str(&format!("\\{b:03o}")),
}
@@ -187,6 +193,83 @@ fn proto_str(s: &str) -> String {
out
}
/// Why `./result.json` can't be collected, phrased for the job log.
const UNVERIFIABLE_DST: &str =
"it cannot be proven to resolve inside the container root (a `..` component, or a \
symlink in the image rootfs)";
const SHADOWED_DST: &str =
"it is under /tmp, /proc, /dev or /sys, which are mounted over the result file";
/// In-jail destination for the `./result.json` bind — `{working_dir}/result.json` — or the
/// reason the image's `WorkingDir` makes it uncollectable.
///
/// nsjail resolves a mount destination against its temporary root *before* pivot_root,
/// with ordinary path resolution, and then creates it. A destination that walks out of
/// that root therefore has nsjail create — and mount over — a path on the *host*, as the
/// worker user. The profile places this mount directly after the rootfs binds and before
/// every other mount, so the extracted rootfs is the only thing that can steer the
/// resolution, and this walks the whole destination against it on the host:
///
/// - relative components (`..`) are rejected outright — they escape a fresh tmpfs too;
/// - `/tmp`, `/proc`, `/dev` and `/sys` are mounted *after* this one (or aren't writable),
/// so a destination under them would be shadowed rather than collected — rejected so
/// the job says so instead of silently dropping the result;
/// - every remaining component, **including the final `result.json`**, must exist as a
/// non-symlink or not exist at all. The first absent component ends the walk: nsjail
/// creates the rest as real directories under a parent already proven non-symlink.
/// Any other error (an unreadable mode-000 directory the jail's uid 0 could still
/// traverse) is treated as unsafe.
///
/// Nothing mutates the rootfs between this walk and the mount — extraction is finished and
/// the container has not started — so there is no window to swap a component.
async fn result_mount_dst(job_dir: &str, working_dir: &str) -> Result<String, &'static str> {
if !working_dir.starts_with('/') {
return Err(UNVERIFIABLE_DST);
}
let mut components = Vec::new();
for c in working_dir.split('/') {
match c {
"" | "." => continue,
".." => return Err(UNVERIFIABLE_DST),
_ => components.push(c),
}
}
if matches!(
components.first(),
Some(&"tmp") | Some(&"proc") | Some(&"dev") | Some(&"sys")
) {
return Err(SHADOWED_DST);
}
components.push("result.json");
let mut path = std::path::PathBuf::from(format!("{job_dir}/rootfs"));
for c in &components {
path.push(c);
match tokio::fs::symlink_metadata(&path).await {
Ok(m) if m.is_symlink() => return Err(UNVERIFIABLE_DST),
Ok(_) => {}
Err(e) if e.kind() == std::io::ErrorKind::NotFound => break,
Err(_) => return Err(UNVERIFIABLE_DST),
}
}
Ok(format!("/{}", components.join("/")))
}
/// Render the nsjail mount that exposes `{job_dir}/result.json` inside the container at
/// `dst`, i.e. the `./result.json` a bash script writes.
///
/// The host side deliberately sits *outside* `{job_dir}/rootfs`: the worker reads the
/// result from a path it created itself, never one the image could have planted as a
/// symlink to a host file. The container can't swap it either — unlinking a
/// bind-mount point fails.
fn render_result_mount(job_dir: &str, dst: &str) -> String {
format!(
"mount {{\n src: {}\n dst: {}\n is_bind: true\n rw: true\n mandatory: false\n}}\n",
proto_str(&format!("{job_dir}/result.json")),
proto_str(dst),
)
}
/// Render container env vars as nsjail `envar:` directives (one per line). Each
/// `KEY=VALUE` is proto-escaped, so image-controlled keys/values can neither break
/// the config nor reach nsjail's own process environment.
@@ -717,6 +800,21 @@ pub async fn handle_docker_v2_job(
// Render the nsjail profile: dynamic per-entry rootfs binds + image WorkingDir.
let nsjail_timeout = resolve_nsjail_timeout(conn, &job.workspace_id, job.id, job.timeout).await;
let rootfs_mounts = generate_rootfs_mounts(&rootfs).await?;
// Bind source for `./result.json` inside the container; empty means "no result".
write_file(job_dir, "result.json", "")?;
let result_dst = result_mount_dst(job_dir, working_dir).await;
if let Err(reason) = result_dst {
append_logs(
&job.id,
&job.workspace_id,
format!(
"WARNING: `./result.json` will not be collected for this job: the image's \
WorkingDir ({working_dir}) is unusable because {reason}\n"
),
conn,
)
.await;
}
write_file(
job_dir,
"run.docker.config.proto",
@@ -733,6 +831,13 @@ pub async fn handle_docker_v2_job(
)
// `# volume` mounts + same-worker shared folder (empty if none).
.replace("{SHARED_MOUNT}", shared_mount)
.replace(
"{RESULT_MOUNT}",
&result_dst
.as_deref()
.map(|dst| render_result_mount(job_dir, dst))
.unwrap_or_default(),
)
// Image env as `envar:` directives (child-only), so it never touches
// nsjail's process env.
.replace("{ENVARS}", &envars)
@@ -774,7 +879,7 @@ pub async fn handle_docker_v2_job(
.stderr(Stdio::piped());
let child = start_child_process(nsjail_cmd, NSJAIL_PATH.as_str(), false).await?;
handle_child(
let child_result = handle_child(
&job.id,
conn,
mem_peak,
@@ -792,6 +897,27 @@ pub async fn handle_docker_v2_job(
)
.await?;
// Same result conventions as a plain bash script: a non-empty `./result.json`
// wins, otherwise the last line of stdout, otherwise a completion message.
let result_json_path = format!("{job_dir}/result.json");
if tokio::fs::metadata(&result_json_path)
.await
.is_ok_and(|m| m.len() > 0)
{
// Checked, not `read_file`: the container is untrusted, and handing malformed
// JSON to `unsafe_raw` is undefined behavior.
return read_and_check_file(&result_json_path).await.map_err(|e| {
Error::ExecutionErr(format!(
"the `./result.json` written by the sandboxed container is not a valid \
JSON result: {e}"
))
});
}
if let Some(last_line) = child_result.last_line {
return Ok(to_raw_value(&json!(last_line.trim())));
}
Ok(to_raw_value(&json!(format!(
"sandboxed container ({image}) completed successfully"
))))
@@ -799,7 +925,90 @@ pub async fn handle_docker_v2_job(
#[cfg(test)]
mod tests {
use super::{digest_key, proto_str, ref_key, registry_qualified, render_envars};
use super::{
digest_key, proto_str, ref_key, registry_qualified, render_envars, render_result_mount,
result_mount_dst, SHADOWED_DST, UNVERIFIABLE_DST,
};
#[tokio::test]
async fn result_mount_dst_targets_working_dir() {
let job = tempfile::tempdir().unwrap();
let job_dir = job.path().to_str().unwrap();
std::fs::create_dir_all(format!("{job_dir}/rootfs/app/sub")).unwrap();
// `./result.json` relative to the image's WorkingDir, with no doubled slash
// when WorkingDir is the root and no trailing-slash artifact.
for (wd, expected) in [
("/app/sub", "/app/sub/result.json"),
("/", "/result.json"),
("/app/", "/app/result.json"),
// Absent from the rootfs — nsjail creates it as real directories under a
// parent already proven non-symlink.
("/app/new/deep", "/app/new/deep/result.json"),
] {
assert_eq!(
result_mount_dst(job_dir, wd).await.as_deref(),
Ok(expected),
"{wd}"
);
}
}
// The rootfs symlinks this plants have no Windows equivalent; the runtime is
// Linux-only (nsjail) anyway.
#[cfg(unix)]
#[tokio::test]
async fn result_mount_dst_rejects_escapes() {
let job = tempfile::tempdir().unwrap();
let job_dir = job.path().to_str().unwrap();
std::fs::create_dir_all(format!("{job_dir}/rootfs/app")).unwrap();
std::fs::create_dir_all(format!("{job_dir}/rootfs/wd")).unwrap();
// Image rootfs entries pointing out of the rootfs: as a directory component at
// both depths, and as the final `result.json` element itself (top level, and
// inside the WorkingDir).
std::os::unix::fs::symlink("/etc", format!("{job_dir}/rootfs/evil")).unwrap();
std::os::unix::fs::symlink("/etc", format!("{job_dir}/rootfs/app/evil")).unwrap();
std::os::unix::fs::symlink("/etc/passwd", format!("{job_dir}/rootfs/result.json")).unwrap();
std::os::unix::fs::symlink("/etc/passwd", format!("{job_dir}/rootfs/wd/result.json"))
.unwrap();
// nsjail resolves the mount dst against its temp root *before* pivot_root, so
// each of these would otherwise have it create a file on the host.
for wd in [
"/../../../etc",
"/app/../../etc",
"/evil",
"/evil/deeper",
"/app/evil",
"relative/path",
"/", // the rootfs ships `result.json` as a symlink
"/wd", // ...and so does this WorkingDir
] {
assert_eq!(
result_mount_dst(job_dir, wd).await,
Err(UNVERIFIABLE_DST),
"{wd} should be rejected as unverifiable"
);
}
// Mounted over the result file rather than escaping it — the job log must not
// blame a symlink that isn't there.
for wd in ["/tmp/work", "/proc/x", "/dev/x", "/sys/x"] {
assert_eq!(
result_mount_dst(job_dir, wd).await,
Err(SHADOWED_DST),
"{wd} should be rejected as shadowed"
);
}
}
#[test]
fn result_mount_renders_escaped() {
// The host side is the worker-created file, never a path inside the rootfs.
let m = render_result_mount("/j", "/app/result.json");
assert!(m.contains("src: \"/j/result.json\""));
assert!(m.contains("dst: \"/app/result.json\""));
}
#[test]
fn digest_key_is_filesystem_safe() {
@@ -870,6 +1079,9 @@ mod tests {
// a raw byte or an invalid `\u{..}` that nsjail's parser would reject).
assert_eq!(proto_str("a\u{1b}b"), "\"a\\033b\""); // ESC (0x1b)
assert_eq!(proto_str("é"), "\"\\303\\251\""); // UTF-8 bytes 0xc3 0xa9
// Braces are escaped so a value can never survive as a `{PLACEHOLDER}` token for
// a later substitution pass to expand (`{ENVARS}` expands to unquoted directives).
assert_eq!(proto_str("/{ENVARS}"), "\"/\\173ENVARS\\175\"");
}
#[test]
+51 -20
View File
@@ -50,7 +50,7 @@ use futures::{
};
use crate::common::{resolve_job_timeout, OccupancyMetrics, StreamNotifier};
use crate::job_logger::{append_job_logs, append_result_stream, append_with_limit};
use crate::job_logger::{append_job_logs, append_result_stream, append_with_limit, strip_nul};
use crate::job_logger_oss::process_streaming_log_lines;
use crate::worker_utils::{ping_job_status, update_worker_ping_from_job};
use crate::{MAX_RESULT_SIZE, MAX_WAIT_FOR_SIGINT, MAX_WAIT_FOR_SIGTERM};
@@ -89,6 +89,18 @@ async fn kill_process_tree(pid: Option<u32>) -> Result<(), String> {
pub struct HandleChildResult {
pub result_stream: Option<String>,
/// Last non-empty line the child wrote to **stdout**, unmasked, for executors
/// whose result convention is "last line of stdout" and that have no wrapper
/// script to tee it to a file (sandboxed containers). stderr is excluded: the two
/// pipes are merged by a fair `select`, so their relative order is arbitrary — a
/// diagnostic on stderr must not be able to win over the real result.
pub last_line: Option<String>,
}
/// A line read from a child, tagged with the pipe it came from.
struct OutputLine {
stderr: bool,
line: String,
}
/// - wait until child exits and return with exit status
@@ -311,6 +323,7 @@ pub async fn handle_child(
};
let mut stream_result = Vec::new();
let mut last_line = String::new();
/* a future that reads output from the child and appends to the database */
let lines = write_lines(
output,
@@ -325,6 +338,7 @@ pub async fn handle_child(
child_name,
&mut stream_result,
stream_notifier,
Some(&mut last_line),
)
.instrument(trace_span!("child_lines"));
@@ -339,7 +353,10 @@ pub async fn handle_child(
_ if *too_many_logs.borrow() => Err(Error::ExecutionErr(format!(
"logs or result reached limit. (current max size: {MAX_RESULT_SIZE} characters)"
))),
Ok(Ok(status)) => process_status(&child_name, status, stream_result),
Ok(Ok(status)) => process_status(&child_name, status, stream_result).map(|mut r| {
r.last_line = (!last_line.is_empty()).then_some(last_line);
r
}),
Ok(Err(kill_reason)) => match kill_reason {
KillReason::AlreadyCompleted => {
Err(Error::AlreadyCompleted("Job already completed".to_string()))
@@ -354,8 +371,8 @@ pub async fn handle_child(
pub const WAC_STEP_PREFIX: &str = "WM_WAC_STEP: ";
pub async fn write_lines(
output: impl stream::Stream<Item = io::Result<String>> + Send,
async fn write_lines(
output: impl stream::Stream<Item = io::Result<OutputLine>> + Send,
job_id: &Uuid,
w_id: &str,
worker: &str,
@@ -367,6 +384,7 @@ pub async fn write_lines(
child_name: &str,
stream_result: &mut Vec<String>,
stream_notifier: Option<StreamNotifier>,
mut last_line: Option<&mut String>,
) {
let max_log_size = if *CLOUD_HOSTED {
MAX_RESULT_SIZE
@@ -434,24 +452,26 @@ pub async fn write_lines(
while let Some(line) = read_lines.next().await {
match line {
Ok(line) => {
Ok(OutputLine { stderr, line }) => {
if line.is_empty() {
continue;
}
let line = if let Some(ref snap) = mask_snapshot {
match snap.mask(&line) {
std::borrow::Cow::Owned(masked) => masked,
std::borrow::Cow::Borrowed(_) => line,
}
} else {
line
};
// Masking is a log concern only: `mask` also appends a multi-line
// notice, which would end up inside a captured result. Keep `line`
// raw and log `logged`.
let masked = mask_snapshot
.as_ref()
.and_then(|snap| match snap.mask(&line) {
std::borrow::Cow::Owned(masked) => Some(masked),
std::borrow::Cow::Borrowed(_) => None,
});
let logged = masked.as_deref().unwrap_or(line.as_str());
if *OTEL_JOB_LOGS {
if let Some(otel_suffix) = line.strip_prefix(OTEL_PREFIX) {
if let Some(otel_suffix) = logged.strip_prefix(OTEL_PREFIX) {
tracing::event!(tracing::Level::INFO, otel_suffix);
}
}
if let Some(step_json) = line.strip_prefix(WAC_STEP_PREFIX) {
if let Some(step_json) = logged.strip_prefix(WAC_STEP_PREFIX) {
// Real-time WAC step start marker — fire-and-forget DB write
let conn = conn.clone();
let job_id = job_id.clone();
@@ -464,7 +484,7 @@ pub async fn write_lines(
});
continue;
}
if let Some(stream) = extract_stream_from_logs(&line) {
if let Some(stream) = extract_stream_from_logs(logged) {
let len = stream.len();
if log_remaining >= len {
log_remaining -= len;
@@ -474,7 +494,15 @@ pub async fn write_lines(
log_remaining = 0;
}
} else {
append_with_limit(&mut joined, &line, &mut log_remaining);
if let Some(buf) = last_line.as_mut() {
if !stderr && !line.trim().is_empty() {
buf.clear();
// Same NUL scrub the log path applies: Postgres rejects
// a NUL in the resulting jsonb too.
buf.push_str(&strip_nul(&line));
}
}
append_with_limit(&mut joined, logged, &mut log_remaining);
}
if log_remaining == 0 {
tracing::info!(%job_id, "Too many logs lines for job {job_id}");
@@ -1029,7 +1057,7 @@ fn child_joined_output_stream(
child: &mut Box<dyn TokioChildWrapper>,
job_id: Uuid,
w_id: String,
) -> impl stream::FusedStream<Item = io::Result<String>> {
) -> impl stream::FusedStream<Item = io::Result<OutputLine>> {
let stderr = child
.stderr()
.take()
@@ -1043,8 +1071,10 @@ fn child_joined_output_stream(
let stdout = BufReader::new(stdout).lines();
let stderr = BufReader::new(stderr).lines();
stream::select(
lines_to_stream(stderr, true, job_id.clone(), w_id.clone()),
lines_to_stream(stdout, false, job_id, w_id),
lines_to_stream(stderr, true, job_id.clone(), w_id.clone())
.map(|l| l.map(|line| OutputLine { stderr: true, line })),
lines_to_stream(stdout, false, job_id, w_id)
.map(|l| l.map(|line| OutputLine { stderr: false, line })),
)
}
@@ -1073,6 +1103,7 @@ pub fn process_status(
} else {
Some(stream_result.join(""))
},
last_line: None,
})
} else if let Some(code) = status.code() {
Err(error::Error::ExitStatus(program.to_string(), code))
+6 -1
View File
@@ -143,6 +143,11 @@ lazy_static::lazy_static! {
static ref RE_00: Regex = Regex::new('\u{00}'.to_string().as_str()).unwrap();
pub static ref NO_LOGS_AT_ALL: bool = std::env::var("NO_LOGS_AT_ALL").ok().is_some_and(|x| x == "1" || x == "true");
}
/// Drop NUL bytes: Postgres rejects them in both `text` logs and a `jsonb` result.
pub fn strip_nul(src: &str) -> std::borrow::Cow<'_, str> {
RE_00.replace_all(src, "")
}
// as a detail, `BufReader::lines()` removes \n and \r\n from the strings it yields,
// so this pushes \n to thd destination string in each call
pub fn append_with_limit(dst: &mut String, src: &str, limit: &mut usize) {
@@ -152,7 +157,7 @@ pub fn append_with_limit(dst: &mut String, src: &str, limit: &mut usize) {
let src_str;
let src = {
src_str = RE_00.replace_all(src, "");
src_str = strip_nul(src);
src_str.as_ref()
};
if !*CLOUD_HOSTED {
+23 -1
View File
@@ -34,6 +34,29 @@ python3 -c "import sys; print('hello', sys.argv[1])" "$name"
- The image's `Env`, `WorkingDir` are applied; the windmill reserved variables
(`WM_TOKEN`, `BASE_INTERNAL_URL`, …) are injected so `wmill`/API calls work.
## Result
Same conventions as a plain bash script, in this order:
1. `./result.json` (relative to the image's `WorkingDir`) if non-empty → returned as
the JSON result; malformed JSON fails the job. It is a host file bind-mounted into
the container, so it works for `WorkingDir: ""` (→ `/`) too, which otherwise lands in
nsjail's ephemeral root and never reaches the job. Being a bind-mount point, it must
be written in place (`> ./result.json`); a write-temp-then-`rename` fails.
The bind is skipped, with a warning in the job logs, when the destination can't be
proven to stay inside the container: a `WorkingDir` with `..`, one whose path in the
extracted rootfs crosses a symlink, or one under `/tmp`, `/proc`, `/dev` or `/sys`
(mounted after this bind, so a result there would be shadowed anyway). nsjail resolves
a mount destination *before* `pivot_root`, so an unverified path would have it create
a file on the **host**. For the same reason the bind is applied directly after the
rootfs binds — a `# volume` mounted over the `WorkingDir` therefore hides it, and the
result falls through to the stdout rule below.
2. Otherwise the last non-empty line of **stdout**, returned as a string. stderr is
excluded on purpose: the two pipes are merged by a fair `select`, so a diagnostic
on stderr could otherwise beat a block-buffered stdout result.
3. Otherwise a completion message.
## How it works
1. **Pull/extract** ([`crane`](https://github.com/google/go-containerregistry), no
@@ -101,7 +124,6 @@ the container inherits exactly the job's confinement:
- Images that drop to a non-root uid or chown to arbitrary uids inside need a
subuid **range** in the jail (single-uid only today — follow-up: `newuidmap`
range mapping).
- The script result is a completion message; capture output via stdout/logs.
## Follow-ups