diff --git a/backend/windmill-common/src/git_sync_oss.rs b/backend/windmill-common/src/git_sync_oss.rs index 1e17532ec7..5f8ba771fb 100644 --- a/backend/windmill-common/src/git_sync_oss.rs +++ b/backend/windmill-common/src/git_sync_oss.rs @@ -38,6 +38,73 @@ pub fn sanitize_git_url(url: &str) -> String { GIT_URL_USERINFO_RE.replace(url, "://***@").into_owned() } +/// Validate a user-supplied git remote URL before it is handed to `git` (`clone`, +/// `ls-remote`, `remote add`, `fetch`, ...). Two classes of abuse are rejected: +/// - Argument injection: a URL that git parses as a command-line option (e.g. +/// `--upload-pack=`) turns `git ls-remote HEAD` into arbitrary command +/// execution on the worker host, outside any job sandbox. +/// - Dangerous transports: git's remote-helper syntax (`ext::sh -c ...`, `fd::...`) runs +/// arbitrary programs, and `file://` / local paths read host files — both escape the +/// intended network-only fetch. +/// +/// Only the standard network transports are allowed: `http(s)`, `ssh`, `git`, and the +/// scp-like `[user@]host:path` shorthand. Validation is transport-syntax based (not git +/// version dependent) so it holds regardless of git's own option/protocol handling. +pub fn validate_git_repo_url(url: &str) -> crate::error::Result<()> { + let reject = + |msg: &str| crate::error::Error::BadRequest(format!("Invalid git repository URL: {msg}")); + + let trimmed = url.trim(); + if trimmed.is_empty() { + return Err(reject("the URL is empty")); + } + // Leading '-' makes git parse the URL as an option (argument injection). + if trimmed.starts_with('-') { + return Err(reject("the URL must not start with '-'")); + } + // `::
` remote-helper transports execute arbitrary programs. + if trimmed.contains("::") { + return Err(reject("remote-helper transports (`::`) are not allowed")); + } + + if let Some((scheme, _rest)) = trimmed.split_once("://") { + // A real scheme is ASCII-alnum plus `+ - .` and holds no slash (a slash means the + // `://` came from the path, so there is no scheme and this is not a valid URL). + let is_scheme = !scheme.is_empty() + && !scheme.contains('/') + && scheme + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '+' | '-' | '.')); + if !is_scheme { + return Err(reject("malformed URL scheme")); + } + match scheme.to_ascii_lowercase().as_str() { + "http" | "https" | "ssh" | "git" => Ok(()), + other => Err(reject(&format!( + "scheme `{other}` is not allowed (use http(s), ssh, or git)" + ))), + } + } else { + // No scheme: accept only the scp-like `[user@]host:path` shorthand. The host (the + // part before the first `:`) must be non-empty and slash-free; a slash there means a + // local path (`./repo`, `/abs/repo`), and a single-letter host is a Windows drive. + let Some((host, _path)) = trimmed.split_once(':') else { + return Err(reject( + "local paths are not allowed; use an http(s), ssh, or git URL", + )); + }; + let bad_host = host.is_empty() + || host.contains('/') + || (host.len() == 1 && host.chars().all(|c| c.is_ascii_alphabetic())); + if bad_host { + return Err(reject( + "local paths are not allowed; use an http(s), ssh, or git URL", + )); + } + Ok(()) + } +} + pub fn prepend_token_to_github_url( github_url: &str, installation_token: &str, @@ -92,4 +159,51 @@ mod tests { "not a url://***@host/repo" ); } + + use super::validate_git_repo_url; + + #[test] + fn accepts_standard_transports() { + for url in [ + "https://github.com/org/repo.git", + "http://internal.example/org/repo.git", + "https://user:token@github.com/org/repo.git", + "ssh://git@github.com/org/repo.git", + "ssh://git@github.com:2222/org/repo.git", + "git://github.com/org/repo.git", + "git@github.com:org/repo.git", + "user@host.example:path/to/repo", + ] { + assert!(validate_git_repo_url(url).is_ok(), "should accept {url}"); + } + } + + #[test] + fn rejects_argument_injection() { + for url in [ + "--upload-pack=touch /tmp/pwned", + "-oProxyCommand=touch /tmp/pwned", + "--config=core.fsmonitor=touch /tmp/pwned", + ] { + assert!(validate_git_repo_url(url).is_err(), "should reject {url}"); + } + } + + #[test] + fn rejects_remote_helpers_and_local_transports() { + for url in [ + "ext::sh -c 'id > /tmp/pwned'", + "fd::17/foo", + "file:///etc/passwd", + "/etc/passwd", + "./local/repo", + "../local/repo", + "ftp://host/repo", + "C:\\path\\to\\repo", + "", + " ", + ] { + assert!(validate_git_repo_url(url).is_err(), "should reject {url:?}"); + } + } } diff --git a/backend/windmill-worker/src/ansible_executor.rs b/backend/windmill-worker/src/ansible_executor.rs index 05bcf930fb..783f472922 100644 --- a/backend/windmill-worker/src/ansible_executor.rs +++ b/backend/windmill-worker/src/ansible_executor.rs @@ -13,7 +13,7 @@ use tokio::process::Command; use uuid::Uuid; use windmill_common::{ error, - git_sync_oss::{prepend_token_to_github_url, sanitize_git_url}, + git_sync_oss::{prepend_token_to_github_url, sanitize_git_url, validate_git_repo_url}, worker::{ is_allowed_file_location, split_python_requirements, to_raw_value, write_file, write_file_at_user_defined_location, Connection, PyVAlias, WORKER_CONFIG, @@ -247,6 +247,24 @@ async fn prepare_socket_root(root: &str, stale_after: std::time::Duration) { } } +/// Validate every user-controlled field of a `GitRepo` before it reaches `git`. The `url` goes +/// through the transport allowlist (`validate_git_repo_url`); `branch` and `commit` are passed as +/// positional/option arguments, so a leading `-` would let git parse them as options (argument +/// injection). Called at each entry point that spawns `git` with these fields. +fn validate_git_repo(repo: &GitRepo) -> error::Result<()> { + validate_git_repo_url(&repo.url)?; + for (field, value) in [("branch", &repo.branch), ("commit", &repo.commit)] { + if let Some(value) = value { + if value.trim_start().starts_with('-') { + return Err(error::Error::BadRequest(format!( + "Invalid git repository `{field}`: must not start with '-'" + ))); + } + } + } + Ok(()) +} + async fn clone_repo( repo: &GitRepo, job_dir: &str, @@ -259,6 +277,7 @@ async fn clone_repo( occupancy_metrics: &mut OccupancyMetrics, git_ssh_cmd: &str, ) -> error::Result { + validate_git_repo(repo)?; let target_path = is_allowed_file_location(job_dir, &repo.target_path)?; let mut clone_cmd = Command::new(GIT_PATH.as_str()); @@ -400,6 +419,7 @@ async fn clone_repo_without_history( occupancy_metrics: &mut OccupancyMetrics, git_ssh_cmd: &str, ) -> error::Result<()> { + validate_git_repo(repo)?; let target_path = is_allowed_file_location(job_dir, &repo.target_path)?; create_empty_dir(&target_path)?; @@ -926,6 +946,7 @@ pub async fn get_git_repo_full_head_commit_hash( repo: &GitRepo, git_ssh_cmd: &str, ) -> anyhow::Result { + validate_git_repo(repo)?; let mut git_cmd = Command::new(GIT_PATH.as_str()); git_cmd