diff --git a/backend/windmill-worker/nsjail/run.docker.config.proto b/backend/windmill-worker/nsjail/run.docker.config.proto index 5cc1e4ca03..a8e327927e 100644 --- a/backend/windmill-worker/nsjail/run.docker.config.proto +++ b/backend/windmill-worker/nsjail/run.docker.config.proto @@ -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). diff --git a/backend/windmill-worker/src/docker_v2.rs b/backend/windmill-worker/src/docker_v2.rs index 3a4936e85d..52d1060b67 100644 --- a/backend/windmill-worker/src/docker_v2.rs +++ b/backend/windmill-worker/src/docker_v2.rs @@ -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 { + 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] diff --git a/backend/windmill-worker/src/handle_child.rs b/backend/windmill-worker/src/handle_child.rs index b2c9de8a9a..e1b365bcf1 100644 --- a/backend/windmill-worker/src/handle_child.rs +++ b/backend/windmill-worker/src/handle_child.rs @@ -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) -> Result<(), String> { pub struct HandleChildResult { pub result_stream: Option, + /// 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, +} + +/// 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> + Send, +async fn write_lines( + output: impl stream::Stream> + 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, stream_notifier: Option, + 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, job_id: Uuid, w_id: String, -) -> impl stream::FusedStream> { +) -> impl stream::FusedStream> { 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)) diff --git a/backend/windmill-worker/src/job_logger.rs b/backend/windmill-worker/src/job_logger.rs index 364c352a21..54c8d923e1 100644 --- a/backend/windmill-worker/src/job_logger.rs +++ b/backend/windmill-worker/src/job_logger.rs @@ -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 { diff --git a/docs/docker-v2-runtime.md b/docs/docker-v2-runtime.md index f38d17d5e6..b0916843ca 100644 --- a/docs/docker-v2-runtime.md +++ b/docs/docker-v2-runtime.md @@ -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