feat: template-injection: filter static envs (#318)

Signed-off-by: William Woodruff <william@yossarian.net>

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"

src/audit/insecure_commands.rs

@@ -105,8 +105,15 @@ impl WorkflowAudit for InsecureCommands {
     fn audit_workflow<'w>(&self, workflow: &'w Workflow) -> anyhow::Result<Vec<Finding<'w>>> {
         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() {

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()

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`].

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(())
 }

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

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 }}