diff --git a/Cargo.lock b/Cargo.lock index c73939ec..b5c66cd6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -585,9 +585,9 @@ checksum = "07e28edb80900c19c28f1072f2e8aeca7fa06b23cd4169cefe1af5aa3260783f" [[package]] name = "github-actions-models" -version = "0.14.0" +version = "0.15.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ef734b63e241501835d10e69e9479f35995dc7bece8fd64934f227841ddd3a16" +checksum = "d1b2a17936ffbdd2c25b57f76efee924ddb0bd8ae402834616117dbc772d2f8a" dependencies = [ "indexmap", "serde", diff --git a/Cargo.toml b/Cargo.toml index 240abede..7cedd56d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,7 +22,7 @@ clap-verbosity-flag = { version = "3.0.0", features = [ "tracing", ], default-features = false } etcetera = "0.8.0" -github-actions-models = "0.14.0" +github-actions-models = "0.15.0" http-cache-reqwest = "0.15.0" human-panic = "2.0.1" indexmap = "2.7.0" diff --git a/src/audit/insecure_commands.rs b/src/audit/insecure_commands.rs index 57b198ae..13f3fa9b 100644 --- a/src/audit/insecure_commands.rs +++ b/src/audit/insecure_commands.rs @@ -105,8 +105,15 @@ impl WorkflowAudit for InsecureCommands { fn audit_workflow<'w>(&self, workflow: &'w Workflow) -> anyhow::Result>> { let mut results = vec![]; - if self.has_insecure_commands_enabled(&workflow.env) { - results.push(self.insecure_commands_allowed(workflow, workflow.location())?) + match &workflow.env { + LoE::Expr(_) => { + results.push(self.insecure_commands_maybe_present(workflow, workflow.location())?) + } + LoE::Literal(env) => { + if self.has_insecure_commands_enabled(env) { + results.push(self.insecure_commands_allowed(workflow, workflow.location())?) + } + } } for job in workflow.jobs() { diff --git a/src/audit/template_injection.rs b/src/audit/template_injection.rs index 33073865..ab682689 100644 --- a/src/audit/template_injection.rs +++ b/src/audit/template_injection.rs @@ -14,10 +14,10 @@ use std::ops::Deref; use github_actions_models::{ common::expr::LoE, - workflow::job::{NormalJob, StepBody, Strategy}, + workflow::job::{StepBody, Strategy}, }; -use super::{audit_meta, WorkflowAudit}; +use super::{audit_meta, Step, WorkflowAudit}; use crate::{ expr::{BinOp, Expr, UnOp}, finding::{Confidence, Persona, Severity}, @@ -128,7 +128,7 @@ impl TemplateInjection { fn injectable_template_expressions( &self, run: &str, - job: &NormalJob, + step: &Step, ) -> Vec<(String, Severity, Confidence, Persona)> { let mut bad_expressions = vec![]; for expr in extract_expressions(run) { @@ -166,14 +166,17 @@ impl TemplateInjection { Confidence::Low, Persona::default(), )); - } else if context.starts_with("env.") { - // Almost never exploitable. - bad_expressions.push(( - context.into(), - Severity::Low, - Confidence::High, - Persona::default(), - )); + } else if let Some(env) = context.strip_prefix("env.") { + let env_is_static = step.env_is_static(env); + + if !env_is_static { + bad_expressions.push(( + context.into(), + Severity::Low, + Confidence::High, + Persona::default(), + )); + } } else if context.starts_with("github.event.") || context == "github.ref_name" { // TODO: Filter these more finely; not everything in the event // context is actually attacker-controllable. @@ -184,7 +187,7 @@ impl TemplateInjection { Persona::default(), )); } else if context.starts_with("matrix.") || context == "matrix" { - if let Some(Strategy { matrix, .. }) = &job.strategy { + if let Some(Strategy { matrix, .. }) = &step.job().strategy { let matrix_is_static = match matrix { // The matrix is generated by an expression, meaning // that it's trivially not static. @@ -254,7 +257,7 @@ impl WorkflowAudit for TemplateInjection { }; for (expr, severity, confidence, persona) in - self.injectable_template_expressions(script, step.job()) + self.injectable_template_expressions(script, step) { findings.push( Self::finding() diff --git a/src/models.rs b/src/models.rs index 6cca05cd..8fd63b87 100644 --- a/src/models.rs +++ b/src/models.rs @@ -422,6 +422,49 @@ impl<'w> Step<'w> { } } + /// Returns whether the given `env.name` environment access is "static," + /// i.e. is not influenced by another expression. + pub(crate) fn env_is_static(&self, name: &str) -> bool { + // Collect each of the step, job, and workflow-level `env` blocks + // and check each. + let mut envs = vec![]; + + match &self.body { + StepBody::Uses { .. } => panic!("API misuse: can't call env_is_static on a uses: step"), + StepBody::Run { + run: _, + working_directory: _, + shell: _, + env, + } => envs.push(env), + }; + + envs.push(&self.job().env); + envs.push(&self.workflow().env); + + for env in envs { + match env { + // Any `env:` that is wholly an expression cannot be static. + LoE::Expr(_) => return false, + LoE::Literal(env) => { + let Some(value) = env.get(name) else { + continue; + }; + + // A present `env:` value is static if it has no interior expressions. + // TODO: We could instead return the interior expressions here + // for further analysis, to further eliminate false positives + // e.g. `env.foo: ${{ something-safe }}`. + return extract_expressions(&value.to_string()).is_empty(); + } + } + } + + // No `env:` blocks explicitly contain this name, so it's trivially static. + // In practice this is probably an invalid workflow. + true + } + /// Returns this step's parent [`NormalJob`]. /// /// Note that this returns the [`NormalJob`], not the wrapper [`Job`]. diff --git a/tests/snapshot.rs b/tests/snapshot.rs index 8b94e42a..5a285ba5 100644 --- a/tests/snapshot.rs +++ b/tests/snapshot.rs @@ -245,8 +245,6 @@ fn template_injection() -> Result<()> { .args(["--persona=auditor"]) .run()?); - // Fixed regressions - insta::assert_snapshot!(zizmor() .workflow(workflow_under_test("template-injection/issue-22-repro.yml")) .run()?); @@ -255,5 +253,9 @@ fn template_injection() -> Result<()> { .workflow(workflow_under_test("template-injection/pr-317-repro.yml")) .run()?); + insta::assert_snapshot!(zizmor() + .workflow(workflow_under_test("template-injection/static-env.yml")) + .run()?); + Ok(()) } diff --git a/tests/snapshots/snapshot__template_injection-5.snap b/tests/snapshots/snapshot__template_injection-5.snap new file mode 100644 index 00000000..2900b2fa --- /dev/null +++ b/tests/snapshots/snapshot__template_injection-5.snap @@ -0,0 +1,39 @@ +--- +source: tests/snapshot.rs +expression: "zizmor().workflow(workflow_under_test(\"template-injection/static-env.yml\")).run()?" +snapshot_kind: text +--- +help[template-injection]: code injection via template expansion + --> @@INPUT@@:39:9 + | +39 | - name: step-level-non-static + | --------------------------- help: this step +40 | / run: | +41 | | echo ${{ env.bar }} + | |_____________________________- help: env.bar may expand into attacker-controllable code + | + = note: audit confidence → High + +help[template-injection]: code injection via template expansion + --> @@INPUT@@:46:9 + | +46 | - name: job-level-non-static + | -------------------------- help: this step +47 | / run: | +48 | | echo ${{ env.foo }} + | |_____________________________- help: env.foo may expand into attacker-controllable code + | + = note: audit confidence → High + +help[template-injection]: code injection via template expansion + --> @@INPUT@@:51:9 + | +51 | - name: workflow-level-non-static + | ------------------------------- help: this step +52 | / run: | +53 | | echo ${{ env.quux }} + | |_______________________________- help: env.quux may expand into attacker-controllable code + | + = note: audit confidence → High + +3 findings: 0 unknown, 0 informational, 3 low, 0 medium, 0 high diff --git a/tests/test-data/template-injection/static-env.yml b/tests/test-data/template-injection/static-env.yml new file mode 100644 index 00000000..db3f677a --- /dev/null +++ b/tests/test-data/template-injection/static-env.yml @@ -0,0 +1,53 @@ +name: static-env +on: + pull_request: + +env: + foo: foo + bar: ${{ github.event.issue.title }} + baz: baz + quux: ${{ github.event.issue.title }} + +jobs: + static-env-1: + name: static-env + runs-on: ubuntu-latest + + env: + foo: ${{ github.event.issue.title }} + bar: bar + + steps: + # OK: foo is static (step-level) + - name: step-level-static + run: | + echo ${{ env.foo }} + env: + foo: foo + + # OK: bar is static (job-level) + - name: job-level-static + run: | + echo ${{ env.bar }} + + # OK: baz is static (workflow-level) + - name: workflow-level-static + run: | + echo ${{ env.baz }} + + # NOT OK: bar is not static (step-level) + - name: step-level-non-static + run: | + echo ${{ env.bar }} + env: + bar: ${{ github.event.issue.title }} + + # NOT OK: foo is not static (job-level) + - name: job-level-non-static + run: | + echo ${{ env.foo }} + + # NOT OK: quux is not static (workflow-level) + - name: workflow-level-non-static + run: | + echo ${{ env.quux }}