diff --git a/backend/tests/jobs_cross_site_get.rs b/backend/tests/jobs_cross_site_get.rs new file mode 100644 index 0000000000..20aa09a0a5 --- /dev/null +++ b/backend/tests/jobs_cross_site_get.rs @@ -0,0 +1,138 @@ +//! Regression test for cross-site GET CSRF on the job-run endpoints that can run a Hub script. +//! +//! `run_wait_result/p/{path}` and `run_and_stream/p/{path}` answer GET and, for a `hub/` path, +//! run any public Hub script. The session cookie is `SameSite=Lax`, so a browser attaches it +//! to a cross-site top-level GET navigation, and an argument written `$var:` or +//! `$res:` is resolved as the caller: an attacker page could make a logged-in browser +//! run a generic Hub script and hand it the victim's secrets. CORS hides the response but not +//! the side effect. +//! +//! `CrossSiteGetGuard` refuses such a request. Workspace scripts are deliberately not refused, +//! since they only run code the workspace's own members deployed. A request carrying its own +//! credential is allowed; the one case that regresses is a signed-in user clicking a `?token=` +//! Hub-script link from another site, because `extract_token` gives the cookie precedence and +//! exempting the parameter would let `?token=junk` reinstate the vector. +//! +//! This test pins down: +//! - both endpoints refuse a cross-site cookie GET to a Hub script (the core fix), whether +//! `Sec-Fetch-Site` says so or, with no such header (plain http), a cross-host `Referer`, +//! - a junk `token` query parameter does not buy a pass, +//! - the scope: the same request to a workspace script is not refused, +//! - a Hub-script request with its own credential (bearer, or `?token=` and no cookie), or +//! sent as a POST, gets through. +//! +//! No runnable exists and no Hub is contacted. A request that gets past the guard fails as +//! not-found on a workspace path, and on the non-numeric version in `hub/x/...` for a Hub +//! path, which is rejected while resolving the runnable, before any call to the Hub. + +use reqwest::StatusCode; +use sqlx::{Pool, Postgres}; +use windmill_test_utils::*; + +const HUB_GETS: [&str; 2] = [ + "run_wait_result/p/hub/x/absent", + "run_and_stream/p/hub/x/absent", +]; +const WORKSPACE_GETS: [&str; 2] = [ + "run_wait_result/p/u/test-user/absent", + "run_and_stream/p/u/test-user/absent", +]; + +async fn send(req: reqwest::RequestBuilder) -> anyhow::Result<(StatusCode, String)> { + let resp = req.send().await?; + let status = resp.status(); + Ok((status, resp.text().await?)) +} + +#[sqlx::test(fixtures("base"))] +async fn test_cross_site_get_cannot_run_hub_scripts(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + + let server = ApiServer::start(db.clone()).await?; + let base = format!( + "http://localhost:{}/api/w/test-workspace/jobs", + server.addr.port() + ); + let client = reqwest::Client::new(); + let cookie_get = |path: &str| { + client + .get(format!("{base}/{path}")) + .header("Cookie", "token=SECRET_TOKEN") + }; + + // ---- CORE REGRESSION: a cross-site cookie GET cannot run a Hub script. + for path in HUB_GETS { + let refused = [ + ( + "Sec-Fetch-Site: cross-site", + cookie_get(path).header("Sec-Fetch-Site", "cross-site"), + ), + // Plain http gets no `Sec-Fetch-*` at all, so `Referer` is the only signal left. + ( + "cross-host Referer with no Sec-Fetch-Site", + cookie_get(path).header("Referer", "http://attacker.example/page"), + ), + // The cookie outranks a `token` query parameter when authenticating. + ( + "junk ?token= next to the cookie", + client + .get(format!("{base}/{path}?token=junk")) + .header("Cookie", "token=SECRET_TOKEN") + .header("Sec-Fetch-Site", "cross-site"), + ), + ]; + for (name, req) in refused { + let (status, body) = send(req).await?; + assert_eq!( + status, + StatusCode::FORBIDDEN, + "{path} [{name}] must be refused: {body}" + ); + } + } + + // ---- Scope: the same request to a workspace script is not refused. + for path in WORKSPACE_GETS { + let (status, body) = send(cookie_get(path).header("Sec-Fetch-Site", "cross-site")).await?; + assert_eq!( + status, + StatusCode::NOT_FOUND, + "{path} is a workspace script and must reach the handler: {body}" + ); + } + + // ---- A Hub-script request that carries its own credential, or is a POST, gets through. + let hub = HUB_GETS[1]; + let allowed = [ + ( + "cross-origin bearer token", + client + .get(format!("{base}/{hub}")) + .header("Authorization", "Bearer SECRET_TOKEN") + .header("Sec-Fetch-Site", "cross-site"), + ), + ( + "cross-origin ?token= with no cookie", + client + .get(format!("{base}/{hub}?token=SECRET_TOKEN")) + .header("Sec-Fetch-Site", "cross-site"), + ), + ( + "POST with the cookie", + client + .post(format!("{base}/{hub}")) + .header("Cookie", "token=SECRET_TOKEN") + .header("Sec-Fetch-Site", "cross-site") + .json(&serde_json::json!({})), + ), + ]; + for (name, req) in allowed { + let (status, body) = send(req).await?; + assert!( + body.contains("Invalid hub script version"), + "{name} must get past the guard to runnable resolution (got {status}): {body}" + ); + } + + Ok(()) +} diff --git a/backend/windmill-api/src/csrf.rs b/backend/windmill-api/src/csrf.rs new file mode 100644 index 0000000000..4b2915f791 --- /dev/null +++ b/backend/windmill-api/src/csrf.rs @@ -0,0 +1,251 @@ +/* + * Author: Ruben Fiszel + * Copyright: Windmill Labs, Inc 2022 + * This file and its contents are licensed under the AGPLv3 License. + * Please see the included NOTICE for copyright information and + * LICENSE-AGPL for a copy of the license. + */ + +use axum::extract::FromRequestParts; +use axum::http::{header, request::Parts, Method}; +use axum::Extension; +use url::Url; +use windmill_common::error::Error; +use windmill_common::users::COOKIE_NAME; + +use crate::triggers::trigger_helpers::RunnableId; + +/// Whether a request is a cross-site GET authenticating on the session cookie alone. A GET +/// handler that runs a script by path resolves it through [`Self::script_runnable`], which +/// refuses a Hub script on such a request. +/// +/// The cookie is `SameSite=Lax`, so browsers attach it to cross-site top-level GET +/// navigations. A `hub/` path runs any public Hub script, and an argument written +/// `$var:` or `$res:` is resolved as the caller before the script sees it: such a +/// GET lets any page pick a generic Hub script and hand it the victim's secrets, which the job +/// can then send anywhere. +/// +/// Workspace scripts and flows are not refused, by choice, so that GET links to them keep +/// working. That is a scope decision, not a safety property: they still take attacker-chosen +/// arguments, `$var:` and `$res:` included, resolved as the victim. What bounds the exposure +/// is that the attacker needs a runnable path and can only run code the workspace deployed. +/// +/// The cookie is the only ambient credential. A bearer header is explicit, and so is the +/// `token` query parameter the webhook URLs carry — a cross-origin `EventSource` has no other +/// way to authenticate, since it cannot set headers. The checks run in `extract_token`'s +/// order, header before cookie, because that is the order it resolves them in: a request +/// carrying both a cookie and `token=` authenticates on the cookie and is therefore still +/// ambient, which is also why a valid `token=` link opened cross-site while signed in is +/// refused. +pub struct CrossSiteGetGuard(Option); + +impl CrossSiteGetGuard { + pub fn script_runnable(&self, script_path: &str) -> windmill_common::error::Result { + let runnable_id = RunnableId::from_script_path(script_path); + let (Some(signal), RunnableId::HubScript(_)) = (&self.0, &runnable_id) else { + return Ok(runnable_id); + }; + // The `Referer` leg is the one that can misfire, on a request that really was + // same-host: it compares against the hosts the backend can see, and a proxy that + // rewrites `Host` without setting `X-Forwarded-Host` leaves none of them matching + // what the browser addressed. Name the comparison so that shows up as a + // misconfiguration rather than as an unexplained 403. + if let CrossSite::RefererMismatch { referer, instance_hosts } = signal { + tracing::warn!( + referer_host = %referer, + ?instance_hosts, + "refusing a cross-site GET Hub script run inferred from Referer; if the request \ + was same-host, set `X-Forwarded-Host` on the proxy or configure `BASE_URL`" + ); + } + Err(Error::PermissionDenied( + "a cross-site GET request cannot run a Hub script with the session cookie, which takes \ + precedence over a `token` query parameter: pass the token in the `Authorization` \ + header, or open the link from the instance itself or from a browser with no Windmill \ + session" + .to_string(), + )) + } +} + +impl FromRequestParts for CrossSiteGetGuard { + type Rejection = std::convert::Infallible; + + async fn from_request_parts( + parts: &mut Parts, + state: &S, + ) -> std::result::Result { + if parts.method != Method::GET { + return Ok(CrossSiteGetGuard(None)); + } + let Some(signal) = cross_site_signal(parts) else { + return Ok(CrossSiteGetGuard(None)); + }; + + let has_bearer = parts + .headers + .get(header::AUTHORIZATION) + .and_then(|v| v.to_str().ok()) + .is_some_and(|v| v.starts_with("Bearer ")); + if has_bearer { + return Ok(CrossSiteGetGuard(None)); + } + + let has_session_cookie = + Extension::::from_request_parts(parts, state) + .await + .is_ok_and(|Extension(cookies)| cookies.get(COOKIE_NAME).is_some()); + Ok(CrossSiteGetGuard(has_session_cookie.then_some(signal))) + } +} + +enum CrossSite { + Declared, + RefererMismatch { referer: String, instance_hosts: Vec }, +} + +fn cross_site_signal(parts: &Parts) -> Option { + if let Some(site) = parts.headers.get("sec-fetch-site") { + return site + .as_bytes() + .eq_ignore_ascii_case(b"cross-site") + .then_some(CrossSite::Declared); + } + // Fetch Metadata rides only on potentially trustworthy URLs, so an instance served + // over plain http never receives `Sec-Fetch-Site` (nor does Safari before 16.4) while + // the cookie, not being `Secure` there either, still arrives. `Referer` is the only + // other thing a top-level GET navigation carries — `Origin` is not sent on one — so it + // is all that is left there, and it is weak: the default `strict-origin-when-cross- + // origin` policy already drops `Referer` on an https-to-http downgrade, so an https + // attacker page pointing a victim at a plain-http instance sends neither header. This + // leg catches an http-served attacker page and pre-16.4 Safari on https; the guard is + // load-bearing on https and best-effort at best on plain http. An absent `Referer` + // reads as not cross-site, matching how `Sec-Fetch-Site: none` (a bookmark, a typed + // URL) is treated. + let referer = referer_host(parts)?; + let instance_hosts: Vec = instance_hosts(parts).collect(); + (!instance_hosts + .iter() + .any(|host| host.eq_ignore_ascii_case(&referer))) + .then_some(CrossSite::RefererMismatch { referer, instance_hosts }) +} + +/// Every host a legitimate same-host request can name. `Host` alone is not enough: a +/// reverse proxy that forwards without preserving it (nginx `proxy_pass` with no +/// `proxy_set_header Host $host`) hands the backend the upstream's name, which no browser +/// `Referer` will ever match. None of these is browser-settable on a navigation — a +/// navigation carries no custom headers, and `BASE_URL` is instance config — so widening +/// the accepted set costs nothing. +fn instance_hosts(parts: &Parts) -> impl Iterator { + let base_url = windmill_common::BASE_URL.load(); + [ + request_host(parts), + header_host(parts, "x-forwarded-host"), + Url::parse(base_url.as_str()) + .ok() + .and_then(|url| url.host_str().map(str::to_owned)), + ] + .into_iter() + .flatten() +} + +fn referer_host(parts: &Parts) -> Option { + let referer = parts.headers.get(header::REFERER)?.to_str().ok()?; + Url::parse(referer).ok()?.host_str().map(str::to_owned) +} + +fn request_host(parts: &Parts) -> Option { + if let Some(host) = parts.uri.host() { + return Some(host.to_owned()); + } + header_host(parts, header::HOST) +} + +fn header_host(parts: &Parts, name: impl header::AsHeaderName) -> Option { + host_of(parts.headers.get(name)?.to_str().ok()?) +} + +/// The host in a `Host`-shaped header value: `host[:port]`, where `host` may be a bracketed +/// IPv6 literal, and where a chain of proxies appends to `X-Forwarded-Host` so only the +/// first entry is the one the browser addressed. The port is split off by the URL parser +/// rather than by hand-rolling the bracket rules. +fn host_of(value: &str) -> Option { + let host = value.split(',').next()?.trim(); + Url::parse(&format!("http://{host}")) + .ok()? + .host_str() + .map(str::to_owned) +} + +#[cfg(test)] +mod tests { + use super::{cross_site_signal, host_of, CrossSite}; + use axum::http::{request::Parts, Request}; + + fn parts(headers: &[(&str, &str)]) -> Parts { + let mut req = Request::get("/api/w/ws/jobs/run_wait_result/p/hub/1/x"); + for (name, value) in headers { + req = req.header(*name, *value); + } + req.body(()).unwrap().into_parts().0 + } + + #[test] + fn sec_fetch_site_decides_when_present() { + let declared = |site| cross_site_signal(&parts(&[("sec-fetch-site", site)])); + assert!(matches!(declared("cross-site"), Some(CrossSite::Declared))); + for site in ["same-origin", "same-site", "none"] { + assert!(declared(site).is_none(), "{site} is not cross-site"); + } + // The header outranks a `Referer` that disagrees with it. + let with_referer = parts(&[ + ("sec-fetch-site", "same-origin"), + ("host", "windmill.example"), + ("referer", "https://attacker.example/page"), + ]); + assert!(cross_site_signal(&with_referer).is_none()); + } + + #[test] + fn referer_stands_in_when_sec_fetch_site_is_absent() { + let signal = |headers: &[(&str, &str)]| cross_site_signal(&parts(headers)); + assert!(matches!( + signal(&[ + ("host", "windmill.example"), + ("referer", "https://attacker.example/p") + ]), + Some(CrossSite::RefererMismatch { .. }) + )); + // Ports differ between the frontend and the API, and do not make a request cross-site. + assert!(signal(&[ + ("host", "windmill.example:8000"), + ("referer", "http://windmill.example:3000/apps"), + ]) + .is_none()); + // A proxy that rewrote `Host` but forwarded the public name. + assert!(signal(&[ + ("host", "windmill-server.internal"), + ("x-forwarded-host", "windmill.example"), + ("referer", "https://windmill.example/apps"), + ]) + .is_none()); + assert!(signal(&[("host", "windmill.example")]).is_none()); + } + + #[test] + fn host_of_strips_port_brackets_and_proxy_chain() { + assert_eq!(host_of("windmill.example"), Some("windmill.example".into())); + assert_eq!( + host_of("windmill.example:8000"), + Some("windmill.example".into()) + ); + assert_eq!(host_of("[::1]:8000"), Some("[::1]".into())); + assert_eq!(host_of("[::1]"), Some("[::1]".into())); + assert_eq!( + host_of("windmill.example, proxy.internal"), + Some("windmill.example".into()) + ); + assert_eq!(host_of(""), None); + assert_eq!(host_of("not a host"), None); + } +} diff --git a/backend/windmill-api/src/jobs.rs b/backend/windmill-api/src/jobs.rs index 9a3db7961f..6d200b2915 100644 --- a/backend/windmill-api/src/jobs.rs +++ b/backend/windmill-api/src/jobs.rs @@ -73,6 +73,7 @@ use crate::{ args::{self, RawWebhookArgs}, auth::{OptTokened, Tokened}, concurrency_groups::join_concurrency_key, + csrf::CrossSiteGetGuard, db::{ApiAuthed, DB}, triggers::trigger_helpers::RunnableId, users::{ @@ -7447,6 +7448,7 @@ async fn log_job_view( } pub async fn run_wait_result_job_by_path_get( + cross_site: CrossSiteGetGuard, method: hyper::http::Method, authed: ApiAuthed, Extension(user_db): Extension, @@ -7459,6 +7461,7 @@ pub async fn run_wait_result_job_by_path_get( check_license_key_valid().await?; let script_path = script_path.to_path(); + let runnable_id = cross_site.script_runnable(script_path)?; check_scopes(&authed, || format!("jobs:run:scripts:{script_path}"))?; if method == http::Method::HEAD { @@ -7471,12 +7474,7 @@ pub async fn run_wait_result_job_by_path_get( args.body = args::Body::HashMap(payload_as_args); let args = args - .to_args_from_runnable( - &db, - &w_id, - RunnableId::from_script_path(script_path), - run_query.skip_preprocessor, - ) + .to_args_from_runnable(&db, &w_id, runnable_id, run_query.skip_preprocessor) .await?; check_queue_too_long(&db, QUEUE_LIMIT_WAIT_RESULT.or(run_query.queue_limit)).await?; @@ -7895,6 +7893,7 @@ pub async fn stream_flow_by_version( } pub async fn stream_script_by_path( + cross_site: CrossSiteGetGuard, authed: ApiAuthed, Extension(db): Extension, Extension(user_db): Extension, @@ -7903,12 +7902,13 @@ pub async fn stream_script_by_path( method: hyper::http::Method, args: RawWebhookArgs, ) -> error::Result { + let runnable_id = cross_site.script_runnable(script_path.to_path())?; stream_job( authed, db, user_db, w_id, - RunnableId::from_script_path(script_path.to_path()), + runnable_id, args, run_query, method == http::Method::GET, diff --git a/backend/windmill-api/src/lib.rs b/backend/windmill-api/src/lib.rs index f404d7be9b..fc2c773703 100644 --- a/backend/windmill-api/src/lib.rs +++ b/backend/windmill-api/src/lib.rs @@ -80,6 +80,7 @@ pub mod azure_proxy_ee; mod azure_proxy_oss; mod capture; mod concurrency_groups; +mod csrf; mod db; mod db_health; mod dbt;