diff --git a/backend/windmill-common/src/lib.rs b/backend/windmill-common/src/lib.rs index ba42f13d80..1f7611a13a 100644 --- a/backend/windmill-common/src/lib.rs +++ b/backend/windmill-common/src/lib.rs @@ -579,13 +579,22 @@ impl PgDatabase { Some(s) => s.to_string(), None => "prefer".to_string(), }; + // Encode host/dbname too: an unencoded '@', '/', '?' or '&' would otherwise + // reshape the parsed URI (inject libpq params / alter host). Bracketed IPv6 + // literals ([::1]) are passed through unencoded — percent-encoding their + // '['/']'/':' would stop them parsing as a host. + let host = if self.host.starts_with('[') && self.host.ends_with(']') { + self.host.clone() + } else { + urlencoding::encode(&self.host).into_owned() + }; format!( "postgres://{user}:{password}@{host}:{port}/{dbname}?sslmode={sslmode}", user = urlencoding::encode(&self.user.as_deref().unwrap_or("postgres")), password = urlencoding::encode(&self.password.as_deref().unwrap_or("")), - host = &self.host, + host = host, port = self.port.unwrap_or(5432), - dbname = self.dbname, + dbname = urlencoding::encode(&self.dbname), sslmode = sslmode ) } diff --git a/backend/windmill-duckdb-ffi-internal/src/lib.rs b/backend/windmill-duckdb-ffi-internal/src/lib.rs index 594b0f6df4..f3ee59cab7 100644 --- a/backend/windmill-duckdb-ffi-internal/src/lib.rs +++ b/backend/windmill-duckdb-ffi-internal/src/lib.rs @@ -266,6 +266,12 @@ fn setup_duckdb_connection( .unwrap_or(("http", &base_internal_url)); let s3_endpoint_ssl = s3_endpoint_ssl == "https"; + // Escape values interpolated into single-quoted SQL literals, consistent with + // configure_duckdb_resource_limits above (a stray quote would break the statement). + let s3_access_key = sql_single_quote(s3_access_key); + let s3_secret_key = sql_single_quote(s3_secret_key); + let endpoint = sql_single_quote(&format!("{s3_endpoint}/api/w/{w_id}/s3_proxy")); + conn.execute_batch(&format!( "INSTALL httpfs; LOAD httpfs; INSTALL azure; LOAD azure; @@ -274,7 +280,7 @@ fn setup_duckdb_connection( PROVIDER config, KEY_ID '{s3_access_key}', SECRET '{s3_secret_key}', - ENDPOINT '{s3_endpoint}/api/w/{w_id}/s3_proxy', + ENDPOINT '{endpoint}', URL_STYLE path, USE_SSL {s3_endpoint_ssl} ); @@ -282,7 +288,7 @@ fn setup_duckdb_connection( TYPE gcs, KEY_ID '{s3_access_key}', SECRET '{s3_secret_key}', - ENDPOINT '{s3_endpoint}/api/w/{w_id}/s3_proxy', + ENDPOINT '{endpoint}', USE_SSL {s3_endpoint_ssl} ); ", diff --git a/backend/windmill-worker/src/bigquery_executor.rs b/backend/windmill-worker/src/bigquery_executor.rs index bfe0874f83..8a80de25ef 100644 --- a/backend/windmill-worker/src/bigquery_executor.rs +++ b/backend/windmill-worker/src/bigquery_executor.rs @@ -378,6 +378,19 @@ pub async fn do_bigquery( .await .map_err(|e| Error::ExecutionErr(e.to_string()))?; + // Validate before it is interpolated into request URLs as a path segment + // (https://bigquery.googleapis.com/.../projects//...). + if project_id.is_empty() + || !project_id + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '-' | '_' | ':')) + { + return Err(Error::ExecutionErr(format!( + "Invalid BigQuery project id '{}': only alphanumeric, '.', '-', '_' and ':' allowed", + project_id.chars().take(64).collect::() + ))); + } + let mut sig = parse_bigquery_sig(&query) .map_err(|x| Error::ExecutionErr(x.to_string()))? .args; diff --git a/backend/windmill-worker/src/duckdb_executor.rs b/backend/windmill-worker/src/duckdb_executor.rs index 4a019be9ab..1a474ee9a0 100644 --- a/backend/windmill-worker/src/duckdb_executor.rs +++ b/backend/windmill-worker/src/duckdb_executor.rs @@ -638,9 +638,13 @@ async fn db_resource_to_attach_statements( db_type: &str, extra_args: Option<&str>, ) -> Result> { + // Escape single quotes: the connection string is built from resource fields + // (host/db/user/password) and embedded in a single-quoted DuckDB literal, so an + // unescaped quote in any field would otherwise break out of the ATTACH statement. + let conn_str = format_attach_db_conn_str(db_resource, db_type)?.replace('\'', "''"); let attach_str = format!( "ATTACH '{}' as {} (TYPE {}{});", - format_attach_db_conn_str(db_resource, db_type)?, + conn_str, ident_name, db_type, extra_args.unwrap_or("") @@ -690,13 +694,18 @@ async fn transform_attach_ducklake( hidden_passwords.lock().unwrap().push(pwd.to_string()); } - let db_conn_str = format_attach_db_conn_str(ducklake.catalog_resource, db_type)?; + // Escape single quotes: db_conn_str, storage and data_path are embedded in + // single-quoted DuckDB literals below, so an unescaped quote in a resource + // field would break out of the ATTACH statement. + let db_conn_str = + format_attach_db_conn_str(ducklake.catalog_resource, db_type)?.replace('\'', "''"); let storage = ducklake .storage .storage .as_deref() - .unwrap_or(DEFAULT_STORAGE); - let data_path = ducklake.storage.path; + .unwrap_or(DEFAULT_STORAGE) + .replace('\'', "''"); + let data_path = ducklake.storage.path.replace('\'', "''"); let extra_args = if let Some(default_extra_args) = ducklake.extra_args { format!("{},{}", extra_args, default_extra_args) diff --git a/backend/windmill-worker/src/pwsh_executor.rs b/backend/windmill-worker/src/pwsh_executor.rs index 0bd02985ae..c089930b42 100644 --- a/backend/windmill-worker/src/pwsh_executor.rs +++ b/backend/windmill-worker/src/pwsh_executor.rs @@ -554,13 +554,17 @@ pub async fn handle_powershell_job( .replace("{job_id}", &job.id.to_string()) .replace("{has_private_repo}", &format!("${has_private_repo}")) .replace("{has_credentials}", &format!("${has_credentials}")) + // Escape single quotes: these are interpolated into single-quoted + // PowerShell literals ($privateRepoUrl/$privateRepoPat) in the install + // script, so an unescaped quote in the configured repo URL/PAT would + // break out of the literal (same sink as the module names above). .replace( "{private_repo_url}", - &powershell_repo_url.unwrap_or_default(), + &powershell_repo_url.unwrap_or_default().replace("'", "''"), ) .replace( "{private_repo_pat}", - &powershell_repo_pat.unwrap_or_default(), + &powershell_repo_pat.unwrap_or_default().replace("'", "''"), ) .replace("{modules}", &modules_list); let mut cmd = Command::new(POWERSHELL_PATH.as_str()); diff --git a/backend/windmill-worker/src/r_executor.rs b/backend/windmill-worker/src/r_executor.rs index 9b2bbc6874..0d4219c0df 100644 --- a/backend/windmill-worker/src/r_executor.rs +++ b/backend/windmill-worker/src/r_executor.rs @@ -314,6 +314,38 @@ struct RenvPackage { dependencies: Vec, } +/// Reject renv package names/versions that could break out of the R string +/// literal they are interpolated into (`renv::install("name@version", ...)`), +/// preventing command injection (CWE-78) via crafted renv.lock content. +/// CRAN package names are letters/digits/'.' starting with a letter; versions +/// are dotted numerics optionally with '-'/'_'/'+' separators. +fn validate_renv_package(name: &str, version: &str) -> Result<(), Error> { + let name_ok = name + .chars() + .next() + .map_or(false, |c| c.is_ascii_alphabetic()) + && name.chars().all(|c| c.is_ascii_alphanumeric() || c == '.'); + let version_ok = !version.is_empty() + && version + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '-' | '_' | '+')); + if !name_ok || !version_ok { + return Err(Error::ExecutionErr(format!( + "Invalid renv package name '{}' / version '{}': name must be alphanumeric or '.', \ + version alphanumeric or '.-_+'", + name.chars().take(50).collect::(), + version.chars().take(50).collect::(), + ))); + } + Ok(()) +} + +/// Escape a value for safe interpolation into an R double-quoted string literal: +/// backslash and double-quote are the only metacharacters inside `"..."`. +fn escape_r_double_quoted(s: &str) -> String { + s.replace('\\', "\\\\").replace('"', "\\\"") +} + /// Parse renv.lock JSON and extract package info including dependency edges. fn parse_renv_lock(lockfile: &str) -> Result, Error> { let lock: serde_json::Value = serde_json::from_str(lockfile) @@ -378,6 +410,7 @@ fn parse_renv_lock(lockfile: &str) -> Result, Error> { // Skip renv itself — it's already loaded and reinstalling it while // loaded triggers a noisy "Restart your R session" message. if !pkg_name.is_empty() && !version.is_empty() && pkg_name != "renv" { + validate_renv_package(&pkg_name, &version)?; result.push(RenvPackage { name: pkg_name, version, repo_url, dependencies }); } } @@ -493,9 +526,9 @@ async fn install<'a>( r#"options(renv.verbose = {verbose_r}, renv.config.install.verbose = {install_verbose_r}, renv.config.restart.enabled = FALSE); renv::install("{pkg}@{version}", library = "{lib}", dependencies = FALSE)"#, verbose_r = verbose_r, install_verbose_r = install_verbose_r, - pkg = dependency.custom_payload.pkg, - version = dependency.custom_payload.version, - lib = install_lib, + pkg = escape_r_double_quoted(&dependency.custom_payload.pkg), + version = escape_r_double_quoted(&dependency.custom_payload.version), + lib = escape_r_double_quoted(&install_lib), ), // install.packages fallback (no version pinning): // &format!( @@ -730,3 +763,65 @@ tryCatch({{ spread = spread, )) } + +#[cfg(test)] +mod tests { + use super::{escape_r_double_quoted, parse_renv_lock, validate_renv_package}; + + #[test] + fn test_validate_renv_package_accepts_real() { + for (n, v) in [ + ("ggplot2", "3.4.4"), + ("data.table", "1.14.8"), + ("Rcpp", "1.0.11"), + ("renv", "1.0.3"), + ("pkg", "1.2-3"), + ("x", "0.9.8.9000"), + ] { + assert!( + validate_renv_package(n, v).is_ok(), + "{n}@{v} should be valid" + ); + } + } + + #[test] + fn test_validate_renv_package_rejects_injection() { + // name breakouts, version breakouts, leading non-letter, empty + for (n, v) in [ + ("ggplot2\", library=system(\"id\"))#", "1.0"), + ("ok", "1.0\"); system(\"id\"); (\""), + ("ok", "1.0\\\"x"), + ("9pkg", "1.0"), + ("pkg name", "1.0"), + ("ok", ""), + ] { + assert!( + validate_renv_package(n, v).is_err(), + "{n:?}@{v:?} should be rejected" + ); + } + } + + #[test] + fn test_escape_r_double_quoted() { + assert_eq!(escape_r_double_quoted(r#"a"b"#), r#"a\"b"#); + assert_eq!(escape_r_double_quoted(r"a\b"), r"a\\b"); + // backslash escaped before quote so \" cannot be reinterpreted + assert_eq!(escape_r_double_quoted(r#"\""#), r#"\\\""#); + } + + #[test] + fn test_parse_renv_lock_rejects_unsafe_name() { + let lock = r#"{"Packages": {"evil": {"Package": "evil\"); system(\"id\"); (\"", "Version": "1.0"}}}"#; + assert!(parse_renv_lock(lock).is_err()); + } + + #[test] + fn test_parse_renv_lock_accepts_clean() { + let lock = r#"{"Packages": {"ggplot2": {"Package": "ggplot2", "Version": "3.4.4"}}}"#; + let pkgs = parse_renv_lock(lock).unwrap(); + assert_eq!(pkgs.len(), 1); + assert_eq!(pkgs[0].name, "ggplot2"); + } +} diff --git a/backend/windmill-worker/src/snowflake_executor.rs b/backend/windmill-worker/src/snowflake_executor.rs index 35051aee98..d3d4555f40 100644 --- a/backend/windmill-worker/src/snowflake_executor.rs +++ b/backend/windmill-worker/src/snowflake_executor.rs @@ -604,6 +604,24 @@ pub async fn do_snowflake( return Err(Error::BadRequest("Missing database argument".to_string())); }; + // Validate before it is interpolated into request URLs as the hostname + // (https://.snowflakecomputing.com/...). + if database.account_identifier.is_empty() + || !database + .account_identifier + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '-' | '_')) + { + return Err(Error::BadRequest(format!( + "Invalid Snowflake account identifier '{}': only alphanumeric, '.', '-' and '_' allowed", + database + .account_identifier + .chars() + .take(64) + .collect::() + ))); + } + let annotations = windmill_common::worker::SqlAnnotations::parse(query); let collection_strategy = if annotations.return_last_result { SqlResultCollectionStrategy::LastStatementAllRows