diff --git a/backend/THREAT_MODEL.md b/backend/THREAT_MODEL.md index 5a891bda77..8cc71ec71c 100644 --- a/backend/THREAT_MODEL.md +++ b/backend/THREAT_MODEL.md @@ -97,7 +97,7 @@ published advisory history (73 GHSA advisories, several rated 9.9 critical). | id | threat | actor | surface | asset | impact | likelihood | status | controls | evidence | |---|---|---|---|---|---|---|---|---|---| | T1 | SQL injection in app/internal query builders and trigger clauses compromises the metadata DB and connected databases | remote_auth | EP8 | Database, downstream connected systems | critical | almost_certain | partially_mitigated | sqlx parameterized queries elsewhere; query-builder safety reviews | GHSA-225c-j3xq-g6x6, GHSA-78p7-jc72-gv66, GHSA-hvc7-f67h-jx3g, GHSA-wrrg-f89m-f84q, GHSA-79vf-3qwm-2w64, GHSA-55p6-fxj4-v983, GHSA-5g4v-49rj-r52r, GHSA-x6cq-7xr8-53x3, 2cf4bb180b | -| T2 | Server-side request forgery via proxies/executors reaches cloud metadata, internal network, and downstream credentials | remote_auth | EP6, EP7 | Cloud metadata, internal network, downstream connected systems, resource creds | critical | almost_certain | partially_mitigated | SSRF URL validation + redirect-following disabled added piecemeal; outbound network isolation (`clone_newnet`) is opt-in and off by default | GHSA-3ggp-h37f-5qfw, GHSA-98qq-g8rh-xhff, GHSA-hfw8-27mx-63jm, GHSA-3r59-qvvc-774j, GHSA-4pj9-w5jc-g8w7, GHSA-8hh3-jf25-78j5, GHSA-3pjm-4w7f-3r2w, GHSA-f44c-x9hq-h68r, GHSA-j4h4-f8fj-3m3c, 4b06881918, 96a8eb63d4, dbd3942ef3 | +| T2 | Server-side request forgery via proxies/executors reaches cloud metadata, internal network, and downstream credentials | remote_auth | EP6, EP7 | Cloud metadata, internal network, downstream connected systems, resource creds | critical | almost_certain | partially_mitigated | SSRF URL validation + redirect-following disabled added piecemeal; MCP private URL access requires the instance-wide `ALLOW_PRIVATE_MCP_SERVER_URLS` opt-in; outbound network isolation (`clone_newnet`) is opt-in and off by default | GHSA-3ggp-h37f-5qfw, GHSA-98qq-g8rh-xhff, GHSA-hfw8-27mx-63jm, GHSA-3r59-qvvc-774j, GHSA-4pj9-w5jc-g8w7, GHSA-8hh3-jf25-78j5, GHSA-3pjm-4w7f-3r2w, GHSA-f44c-x9hq-h68r, GHSA-j4h4-f8fj-3m3c, 4b06881918, 96a8eb63d4, dbd3942ef3 | | T3 | Broken authorization / IDOR lets a scoped token or low-privilege member read scripts, job data, and secrets across folders and workspaces | remote_auth | EP5, EP2, EP1 | Scripts, job data, secrets, isolation | critical | almost_certain | partially_mitigated | RLS, token scopes, folder ACLs, view-token HMAC (added incrementally); on managed, sensitive tenants can opt into dedicated DB/worker/namespace, but the shared tier IS the software boundary | GHSA-qfg7-x243-5hg4, GHSA-8x8x-88qc-qp4r, GHSA-2ppx-66jv-wpw5, GHSA-x3x7-g97v-mp59, GHSA-j276-g4h8-g6h5, GHSA-8mv7-hmrg-96xv, GHSA-x2wf-f962-7frq, GHSA-qc7c-gcw6-h4xp, GHSA-vxc5-w28p-m9xw, GHSA-2g34-wfvr-5qqj, GHSA-w7p6-wpxm-pp66, 7edf3f0212, 89a7a37776, ab11c7747a, 664edcdfb7 | | T4 | Remote code execution by injecting attacker-controlled identifiers into generated worker wrappers | remote_auth | EP10 | Worker host, isolation, downstream | critical | likely | partially_mitigated | entrypoint/env-var-name validation added | GHSA-wxjq-w5pj-jqhx, GHSA-5f5q-2vg2-r2x4, GHSA-8q8j-mm3g-5c2q (CVE-2026-33881), bf93657fee, bd05bcadde, 22ec4da5f0 | | T5 | Worker compromise & cross-tenant access via weak-by-default isolation (nsjail off by default → user code runs with only PID-ns `unshare`); sandbox escape where nsjail/dind/podman is enabled | remote_auth | EP9, EP15 | Worker host, isolation, downstream | critical | likely | unmitigated | nsjail off by default everywhere (`DISABLE_NSJAIL=true`); shipped compose gives PID-ns `unshare` only (`FAVOR_UNSHARE_PID=true`), bare installs get no isolation. Where nsjail enabled: read-only remounts, jail-tmp refusal, podman socket gating | GHSA-6qr8-xhg4-453q, GHSA-3vpp-vf62-wqp6, f8467f38c8, df5aec0f5d, f1b6746e0e | diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 9dad41fdc8..84d5414fa2 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -2c7964460327fab5e3a27c0f74b8d6f26ab7f79a +481ea7f28dc5af6b72390c82f494f34cb9809546 diff --git a/backend/windmill-common/src/ssrf.rs b/backend/windmill-common/src/ssrf.rs index 507a773431..2f100d5ae1 100644 --- a/backend/windmill-common/src/ssrf.rs +++ b/backend/windmill-common/src/ssrf.rs @@ -2,6 +2,8 @@ use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; use crate::error::Error; +pub const ALLOW_PRIVATE_MCP_SERVER_URLS_ENV: &str = "ALLOW_PRIVATE_MCP_SERVER_URLS"; + /// Why a URL failed SSRF validation. /// /// The distinction matters for callers that gate private endpoints behind a @@ -116,6 +118,49 @@ pub async fn validate_url_for_ssrf(url: &str) -> Result<(), SsrfValidationError> Ok(()) } +pub fn allow_private_mcp_server_urls() -> bool { + std::env::var(ALLOW_PRIVATE_MCP_SERVER_URLS_ENV) + .ok() + .is_some_and(|v| v == "true" || v == "1") +} + +pub async fn validate_mcp_server_url(url: &str) -> Result<(), SsrfValidationError> { + let parsed = + url::Url::parse(url).map_err(|e| SsrfValidationError::InvalidUrl(e.to_string()))?; + + match parsed.scheme() { + "http" | "https" => {} + scheme => return Err(SsrfValidationError::DisallowedScheme(scheme.to_string())), + } + + parsed.host_str().ok_or(SsrfValidationError::MissingHost)?; + + if allow_private_mcp_server_urls() { + return Ok(()); + } + + validate_url_for_ssrf(url).await +} + +pub async fn validate_mcp_server_url_for_bad_request(url: &str, label: &str) -> Result<(), Error> { + validate_mcp_server_url(url).await.map_err(|e| { + Error::BadRequest(format!( + "{label} is not allowed: {}", + mcp_ssrf_error_message(&e) + )) + }) +} + +pub fn mcp_ssrf_error_message(e: &SsrfValidationError) -> String { + match e { + SsrfValidationError::Private { .. } => format!( + "{e}. If you need to use private/internal MCP server URLs, \ + set the {ALLOW_PRIVATE_MCP_SERVER_URLS_ENV}=true environment variable" + ), + _ => e.to_string(), + } +} + fn is_private_ip(ip: &IpAddr) -> bool { match ip { IpAddr::V4(ipv4) => is_private_ipv4(ipv4), @@ -152,6 +197,32 @@ fn is_private_ipv6(ip: &Ipv6Addr) -> bool { mod tests { use super::*; + static TEST_ENV_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); + + struct PrivateMcpServerUrlsEnvGuard { + previous: Option, + } + + impl PrivateMcpServerUrlsEnvGuard { + fn set(value: Option<&str>) -> Self { + let previous = std::env::var(ALLOW_PRIVATE_MCP_SERVER_URLS_ENV).ok(); + match value { + Some(value) => std::env::set_var(ALLOW_PRIVATE_MCP_SERVER_URLS_ENV, value), + None => std::env::remove_var(ALLOW_PRIVATE_MCP_SERVER_URLS_ENV), + } + Self { previous } + } + } + + impl Drop for PrivateMcpServerUrlsEnvGuard { + fn drop(&mut self) { + match &self.previous { + Some(value) => std::env::set_var(ALLOW_PRIVATE_MCP_SERVER_URLS_ENV, value), + None => std::env::remove_var(ALLOW_PRIVATE_MCP_SERVER_URLS_ENV), + } + } + } + #[test] fn test_private_ipv4() { assert!(is_private_ipv4(&"127.0.0.1".parse().unwrap())); @@ -227,4 +298,66 @@ mod tests { Err(SsrfValidationError::Private { resolved: false }) )); } + + #[tokio::test] + async fn validate_mcp_server_url_blocks_private_by_default() { + let _lock = TEST_ENV_LOCK.lock().await; + let _guard = PrivateMcpServerUrlsEnvGuard::set(None); + + assert!(matches!( + validate_mcp_server_url("http://127.0.0.1/foo").await, + Err(SsrfValidationError::Private { resolved: false }) + )); + } + + #[tokio::test] + async fn validate_mcp_server_url_allows_private_when_env_is_enabled() { + let _lock = TEST_ENV_LOCK.lock().await; + let _guard = PrivateMcpServerUrlsEnvGuard::set(Some("true")); + + assert!(validate_mcp_server_url("http://127.0.0.1/foo") + .await + .is_ok()); + } + + #[tokio::test] + async fn validate_mcp_server_url_allows_private_when_env_is_one() { + let _lock = TEST_ENV_LOCK.lock().await; + let _guard = PrivateMcpServerUrlsEnvGuard::set(Some("1")); + + assert!(validate_mcp_server_url("http://10.0.0.1/foo").await.is_ok()); + } + + #[tokio::test] + async fn validate_mcp_server_url_keeps_syntax_guards_when_private_urls_are_allowed() { + let _lock = TEST_ENV_LOCK.lock().await; + let _guard = PrivateMcpServerUrlsEnvGuard::set(Some("true")); + + assert!(matches!( + validate_mcp_server_url("localhost:11434/v1").await, + Err(SsrfValidationError::DisallowedScheme(_)) + )); + assert!(matches!( + validate_mcp_server_url("file:///tmp/socket").await, + Err(SsrfValidationError::DisallowedScheme(_)) + )); + } + + #[tokio::test] + async fn private_mcp_error_message_includes_env_hint_only_for_private_urls() { + let _lock = TEST_ENV_LOCK.lock().await; + let _guard = PrivateMcpServerUrlsEnvGuard::set(None); + + let private_error = validate_mcp_server_url("http://127.0.0.1/foo") + .await + .unwrap_err(); + assert!( + mcp_ssrf_error_message(&private_error).contains("ALLOW_PRIVATE_MCP_SERVER_URLS=true") + ); + + let invalid_error = validate_mcp_server_url("localhost:11434/v1") + .await + .unwrap_err(); + assert!(!mcp_ssrf_error_message(&invalid_error).contains(ALLOW_PRIVATE_MCP_SERVER_URLS_ENV)); + } } diff --git a/backend/windmill-mcp/src/client/mod.rs b/backend/windmill-mcp/src/client/mod.rs index 1a555c141f..34dc141209 100644 --- a/backend/windmill-mcp/src/client/mod.rs +++ b/backend/windmill-mcp/src/client/mod.rs @@ -43,9 +43,14 @@ impl McpClient { // The resource URL is author-controlled and we send a (potentially // secret) bearer token to it, so it must be validated against SSRF // before we connect (e.g. cloud metadata endpoints, internal services). - windmill_common::ssrf::validate_url_for_ssrf(&resource.url) + windmill_common::ssrf::validate_mcp_server_url(&resource.url) .await - .map_err(|e| anyhow::anyhow!("MCP server URL is not allowed: {}", e))?; + .map_err(|e| { + anyhow::anyhow!( + "MCP server URL is not allowed: {}", + windmill_common::ssrf::mcp_ssrf_error_message(&e) + ) + })?; // Build custom reqwest client with headers if provided let mut headers = HeaderMap::new(); @@ -230,6 +235,33 @@ impl McpClient { mod tests { use super::*; + struct PrivateMcpServerUrlsEnvGuard { + previous: Option, + } + + impl PrivateMcpServerUrlsEnvGuard { + fn unset() -> Self { + let previous = + std::env::var(windmill_common::ssrf::ALLOW_PRIVATE_MCP_SERVER_URLS_ENV).ok(); + std::env::remove_var(windmill_common::ssrf::ALLOW_PRIVATE_MCP_SERVER_URLS_ENV); + Self { previous } + } + } + + impl Drop for PrivateMcpServerUrlsEnvGuard { + fn drop(&mut self) { + match &self.previous { + Some(value) => std::env::set_var( + windmill_common::ssrf::ALLOW_PRIVATE_MCP_SERVER_URLS_ENV, + value, + ), + None => { + std::env::remove_var(windmill_common::ssrf::ALLOW_PRIVATE_MCP_SERVER_URLS_ENV) + } + } + } + } + /// Regression test: `from_resource` must refuse to connect to a URL that /// targets a private/internal address (here the AWS /// instance-metadata endpoint), so a resource author cannot use the MCP @@ -237,6 +269,8 @@ mod tests { /// before any connection attempt, so this fails fast without network access. #[tokio::test] async fn from_resource_rejects_ssrf_url() { + let _guard = PrivateMcpServerUrlsEnvGuard::unset(); + let resource = McpResource { name: "evil".to_string(), url: "http://169.254.169.254".to_string(), diff --git a/backend/windmill-mcp/src/client_registration.rs b/backend/windmill-mcp/src/client_registration.rs index eca4ed650f..b0b07cada1 100644 --- a/backend/windmill-mcp/src/client_registration.rs +++ b/backend/windmill-mcp/src/client_registration.rs @@ -17,7 +17,7 @@ use windmill_common::db::DB; use windmill_common::error; use windmill_common::variables::{build_crypt, decrypt, encrypt}; -use crate::oauth::AuthorizationManager; +use crate::oauth::{no_redirect_http_client, AuthorizationManager}; /// MCP client credentials returned by [`get_or_refresh_mcp_client`]. pub struct McpClientCredentials { @@ -77,7 +77,14 @@ async fn register_client( redirect_uri: &str, client_name: &str, ) -> Result { - let client = reqwest::Client::new(); + windmill_common::ssrf::validate_mcp_server_url_for_bad_request( + registration_endpoint, + "MCP server registration endpoint URL", + ) + .await?; + + let client = no_redirect_http_client() + .map_err(|e| error::Error::BadRequest(format!("Failed to build DCR client: {e}")))?; let request = DcrRequest { client_name: client_name.to_string(), redirect_uris: vec![redirect_uri.to_string()], @@ -121,6 +128,12 @@ pub async fn get_or_refresh_mcp_client( let base_url = (**windmill_common::BASE_URL.load()).clone(); let redirect_uri = format!("{}/api/mcp/oauth/callback", base_url); + windmill_common::ssrf::validate_mcp_server_url_for_bad_request( + mcp_server_url, + "MCP server URL", + ) + .await?; + let cached_client: Option = sqlx::query_as("SELECT mcp_server_url, client_id, client_secret, client_secret_expires_at, token_endpoint FROM mcp_oauth_client WHERE mcp_server_url = $1") .bind(mcp_server_url) @@ -131,6 +144,11 @@ pub async fn get_or_refresh_mcp_client( if let Some(client) = cached_client { if !client.is_expired() { tracing::debug!("Using cached MCP client for {}", mcp_server_url); + windmill_common::ssrf::validate_mcp_server_url_for_bad_request( + &client.token_endpoint, + "MCP server token endpoint URL", + ) + .await?; let decrypted_secret = if let Some(ref encrypted_secret) = client.client_secret { Some(decrypt_client_secret(db, encrypted_secret).await?) } else { @@ -145,17 +163,27 @@ pub async fn get_or_refresh_mcp_client( tracing::debug!("Cached MCP client expired, re-registering"); } - windmill_common::ssrf::validate_url_for_ssrf(mcp_server_url).await?; - - let manager = AuthorizationManager::new(mcp_server_url) + let mut manager = AuthorizationManager::new(mcp_server_url) .await .map_err(|e| error::Error::BadRequest(format!("Failed to create auth manager: {e}")))?; + let discovery_client = no_redirect_http_client().map_err(|e| { + error::Error::BadRequest(format!("Failed to build MCP OAuth discovery client: {e}")) + })?; + manager + .with_client(discovery_client) + .map_err(|e| error::Error::BadRequest(format!("Failed to configure auth manager: {e}")))?; let metadata = manager .discover_metadata() .await .map_err(|e| error::Error::BadRequest(format!("OAuth discovery failed: {e}")))?; + windmill_common::ssrf::validate_mcp_server_url_for_bad_request( + &metadata.token_endpoint, + "MCP server token endpoint URL", + ) + .await?; + let supports_dynamic_registration = metadata.registration_endpoint.is_some(); let (client_id, client_secret, expires_at) = if supports_dynamic_registration { diff --git a/backend/windmill-mcp/src/lib.rs b/backend/windmill-mcp/src/lib.rs index 7df6ee9f39..d9cae883de 100644 --- a/backend/windmill-mcp/src/lib.rs +++ b/backend/windmill-mcp/src/lib.rs @@ -38,11 +38,70 @@ pub mod client_registration; pub mod oauth { //! Re-exports of rmcp auth and oauth2 types for MCP OAuth implementations + use std::time::Duration; + pub use rmcp::transport::auth::AuthorizationManager; + const DEFAULT_OAUTH_HTTP_TIMEOUT: Duration = Duration::from_secs(30); + + pub fn no_redirect_http_client() -> Result { + no_redirect_http_client_with_timeout(DEFAULT_OAUTH_HTTP_TIMEOUT) + } + + pub(crate) fn no_redirect_http_client_with_timeout( + timeout: Duration, + ) -> Result { + reqwest::Client::builder() + .timeout(timeout) + .redirect(reqwest::redirect::Policy::none()) + .build() + } + // Re-export oauth2 types needed for MCP OAuth flow pub use oauth2::{ basic::BasicClient, AuthUrl, ClientId, ClientSecret, CsrfToken, PkceCodeChallenge, RedirectUrl, Scope, TokenUrl, }; + + #[cfg(test)] + mod tests { + use super::*; + use std::{ + io::Read, + net::TcpListener, + thread, + time::{Duration, Instant}, + }; + + #[tokio::test] + async fn no_redirect_http_client_times_out_stalled_responses() { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = listener.local_addr().unwrap(); + + let handle = thread::spawn(move || { + if let Ok((mut stream, _)) = listener.accept() { + let _ = stream.set_read_timeout(Some(Duration::from_millis(200))); + let mut buffer = [0; 1024]; + let _ = stream.read(&mut buffer); + thread::sleep(Duration::from_millis(300)); + } + }); + + let client = no_redirect_http_client_with_timeout(Duration::from_millis(50)).unwrap(); + let started = Instant::now(); + let err = client + .get(format!("http://{addr}/stall")) + .send() + .await + .expect_err("stalled response should time out"); + + assert!(err.is_timeout(), "expected timeout error, got: {err}"); + assert!( + started.elapsed() < Duration::from_secs(2), + "stalled request should fail promptly" + ); + + handle.join().unwrap(); + } + } }