From 793a842ffefff3c1d544d6a3bdd195d9f3c2943a Mon Sep 17 00:00:00 2001 From: James Devine Date: Tue, 14 Apr 2026 12:56:33 +0100 Subject: [PATCH] fix: address injection vulnerabilities from red team audit (#171) - Validate engine.model against [A-Za-z0-9._:-]+ to prevent shell injection via single-quote breakout in AWF command (Finding 1, High) - Validate tools.bash entries reject single quotes (same attack surface) - Reject ADO template expressions (${{ and $() in front matter name and description fields to prevent secret disclosure (Finding 2, Medium) - Reject newlines in name/description to prevent YAML structure injection - Escape single quotes in pipeline trigger config by doubling them per YAML spec (Finding 3, Medium) - Quote {{ allowed_domains }} in base.yml template to prevent AWF argument injection via spaces in domain patterns (Finding 4, Medium) - Clarify network.blocked docs: exact-match removal, not wildcard-aware blocking (Finding 6, Low) - Add 9 security validation test cases Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- AGENTS.md | 2 +- README.md | 2 +- src/compile/common.rs | 199 +++++++++++++++++++++++++++++++++----- src/compile/onees.rs | 11 ++- src/compile/standalone.rs | 29 ++++-- templates/base.yml | 4 +- 6 files changed, 210 insertions(+), 37 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index cd65383..61b4173 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -189,7 +189,7 @@ teardown: # separate job AFTER safe outputs processing network: # optional network policy (standalone target only) allow: # additional allowed host patterns - "*.mycompany.com" - blocked: # blocked host patterns (takes precedence over allow) + blocked: # blocked host patterns (removes exact entries from the allow list) - "evil.example.com" permissions: # optional ADO access token configuration read: my-read-arm-connection # ARM service connection for read-only ADO access (Stage 1 agent) diff --git a/README.md b/README.md index 09342ca..d8846c5 100644 --- a/README.md +++ b/README.md @@ -378,7 +378,7 @@ reachable. The allowlist is built from: 1. **Core domains** — Azure DevOps, GitHub, Microsoft auth, Azure storage 2. **MCP domains** — automatically added per enabled MCP 3. **User domains** — from `network.allow` in front matter -4. **Minus blocked** — `network.blocked` entries are removed +4. **Minus blocked** — `network.blocked` entries are removed by exact match (wildcard patterns like `*.example.com` are not affected by blocking a specific subdomain) ```yaml network: diff --git a/src/compile/common.rs b/src/compile/common.rs index ff20b52..cae57eb 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -145,14 +145,15 @@ fn is_valid_parameter_name(name: &str) -> bool { && chars.all(|c| c.is_ascii_alphanumeric() || c == '_') } -/// Reject ADO template expressions (`${{`) and macro expressions (`$(`) in a string value. -/// Parameter definitions should only contain literal values — expressions could enable -/// information disclosure or logic manipulation in the generated pipeline. +/// Reject ADO template expressions (`${{`), macro expressions (`$(`), and runtime +/// expressions (`$[`) in a string value. Parameter definitions should only contain +/// literal values — expressions could enable information disclosure or logic manipulation +/// in the generated pipeline. fn reject_ado_expressions(value: &str, param_name: &str, field_name: &str) -> Result<()> { - if value.contains("${{") || value.contains("$(") { + if value.contains("${{") || value.contains("$(") || value.contains("$[") { anyhow::bail!( - "Parameter '{}' field '{}' contains an ADO expression ('${{{{' or '$(') which is not \ - allowed in parameter definitions. Use literal values only.", + "Parameter '{}' field '{}' contains an ADO expression ('${{{{', '$(', or '$[') which \ + is not allowed in parameter definitions. Use literal values only.", param_name, field_name, ); @@ -160,6 +161,32 @@ fn reject_ado_expressions(value: &str, param_name: &str, field_name: &str) -> Re Ok(()) } +/// Validate front matter `name` and `description` fields. +/// +/// These values are substituted directly into the pipeline YAML template and must not +/// contain ADO expressions (`${{`, `$(`, `$[`) which could disclose secrets or manipulate +/// pipeline logic. Newlines are also rejected to prevent YAML structure injection. +pub fn validate_front_matter_identity(front_matter: &FrontMatter) -> Result<()> { + for (field, value) in [("name", &front_matter.name), ("description", &front_matter.description)] { + if value.contains("${{") || value.contains("$(") || value.contains("$[") { + anyhow::bail!( + "Front matter '{}' contains an ADO expression ('${{{{', '$(', or '$[') which is not allowed. \ + Use literal values only. Found: '{}'", + field, + value, + ); + } + if value.contains('\n') || value.contains('\r') { + anyhow::bail!( + "Front matter '{}' must be a single line (no newlines). \ + Multi-line values could inject YAML structure into the generated pipeline.", + field, + ); + } + } + Ok(()) +} + /// Reject ADO expressions in a serde_yaml::Value, recursing into strings within sequences. fn reject_ado_expressions_in_value( value: &serde_yaml::Value, @@ -265,10 +292,10 @@ pub fn generate_pipeline_resources(triggers: &Option) -> Result) -> Result String { +pub fn generate_copilot_params(front_matter: &FrontMatter) -> Result { let mut allowed_tools: Vec = vec!["github".to_string(), "safeoutputs".to_string()]; // Edit tool: enabled by default, can be disabled with `edit: false` @@ -422,12 +449,35 @@ pub fn generate_copilot_params(front_matter: &FrontMatter) -> String { } }; for cmd in bash_commands { + // Reject single quotes in bash commands — copilot_params are embedded inside + // a single-quoted bash string in the AWF command. + if cmd.contains('\'') { + anyhow::bail!( + "Bash command '{}' contains a single quote, which is not allowed \ + (would break AWF shell quoting).", + cmd + ); + } allowed_tools.push(format!("shell({})", cmd)); } let mut params = Vec::new(); - params.push(format!("--model {}", front_matter.engine.model())); + // Validate model name to prevent shell injection — copilot_params are embedded + // inside a single-quoted bash string in the AWF command. + let model = front_matter.engine.model(); + if model.is_empty() + || !model + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | ':' | '-')) + { + anyhow::bail!( + "Model name '{}' contains invalid characters. \ + Only ASCII alphanumerics, '.', '_', ':', and '-' are allowed.", + model + ); + } + params.push(format!("--model {}", model)); if front_matter.engine.max_turns().is_some() { eprintln!( "Warning: Agent '{}' has max-turns set, but max-turns is not supported by Copilot CLI \ @@ -458,7 +508,7 @@ pub fn generate_copilot_params(front_matter: &FrontMatter) -> String { } } - params.join(" ") + Ok(params.join(" ")) } /// Compute the effective workspace based on explicit setting and checkout configuration. @@ -1194,7 +1244,7 @@ mod tests { cache_memory: None, azure_devops: None, }); - let params = generate_copilot_params(&fm); + let params = generate_copilot_params(&fm).unwrap(); assert!(params.contains("--allow-tool \"shell(:*)\"")); } @@ -1207,7 +1257,7 @@ mod tests { cache_memory: None, azure_devops: None, }); - let params = generate_copilot_params(&fm); + let params = generate_copilot_params(&fm).unwrap(); assert!(!params.contains("shell(")); } @@ -1221,7 +1271,7 @@ mod tests { ..Default::default() }), ); - let params = generate_copilot_params(&fm); + let params = generate_copilot_params(&fm).unwrap(); assert!(!params.contains("--mcp my-tool")); } @@ -1230,7 +1280,7 @@ mod tests { let mut fm = minimal_front_matter(); fm.mcp_servers .insert("ado".to_string(), McpConfig::Enabled(true)); - let params = generate_copilot_params(&fm); + let params = generate_copilot_params(&fm).unwrap(); // Copilot CLI has no built-in MCPs — all MCPs are handled via the MCP firewall assert!(!params.contains("--mcp ado")); } @@ -1241,14 +1291,14 @@ mod tests { "---\nname: test\ndescription: test\nengine:\n model: claude-opus-4.5\n max-turns: 50\n---\n", ) .unwrap(); - let params = generate_copilot_params(&fm); + let params = generate_copilot_params(&fm).unwrap(); assert!(!params.contains("--max-turns"), "max-turns should not be emitted as a CLI arg"); } #[test] fn test_copilot_params_no_max_turns_when_simple_engine() { let fm = minimal_front_matter(); - let params = generate_copilot_params(&fm); + let params = generate_copilot_params(&fm).unwrap(); assert!(!params.contains("--max-turns")); } @@ -1258,14 +1308,14 @@ mod tests { "---\nname: test\ndescription: test\nengine:\n model: claude-opus-4.5\n timeout-minutes: 30\n---\n", ) .unwrap(); - let params = generate_copilot_params(&fm); + let params = generate_copilot_params(&fm).unwrap(); assert!(!params.contains("--max-timeout"), "timeout-minutes should not be emitted as a CLI arg"); } #[test] fn test_copilot_params_no_max_timeout_when_simple_engine() { let fm = minimal_front_matter(); - let params = generate_copilot_params(&fm); + let params = generate_copilot_params(&fm).unwrap(); assert!(!params.contains("--max-timeout")); } @@ -1275,7 +1325,7 @@ mod tests { "---\nname: test\ndescription: test\nengine:\n model: claude-opus-4.5\n max-turns: 0\n---\n", ) .unwrap(); - let params = generate_copilot_params(&fm); + let params = generate_copilot_params(&fm).unwrap(); assert!(!params.contains("--max-turns"), "max-turns should not be emitted as a CLI arg"); } @@ -1285,7 +1335,7 @@ mod tests { "---\nname: test\ndescription: test\nengine:\n model: claude-opus-4.5\n timeout-minutes: 0\n---\n", ) .unwrap(); - let params = generate_copilot_params(&fm); + let params = generate_copilot_params(&fm).unwrap(); assert!(!params.contains("--max-timeout"), "timeout-minutes should not be emitted as a CLI arg"); } @@ -2200,4 +2250,109 @@ mod tests { "None service connection should produce empty env block" ); } + + // ─── Security validation tests ──────────────────────────────────────────── + + #[test] + fn test_model_name_rejects_single_quote() { + let (mut fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine: claude-opus-4.5\n---\n", + ) + .unwrap(); + fm.engine = crate::compile::types::EngineConfig::Simple("model' && echo pwned".to_string()); + let result = generate_copilot_params(&fm); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("invalid characters")); + } + + #[test] + fn test_model_name_rejects_space() { + let mut fm = minimal_front_matter(); + fm.engine = crate::compile::types::EngineConfig::Simple("model && curl evil.com".to_string()); + let result = generate_copilot_params(&fm); + assert!(result.is_err()); + } + + #[test] + fn test_model_name_allows_valid_names() { + for name in &["claude-opus-4.5", "gpt-5.2-codex", "gemini-3-pro-preview", "my_model:latest"] { + let mut fm = minimal_front_matter(); + fm.engine = crate::compile::types::EngineConfig::Simple(name.to_string()); + let result = generate_copilot_params(&fm); + assert!(result.is_ok(), "Model name '{}' should be valid", name); + } + } + + #[test] + fn test_bash_command_rejects_single_quote() { + let mut fm = minimal_front_matter(); + fm.tools = Some(crate::compile::types::ToolsConfig { + bash: Some(vec!["cat'".to_string()]), + edit: None, + cache_memory: None, + azure_devops: None, + }); + let result = generate_copilot_params(&fm); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("single quote")); + } + + #[test] + fn test_validate_front_matter_identity_rejects_ado_expression_in_name() { + let mut fm = minimal_front_matter(); + fm.name = "My Agent ${{ variables['System.AccessToken'] }}".to_string(); + let result = validate_front_matter_identity(&fm); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("ADO expression")); + } + + #[test] + fn test_validate_front_matter_identity_rejects_macro_in_description() { + let mut fm = minimal_front_matter(); + fm.description = "Agent $(System.AccessToken)".to_string(); + let result = validate_front_matter_identity(&fm); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("ADO expression")); + } + + #[test] + fn test_validate_front_matter_identity_rejects_newline_in_name() { + let mut fm = minimal_front_matter(); + fm.name = "My Agent\ninjected: true".to_string(); + let result = validate_front_matter_identity(&fm); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("single line")); + } + + #[test] + fn test_validate_front_matter_identity_allows_valid_values() { + let mut fm = minimal_front_matter(); + fm.name = "Daily Code Review Agent".to_string(); + fm.description = "Reviews code daily for quality issues".to_string(); + let result = validate_front_matter_identity(&fm); + assert!(result.is_ok()); + } + + #[test] + fn test_validate_front_matter_identity_rejects_runtime_expression() { + let mut fm = minimal_front_matter(); + fm.name = "Agent $[variables['System.AccessToken']]".to_string(); + let result = validate_front_matter_identity(&fm); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("ADO expression")); + } + + #[test] + fn test_pipeline_resources_escapes_single_quotes() { + let triggers = Some(TriggerConfig { + pipeline: Some(crate::compile::types::PipelineTrigger { + name: "Build's Pipeline".to_string(), + project: Some("My'Project".to_string()), + branches: vec!["main".to_string()], + }), + }); + let result = generate_pipeline_resources(&triggers).unwrap(); + assert!(result.contains("source: 'Build''s Pipeline'")); + assert!(result.contains("project: 'My''Project'")); + } } diff --git a/src/compile/onees.rs b/src/compile/onees.rs index a20ab8d..d6b5b2e 100644 --- a/src/compile/onees.rs +++ b/src/compile/onees.rs @@ -24,9 +24,9 @@ use super::common::{ generate_job_timeout, generate_parameters, generate_pipeline_path, generate_pipeline_resources, generate_pr_trigger, generate_repositories, generate_schedule, generate_source_path, generate_working_directory, is_custom_mcp, replace_with_indent, - validate_comment_target, validate_resolve_pr_thread_statuses, - validate_submit_pr_review_events, validate_update_pr_votes, - validate_update_work_item_target, validate_write_permissions, + validate_comment_target, validate_front_matter_identity, + validate_resolve_pr_thread_statuses, validate_submit_pr_review_events, + validate_update_pr_votes, validate_update_work_item_target, validate_write_permissions, }; use super::types::{FrontMatter, McpConfig}; @@ -48,6 +48,9 @@ impl Compiler for OneESCompiler { ) -> Result { info!("Compiling for 1ES target"); + // Validate inputs early, before any values are used in template substitution + validate_front_matter_identity(front_matter)?; + // Load 1ES template let template = include_str!("../../templates/1es-base.yml"); @@ -61,7 +64,7 @@ impl Compiler for OneESCompiler { let repositories = generate_repositories(&front_matter.repositories); let checkout_steps = generate_checkout_steps(&front_matter.checkout); let checkout_self = generate_checkout_self(); - let copilot_params = generate_copilot_params(front_matter); + let copilot_params = generate_copilot_params(front_matter)?; let has_memory = front_matter .tools .as_ref() diff --git a/src/compile/standalone.rs b/src/compile/standalone.rs index d9799ca..1f603e3 100644 --- a/src/compile/standalone.rs +++ b/src/compile/standalone.rs @@ -23,8 +23,9 @@ use super::common::{ generate_job_timeout, generate_parameters, generate_pipeline_path, generate_pipeline_resources, generate_pr_trigger, generate_repositories, generate_schedule, generate_source_path, generate_working_directory, replace_with_indent, sanitize_filename, validate_comment_target, - validate_resolve_pr_thread_statuses, validate_submit_pr_review_events, - validate_update_pr_votes, validate_update_work_item_target, validate_write_permissions, + validate_front_matter_identity, validate_resolve_pr_thread_statuses, + validate_submit_pr_review_events, validate_update_pr_votes, validate_update_work_item_target, + validate_write_permissions, }; use super::types::{FrontMatter, McpConfig}; use crate::allowed_hosts::{CORE_ALLOWED_HOSTS, mcp_required_hosts}; @@ -49,6 +50,9 @@ impl Compiler for StandaloneCompiler { ) -> Result { info!("Compiling for standalone target"); + // Validate inputs early, before any values are used in template substitution + validate_front_matter_identity(front_matter)?; + // Load base template let template = include_str!("../../templates/base.yml"); @@ -62,7 +66,7 @@ impl Compiler for StandaloneCompiler { let repositories = generate_repositories(&front_matter.repositories); let checkout_steps = generate_checkout_steps(&front_matter.checkout); let checkout_self = generate_checkout_self(); - let copilot_params = generate_copilot_params(front_matter); + let copilot_params = generate_copilot_params(front_matter)?; let agent_name = sanitize_filename(&front_matter.name); // Compute effective workspace @@ -85,7 +89,7 @@ impl Compiler for StandaloneCompiler { let pipeline_path = generate_pipeline_path(output_path); // Generate comma-separated domain list for AWF - let allowed_domains = generate_allowed_domains(front_matter); + let allowed_domains = generate_allowed_domains(front_matter)?; // Generate --enabled-tools args for SafeOutputs tool filtering let enabled_tools_args = generate_enabled_tools_args(front_matter); @@ -283,7 +287,7 @@ impl Compiler for StandaloneCompiler { /// 1. Core Azure DevOps/GitHub endpoints /// 2. MCP-specific endpoints for each enabled MCP /// 3. User-specified additional hosts from network.allow -fn generate_allowed_domains(front_matter: &FrontMatter) -> String { +fn generate_allowed_domains(front_matter: &FrontMatter) -> Result { // Collect enabled MCP names let enabled_mcps: Vec = front_matter .mcp_servers @@ -336,8 +340,19 @@ fn generate_allowed_domains(front_matter: &FrontMatter) -> String { } } - // Add user-specified hosts + // Add user-specified hosts (validated against DNS-safe characters) for host in &user_hosts { + let valid = !host.is_empty() + && host + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '-' | '*')); + if !valid { + anyhow::bail!( + "network.allow domain '{}' contains characters invalid in DNS names. \ + Only ASCII alphanumerics, '.', '-', and '*' are allowed.", + host + ); + } hosts.insert(host.clone()); } @@ -356,7 +371,7 @@ fn generate_allowed_domains(front_matter: &FrontMatter) -> String { allowlist.sort(); // Format as comma-separated list for AWF --allow-domains - allowlist.join(",") + Ok(allowlist.join(",")) } /// Generate the setup job YAML diff --git a/templates/base.yml b/templates/base.yml index 203c484..591057b 100644 --- a/templates/base.yml +++ b/templates/base.yml @@ -310,7 +310,7 @@ jobs: # tee writes to both stdout (ADO pipeline log) and the artifact file. # pipefail (set above) ensures AWF's exit code propagates through the pipe. sudo -E "$(Pipeline.Workspace)/awf/awf" \ - --allow-domains {{ allowed_domains }} \ + --allow-domains "{{ allowed_domains }}" \ --skip-pull \ --env-all \ --enable-host-access \ @@ -495,7 +495,7 @@ jobs: # Stream threat analysis output in real-time with VSO command filtering sudo -E "$(Pipeline.Workspace)/awf/awf" \ - --allow-domains {{ allowed_domains }} \ + --allow-domains "{{ allowed_domains }}" \ --skip-pull \ --env-all \ --container-workdir "{{ working_directory }}" \