fix: handle non-static env: in job steps (#246)

woodruffw · Dec 8, 2024 · e50f954 · e50f954
1 parent eb95b08
commit e50f954
Show file tree

Hide file tree

Showing 7 changed files with 103 additions and 14 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -20,7 +20,7 @@ camino = { version = "1.1.9", features = ["serde1"] }
 clap = { version = "4.5.21", features = ["derive", "env"] }
 clap-verbosity-flag = "3.0.0"
 env_logger = "0.11.5"
-github-actions-models = "0.12.0"
+github-actions-models = "0.13.0"
 human-panic = "2.0.1"
 indexmap = "2.7.0"
 indicatif = "0.17.9"

diff --git a/src/audit/insecure_commands.rs b/src/audit/insecure_commands.rs
@@ -1,7 +1,9 @@
 use crate::audit::WorkflowAudit;
-use crate::finding::{Confidence, Finding, Severity, SymbolicLocation};
+use crate::finding::{Confidence, Finding, Persona, Severity, SymbolicLocation};
 use crate::models::{Steps, Workflow};
 use crate::state::AuditState;
+use anyhow::Result;
+use github_actions_models::common::expr::LoE;
 use github_actions_models::common::{Env, EnvValue};
 use github_actions_models::workflow::job::StepBody;
 use github_actions_models::workflow::Job;
@@ -22,7 +24,7 @@ impl InsecureCommands {
         &self,
         workflow: &'w Workflow,
         location: SymbolicLocation<'w>,
-    ) -> Finding<'w> {
+    ) -> Result<Finding<'w>> {
         Self::finding()
             .confidence(Confidence::High)
             .severity(Severity::High)
@@ -32,7 +34,6 @@ impl InsecureCommands {
                     .annotated("insecure commands enabled here"),
             )
             .build(workflow)
-            .expect("Cannot build a Finding instance")
     }
 
     fn has_insecure_commands_enabled(&self, env: &Env) -> bool {
@@ -43,23 +44,46 @@ impl InsecureCommands {
         }
     }
 
-    fn audit_steps<'w>(&self, workflow: &'w Workflow, steps: Steps<'w>) -> Vec<Finding<'w>> {
+    fn audit_steps<'w>(
+        &self,
+        workflow: &'w Workflow,
+        steps: Steps<'w>,
+    ) -> Result<Vec<Finding<'w>>> {
         steps
             .into_iter()
-            .filter(|step| {
+            .filter_map(|step| {
                 let StepBody::Run {
                     run: _,
                     working_directory: _,
                     shell: _,
                     ref env,
                 } = &step.deref().body
                 else {
-                    return false;
+                    return None;
                 };
 
-                self.has_insecure_commands_enabled(env)
+                match env {
+                    // The entire environment block is an expression, which we
+                    // can't follow (for now). Emit an auditor-only finding.
+                    LoE::Expr(_) => Some(
+                        Self::finding()
+                            .confidence(Confidence::Low)
+                            .severity(Severity::High)
+                            .persona(Persona::Auditor)
+                            .add_location(
+                                step.location()
+                                .with_keys(&["env".into()])
+                                .annotated(
+                                    "non-static environment may contain ACTIONS_ALLOW_UNSECURE_COMMANDS"
+                                )
+                            )
+                            .build(workflow)
+                    ),
+                    LoE::Literal(env) => self
+                        .has_insecure_commands_enabled(env)
+                        .then(|| self.insecure_commands_allowed(workflow, step.location())),
+                }
             })
-            .map(|step| self.insecure_commands_allowed(workflow, step.location()))
             .collect()
     }
 }
@@ -76,16 +100,16 @@ impl WorkflowAudit for InsecureCommands {
         let mut results = vec![];
 
         if self.has_insecure_commands_enabled(&workflow.env) {
-            results.push(self.insecure_commands_allowed(workflow, workflow.location()))
+            results.push(self.insecure_commands_allowed(workflow, workflow.location())?)
         }
 
         for job in workflow.jobs() {
             if let Job::NormalJob(normal) = *job {
                 if self.has_insecure_commands_enabled(&normal.env) {
-                    results.push(self.insecure_commands_allowed(workflow, job.location()))
+                    results.push(self.insecure_commands_allowed(workflow, job.location())?)
                 }
 
-                results.extend(self.audit_steps(workflow, job.steps()))
+                results.extend(self.audit_steps(workflow, job.steps())?)
             }
         }
 

diff --git a/tests/snapshot.rs b/tests/snapshot.rs
@@ -168,3 +168,17 @@ fn unpinned_uses() -> Result<()> {
 
     Ok(())
 }
+
+#[test]
+fn insecure_commands() -> Result<()> {
+    insta::assert_snapshot!(zizmor()
+        .workflow(workflow_under_test("insecure-commands.yml"))
+        .args(["--persona=auditor"])
+        .run()?);
+
+    insta::assert_snapshot!(zizmor()
+        .workflow(workflow_under_test("insecure-commands.yml"))
+        .run()?);
+
+    Ok(())
+}
diff --git a/tests/snapshots/snapshot__insecure_commands-2.snap b/tests/snapshots/snapshot__insecure_commands-2.snap
@@ -0,0 +1,16 @@
+---
+source: tests/snapshot.rs
+expression: "zizmor().workflow(workflow_under_test(\"insecure-commands.yml\")).run()?"
+snapshot_kind: text
+---
+error[insecure-commands]: execution of insecure workflow commands is enabled
+ --> @@INPUT@@:8:5
+  |
+8 |       env:
+  |  _____^
+9 | |       ACTIONS_ALLOW_UNSECURE_COMMANDS: yes
+  | |__________________________________________^ insecure commands enabled here
+  |
+  = note: audit confidence → High
+
+2 findings (1 suppressed): 0 unknown, 0 informational, 0 low, 0 medium, 1 high
diff --git a/tests/snapshots/snapshot__insecure_commands.snap b/tests/snapshots/snapshot__insecure_commands.snap
@@ -0,0 +1,24 @@
+---
+source: tests/snapshot.rs
+expression: "zizmor().workflow(workflow_under_test(\"insecure-commands.yml\")).args([\"--persona=auditor\"]).run()?"
+snapshot_kind: text
+---
+error[insecure-commands]: execution of insecure workflow commands is enabled
+ --> @@INPUT@@:8:5
+  |
+8 |       env:
+  |  _____^
+9 | |       ACTIONS_ALLOW_UNSECURE_COMMANDS: yes
+  | |__________________________________________^ insecure commands enabled here
+  |
+  = note: audit confidence → High
+
+error[insecure-commands]: execution of insecure workflow commands is enabled
+  --> @@INPUT@@:22:9
+   |
+22 |         env: ${{ matrix.env }}
+   |         ^^^^^^^^^^^^^^^^^^^^^^ non-static environment may contain ACTIONS_ALLOW_UNSECURE_COMMANDS
+   |
+   = note: audit confidence → Low
+
+2 findings: 0 unknown, 0 informational, 0 low, 0 medium, 2 high
diff --git a/tests/test-data/insecure-commands.yml b/tests/test-data/insecure-commands.yml
@@ -9,3 +9,14 @@ jobs:
       ACTIONS_ALLOW_UNSECURE_COMMANDS: yes
     steps:
       - run: echo "don't do this"
+
+  env-via-matrix:
+    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        env:
+          - ACTIONS_ALLOW_UNSECURE_COMMANDS: yes
+
+    steps:
+      - run: echo "don't do this"
+        env: ${{ matrix.env }}