Repository: woodruffw/zizmor
Pull request #192 (2024-11-24 00:03:05)

feat: adds new github-env audit
- Pulls dangerous triggers detection logic from `WorkflowAudit` and moves to `impl DangerousTriggers` instance methods
- Reuses only that logic in the context of to implement a new audit

```bash
cargo run -- tests/test-data/github_env.yml

error[github-env]: dangerous use of $GITHUB_ENV
  --> tests/test-data/github_env.yml:12:9
   |
12 |           run: |
   |  _________^
13 | |           message=$(echo "$TITLE" | grep -oP '[{\[][^}\]]+[}\]]' | sed 's/{\|}\|\[\|\]//g')
14 | |           echo "message=$message" >> $GITHUB_ENV
   | |_________________________________________________^ GITHUB_ENV used in the context of a dangerous Workflow trigger
   |
   = note: audit confidence → High

2 findings (1 ignored): 0 unknown, 0 informational, 0 low, 0 medium, 1 high
```

Some questions below!

woodruffw: Thanks @ubiratansoares! I'll give this a more detailed review later today.

> Pulls dangerous triggers detection logic from `WorkflowAudit` and moves to `impl DangerousTriggers` instance methods

Rather than use sharing APIs across audits, I think we can probably turn these into APIs on `models::Workflow` -- `Workflow::has_pull_request_target` and similar would probably be good.


ubiratansoares: I address all the comments, including moving dangerous triggers detection logic to  `models::Workflow`. 

I also added some docs covering the basics based on the discussions we have in the related issue, but I did not go deep there are safe ways to use `$GITHUB_ENV`. Happy to extended those docs based on additional input!
ubiratansoares: @woodruffw we miss an example [here](https://github.com/woodruffw/gha-hazmat/blob/main/.github/workflows/github-env.yml), happy to raise a PR there, lmk!
woodruffw: > @woodruffw we miss an example [here](https://github.com/woodruffw/gha-hazmat/blob/main/.github/workflows/github-env.yml?rgh-link-date=2024-11-24T21%3A49%3A34Z), happy to raise a PR there, lmk!

Please do, thanks!

Patch:
diff --git a/docs/audits.md b/docs/audits.md
index bba676c..34bbe8e 100644
--- a/docs/audits.md
+++ b/docs/audits.md
@@ -642,6 +642,34 @@ In general, users should use for [Github Actions environment files]
         echo "$HOME/.local/my-bin" >> "$GITHUB_PATH"
     ```
 
+## `github-env`
+
+| Type     | Examples           | Introduced in | Works offline  | Enabled by default |
+|----------|--------------------|---------------|----------------|--------------------|
+| Workflow  | [github-env.yml]   | v0.6.0        | ✅             | ✅                 |
+
+[github-env.yml]: https://github.com/woodruffw/gha-hazmat/blob/main/.github/workflows/github-env.yml
+
+Detects dangerous usages of the `GITHUB_ENV` environment variable.
+
+When used in workflows with dangerous triggers (such as `pull_request_target` and `workflow_run`), 
+`GITHUB_ENV` can be an arbitrary code execution risk. In particular, if the attacker is able to set
+arbitrary variables or variable contents via `GITHUB_ENV`, they made be able to set `LD_PRELOAD`
+or otherwise induce code execution implicitly within subsequent steps.
+
+Other resources:
+
+* [GitHub Actions exploitation: environment manipulation]
+* [GHSL-2024-177: Environment Variable injection in an Actions workflow of Litestar]
+
+### Remediation
+
+In general, you should avoid setting `GITHUB_ENV` within workflows that are attacker-triggered, 
+like `pull_request_target`.
+
+If you need to pass state between steps, consider using `GITHUB_OUTPUT` instead.
+
+
 [ArtiPACKED: Hacking Giants Through a Race Condition in GitHub Actions Artifacts]: https://unit42.paloaltonetworks.com/github-repo-artifacts-leak-tokens/
 [Keeping your GitHub Actions and workflows secure Part 1: Preventing pwn requests]: https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/
 [What the fork? Imposter commits in GitHub Actions and CI/CD]: https://www.chainguard.dev/unchained/what-the-fork-imposter-commits-in-github-actions-and-ci-cd
@@ -654,3 +682,5 @@ In general, users should use for [Github Actions environment files]
 [were deprecated by Github]: https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/
 [Github Actions environment files]: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions#environment-files
 [Semgrep audit]: https://semgrep.dev/r?q=yaml.github-actions.security.allowed-unsecure-commands.allowed-unsecure-commands
+[GitHub Actions exploitation: environment manipulation]: https://www.synacktiv.com/en/publications/github-actions-exploitation-repo-jacking-and-environment-manipulation
+[GHSL-2024-177: Environment Variable injection in an Actions workflow of Litestar]: https://securitylab.github.com/advisories/GHSL-2024-177_Litestar/
diff --git a/docs/snippets/help.txt b/docs/snippets/help.txt
index 30cde93..dae45ec 100644
--- a/docs/snippets/help.txt
+++ b/docs/snippets/help.txt
@@ -6,16 +6,31 @@ Arguments:
   <INPUTS>...  The workflow filenames or directories to audit
 
 Options:
-  -p, --pedantic                     Emit findings even when the context suggests an explicit security decision made by the user
-  -o, --offline                      Only perform audits that don't require network access
-  -v, --verbose...                   Increase logging verbosity
-  -q, --quiet...                     Decrease logging verbosity
-  -n, --no-progress                  Disable the progress bar. This is useful primarily when running with a high verbosity level, as the two will fight for stderr
-      --gh-token <GH_TOKEN>          The GitHub API token to use [env: GH_TOKEN=]
-      --format <FORMAT>              The output format to emit. By default, plain text will be emitted [possible values: plain, json, sarif]
-  -c, --config <CONFIG>              The configuration file to load. By default, any config will be discovered relative to $CWD
-      --no-config                    Disable all configuration loading
-      --no-exit-codes                Disable all error codes besides success and tool failure
-      --min-severity <MIN_SEVERITY>  Filter all results below this severity [possible values: unknown, informational, low, medium, high]
-  -h, --help                         Print help
-  -V, --version                      Print version
+  -p, --pedantic
+          Emit findings even when the context suggests an explicit security decision made by the user
+  -o, --offline
+          Only perform audits that don't require network access
+  -v, --verbose...
+          Increase logging verbosity
+  -q, --quiet...
+          Decrease logging verbosity
+  -n, --no-progress
+          Disable the progress bar. This is useful primarily when running with a high verbosity level, as the two will fight for stderr
+      --gh-token <GH_TOKEN>
+          The GitHub API token to use [env: GH_TOKEN=]
+      --format <FORMAT>
+          The output format to emit. By default, plain text will be emitted [possible values: plain, json, sarif]
+  -c, --config <CONFIG>
+          The configuration file to load. By default, any config will be discovered relative to $CWD
+      --no-config
+          Disable all configuration loading
+      --no-exit-codes
+          Disable all error codes besides success and tool failure
+      --min-severity <MIN_SEVERITY>
+          Filter all results below this severity [possible values: unknown, informational, low, medium, high]
+      --min-confidence <MIN_CONFIDENCE>
+          Filter all results below this confidence [possible values: unknown, low, medium, high]
+  -h, --help
+          Print help
+  -V, --version
+          Print version
diff --git a/docs/usage.md b/docs/usage.md
index a3316f0..f699c58 100644
--- a/docs/usage.md
+++ b/docs/usage.md
@@ -73,16 +73,17 @@ All other exit codes are currently reserved.
 
 There are two straightforward ways to filter `zizmor`'s results:
 
-1. If all you need is severity filtering (e.g. "I want only medium-severity
-   and above results"), then you can use the `--min-severity` flag:
+1. If all you need is severity or confidence filtering (e.g. "I want only
+   medium-severity and/or medium-confidence and above results"), then you can use
+   the `--min-severity` and `--min-confidence` flags:
 
     !!! tip
 
-        `--min-severity` is available in `v0.6.0` and later.
+        `--min-severity` and `--min-confidence` are available in `v0.6.0` and later.
 
      ```bash
-     # filter unknown, informational, and low findings
-     zizmor --min-severity=medium ...
+     # filter unknown, informational, and low findings with unknown, low confidence
+     zizmor --min-severity=medium --min-confidence=medium ...
      ```
 
 2. If you need more advanced filtering (with nontrivial conditions or
diff --git a/src/audit/dangerous_triggers.rs b/src/audit/dangerous_triggers.rs
index 5930655..df3b285 100644
--- a/src/audit/dangerous_triggers.rs
+++ b/src/audit/dangerous_triggers.rs
@@ -1,15 +1,11 @@
 use anyhow::Result;
-use github_actions_models::workflow::event::{BareEvent, OptionalBody};
-use github_actions_models::workflow::Trigger;
 
 use super::{audit_meta, WorkflowAudit};
 use crate::finding::{Confidence, Finding, Severity};
 use crate::models::Workflow;
 use crate::state::AuditState;
 
-pub(crate) struct DangerousTriggers {
-    pub(crate) _state: AuditState,
-}
+pub(crate) struct DangerousTriggers;
 
 audit_meta!(
     DangerousTriggers,
@@ -18,27 +14,13 @@ audit_meta!(
 );
 
 impl WorkflowAudit for DangerousTriggers {
-    fn new(state: AuditState) -> Result<Self> {
-        Ok(Self { _state: state })
+    fn new(_: AuditState) -> Result<Self> {
+        Ok(Self)
     }
 
     fn audit<'w>(&self, workflow: &'w Workflow) -> Result<Vec<Finding<'w>>> {
-        let trigger = &workflow.on;
-
-        let has_pull_request_target = match trigger {
-            Trigger::BareEvent(event) => *event == BareEvent::PullRequestTarget,
-            Trigger::BareEvents(events) => events.contains(&BareEvent::PullRequestTarget),
-            Trigger::Events(events) => !matches!(events.pull_request_target, OptionalBody::Missing),
-        };
-
-        let has_workflow_run = match trigger {
-            Trigger::BareEvent(event) => *event == BareEvent::WorkflowRun,
-            Trigger::BareEvents(events) => events.contains(&BareEvent::WorkflowRun),
-            Trigger::Events(events) => !matches!(events.workflow_run, OptionalBody::Missing),
-        };
-
         let mut findings = vec![];
-        if has_pull_request_target {
+        if workflow.has_pull_request_target() {
             findings.push(
                 Self::finding()
                     .confidence(Confidence::Medium)
@@ -52,7 +34,7 @@ impl WorkflowAudit for DangerousTriggers {
                     .build(workflow)?,
             );
         }
-        if has_workflow_run {
+        if workflow.has_workflow_run() {
             findings.push(
                 Self::finding()
                     .confidence(Confidence::Medium)
diff --git a/src/audit/github_env.rs b/src/audit/github_env.rs
new file mode 100644
index 0000000..96a2a91
--- /dev/null
+++ b/src/audit/github_env.rs
@@ -0,0 +1,58 @@
+use super::{audit_meta, WorkflowAudit};
+use crate::finding::{Confidence, Finding, Severity};
+use crate::models::Step;
+use crate::state::AuditState;
+use github_actions_models::workflow::job::StepBody;
+use std::ops::Deref;
+
+pub(crate) struct GitHubEnv;
+
+audit_meta!(GitHubEnv, "github-env", "dangerous use of $GITHUB_ENV");
+
+impl GitHubEnv {
+    fn uses_github_environment(&self, run_step_body: &str) -> bool {
+        // In the future we can improve over this implementation,
+        // eventually detecting how $GITHUB_ENV is being used
+        // and returning an Option<Confidence> instead
+
+        run_step_body.contains("$GITHUB_ENV") || run_step_body.contains("${GITHUB_ENV}")
+    }
+}
+
+impl WorkflowAudit for GitHubEnv {
+    fn new(_: AuditState) -> anyhow::Result<Self>
+    where
+        Self: Sized,
+    {
+        Ok(Self)
+    }
+
+    fn audit_step<'w>(&self, step: &Step<'w>) -> anyhow::Result<Vec<Finding<'w>>> {
+        let mut findings = vec![];
+
+        let workflow = step.workflow();
+
+        let has_dangerous_triggers =
+            workflow.has_workflow_run() || workflow.has_pull_request_target();
+
+        if !has_dangerous_triggers {
+            return Ok(findings);
+        }
+
+        if let StepBody::Run { run, .. } = &step.deref().body {
+            if self.uses_github_environment(run) {
+                findings.push(
+                    Self::finding()
+                        .severity(Severity::High)
+                        .confidence(Confidence::Low)
+                        .add_location(step.location().with_keys(&["run".into()]).annotated(
+                            "GITHUB_ENV used in the context of a dangerous Workflow trigger",
+                        ))
+                        .build(step.workflow())?,
+                )
+            }
+        }
+
+        Ok(findings)
+    }
+}
diff --git a/src/audit/mod.rs b/src/audit/mod.rs
index 395a1e1..f188b4c 100644
--- a/src/audit/mod.rs
+++ b/src/audit/mod.rs
@@ -11,6 +11,7 @@ use crate::{
 pub(crate) mod artipacked;
 pub(crate) mod dangerous_triggers;
 pub(crate) mod excessive_permissions;
+pub(crate) mod github_env;
 pub(crate) mod hardcoded_container_credentials;
 pub(crate) mod impostor_commit;
 pub(crate) mod insecure_commands;
diff --git a/src/finding/mod.rs b/src/finding/mod.rs
index 9c6baeb..ccd8bc3 100644
--- a/src/finding/mod.rs
+++ b/src/finding/mod.rs
@@ -14,7 +14,9 @@ pub(crate) mod locate;
 
 // TODO: Traits + more flexible models here.
 
-#[derive(Copy, Clone, Debug, Default, Eq, Hash, PartialEq, Serialize, ValueEnum)]
+#[derive(
+    Copy, Clone, Debug, Default, Eq, Hash, Ord, PartialOrd, PartialEq, Serialize, ValueEnum,
+)]
 pub(crate) enum Confidence {
     #[default]
     Unknown,
diff --git a/src/main.rs b/src/main.rs
index 23dea55..6da2e0f 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -5,7 +5,7 @@ use anyhow::{anyhow, Context, Result};
 use audit::WorkflowAudit;
 use clap::{Parser, ValueEnum};
 use config::Config;
-use finding::Severity;
+use finding::{Confidence, Severity};
 use indicatif::{ProgressBar, ProgressDrawTarget, ProgressStyle};
 use owo_colors::OwoColorize;
 use registry::{AuditRegistry, FindingRegistry, WorkflowRegistry};
@@ -68,6 +68,10 @@ struct App {
     #[arg(long)]
     min_severity: Option<Severity>,
 
+    /// Filter all results below this confidence.
+    #[arg(long)]
+    min_confidence: Option<Confidence>,
+
     /// The workflow filenames or directories to audit.
     #[arg(required = true)]
     inputs: Vec<PathBuf>,
@@ -161,6 +165,7 @@ fn run() -> Result<ExitCode> {
     register_audit!(audit::known_vulnerable_actions::KnownVulnerableActions);
     register_audit!(audit::unpinned_uses::UnpinnedUses);
     register_audit!(audit::insecure_commands::InsecureCommands);
+    register_audit!(audit::github_env::GitHubEnv);
 
     let bar = ProgressBar::new((workflow_registry.len() * audit_registry.len()) as u64);
 
diff --git a/src/models.rs b/src/models.rs
index c5ed20b..6001893 100644
--- a/src/models.rs
+++ b/src/models.rs
@@ -3,14 +3,15 @@
 
 use std::{collections::hash_map, iter::Enumerate, ops::Deref, path::Path};
 
+use crate::finding::{Route, SymbolicLocation};
 use anyhow::{anyhow, Context, Result};
+use github_actions_models::workflow::event::{BareEvent, OptionalBody};
 use github_actions_models::workflow::{
     self,
     job::{NormalJob, StepBody},
+    Trigger,
 };
 
-use crate::finding::{Route, SymbolicLocation};
-
 /// Represents an entire GitHub Actions workflow.
 ///
 /// This type implements [`Deref`] for [`workflow::Workflow`],
@@ -74,6 +75,24 @@ impl Workflow {
     pub(crate) fn jobs(&self) -> Jobs<'_> {
         Jobs::new(self)
     }
+
+    /// Whether this workflow's is triggered by pull_request_target.
+    pub(crate) fn has_pull_request_target(&self) -> bool {
+        match &self.on {
+            Trigger::BareEvent(event) => *event == BareEvent::PullRequestTarget,
+            Trigger::BareEvents(events) => events.contains(&BareEvent::PullRequestTarget),
+            Trigger::Events(events) => !matches!(events.pull_request_target, OptionalBody::Missing),
+        }
+    }
+
+    /// Whether this workflow's is triggered by workflow_run.
+    pub(crate) fn has_workflow_run(&self) -> bool {
+        match &self.on {
+            Trigger::BareEvent(event) => *event == BareEvent::WorkflowRun,
+            Trigger::BareEvents(events) => events.contains(&BareEvent::WorkflowRun),
+            Trigger::Events(events) => !matches!(events.workflow_run, OptionalBody::Missing),
+        }
+    }
 }
 
 /// Represents a single GitHub Actions job.
diff --git a/src/registry.rs b/src/registry.rs
index b45b387..6d67086 100644
--- a/src/registry.rs
+++ b/src/registry.rs
@@ -6,7 +6,7 @@ use std::{collections::HashMap, path::Path, process::ExitCode};
 use crate::{
     audit::WorkflowAudit,
     config::Config,
-    finding::{Finding, Severity},
+    finding::{Confidence, Finding, Severity},
     models::Workflow,
     App,
 };
@@ -113,6 +113,7 @@ impl AuditRegistry {
 pub(crate) struct FindingRegistry<'a> {
     config: &'a Config,
     minimum_severity: Option<Severity>,
+    minimum_confidence: Option<Confidence>,
     ignored: Vec<Finding<'a>>,
     findings: Vec<Finding<'a>>,
     highest_seen_severity: Option<Severity>,
@@ -123,6 +124,7 @@ impl<'a> FindingRegistry<'a> {
         Self {
             config,
             minimum_severity: app.min_severity,
+            minimum_confidence: app.min_confidence,
             ignored: Default::default(),
             findings: Default::default(),
             highest_seen_severity: None,
@@ -139,6 +141,9 @@ impl<'a> FindingRegistry<'a> {
                 || self
                     .minimum_severity
                     .map_or(false, |min| min > finding.determinations.severity)
+                || self
+                    .minimum_confidence
+                    .map_or(false, |min| min > finding.determinations.confidence)
                 || self.config.ignores(&finding)
             {
                 self.ignored.push(finding);


Test patch:
diff --git a/tests/acceptance.rs b/tests/acceptance.rs
index d1acd9e..0b84c4b 100644
--- a/tests/acceptance.rs
+++ b/tests/acceptance.rs
@@ -239,3 +239,25 @@ fn audit_unsecure_commands_allowed() -> anyhow::Result<()> {
 
     Ok(())
 }
+
+#[test]
+fn audit_github_env_injection() -> anyhow::Result<()> {
+    let auditable = workflow_under_test("github_env.yml");
+
+    let cli_args = [&auditable];
+
+    let execution = zizmor().args(cli_args).output()?;
+
+    assert_eq!(execution.status.code(), Some(14));
+
+    let findings = serde_json::from_slice(&execution.stdout)?;
+
+    assert_value_match(&findings, "$[0].determinations.confidence", "Low");
+    assert_value_match(
+        &findings,
+        "$[0].locations[0].concrete.feature",
+        "GITHUB_ENV",
+    );
+
+    Ok(())
+}
diff --git a/tests/test-data/github_env.yml b/tests/test-data/github_env.yml
new file mode 100644
index 0000000..029946e
--- /dev/null
+++ b/tests/test-data/github_env.yml
@@ -0,0 +1,14 @@
+on:
+  pull_request_target: # zizmor: ignore[dangerous-triggers]
+
+jobs:
+  vulnerable:
+    runs-on: ubuntu-latest
+
+    steps:
+      - name: Passes the title around
+        env:
+          TITLE: ${{ github.event.pull_request.title }}
+        run: |
+          message=$(echo "$TITLE" | grep -oP '[{\[][^}\]]+[}\]]' | sed 's/{\|}\|\[\|\]//g')
+          echo "message=$message" >> $GITHUB_ENV