diff --git a/backend/windmill-api-auth/src/lib.rs b/backend/windmill-api-auth/src/lib.rs index 77aeeba947..d2347aa22e 100644 --- a/backend/windmill-api-auth/src/lib.rs +++ b/backend/windmill-api-auth/src/lib.rs @@ -1023,6 +1023,18 @@ pub fn require_path_read_access_for_preview( return Ok(()); }; + // Reject path traversal before any privilege-based short-circuit. A Preview's + // path is request-supplied and bypasses the DB `proper_id` CHECK that deployed + // runnables get; it then flows to the worker where it builds on-disk module + // directories. A `..` segment or an absolute path could let a write escape the + // per-job dir. + if path.starts_with('/') || path.split('/').any(|seg| seg == "..") || path.contains('\0') { + return Err(Error::BadRequest(format!( + "Invalid path for preview job: {}", + path + ))); + } + if authed.is_admin { return Ok(()); } @@ -1080,6 +1092,45 @@ mod tests { } } + // Regression tests for the Preview path traversal: a Preview's path skips the + // DB `proper_id` CHECK and reaches the worker, where it builds on-disk module + // dirs. Traversal must be rejected even for admins, who otherwise bypass the + // namespace/folder access check. + #[test] + fn preview_path_rejects_traversal() { + let admin = ApiAuthed { is_admin: true, username: "admin".into(), ..Default::default() }; + for path in [ + "u/admin/../../../../../../tmp/evil/payload", + "../../tmp/evil", + "/tmp/evil", + "u/admin/ok/../../../../etc/cron.d/x", + ] { + assert!( + require_path_read_access_for_preview(&admin, &Some(path.to_string())).is_err(), + "expected traversal path to be rejected: {path}" + ); + } + } + + #[test] + fn preview_path_allows_legitimate_paths() { + let alice = ApiAuthed { username: "alice".into(), ..Default::default() }; + assert!(require_path_read_access_for_preview(&alice, &None).is_ok()); + assert!(require_path_read_access_for_preview(&alice, &Some(String::new())).is_ok()); + assert!( + require_path_read_access_for_preview(&alice, &Some("u/alice/my_script".into())).is_ok() + ); + + let admin = ApiAuthed { is_admin: true, username: "admin".into(), ..Default::default() }; + assert!( + require_path_read_access_for_preview(&admin, &Some("hub/foo/bar/baz".into())).is_ok() + ); + // `..` only as a substring of a segment is a valid name, not traversal. + assert!( + require_path_read_access_for_preview(&admin, &Some("f/team/my..script".into())).is_ok() + ); + } + #[test] fn predicate_no_scopes_allows_all() { let authed = authed_with_scopes(None); diff --git a/backend/windmill-worker/src/python_executor.rs b/backend/windmill-worker/src/python_executor.rs index 409dc362af..e58efff27e 100644 --- a/backend/windmill-worker/src/python_executor.rs +++ b/backend/windmill-worker/src/python_executor.rs @@ -37,8 +37,8 @@ use windmill_common::{ scripts::ScriptLang, utils::calculate_hash, worker::{ - copy_dir_recursively, pad_string, split_python_requirements, write_file, Connection, - PyVAlias, PythonAnnotations, WORKER_CONFIG, + copy_dir_recursively, is_allowed_file_location, pad_string, split_python_requirements, + write_file, Connection, PyVAlias, PythonAnnotations, WORKER_CONFIG, }, }; @@ -664,10 +664,16 @@ pub fn compute_python_module_dir(script_path: &str) -> String { .replace("-", "_") .replace("@", "."); if dirs_full.len() > 0 { - dirs_full - .strip_prefix("/") - .unwrap_or(&dirs_full) - .to_string() + let dirs = dirs_full.strip_prefix("/").unwrap_or(&dirs_full); + // This directory is appended to job_dir and written to. Neutralize any + // `.`/`..` segment so the result stays a relative path inside job_dir: a + // Preview path is request-supplied and skips the DB `proper_id` CHECK that + // deployed runnables get, and the `@`->`.` rewrite above can also turn a + // segment like `@.` into `..`. + dirs.split('/') + .map(|seg| if seg == "." || seg == ".." { "_" } else { seg }) + .collect::>() + .join("/") } else { "tmp".to_string() } @@ -1668,6 +1674,10 @@ async fn prepare_wrapper( last }; let module_dir = format!("{}/{}", job_dir, dirs); + // Defense-in-depth: `dirs`/`last` derive from the (request-supplied for + // previews) script path. compute_python_module_dir already neutralizes `..`, + // but assert containment here too so the write can never escape job_dir. + is_allowed_file_location(job_dir, &format!("{dirs}/{last}.py"))?; tokio::fs::create_dir_all(format!("{module_dir}/")).await?; let _ = write_file(&module_dir, &format!("{last}.py"), inner_content)?; @@ -3357,6 +3367,17 @@ mod tests { assert_eq!(compute_python_module_dir("f/in/script"), "f/_in"); } + #[test] + fn test_compute_python_module_dir_neutralizes_traversal() { + // A Preview path skips the DB `proper_id` CHECK, so it can carry `..`. + // `..`/`.` segments must be neutralized so the dir stays inside job_dir. + let dirs = compute_python_module_dir("u/x/../../../../tmp/evil/payload"); + assert!(!dirs.split('/').any(|s| s == ".." || s == ".")); + assert_eq!(dirs, "u/x/_/_/_/_/tmp/evil"); + // The `@`->`.` rewrite must not be able to synthesize a `..` segment. + assert_eq!(compute_python_module_dir("u/@./script"), "u/_"); + } + #[test] fn test_compute_py_codegen_basic_args() { let code = "def main(x: str, y: int):\n return x\n";