mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-08-25 08:00:59 +00:00
fix(security): sanitize dependency names & connection strings against command/SQL injection (#9590)
* fix(security): sanitize dependency names & connection strings against command/SQL injection Follow-up to the PowerShell module-name injection fix (#9587, CWE-78): the same audit surfaced the identical "secondary identifier interpolated into an interpreter/SQL command without escaping" pattern in a few other executors. - R executor (the real twin, HIGH): package name/version parsed from a user-supplied renv.lock were interpolated raw into an `Rscript -e "...renv::install(\"{pkg}@{version}\"...)"` eval string. A double-quote in the name/version broke out → arbitrary R (unsandboxed under DISABLE_NSJAIL / non-Linux). Now validated in parse_renv_lock (charset) and escaped at the sink as defense-in-depth (also escapes the lib path, which holds backslashes on Windows). - DuckDB ATTACH (MED): the connection string built from resource fields (host/db/user/password) is embedded in a single-quoted DuckDB literal; escape quotes so a field value can't break out of the ATTACH statement. - PgDatabase::to_uri: URL-encode host and dbname (user/password already were), so '@'/'/'/'?'/'&' can't reshape the parsed URI (feeds live PG connect and DuckDB ATTACH). - DuckDB CREATE SECRET (FFI): wrap the interpolated S3 key/secret/endpoint in the existing sql_single_quote() helper, consistent with the resource-limits setup right above it. - PowerShell: also escape the configured private repo URL/PAT in the install template (same sink as the module names; the escape landed after #9587 was squash-merged so it was not in the merged change). Adds unit tests for the R validation/escaping. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(security): escape ducklake/snowflake/bigquery identifiers; preserve IPv6 host - to_uri: don't percent-encode bracketed IPv6 literal hosts ([::1]) — encoding their brackets/colons would stop them parsing as a host (review fix). - duckdb ducklake ATTACH: the catalog conn string, storage and data_path are embedded in single-quoted DuckDB literals; escape quotes so a resource field can't break out (the ducklake path bypassed the ATTACH escape added earlier). - snowflake: validate account_identifier (it forms the request hostname). - bigquery: validate project_id (it forms a request URL path segment). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ruben Fiszel
GitHub
@@ -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
|
||||
)
|
||||
}
|
||||
|
||||
@@ -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}
|
||||
);
|
||||
",
|
||||
|
||||
@@ -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/<project_id>/...).
|
||||
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::<String>()
|
||||
)));
|
||||
}
|
||||
|
||||
let mut sig = parse_bigquery_sig(&query)
|
||||
.map_err(|x| Error::ExecutionErr(x.to_string()))?
|
||||
.args;
|
||||
|
||||
@@ -638,9 +638,13 @@ async fn db_resource_to_attach_statements(
|
||||
db_type: &str,
|
||||
extra_args: Option<&str>,
|
||||
) -> Result<Vec<String>> {
|
||||
// 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)
|
||||
|
||||
@@ -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());
|
||||
|
||||
@@ -314,6 +314,38 @@ struct RenvPackage {
|
||||
dependencies: Vec<String>,
|
||||
}
|
||||
|
||||
/// 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::<String>(),
|
||||
version.chars().take(50).collect::<String>(),
|
||||
)));
|
||||
}
|
||||
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<Vec<RenvPackage>, Error> {
|
||||
let lock: serde_json::Value = serde_json::from_str(lockfile)
|
||||
@@ -378,6 +410,7 @@ fn parse_renv_lock(lockfile: &str) -> Result<Vec<RenvPackage>, 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");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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://<account_identifier>.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::<String>()
|
||||
)));
|
||||
}
|
||||
|
||||
let annotations = windmill_common::worker::SqlAnnotations::parse(query);
|
||||
let collection_strategy = if annotations.return_last_result {
|
||||
SqlResultCollectionStrategy::LastStatementAllRows
|
||||
|
||||
Reference in New Issue
Block a user