diff --git a/backend/tests/app_run_mode_lock_strip.rs b/backend/tests/app_run_mode_lock_strip.rs index 7a1bba75f6..77265f2bcd 100644 --- a/backend/tests/app_run_mode_lock_strip.rs +++ b/backend/tests/app_run_mode_lock_strip.rs @@ -110,10 +110,13 @@ async fn test_run_mode_strips_caller_lock_and_modules(db: Pool) -> any .json(&json!({ // The args map is the other injection channel: an inline run is a // `JobKind::Preview` job, so the worker/executors read `_MODULES` and - // `_TEMP_SCRIPT_REFS` back out of the job args. Both must be stripped. + // `_TEMP_SCRIPT_REFS` back out of the job args. Both must be stripped — + // including the NUL spellings, which `push` stores under the plain name. "args": { "_MODULES": { "m.py": { "content": "print('evil')\n", "language": "python3" } }, - "_TEMP_SCRIPT_REFS": { "../evil": "deadbeef" } + "_MODULES\u{0}": { "m.py": { "content": "print('evil')\n", "language": "python3" } }, + "_TEMP_SCRIPT_REFS": { "../evil": "deadbeef" }, + "_TEMP_SCRIPT_REFS\u{0}": { "../evil": "deadbeef" } }, "component": "comp", "raw_code": inline_raw_code(Some(CALLER_HASH), true) diff --git a/backend/tests/caller_modules_strip.rs b/backend/tests/caller_modules_strip.rs index a65ca22d1b..bf4a096ee1 100644 --- a/backend/tests/caller_modules_strip.rs +++ b/backend/tests/caller_modules_strip.rs @@ -20,10 +20,16 @@ fn modules(v: &str) -> serde_json::Value { /// Pushes `payload` with a caller `_MODULES` both in `args` and in `extra` (where webhook /// query and headers land), and returns the stored `_MODULES`. +/// +/// Every spelling `push` stores as `_MODULES` is sent, not just the exact name: it writes +/// the args with their `\u0000` escapes removed, keys included, so `_MODULES\0` lands in +/// the job as `_MODULES` unless the removal compares the name that will be stored. async fn stored_modules(db: &Pool, payload: JobPayload) -> Option { let caller: Box = serde_json::value::to_raw_value(&modules("caller")).unwrap(); - let args = HashMap::from([("_MODULES".to_string(), caller.clone())]); - let extra = HashMap::from([("_MODULES".to_string(), caller)]); + let spellings = + || ["_MODULES", "_MODULES\0", "_MOD\0ULES"].map(|k| (k.to_string(), caller.clone())); + let args = HashMap::from(spellings()); + let extra = HashMap::from(spellings()); let (id, tx) = windmill_queue::push( db, PushIsolationLevel::IsolatedRoot(db.clone()), diff --git a/backend/tests/reserved_control_args.rs b/backend/tests/reserved_control_args.rs new file mode 100644 index 0000000000..a733546e51 --- /dev/null +++ b/backend/tests/reserved_control_args.rs @@ -0,0 +1,436 @@ +//! `_ENTRYPOINT_OVERRIDE`, `_MODULES` and `_TEMP_SCRIPT_REFS` steer *how* a runnable runs: +//! which function the worker calls instead of `main`, what module files land in the job +//! directory, where its workspace imports resolve from. They are legitimate for a caller +//! running its own code under its own identity, and must not be settable by a third party +//! whose payload runs under somebody else's: an HTTP route body or native trigger message +//! (the job runs as the trigger's owner) and a public app viewer's inputs (as the +//! publisher). +//! +//! `push` stores the args with their `\u0000` escapes removed, keys included, so every +//! check also has to hold for a NUL-spelled key: `_ENTRYPOINT_OVERRIDE\0` is read back as +//! `_ENTRYPOINT_OVERRIDE`. The same rename is what lets a viewer's `tenant\0` land as +//! `tenant` over an app policy's pinned `tenant`. + +use std::collections::HashMap; + +use serde_json::{json, value::RawValue, Value}; +use sqlx::{Pool, Postgres}; +use windmill_common::{ + jobs::JobPayload, + runnable_settings::{ConcurrencySettings, DebouncingSettings}, + scripts::{ScriptHash, ScriptLang}, + triggers::TriggerMetadata, +}; +use windmill_queue::{PushArgs, PushArgsOwned, PushIsolationLevel}; +use windmill_test_utils::*; + +const SCRIPT_PATH: &str = "f/system/hello"; +const SCRIPT_HASH: i64 = 123412; +/// Two top-level functions, as in the report: `main` authenticates, `apply` acts. +const SCRIPT_CONTENT: &str = "export async function main(data: string) { return data }\n\ + export async function apply(action: string) { return action }\n"; + +fn client() -> reqwest::Client { + reqwest::Client::new() +} + +fn authed(b: reqwest::RequestBuilder, token: &str) -> reqwest::RequestBuilder { + b.header("Authorization", format!("Bearer {}", token)) +} + +fn raw(v: Value) -> Box { + serde_json::value::to_raw_value(&v).unwrap() +} + +fn modules() -> Value { + json!({ "bunfig.toml": { "content": "preload = [\"./x.ts\"]", "language": "bun" } }) +} + +/// Deploy `SCRIPT_PATH` (a non-null `lock` is what makes a script count as deployed). +async fn deploy_script(db: &Pool) -> anyhow::Result<()> { + sqlx::query( + r#"INSERT INTO script (workspace_id, hash, path, content, language, kind, created_by, + schema, summary, description, lock) + VALUES ('test-workspace', $1, $2, $3, 'bun', 'script', 'test-user', '{}', '', '', + E'{}\n//bun.lock\n')"#, + ) + .bind(SCRIPT_HASH) + .bind(SCRIPT_PATH) + .bind(SCRIPT_CONTENT) + .execute(db) + .await?; + Ok(()) +} + +async fn job_args_and_entrypoint( + db: &Pool, + id: uuid::Uuid, +) -> anyhow::Result<(Value, Option)> { + Ok( + sqlx::query_as("SELECT args, script_entrypoint_override FROM v2_job WHERE id = $1") + .bind(id) + .fetch_one(db) + .await?, + ) +} + +fn trigger_authed() -> windmill_api_auth::ApiAuthed { + windmill_api_auth::ApiAuthed { + email: "test@windmill.dev".to_string(), + username: "test-user".to_string(), + is_admin: true, + is_operator: false, + groups: vec![], + folders: vec![], + scopes: None, + username_override: None, + username_override_is_token_label: false, + is_session_token: false, + token_prefix: None, + read_only: false, + job_id: None, + credential_expiry: None, + } +} + +/// `trigger_runnable_inner` is the one funnel every trigger reaches `push` through, so it +/// stands in here for the HTTP route whose body keys become the job's top-level args. The +/// sender is anonymous on a `none`/`custom_script` route; the job runs as the route owner. +#[sqlx::test(fixtures("base"))] +async fn trigger_args_lose_every_reserved_control_key(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + deploy_script(&db).await?; + + let args = PushArgsOwned { + args: HashMap::from([ + // Picks `apply` instead of `main`, skipping whatever `main` authenticates. + ("_ENTRYPOINT_OVERRIDE".to_string(), raw(json!("apply"))), + ("_ENTRYPOINT_OVERRIDE\u{0}".to_string(), raw(json!("apply"))), + ("_ENTRY\u{0}POINT_OVERRIDE".to_string(), raw(json!("apply"))), + ("_MODULES\u{0}".to_string(), raw(modules())), + ( + "_TEMP_SCRIPT_REFS\u{0}".to_string(), + raw(json!({ "f/system/dep": "deadbeef" })), + ), + // An ordinary input must still get through, NUL-renamed or not. + ("data".to_string(), raw(json!("payload"))), + ("action\u{0}".to_string(), raw(json!("refund"))), + ]), + extra: None, + }; + + let (id, _, _, _, _) = windmill_trigger::trigger_helpers::trigger_runnable_inner( + &db, + None, + None, + trigger_authed(), + "test-workspace", + SCRIPT_PATH, + false, + args, + None, + None, + None, + "u/test-user/route".to_string(), + None, + TriggerMetadata::new( + Some("u/test-user/route".to_string()), + windmill_common::jobs::JobTriggerKind::Http, + ), + None, + ) + .await?; + + let (args, entrypoint) = job_args_and_entrypoint(&db, id).await?; + assert_eq!( + args, + json!({ "data": "payload", "action": "refund" }), + "a trigger payload may carry the runnable's inputs and nothing that steers how it runs" + ); + assert_eq!( + entrypoint, None, + "a trigger payload must not pick the function the worker calls instead of `main`" + ); + Ok(()) +} + +/// The counterpart: `_ENTRYPOINT_OVERRIDE` stays for a caller that runs the script under +/// its own identity with `jobs:run` — the script editor's preprocessor test, dynamic-select +/// helpers, the CLI and workflow-as-code all send it this way (GHSA-wxjq-w5pj-jqhx bounds +/// what it may contain, not who may set it). Stripping it in `push` would break them. +#[sqlx::test(fixtures("base"))] +async fn an_authenticated_push_keeps_the_entrypoint_override( + db: Pool, +) -> anyhow::Result<()> { + deploy_script(&db).await?; + + // Both spellings: `push` normalizes the names to what it stores, so the NUL-spelled one + // selects the function exactly like the plain one instead of being silently different. + for key in ["_ENTRYPOINT_OVERRIDE", "_ENTRYPOINT_OVERRIDE\u{0}"] { + let args = HashMap::from([ + (key.to_string(), raw(json!("apply"))), + ("action".to_string(), raw(json!("refund"))), + ]); + let (id, tx) = windmill_queue::push( + &db, + PushIsolationLevel::IsolatedRoot(db.clone()), + "test-workspace", + JobPayload::ScriptHash { + hash: ScriptHash(SCRIPT_HASH), + path: SCRIPT_PATH.to_string(), + cache_ttl: None, + cache_ignore_s3_path: None, + dedicated_worker: None, + language: ScriptLang::Bun, + priority: None, + apply_preprocessor: false, + concurrency_settings: ConcurrencySettings::default(), + debouncing_settings: DebouncingSettings::default(), + labels: None, + job_token_scopes: None, + }, + PushArgs { args: &args, extra: None }, + "test-user", + "test@windmill.dev", + "u/test-user".to_string(), + None, + None, + None, + None, + None, + None, + None, + None, + false, + false, + None, + true, + None, + None, + None, + None, + None, + false, + None, + None, + None, + None, + ) + .await?; + tx.commit().await?; + let (args, entrypoint) = job_args_and_entrypoint(&db, id).await?; + assert_eq!( + entrypoint.as_deref(), + Some("apply"), + "an authenticated caller running a script as itself may still pick the entrypoint \ + (sent as {key:?})" + ); + assert_eq!( + args, + json!({ "_ENTRYPOINT_OVERRIDE": "apply", "action": "refund" }), + "the stored name is the normalized one (sent as {key:?})" + ); + } + Ok(()) +} + +/// Create a public (anonymous) app whose single component is the deployed script, with the +/// given policy inputs, and return its path. +async fn create_public_app( + ws: &str, + path: &str, + static_inputs: Value, + one_of_inputs: Value, +) -> anyhow::Result<()> { + let resp = authed(client().post(format!("{ws}/apps/create")), "SECRET_TOKEN") + .json(&json!({ + "path": path, + "summary": "", + "value": {}, + "policy": { + "execution_mode": "anonymous", + "triggerables_v2": { + format!("comp:script/{SCRIPT_PATH}"): { + "static_inputs": static_inputs, + "one_of_inputs": one_of_inputs, + } + } + } + })) + .send() + .await?; + assert_eq!(resp.status(), 201, "create app: {}", resp.text().await?); + Ok(()) +} + +/// A viewer of a public app runs the publisher's pinned script as the publisher: it may +/// fill in the inputs the policy leaves free and nothing else. +#[sqlx::test(fixtures("base"))] +async fn public_app_viewer_cannot_steer_the_pinned_runnable( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let ws = format!( + "http://localhost:{}/api/w/test-workspace", + server.addr.port() + ); + deploy_script(&db).await?; + + let app_path = "u/test-user/reserved_args"; + create_public_app( + &ws, + app_path, + json!({ "tenant": "acme" }), + json!({ "greeting": ["Hello,", "Bonjour,"] }), + ) + .await?; + + // No `Authorization` header at all: the anonymous visitor of a public app. + let run = |args: &'static str| { + client() + .post(format!("{ws}/apps_u/execute_component/{app_path}")) + .header("content-type", "application/json") + .body(format!( + r#"{{"component": "comp", "path": "script/{SCRIPT_PATH}", "args": {args}}}"# + )) + .send() + }; + + // A name spelled with a NUL is the same name once stored, so the policy's pinned value + // must still win and the one-of list must still be enforced. + let resp = run(r#"{"tenant\u0000": "globex", "greeting": "Hello,"}"#).await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!(status, 200, "run must be accepted: {body}"); + let (args, _) = job_args_and_entrypoint(&db, uuid::Uuid::parse_str(body.trim())?).await?; + assert_eq!( + args["tenant"], + json!("acme"), + "a NUL-renamed arg must not override an input the policy pins: {args}" + ); + + let resp = run(r#"{"greeting\u0000": "anything"}"#).await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!( + status, 400, + "a NUL-renamed arg must still be held to the policy's one_of list: {body}" + ); + assert!( + body.contains("must be one of"), + "expected the one_of rejection, got: {body}" + ); + + // The control keys, in both spellings, in one request. + let resp = run( + r#"{"_ENTRYPOINT_OVERRIDE": "apply", "_ENTRYPOINT_OVERRIDE\u0000": "apply", + "_MODULES": {"bunfig.toml": {"content": "x", "language": "bun"}}, + "_MODULES\u0000": {"bunfig.toml": {"content": "x", "language": "bun"}}, + "_TEMP_SCRIPT_REFS": {"f/system/dep": "deadbeef"}, + "_TEMP_SCRIPT_REFS\u0000": {"f/system/dep": "deadbeef"}, + "greeting": "Hello,"}"#, + ) + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!(status, 200, "run must be accepted: {body}"); + let (args, entrypoint) = + job_args_and_entrypoint(&db, uuid::Uuid::parse_str(body.trim())?).await?; + assert_eq!( + entrypoint, None, + "a viewer must not pick the function the worker calls instead of `main`" + ); + assert_eq!( + args, + json!({ "tenant": "acme", "greeting": "Hello," }), + "only the policy's pinned input and the viewer's allowed one may reach the job: {args}" + ); + Ok(()) +} + +/// A deployed flow's dynamic-select code has no deployed runnable, so it is queued as a +/// `JobKind::Preview` job — the one kind on which the bun and python executors honor +/// `_TEMP_SCRIPT_REFS` and resolve the code's workspace imports from the temporary script +/// store, which any workspace member can write. The caller of this route only fills in that +/// code's inputs (it may be an operator, who is refused `jobs/run/preview` outright), so the +/// key must not reach the job, in either spelling. +#[sqlx::test(fixtures("base"))] +async fn flow_dynamic_select_strips_caller_control_args(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let ws = format!( + "http://localhost:{}/api/w/test-workspace", + server.addr.port() + ); + + // Someone else's flow: `test-user-2` below can run it, not edit it. + let flow_path = "u/test-user/dynselect_control_args"; + let resp = authed(client().post(format!("{ws}/flows/create")), "SECRET_TOKEN") + .json(&json!({ + "path": flow_path, + "summary": "", + "description": "", + "value": { "modules": [] }, + "schema": { + "type": "object", + "properties": { "region": { "type": "object", "format": "dynselect-region" } }, + "x-windmill-dyn-select-lang": "bun", + "x-windmill-dyn-select-code": + "import { regions } from \"/f/system/regions\"\n\ + export async function region() { return regions() }\n" + } + })) + .send() + .await?; + assert_eq!(resp.status(), 201, "create flow: {}", resp.text().await?); + + // Read-only for the caller below: running the dyn-select code is all it may do. + sqlx::query( + "UPDATE flow SET extra_perms = '{\"u/test-user-2\": false}' + WHERE path = $1 AND workspace_id = 'test-workspace'", + ) + .bind(flow_path) + .execute(&db) + .await?; + + let refs = json!({ "f/system/regions": "deadbeef" }); + let modules = json!({ "regions.ts": { "content": "export const v = 1", "language": "bun" } }); + let resp = authed( + client().post(format!("{ws}/jobs/run/dynamic_select")), + "SECRET_TOKEN_2", + ) + .json(&json!({ + "entrypoint_function": "region", + "runnable_ref": { "source": "deployed", "path": flow_path, "runnable_kind": "flow" }, + "args": { + "_TEMP_SCRIPT_REFS": refs, + "_TEMP_SCRIPT_REFS\u{0}": refs, + "_MODULES": modules, + "_MODULES\u{0}": modules, + "filterText": "eu" + } + })) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!(status, 201, "dynamic select must be accepted: {body}"); + + let id = uuid::Uuid::parse_str(body.trim())?; + let kind: String = sqlx::query_scalar("SELECT kind::text FROM v2_job WHERE id = $1") + .bind(id) + .fetch_one(&db) + .await?; + assert_eq!( + kind, "preview", + "the premise of this test: a deployed flow's dyn-select code runs as a preview job" + ); + let (args, _) = job_args_and_entrypoint(&db, id).await?; + assert_eq!( + args, + json!({ "_ENTRYPOINT_OVERRIDE": "region", "filterText": "eu" }), + "only the caller's inputs and the server-set entrypoint may reach the job" + ); + Ok(()) +} diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index 575306fe95..cdc66496c0 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -66,8 +66,8 @@ use windmill_common::{ db::{DbWithOptAuthed, UserDB}, error::{to_anyhow, Error, JsonResult, Result}, jobs::{ - get_payload_tag_from_prefixed_path, resolve_delete_after_secs, schedule_job_deletion, - JobPayload, JobTriggerKind, RawCode, + get_payload_tag_from_prefixed_path, normalize_nul_in_args, resolve_delete_after_secs, + sanitize_untrusted_args, schedule_job_deletion, JobPayload, JobTriggerKind, RawCode, }, scripts::ScriptLang, triggers::TriggerMetadata, @@ -4963,11 +4963,15 @@ async fn execute_component( let resolved_delete_secs = resolve_delete_after_secs(None, policy_triggerables.delete_after_secs); - // `_TEMP_SCRIPT_REFS` is a server-injected control key (into `extra`) that the - // worker reads back for a `Preview` job, which an inline run is. A caller - // supplying it in `args` would redirect relative-import resolution, unpinned, as - // the app identity. Drop it; the legitimate value rides in `extra`. - payload.args.remove("_TEMP_SCRIPT_REFS"); + // The caller here is a viewer of the app, down to an anonymous visitor of a public one, + // and the job runs as the app's identity on code the policy pins. So the viewer fills in + // the runnable's inputs and nothing else: not the function called instead of `main` + // (which would reach helpers the app never exposes, as the publisher), not the module + // files written into the job directory, and not `_TEMP_SCRIPT_REFS`, which would + // redirect relative-import resolution, unpinned, as the app identity. The legitimate + // `_TEMP_SCRIPT_REFS` is server-injected into `extra` further below, which this leaves + // alone — as it does the policy's own `static_inputs`, which the publisher writes. + sanitize_untrusted_args(&mut payload.args); // An agent runnable takes the message and its files, plus optionally the conversation they // belong to, which names the agent's memory rather than an input of its run. Without it the @@ -6640,6 +6644,16 @@ async fn build_args( let mut job_id: Option = None; let mut safe_args = HashMap::>::new(); + // Every check below matches an argument by name against the policy the publisher wrote, + // and `push` stores the args with their `\u0000` escapes removed, names included. So + // compare the names that will actually be stored: otherwise a viewer's `tenant\0` is + // never recognized as the `tenant` the policy pins in `static_inputs` (and is stored as + // `tenant`, winning over the pinned value), and `greeting\0` is never recognized as the + // `greeting` restricted by `one_of_inputs`, so neither list is enforced. Values are + // normalized along with the names, so the checks that look at a value see what the job + // will receive. + normalize_nul_in_args(&mut args); + // Before anything reads `args`: a context input the runnable declares is never taken // from the request, or a viewer could name someone else as themselves. for (k, prop) in ctx_inputs.into_iter().flatten() { diff --git a/backend/windmill-api/src/jobs.rs b/backend/windmill-api/src/jobs.rs index c5dd374c7a..bf3d7ea24c 100644 --- a/backend/windmill-api/src/jobs.rs +++ b/backend/windmill-api/src/jobs.rs @@ -36,7 +36,7 @@ use windmill_common::error::JsonResult; use windmill_common::flow_status::{JobResult, RestartedFrom}; use windmill_common::jobs::{ format_completed_job_result, format_result, is_safe_log_file_path, is_valid_entrypoint_name, - DynamicInput, ENTRYPOINT_OVERRIDE, + sanitize_untrusted_args, DynamicInput, ENTRYPOINT_OVERRIDE, }; #[cfg(feature = "run_inline")] use windmill_common::jobs::{ @@ -10488,7 +10488,7 @@ async fn run_dynamic_select( Extension(user_db): Extension, Path(w_id): Path, Query(run_query): Query, - Json(request): Json, + Json(mut request): Json, ) -> error::Result { #[cfg(feature = "enterprise")] check_license_key_valid().await?; @@ -10510,6 +10510,20 @@ async fn run_dynamic_select( ))); } + // The dyn-select code is the runnable's, not necessarily the caller's: a deployed flow + // has no deployed runnable for it, so it runs as a `JobKind::Preview` job, and the + // executors honor `_TEMP_SCRIPT_REFS` on every preview — it resolves the code's + // workspace imports from the temporary script store, which any workspace member can + // write. A caller of this route (an operator, for a deployed flow) only fills in that + // code's inputs; the function to call is taken from `entrypoint_function`, validated + // above and inserted server-side below, never from the args. So the reserved control + // keys are dropped here, in the spellings `push` would store them under. Nothing + // server-side puts them in `args`: the request has no `temp_script_refs` field, and the + // run form does not send them. + if let Some(args) = request.args.as_mut() { + sanitize_untrusted_args(args); + } + // Deployed scripts keep their normal deployed-run path (a `script` job, resolved below by // push_script_job_by_path_into_queue). Deployed flows and inline snippets have no deployed // runnable, so they run their dyn-select code as a `preview`; the flow carries its path and diff --git a/backend/windmill-common/src/jobs.rs b/backend/windmill-common/src/jobs.rs index d7190ff45f..5ca70d3b58 100644 --- a/backend/windmill-common/src/jobs.rs +++ b/backend/windmill-common/src/jobs.rs @@ -16,12 +16,77 @@ use crate::{ flows::get_full_hub_flow_by_path, get_latest_deployed_hash_for_path, get_latest_flow_version_info_for_path, scripts::{get_full_hub_script_by_path, ScriptHash, ScriptLang}, - utils::{StripPath, HTTP_CLIENT}, + utils::{strip_json_nul, StripPath, HTTP_CLIENT}, worker::{custom_tag_matches, to_raw_value, CUSTOM_TAGS_PER_WORKSPACE, WINDMILL_DIR}, workspaces::workspace_with_fork_ancestors, FlowVersionInfo, ScriptHashInfo, Tag, }; +/// Whether `args` already reads the way `push` will store it, i.e. holds no NUL escape in a +/// name or a value. Cheap (one SIMD substring scan per value) so the normalization below can +/// be a no-op on the overwhelmingly common clean case. +pub fn args_contain_nul(args: &HashMap>) -> bool { + args.iter().any(|(k, v)| { + k.contains('\0') || memchr::memmem::find(v.get().as_bytes(), b"\\u0000").is_some() + }) +} + +/// Rewrite `args` the way `push` stores it: `push` runs [`strip_json_nul`] over the +/// serialized args just before the INSERT, which removes `\u0000` escapes from **names as +/// well as values** (jsonb rejects them with 22P05). Any decision taken on a name before +/// that — `push`'s own removal of a caller [`MODULES_ARG`], an app policy's `static_inputs` +/// and `one_of_inputs`, the `PushArgs` serializer's "`extra` wins this name" rule — would +/// otherwise compare a name nobody knows against the name that actually lands in the job: +/// an arg sent as `_MODULES\0` is stored as `_MODULES`, and a viewer's `tenant\0` is stored +/// as `tenant`, after the publisher's pinned value, which jsonb then discards as the earlier +/// of two equal keys. +/// +/// Normalizing first makes every such check see the stored name. Values are normalized too, +/// so a check on a value sees what the job will receive. +/// +/// Two names that collide once normalized keep a single value, deterministically: the +/// spelling without NULs wins, then the lexicographically smallest original name — never the +/// one that `HashMap` iteration happened to serialize last. +pub fn normalize_nul_in_args(args: &mut HashMap>) { + if !args_contain_nul(args) { + return; + } + let mut entries = args.drain().collect::>(); + entries.sort_by(|(a, _), (b, _)| { + a.contains('\0') + .cmp(&b.contains('\0')) + .then_with(|| a.cmp(b)) + }); + for (key, value) in entries { + let key = if key.contains('\0') { + key.replace('\0', "") + } else { + key + }; + let stripped = match strip_json_nul(value.get()) { + std::borrow::Cow::Owned(stripped) => RawValue::from_string(stripped).ok(), + std::borrow::Cow::Borrowed(_) => None, + }; + args.entry(key).or_insert(stripped.unwrap_or(value)); + } +} + +/// Drop every [`RESERVED_CONTROL_ARGS`] key from args that a third party supplied for a job +/// running under somebody else's identity (an HTTP route body or native trigger message run +/// as the trigger's owner, a public app viewer's inputs run as the app's publisher). Those +/// keys pick the function the worker calls instead of `main`, the module files written into +/// the job directory, and where workspace imports resolve from: the third party must not +/// choose any of them, while a caller running its own code under its own identity still may. +/// +/// Normalizes first, so a key spelled `_ENTRYPOINT_OVERRIDE\0` — which `push` stores as +/// `_ENTRYPOINT_OVERRIDE` — cannot slip past the comparison. +pub fn sanitize_untrusted_args(args: &mut HashMap>) { + normalize_nul_in_args(args); + for key in RESERVED_CONTROL_ARGS { + args.remove(key); + } +} + pub fn get_has_preprocessor_from_content_and_lang( content: &str, language: &ScriptLang, @@ -633,6 +698,97 @@ pub async fn delete_jobs(conn: &mut sqlx::PgConnection, ids: &[uuid::Uuid]) -> e Ok(()) } +#[cfg(test)] +mod reserved_args_tests { + use super::{normalize_nul_in_args, sanitize_untrusted_args}; + use serde_json::value::RawValue; + use std::collections::HashMap; + + fn raw(json: &str) -> Box { + RawValue::from_string(json.to_string()).unwrap() + } + + fn args(entries: &[(&str, &str)]) -> HashMap> { + entries + .iter() + .map(|(k, v)| (k.to_string(), raw(v))) + .collect() + } + + fn as_json(args: &HashMap>) -> serde_json::Value { + serde_json::to_value(args).unwrap() + } + + /// A clean map must come out byte for byte the same: this runs on every push. + #[test] + fn normalization_leaves_clean_args_alone() { + let mut map = args(&[ + ("tenant", r#""acme""#), + // A legitimately escaped backslash followed by the literal text `u0000` + // (minified JS regexes contain these) is not a NUL and must survive. + ("src", r#""[^\\u0000]+""#), + ]); + let before = as_json(&map); + normalize_nul_in_args(&mut map); + assert_eq!(as_json(&map), before); + } + + /// `push` stores the args with their NUL escapes removed, names included, so a name + /// that only differs by NULs is the same name and must be compared as such. + #[test] + fn normalization_renames_nul_spelled_names_and_cleans_values() { + let mut map = args(&[ + ("_MODULES\u{0}", r#"{"m.py": "evil"}"#), + ("_MOD\u{0}ULES", r#"{"m.py": "evil"}"#), + ("greeting\u{0}", r#""anything""#), + ("note", r#""a\u0000b""#), + ]); + normalize_nul_in_args(&mut map); + let mut names = map.keys().cloned().collect::>(); + names.sort(); + assert_eq!(names, vec!["_MODULES", "greeting", "note"]); + assert_eq!(map["note"].get(), r#""ab""#); + } + + /// Which of two names that collide once normalized survives may not depend on + /// `HashMap` iteration order: the spelling the publisher/server used wins. + #[test] + fn normalization_keeps_the_nul_free_spelling_on_collision() { + for _ in 0..16 { + let mut map = args(&[ + ("tenant", r#""acme""#), + ("tenant\u{0}", r#""globex""#), + ("\u{0}tenant", r#""initech""#), + ]); + normalize_nul_in_args(&mut map); + assert_eq!(map.len(), 1); + assert_eq!(map["tenant"].get(), r#""acme""#); + } + } + + /// Every reserved control key goes, in every spelling, and nothing else does. + #[test] + fn untrusted_args_lose_every_reserved_control_key() { + let mut map = args(&[ + ("_ENTRYPOINT_OVERRIDE", r#""apply""#), + ("_ENTRYPOINT_OVERRIDE\u{0}", r#""apply""#), + ("_ENTRY\u{0}POINT_OVERRIDE", r#""apply""#), + ("_MODULES", r#"{"m.py": "evil"}"#), + ("_MODULES\u{0}", r#"{"m.py": "evil"}"#), + ("_TEMP_SCRIPT_REFS", r#"{"f/a/b": "deadbeef"}"#), + ("_TEMP_SCRIPT_REFS\u{0}", r#"{"f/a/b": "deadbeef"}"#), + // Neighbours that must not be caught by a prefix/contains match. + ("_TEMP_SCRIPT_REFS_", r#"1"#), + ("entrypoint", r#""main""#), + ("action", r#""refund""#), + ]); + sanitize_untrusted_args(&mut map); + let mut names = map.keys().cloned().collect::>(); + names.sort(); + assert_eq!(names, vec!["_TEMP_SCRIPT_REFS_", "action", "entrypoint"]); + } +} + #[cfg(test)] mod tests { use super::is_safe_log_file_path; diff --git a/backend/windmill-queue/src/jobs.rs b/backend/windmill-queue/src/jobs.rs index b8d2458b90..b78da75edf 100644 --- a/backend/windmill-queue/src/jobs.rs +++ b/backend/windmill-queue/src/jobs.rs @@ -42,8 +42,8 @@ use windmill_common::auth::JobPerms; #[cfg(feature = "benchmark")] use windmill_common::bench::BenchmarkIter; use windmill_common::jobs::{ - script_path_to_payload, JobTriggerKind, TriggerKindLabel, EMAIL_ERROR_HANDLER_USER_EMAIL, - MODULES_ARG, + args_contain_nul, normalize_nul_in_args, sanitize_untrusted_args, script_path_to_payload, + JobTriggerKind, TriggerKindLabel, EMAIL_ERROR_HANDLER_USER_EMAIL, MODULES_ARG, }; use windmill_common::min_version::{ MIN_VERSION_SUPPORTS_DEBOUNCING, MIN_VERSION_SUPPORTS_DEBOUNCING_V2, @@ -6153,6 +6153,18 @@ impl PushArgsOwned { pub fn empty() -> Self { PushArgsOwned { extra: None, args: HashMap::new() } } + + /// Mark these args as coming from an untrusted third party: see + /// [`windmill_common::jobs::sanitize_untrusted_args`]. Applied to both maps, because a + /// name in `extra` wins over the same name in `args` when the job is serialized, and no + /// source that builds args from a third party's payload sets a reserved control key of + /// its own. + pub fn sanitize_untrusted(&mut self) { + sanitize_untrusted_args(&mut self.args); + if let Some(extra) = self.extra.as_mut() { + sanitize_untrusted_args(extra); + } + } } pub fn empty_result() -> Box { @@ -6398,21 +6410,34 @@ async fn push_inner<'c, 'd>( suspended_mode: Option, scope_ceiling: Option<&[String]>, ) -> Result<(Uuid, Transaction<'c, Postgres>), Error> { - // The worker builds a preview's `_MODULES` arg into the job as its module code. Every - // caller-reachable value lands in `args` or `extra` (webhook query and headers go to - // `extra`, WAC children copy their parent's args), so it is dropped from both and only - // the `JobPayload::Code` arm below sets it, from the server-side `RawCode::modules`. - let args_without_modules; + // Normalize the names and values to what the INSERT below will actually store before + // anything here goes by a name: `strip_json_nul` runs over the serialized args just + // before that INSERT and removes `\u0000` escapes from keys as well as values, so a + // name compared as sent is not the name that lands in the job. Without this, `_MODULES` + // is dropped below while `_MODULES\0` is stored as `_MODULES` and built into a preview's + // job directory as module code; likewise the serializer's "`extra` wins this name" rule + // (which is how an app policy's `static_inputs` override a viewer's args) compares the + // name as sent, so `tenant\0` is written after the pinned `tenant` and jsonb keeps the + // viewer's, as the later of two equal keys. The final strip stays, harmless once this + // has run. + let normalized_args; let mut args = { let mut extra = args.extra; if let Some(extra) = extra.as_mut() { + normalize_nul_in_args(extra); + // The worker builds a preview's `_MODULES` arg into the job as its module code. + // Every caller-reachable value lands in `args` or `extra` (webhook query and + // headers go to `extra`, WAC children copy their parent's args), so it is + // dropped from both and only the `JobPayload::Code` arm below sets it, from the + // server-side `RawCode::modules`. extra.remove(MODULES_ARG); } - let args = if args.args.contains_key(MODULES_ARG) { + let args = if args_contain_nul(args.args) || args.args.contains_key(MODULES_ARG) { let mut stripped = args.args.clone(); + normalize_nul_in_args(&mut stripped); stripped.remove(MODULES_ARG); - args_without_modules = stripped; - &args_without_modules + normalized_args = stripped; + &normalized_args } else { args.args }; diff --git a/backend/windmill-trigger/src/trigger_helpers.rs b/backend/windmill-trigger/src/trigger_helpers.rs index ae805d195e..4056ae85e6 100644 --- a/backend/windmill-trigger/src/trigger_helpers.rs +++ b/backend/windmill-trigger/src/trigger_helpers.rs @@ -512,7 +512,7 @@ pub async fn trigger_runnable_inner<'c>( workspace_id: &str, runnable_path: &str, is_flow: bool, - args: PushArgsOwned, + mut args: PushArgsOwned, retry: Option<&sqlx::types::Json>, error_handler_path: Option<&str>, error_handler_args: Option<&sqlx::types::Json>>, @@ -527,6 +527,18 @@ pub async fn trigger_runnable_inner<'c>( bool, Option>, )> { + // A trigger's args are built from a third party's payload — for an HTTP route without + // preprocessor the request body's keys *are* the job's top-level args — and the job runs + // as the trigger's owner, not as whoever sent that payload. So the sender may not steer + // how the runnable runs: pick the function called instead of `main` (and skip whatever + // `main` authenticates, e.g. the signature check of a `custom_script` route), plant + // module files in the job directory, or redirect where its workspace imports resolve. + // Every trigger kind funnels through here on its way to `push`, and none of them sets + // one of those keys itself, so this is the single place to do it rather than each args + // builder. It does not restrict what the runnable is *given*: ordinary inputs pass + // through untouched. + args.sanitize_untrusted(); + let error_handler_args = error_handler_args.map(|args| { let args = args .0 diff --git a/backend/windmill-types/src/jobs.rs b/backend/windmill-types/src/jobs.rs index 9768373b91..17e5235c5f 100644 --- a/backend/windmill-types/src/jobs.rs +++ b/backend/windmill-types/src/jobs.rs @@ -724,6 +724,26 @@ pub const ENTRYPOINT_OVERRIDE: &str = "_ENTRYPOINT_OVERRIDE"; /// `RawCode::modules`: a caller-supplied one is dropped there. pub const MODULES_ARG: &str = "_MODULES"; +/// Job-arg key mapping a runnable's workspace imports to contents of the temporary script +/// store (`scripts/raw_temp/store`, writable by any workspace member). The preview routes +/// set it server-side from their `temp_script_refs` field; the bun and python executors +/// honor it on every `JobKind::Preview` job. +pub const TEMP_SCRIPT_REFS_ARG: &str = "_TEMP_SCRIPT_REFS"; + +/// Job-arg keys that steer *how* a runnable runs rather than what it is given: which +/// function the worker calls instead of `main` ([`ENTRYPOINT_OVERRIDE`]), what module files +/// are written into the job directory ([`MODULES_ARG`]) and where its workspace imports +/// resolve from ([`TEMP_SCRIPT_REFS_ARG`]). +/// +/// Each is legitimate for a caller that runs code it authored under its own identity (the +/// script editor's preprocessor test, dynamic-select helpers, the CLI, workflow-as-code), so +/// they are not dropped for every job. None of them may come from a third party whose +/// payload runs under somebody else's identity — an HTTP route body, a native trigger +/// message, a public app viewer's inputs — which is what `sanitize_untrusted_args` in +/// `windmill_common::jobs` drops. +pub const RESERVED_CONTROL_ARGS: [&str; 3] = + [ENTRYPOINT_OVERRIDE, MODULES_ARG, TEMP_SCRIPT_REFS_ARG]; + /// Reserved job-arg key holding the inbound W3C `traceparent` captured from the /// request that enqueued the job (run endpoints). It rides the `args` jsonb like /// [`ENTRYPOINT_OVERRIDE`]; normal scripts never see it because args are bound by