From d98efb5711cdb9a619e6aaebfc6feed1fc79302b Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sun, 14 Jun 2026 22:31:18 +0200 Subject: [PATCH] prevent path traversal via log_file_index in log endpoints (#9569) Co-authored-by: Claude Opus 4.8 (1M context) --- backend/windmill-api-jobs/src/jobs_export.rs | 16 +++++- backend/windmill-api/src/jobs.rs | 17 ++++--- backend/windmill-common/src/jobs.rs | 53 ++++++++++++++++++-- backend/windmill-object-store/src/lib.rs | 4 ++ 4 files changed, 79 insertions(+), 11 deletions(-) diff --git a/backend/windmill-api-jobs/src/jobs_export.rs b/backend/windmill-api-jobs/src/jobs_export.rs index 5de2c51795..d8f1826948 100644 --- a/backend/windmill-api-jobs/src/jobs_export.rs +++ b/backend/windmill-api-jobs/src/jobs_export.rs @@ -16,7 +16,7 @@ use uuid::Uuid; use windmill_common::{ db::UserDB, error, - jobs::{JobKind, JobStatus, JobTriggerKind}, + jobs::{is_safe_log_file_path, JobKind, JobStatus, JobTriggerKind}, scripts::ScriptLang, utils::{paginate, paginate_without_limits, require_admin, Pagination}, }; @@ -328,6 +328,20 @@ pub async fn import_completed_jobs( ) -> error::Result { require_admin(authed.is_admin, &authed.username)?; + // log_file_index is read back by the log endpoints as paths under the windmill + // log directory; an attacker-supplied traversal here would become an arbitrary + // file read. Reject anything that could escape the log directory at ingestion. + for job in &jobs { + if let Some(file_index) = &job.log_file_index { + if file_index.iter().any(|p| !is_safe_log_file_path(p)) { + return Err(error::Error::BadRequest(format!( + "Invalid log_file_index for job {}: entries must be relative paths without '..'", + job.id + ))); + } + } + } + let mut tx = user_db.begin(&authed).await?; for job in jobs { diff --git a/backend/windmill-api/src/jobs.rs b/backend/windmill-api/src/jobs.rs index 895dbed886..45de557aa8 100644 --- a/backend/windmill-api/src/jobs.rs +++ b/backend/windmill-api/src/jobs.rs @@ -34,8 +34,8 @@ use windmill_common::db::UserDbWithAuthed; use windmill_common::error::JsonResult; use windmill_common::flow_status::{JobResult, RestartedFrom}; use windmill_common::jobs::{ - format_completed_job_result, format_result, is_valid_entrypoint_name, DynamicInput, - ENTRYPOINT_OVERRIDE, + format_completed_job_result, format_result, is_safe_log_file_path, is_valid_entrypoint_name, + DynamicInput, ENTRYPOINT_OVERRIDE, }; #[cfg(feature = "run_inline")] use windmill_common::jobs::{ @@ -1912,12 +1912,17 @@ async fn get_logs_from_disk( if log_offset > 0 { if let Some(file_index) = log_file_index.clone() { for file_p in &file_index { - if !tokio::fs::metadata(format!("{}/{file_p}", *WINDMILL_DIR)) - .await - .is_ok() - { + if !is_safe_log_file_path(file_p) { return None; } + let local_file = format!("{}/{file_p}", *WINDMILL_DIR); + // Defense in depth: refuse to read through a symlink so a planted + // symlink under the log directory cannot exfiltrate arbitrary files. + match tokio::fs::symlink_metadata(&local_file).await { + Ok(meta) if meta.file_type().is_symlink() => return None, + Ok(_) => {} + Err(_) => return None, + } } let logs = logs.to_string(); diff --git a/backend/windmill-common/src/jobs.rs b/backend/windmill-common/src/jobs.rs index fd05c03ec0..78f5a6ee5a 100644 --- a/backend/windmill-common/src/jobs.rs +++ b/backend/windmill-common/src/jobs.rs @@ -283,6 +283,19 @@ pub fn format_completed_job_result(mut cj: CompletedJob) -> CompletedJob { cj } +/// `log_file_index` is normally written by the worker as job-id-scoped relative +/// paths under the windmill log directory. Any code path that lets a request +/// control this value (e.g. job import) must reject entries that could escape +/// that directory, otherwise the log-reading endpoints become an arbitrary file +/// read primitive. Rejects path traversal (`..`) and absolute paths; on-disk +/// readers additionally refuse symlinks (see `get_logs_from_disk`). +pub fn is_safe_log_file_path(file_p: &str) -> bool { + !file_p.is_empty() + && !file_p.starts_with('/') + && !file_p.starts_with('\\') + && !file_p.split(['/', '\\']).any(|c| c == "..") +} + pub async fn get_logs_from_disk( log_offset: i32, logs: &str, @@ -291,12 +304,17 @@ pub async fn get_logs_from_disk( if log_offset > 0 { if let Some(file_index) = log_file_index.clone() { for file_p in &file_index { - if !tokio::fs::metadata(format!("{}/{file_p}", *WINDMILL_DIR)) - .await - .is_ok() - { + if !is_safe_log_file_path(file_p) { return None; } + let local_file = format!("{}/{file_p}", *WINDMILL_DIR); + // Defense in depth: refuse to read through a symlink so a planted + // symlink under the log directory cannot exfiltrate arbitrary files. + match tokio::fs::symlink_metadata(&local_file).await { + Ok(meta) if meta.file_type().is_symlink() => return None, + Ok(_) => {} + Err(_) => return None, + } } let logs = logs.to_string(); @@ -439,3 +457,30 @@ pub struct WorkerInternalServerInlineUtils { // The server cannot call the worker functions directly because they are independent crates pub static WORKER_INTERNAL_SERVER_INLINE_UTILS: OnceCell = OnceCell::new(); + +#[cfg(test)] +mod tests { + use super::is_safe_log_file_path; + + #[test] + fn safe_log_file_paths_are_accepted() { + // Legit worker-written entries are job-id-scoped relative paths. + assert!(is_safe_log_file_path( + "0190d3e2-0000-7000-8000-000000000000/0.txt" + )); + assert!(is_safe_log_file_path("logs/abc/chunk1.log")); + assert!(is_safe_log_file_path("file..with..dots.txt")); + } + + #[test] + fn traversal_and_absolute_paths_are_rejected() { + assert!(!is_safe_log_file_path("")); + assert!(!is_safe_log_file_path("../../../../etc/passwd")); + assert!(!is_safe_log_file_path("a/../../etc/passwd")); + assert!(!is_safe_log_file_path("..")); + assert!(!is_safe_log_file_path("/etc/passwd")); + assert!(!is_safe_log_file_path("/proc/self/environ")); + assert!(!is_safe_log_file_path("\\windows\\path")); + assert!(!is_safe_log_file_path("a\\..\\..\\b")); + } +} diff --git a/backend/windmill-object-store/src/lib.rs b/backend/windmill-object-store/src/lib.rs index aea5a0e4cd..fe2d8ce347 100644 --- a/backend/windmill-object-store/src/lib.rs +++ b/backend/windmill-object-store/src/lib.rs @@ -52,6 +52,7 @@ use tokio::task; #[cfg(feature = "parquet")] use windmill_common::error::to_anyhow; #[cfg(feature = "parquet")] +use windmill_common::jobs::is_safe_log_file_path; use windmill_common::utils::rd_string; #[cfg(all(feature = "parquet", feature = "private"))] pub mod job_s3_helpers_ee; @@ -1296,6 +1297,9 @@ pub async fn get_logs_from_store( ) -> Option>> { if log_offset > 0 { if let Some(file_index) = log_file_index.clone() { + if file_index.iter().any(|p| !is_safe_log_file_path(p)) { + return None; + } if let Some(os) = get_object_store().await { let logs = logs.to_string(); let stream = async_stream::stream! {