From 3e9f0ad28e4551ea77a2935cf6b25a6d79399ff8 Mon Sep 17 00:00:00 2001 From: Adriano Di Luzio Date: Mon, 31 Mar 2025 13:45:44 +0200 Subject: [PATCH 01/12] feat: Audit secrets outside an environment --- src/audit/mod.rs | 1 + src/audit/secrets_outside_environment.rs | 79 +++++++++++++++++++ src/main.rs | 11 +-- src/models.rs | 14 ++-- .../test-data/secrets-outside-environment.yml | 12 +++ 5 files changed, 107 insertions(+), 10 deletions(-) create mode 100644 src/audit/secrets_outside_environment.rs create mode 100644 tests/integration/test-data/secrets-outside-environment.yml diff --git a/src/audit/mod.rs b/src/audit/mod.rs index c684c3f91..0f34f564f 100644 --- a/src/audit/mod.rs +++ b/src/audit/mod.rs @@ -26,6 +26,7 @@ pub(crate) mod known_vulnerable_actions; pub(crate) mod overprovisioned_secrets; pub(crate) mod ref_confusion; pub(crate) mod secrets_inherit; +pub(crate) mod secrets_outside_environment; pub(crate) mod self_hosted_runner; pub(crate) mod template_injection; pub(crate) mod unpinned_uses; diff --git a/src/audit/secrets_outside_environment.rs b/src/audit/secrets_outside_environment.rs new file mode 100644 index 000000000..22fbd62c9 --- /dev/null +++ b/src/audit/secrets_outside_environment.rs @@ -0,0 +1,79 @@ +use github_actions_models::{ + common::{expr::LoE, Env, EnvValue}, + workflow::job::StepBody, +}; + +use super::{audit_meta, Audit}; +use crate::{finding::Confidence, models::Job}; + +pub(crate) struct SecretsOutsideEnvironment; + +audit_meta!( + SecretsOutsideEnvironment, + "secrets-outside-environment", + "secrets used without an environment to gate them" +); + +impl Audit for SecretsOutsideEnvironment { + fn new(_state: super::AuditState) -> anyhow::Result + where + Self: Sized, + { + Ok(Self) + } + + fn audit_raw<'w>( + &self, + input: &'w super::AuditInput, + ) -> anyhow::Result>> { + let mut findings = vec![]; + + if let super::AuditInput::Workflow(w) = input { + for job in w.jobs() { + if let Job::NormalJob(j) = job { + if j.environment().is_some() { + continue; + } + + for step in j.steps() { + let body = &step.body; + let eenv: &Env; + + match body { + StepBody::Uses { uses: _, with } => { + eenv = with; + } + StepBody::Run { + run: _, + shell: _, + env, + working_directory: _, + } => match env { + LoE::Expr(_) => { + panic!("We don't handle Expr yet!") + } + LoE::Literal(env) => eenv = env, + }, + } + + for v in eenv.values() { + if let EnvValue::String(s) = v { + if s.contains("secrets") { + findings.push( + Self::finding() + .add_location(step.location().primary()) + .confidence(Confidence::High) + .severity(crate::finding::Severity::High) + .build(input)?, + ); + } + } + } + } + } + } + } + + Ok(findings) + } +} diff --git a/src/main.rs b/src/main.rs index 87bfa1bf7..11ab424be 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,12 +1,12 @@ use std::{ - io::{Write, stdout}, + io::{stdout, Write}, process::ExitCode, str::FromStr, }; use annotate_snippets::{Level, Renderer}; use anstream::{eprintln, stream::IsTerminal}; -use anyhow::{Context, Result, anyhow}; +use anyhow::{anyhow, Context, Result}; use audit::Audit; use camino::{Utf8Path, Utf8PathBuf}; use clap::{Parser, ValueEnum}; @@ -21,9 +21,9 @@ use models::Action; use owo_colors::OwoColorize; use registry::{AuditRegistry, FindingRegistry, InputRegistry}; use state::AuditState; -use tracing::{Span, info_span, instrument}; -use tracing_indicatif::{IndicatifLayer, span_ext::IndicatifSpanExt}; -use tracing_subscriber::{EnvFilter, layer::SubscriberExt as _, util::SubscriberInitExt as _}; +use tracing::{info_span, instrument, Span}; +use tracing_indicatif::{span_ext::IndicatifSpanExt, IndicatifLayer}; +use tracing_subscriber::{layer::SubscriberExt as _, util::SubscriberInitExt as _, EnvFilter}; mod audit; mod config; @@ -501,6 +501,7 @@ fn run() -> Result { register_audit!(audit::github_env::GitHubEnv); register_audit!(audit::cache_poisoning::CachePoisoning); register_audit!(audit::secrets_inherit::SecretsInherit); + register_audit!(audit::secrets_outside_environment::SecretsOutsideEnvironment); register_audit!(audit::bot_conditions::BotConditions); register_audit!(audit::overprovisioned_secrets::OverprovisionedSecrets); register_audit!(audit::unredacted_secrets::UnredactedSecrets); diff --git a/src/models.rs b/src/models.rs index 22031331a..3430ea513 100644 --- a/src/models.rs +++ b/src/models.rs @@ -5,17 +5,17 @@ use std::collections::HashMap; use std::fmt::Debug; use std::{iter::Enumerate, ops::Deref}; -use anyhow::{Context, Result, bail}; +use anyhow::{bail, Context, Result}; use camino::Utf8Path; -use github_actions_models::common::Env; use github_actions_models::common::expr::LoE; +use github_actions_models::common::Env; use github_actions_models::workflow::event::{BareEvent, OptionalBody}; -use github_actions_models::workflow::job::{RunsOn, Strategy}; -use github_actions_models::workflow::{self, Trigger, job, job::StepBody}; +use github_actions_models::workflow::job::{DeploymentEnvironment, RunsOn, Strategy}; +use github_actions_models::workflow::{self, job, job::StepBody, Trigger}; use github_actions_models::{action, common}; use indexmap::IndexMap; use line_index::LineIndex; -use serde_json::{Value, json}; +use serde_json::{json, Value}; use terminal_link::Link; use crate::finding::{Route, SymbolicLocation}; @@ -210,6 +210,10 @@ impl<'w> NormalJob<'w> { Steps::new(self) } + pub(crate) fn environment(&self) -> Option<&DeploymentEnvironment> { + self.inner.environment.as_ref() + } + /// Perform feats of heroism to figure of what this job's runner's /// default shell is. /// diff --git a/tests/integration/test-data/secrets-outside-environment.yml b/tests/integration/test-data/secrets-outside-environment.yml new file mode 100644 index 000000000..9dea993b5 --- /dev/null +++ b/tests/integration/test-data/secrets-outside-environment.yml @@ -0,0 +1,12 @@ +name: Action +on: push +jobs: + build: + name: Job + runs-on: ubuntu-latest + steps: + - name: Docker setup + uses: stacks-network/actions/docker@main + with: + username: ${{ secrets.DOCKERHUB_USERNAME }} + password: ${{ secrets.DOCKERHUB_PASSWORD }} From 13f921369cfcc611dc83c836149e35e93be2aee5 Mon Sep 17 00:00:00 2001 From: Adriano Di Luzio Date: Mon, 31 Mar 2025 13:54:03 +0200 Subject: [PATCH 02/12] Add TODO --- src/audit/secrets_outside_environment.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/audit/secrets_outside_environment.rs b/src/audit/secrets_outside_environment.rs index 22fbd62c9..d7e43cb85 100644 --- a/src/audit/secrets_outside_environment.rs +++ b/src/audit/secrets_outside_environment.rs @@ -50,6 +50,7 @@ impl Audit for SecretsOutsideEnvironment { working_directory: _, } => match env { LoE::Expr(_) => { + // TODO: Implement this. panic!("We don't handle Expr yet!") } LoE::Literal(env) => eenv = env, From acaaef1266aa9c9bc27ea248f3f68c87a1e0a8e0 Mon Sep 17 00:00:00 2001 From: Adriano Di Luzio Date: Mon, 31 Mar 2025 13:55:25 +0200 Subject: [PATCH 03/12] Make name more generic --- tests/integration/test-data/secrets-outside-environment.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test-data/secrets-outside-environment.yml b/tests/integration/test-data/secrets-outside-environment.yml index 9dea993b5..a0a3362f5 100644 --- a/tests/integration/test-data/secrets-outside-environment.yml +++ b/tests/integration/test-data/secrets-outside-environment.yml @@ -6,7 +6,7 @@ jobs: runs-on: ubuntu-latest steps: - name: Docker setup - uses: stacks-network/actions/docker@main + uses: actions_repo/actions/docker@main with: username: ${{ secrets.DOCKERHUB_USERNAME }} password: ${{ secrets.DOCKERHUB_PASSWORD }} From 992b15b6e656d95ce3a06c649dbddc33496ed5a9 Mon Sep 17 00:00:00 2001 From: Adriano Di Luzio Date: Wed, 9 Apr 2025 15:08:57 +0200 Subject: [PATCH 04/12] Add snapshot test --- tests/integration/snapshot.rs | 11 +++++++++++ .../test-data/secrets-outside-environment.yml | 8 +++----- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/tests/integration/snapshot.rs b/tests/integration/snapshot.rs index 90264f242..757e528de 100644 --- a/tests/integration/snapshot.rs +++ b/tests/integration/snapshot.rs @@ -533,3 +533,14 @@ fn unredacted_secrets() -> Result<()> { Ok(()) } + +#[test] +fn secrets_outside_environment() -> Result<()> { + insta::assert_snapshot!( + zizmor() + .input(input_under_test("secrets-outside-environment.yml")) + .run()? + ); + + Ok(()) +} diff --git a/tests/integration/test-data/secrets-outside-environment.yml b/tests/integration/test-data/secrets-outside-environment.yml index a0a3362f5..de4c2cba8 100644 --- a/tests/integration/test-data/secrets-outside-environment.yml +++ b/tests/integration/test-data/secrets-outside-environment.yml @@ -1,12 +1,10 @@ -name: Action on: push +permissions: {} jobs: build: - name: Job runs-on: ubuntu-latest steps: - - name: Docker setup - uses: actions_repo/actions/docker@main + - uses: actions_repo/actions/docker@main with: - username: ${{ secrets.DOCKERHUB_USERNAME }} + # NOT OK: Anyone with write access can exfiltrate this secret. password: ${{ secrets.DOCKERHUB_PASSWORD }} From 1d5b2df2bf81249bd483fc1e35c268e44d3c46e3 Mon Sep 17 00:00:00 2001 From: Adriano Di Luzio Date: Thu, 10 Apr 2025 09:07:29 +0200 Subject: [PATCH 05/12] Switch to `audit_step` and handle expressions --- src/audit/secrets_outside_environment.rs | 96 ++++++++++++------------ 1 file changed, 49 insertions(+), 47 deletions(-) diff --git a/src/audit/secrets_outside_environment.rs b/src/audit/secrets_outside_environment.rs index d7e43cb85..7a3dd1fd1 100644 --- a/src/audit/secrets_outside_environment.rs +++ b/src/audit/secrets_outside_environment.rs @@ -1,10 +1,10 @@ use github_actions_models::{ - common::{expr::LoE, Env, EnvValue}, + common::{Env, EnvValue, expr::LoE}, workflow::job::StepBody, }; -use super::{audit_meta, Audit}; -use crate::{finding::Confidence, models::Job}; +use super::{Audit, audit_meta}; +use crate::finding::Confidence; pub(crate) struct SecretsOutsideEnvironment; @@ -22,59 +22,61 @@ impl Audit for SecretsOutsideEnvironment { Ok(Self) } - fn audit_raw<'w>( + fn audit_step<'w>( &self, - input: &'w super::AuditInput, + step: &crate::models::Step<'w>, ) -> anyhow::Result>> { let mut findings = vec![]; - if let super::AuditInput::Workflow(w) = input { - for job in w.jobs() { - if let Job::NormalJob(j) = job { - if j.environment().is_some() { - continue; - } - - for step in j.steps() { - let body = &step.body; - let eenv: &Env; - - match body { - StepBody::Uses { uses: _, with } => { - eenv = with; - } - StepBody::Run { - run: _, - shell: _, - env, - working_directory: _, - } => match env { - LoE::Expr(_) => { - // TODO: Implement this. - panic!("We don't handle Expr yet!") - } - LoE::Literal(env) => eenv = env, - }, - } + if step.parent.environment().is_some() { + return Ok(findings); + } - for v in eenv.values() { - if let EnvValue::String(s) = v { - if s.contains("secrets") { - findings.push( - Self::finding() - .add_location(step.location().primary()) - .confidence(Confidence::High) - .severity(crate::finding::Severity::High) - .build(input)?, - ); - } - } - } - } + let eenv: &Env; + match &step.body { + StepBody::Uses { uses: _, with } => { + eenv = with; + } + StepBody::Run { + run: _, + shell: _, + env, + working_directory: _, + } => match env { + LoE::Expr(e) => { + Self::check_secrets_access(e.as_bare(), step, &mut findings)?; + return Ok(findings); } + LoE::Literal(env) => eenv = env, + }, + } + + for v in eenv.values() { + if let EnvValue::String(s) = v { + Self::check_secrets_access(s, step, &mut findings)? } } Ok(findings) } } + +impl SecretsOutsideEnvironment { + fn check_secrets_access<'w>( + s: &str, + step: &crate::models::Step<'w>, + findings: &mut Vec>, + ) -> anyhow::Result<()> { + if s.contains("secrets") { + findings.push( + Self::finding() + .add_location(step.location().primary()) + .confidence(Confidence::High) + .severity(crate::finding::Severity::High) + .build(step.workflow())?, + ); + } + + Ok(()) + } +} From 6b963b69d7b4f6f331e55023f69bf40a53b56e7e Mon Sep 17 00:00:00 2001 From: Adriano Di Luzio Date: Thu, 10 Apr 2025 09:11:30 +0200 Subject: [PATCH 06/12] Add `environment` to `use-trusted-publishing.yml` --- tests/integration/test-data/use-trusted-publishing.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/integration/test-data/use-trusted-publishing.yml b/tests/integration/test-data/use-trusted-publishing.yml index 39ec8fbc4..e6c51a1b0 100644 --- a/tests/integration/test-data/use-trusted-publishing.yml +++ b/tests/integration/test-data/use-trusted-publishing.yml @@ -6,6 +6,7 @@ on: [push] jobs: publish: runs-on: ubuntu-latest + environment: "secret environment" permissions: id-token: write steps: From 2ae5791ef4ab90c8d8bd81aec6e9bb104e3cbce1 Mon Sep 17 00:00:00 2001 From: Adriano Di Luzio Date: Tue, 22 Apr 2025 10:09:11 +0200 Subject: [PATCH 07/12] Fix `merge` --- tests/integration/snapshot.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/integration/snapshot.rs b/tests/integration/snapshot.rs index 44cb9902b..31f339b4e 100644 --- a/tests/integration/snapshot.rs +++ b/tests/integration/snapshot.rs @@ -657,6 +657,9 @@ fn secrets_outside_environment() -> Result<()> { .run()? ); + Ok(()) +} + #[test] fn forbidden_uses() -> Result<()> { for config in ["allow-all", "deny-all", "allow-some", "deny-some"] { @@ -674,4 +677,4 @@ fn forbidden_uses() -> Result<()> { Ok(()) -} \ No newline at end of file +} From cf1b0e5b17a09410e272d36aeef19af4318260c7 Mon Sep 17 00:00:00 2001 From: Adriano Di Luzio Date: Tue, 22 Apr 2025 10:09:18 +0200 Subject: [PATCH 08/12] Update to `Audit` from `main` --- src/audit/secrets_outside_environment.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/audit/secrets_outside_environment.rs b/src/audit/secrets_outside_environment.rs index 7a3dd1fd1..621127d8c 100644 --- a/src/audit/secrets_outside_environment.rs +++ b/src/audit/secrets_outside_environment.rs @@ -4,7 +4,7 @@ use github_actions_models::{ }; use super::{Audit, audit_meta}; -use crate::finding::Confidence; +use crate::{finding::Confidence, AuditState, AuditLoadError}; pub(crate) struct SecretsOutsideEnvironment; @@ -15,7 +15,7 @@ audit_meta!( ); impl Audit for SecretsOutsideEnvironment { - fn new(_state: super::AuditState) -> anyhow::Result + fn new(_state: &AuditState<'_>) -> Result where Self: Sized, { From 013261777f419b7a31f864b5bea50463a6460a42 Mon Sep 17 00:00:00 2001 From: Adriano Di Luzio Date: Tue, 22 Apr 2025 10:09:49 +0200 Subject: [PATCH 09/12] Update integration tests and snaps --- ...tegration__snapshot__cache_poisoning-10.snap | 10 +++++----- ...ntegration__snapshot__cache_poisoning-4.snap | 8 ++++---- ...tion__snapshot__overprovisioned_secrets.snap | 11 +++++------ ...__snapshot__secrets_outside_environment.snap | 17 +++++++++++++++++ ...tegration__snapshot__unredacted_secrets.snap | 11 +++++------ tests/integration/test-data/cache-poisoning.yml | 1 + .../caching-opt-in-expression.yml | 1 + .../cache-poisoning/publisher-step.yml | 1 + .../test-data/overprovisioned-secrets.yml | 1 + .../test-data/secrets-outside-environment.yml | 2 +- .../test-data/unredacted-secrets.yml | 1 + 11 files changed, 42 insertions(+), 22 deletions(-) create mode 100644 tests/integration/snapshots/integration__snapshot__secrets_outside_environment.snap diff --git a/tests/integration/snapshots/integration__snapshot__cache_poisoning-10.snap b/tests/integration/snapshots/integration__snapshot__cache_poisoning-10.snap index be5022660..7e51418ea 100644 --- a/tests/integration/snapshots/integration__snapshot__cache_poisoning-10.snap +++ b/tests/integration/snapshots/integration__snapshot__cache_poisoning-10.snap @@ -3,14 +3,14 @@ source: tests/integration/snapshot.rs expression: "zizmor().input(input_under_test(\"cache-poisoning/publisher-step.yml\")).run()?" --- error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache poisoning attack - --> @@INPUT@@:25:9 + --> @@INPUT@@:26:9 | -18 | uses: Swatinem/rust-cache@82a92a6e8fbeee089604da2575dc567ae9ddeaab +19 | uses: Swatinem/rust-cache@82a92a6e8fbeee089604da2575dc567ae9ddeaab | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cache enabled by default here -19 | +20 | ... -24 | - name: Publish draft release on Github -25 | uses: softprops/action-gh-release@01570a1f39cb168c169c802c3bceb9e93fb10974 +25 | - name: Publish draft release on Github +26 | uses: softprops/action-gh-release@01570a1f39cb168c169c802c3bceb9e93fb10974 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ runtime artifacts usually published here | = note: audit confidence → Low diff --git a/tests/integration/snapshots/integration__snapshot__cache_poisoning-4.snap b/tests/integration/snapshots/integration__snapshot__cache_poisoning-4.snap index 887a362ab..7c8558c5b 100644 --- a/tests/integration/snapshots/integration__snapshot__cache_poisoning-4.snap +++ b/tests/integration/snapshots/integration__snapshot__cache_poisoning-4.snap @@ -9,10 +9,10 @@ error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache pois | ^^^^^^^^^^^ generally used when publishing artifacts generated at runtime 2 | ... -15 | uses: astral-sh/setup-uv@38f3f104447c67c051c4a08e39b64a148898af3a -16 | / with: -17 | | python-version: "3.12" -18 | | enable-cache: ${{ github.ref == 'refs/heads/main' }} +16 | uses: astral-sh/setup-uv@38f3f104447c67c051c4a08e39b64a148898af3a +17 | / with: +18 | | python-version: "3.12" +19 | | enable-cache: ${{ github.ref == 'refs/heads/main' }} | |______________________________________________________________^ opt-in for caching might happen here | = note: audit confidence → Low diff --git a/tests/integration/snapshots/integration__snapshot__overprovisioned_secrets.snap b/tests/integration/snapshots/integration__snapshot__overprovisioned_secrets.snap index bf0219c96..0086f0f33 100644 --- a/tests/integration/snapshots/integration__snapshot__overprovisioned_secrets.snap +++ b/tests/integration/snapshots/integration__snapshot__overprovisioned_secrets.snap @@ -1,20 +1,19 @@ --- source: tests/integration/snapshot.rs -expression: "zizmor().input(workflow_under_test(\"overprovisioned-secrets.yml\")).run()?" -snapshot_kind: text +expression: "zizmor().input(input_under_test(\"overprovisioned-secrets.yml\")).run()?" --- warning[overprovisioned-secrets]: excessively provisioned secrets - --> @@INPUT@@:12:18 + --> @@INPUT@@:13:18 | -12 | stuff: ${{ format('{0}', toJSON(secrets)) }} +13 | stuff: ${{ format('{0}', toJSON(secrets)) }} | ------------------------------------- injects the entire secrets context into the runner | = note: audit confidence → High warning[overprovisioned-secrets]: excessively provisioned secrets - --> @@INPUT@@:21:25 + --> @@INPUT@@:22:25 | -21 | secrets_json: ${{ toJSON(secrets) }} +22 | secrets_json: ${{ toJSON(secrets) }} | ---------------------- injects the entire secrets context into the runner | = note: audit confidence → High diff --git a/tests/integration/snapshots/integration__snapshot__secrets_outside_environment.snap b/tests/integration/snapshots/integration__snapshot__secrets_outside_environment.snap new file mode 100644 index 000000000..af0a61b06 --- /dev/null +++ b/tests/integration/snapshots/integration__snapshot__secrets_outside_environment.snap @@ -0,0 +1,17 @@ +--- +source: tests/integration/snapshot.rs +expression: "zizmor().input(input_under_test(\"secrets-outside-environment.yml\")).run()?" +--- +error[secrets-outside-environment]: secrets used without an environment to gate them + --> @@INPUT@@:7:9 + | + 7 | - uses: actions_repo/actions/docker@13c8cf37e54dd9488afe8c067575444cc58bf155 + | _________^ + 8 | | with: + 9 | | # NOT OK: Anyone with write access can exfiltrate this secret. +10 | | password: ${{ secrets.DOCKERHUB_PASSWORD }} + | |______________________________________________________^ this step + | + = note: audit confidence → High + +1 finding: 0 unknown, 0 informational, 0 low, 0 medium, 1 high diff --git a/tests/integration/snapshots/integration__snapshot__unredacted_secrets.snap b/tests/integration/snapshots/integration__snapshot__unredacted_secrets.snap index 67f38f40f..343799dfe 100644 --- a/tests/integration/snapshots/integration__snapshot__unredacted_secrets.snap +++ b/tests/integration/snapshots/integration__snapshot__unredacted_secrets.snap @@ -1,20 +1,19 @@ --- source: tests/integration/snapshot.rs -expression: "zizmor().input(workflow_under_test(\"unredacted-secrets.yml\")).run()?" -snapshot_kind: text +expression: "zizmor().input(input_under_test(\"unredacted-secrets.yml\")).run()?" --- warning[unredacted-secrets]: leaked secret values - --> @@INPUT@@:14:18 + --> @@INPUT@@:15:18 | -14 | stuff: ${{ fromJSON(secrets.password) }} +15 | stuff: ${{ fromJSON(secrets.password) }} | --------------------------------- bypasses secret redaction | = note: audit confidence → High warning[unredacted-secrets]: leaked secret values - --> @@INPUT@@:17:23 + --> @@INPUT@@:18:23 | -17 | otherstuff: ${{ fromJson(secrets.otherstuff).field }} +18 | otherstuff: ${{ fromJson(secrets.otherstuff).field }} | ----------------------------------------- bypasses secret redaction | = note: audit confidence → High diff --git a/tests/integration/test-data/cache-poisoning.yml b/tests/integration/test-data/cache-poisoning.yml index f0a25a874..339977ffb 100644 --- a/tests/integration/test-data/cache-poisoning.yml +++ b/tests/integration/test-data/cache-poisoning.yml @@ -5,6 +5,7 @@ permissions: {} jobs: publish: runs-on: ubuntu-latest + environment: "Pypi" steps: - name: Setup uv uses: astral-sh/setup-uv@38f3f104447c67c051c4a08e39b64a148898af3a diff --git a/tests/integration/test-data/cache-poisoning/caching-opt-in-expression.yml b/tests/integration/test-data/cache-poisoning/caching-opt-in-expression.yml index 935f2b4e3..18c87c92b 100644 --- a/tests/integration/test-data/cache-poisoning/caching-opt-in-expression.yml +++ b/tests/integration/test-data/cache-poisoning/caching-opt-in-expression.yml @@ -5,6 +5,7 @@ permissions: {} jobs: publish: runs-on: ubuntu-latest + environment: "Pypi" steps: - name: Project Checkout uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 diff --git a/tests/integration/test-data/cache-poisoning/publisher-step.yml b/tests/integration/test-data/cache-poisoning/publisher-step.yml index fdbc3b56a..b9b78279c 100644 --- a/tests/integration/test-data/cache-poisoning/publisher-step.yml +++ b/tests/integration/test-data/cache-poisoning/publisher-step.yml @@ -8,6 +8,7 @@ permissions: {} jobs: publish: runs-on: macos-latest + environment: "GitHub release" steps: - name: Project Checkout uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 diff --git a/tests/integration/test-data/overprovisioned-secrets.yml b/tests/integration/test-data/overprovisioned-secrets.yml index 33752404d..ecead96e6 100644 --- a/tests/integration/test-data/overprovisioned-secrets.yml +++ b/tests/integration/test-data/overprovisioned-secrets.yml @@ -5,6 +5,7 @@ permissions: {} jobs: test: runs-on: ubuntu-latest + environment: "overprovisioned-secrets" steps: - run: echo "${stuff} ${otherstuff} ${morestuff}" env: diff --git a/tests/integration/test-data/secrets-outside-environment.yml b/tests/integration/test-data/secrets-outside-environment.yml index de4c2cba8..f8f2badd1 100644 --- a/tests/integration/test-data/secrets-outside-environment.yml +++ b/tests/integration/test-data/secrets-outside-environment.yml @@ -4,7 +4,7 @@ jobs: build: runs-on: ubuntu-latest steps: - - uses: actions_repo/actions/docker@main + - uses: actions_repo/actions/docker@13c8cf37e54dd9488afe8c067575444cc58bf155 with: # NOT OK: Anyone with write access can exfiltrate this secret. password: ${{ secrets.DOCKERHUB_PASSWORD }} diff --git a/tests/integration/test-data/unredacted-secrets.yml b/tests/integration/test-data/unredacted-secrets.yml index c809caaaf..0e8296473 100644 --- a/tests/integration/test-data/unredacted-secrets.yml +++ b/tests/integration/test-data/unredacted-secrets.yml @@ -7,6 +7,7 @@ permissions: {} jobs: test: runs-on: ubuntu-latest + environment: "Unredacted secrets" steps: - run: echo "${stuff} ${otherstuff}" env: From f328751fe913bcfbb9f466e7f3937b88c194e64e Mon Sep 17 00:00:00 2001 From: Adriano Di Luzio Date: Mon, 5 May 2025 08:14:11 +0200 Subject: [PATCH 10/12] Make this a pedantic audit --- src/audit/secrets_outside_environment.rs | 3 ++- src/models.rs | 1 - tests/integration/snapshot.rs | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/audit/secrets_outside_environment.rs b/src/audit/secrets_outside_environment.rs index 621127d8c..830394e34 100644 --- a/src/audit/secrets_outside_environment.rs +++ b/src/audit/secrets_outside_environment.rs @@ -4,7 +4,7 @@ use github_actions_models::{ }; use super::{Audit, audit_meta}; -use crate::{finding::Confidence, AuditState, AuditLoadError}; +use crate::{finding::Confidence, models::StepCommon, AuditState, AuditLoadError, Persona}; pub(crate) struct SecretsOutsideEnvironment; @@ -73,6 +73,7 @@ impl SecretsOutsideEnvironment { .add_location(step.location().primary()) .confidence(Confidence::High) .severity(crate::finding::Severity::High) + .persona(Persona::Pedantic) .build(step.workflow())?, ); } diff --git a/src/models.rs b/src/models.rs index d019772a2..2a23b46c1 100644 --- a/src/models.rs +++ b/src/models.rs @@ -8,7 +8,6 @@ use std::{iter::Enumerate, ops::Deref}; use anyhow::{Context, bail}; use github_actions_models::common::Env; use github_actions_models::common::expr::LoE; -use github_actions_models::common::Env; use github_actions_models::workflow::event::{BareEvent, OptionalBody}; use github_actions_models::workflow::job::{DeploymentEnvironment, RunsOn, Strategy}; use github_actions_models::workflow::{self, job, job::StepBody, Trigger}; diff --git a/tests/integration/snapshot.rs b/tests/integration/snapshot.rs index bdba4cb08..769ddb89c 100644 --- a/tests/integration/snapshot.rs +++ b/tests/integration/snapshot.rs @@ -652,6 +652,7 @@ fn secrets_outside_environment() -> Result<()> { insta::assert_snapshot!( zizmor() .input(input_under_test("secrets-outside-environment.yml")) + .args(["--persona=pedantic"]) .run()? ); From 96c90ace2bc5efdc5e6aa6d6e648d9ac3cb59abd Mon Sep 17 00:00:00 2001 From: Adriano Di Luzio Date: Tue, 6 May 2025 08:20:57 +0200 Subject: [PATCH 11/12] Rename to `secret_without_env` --- src/audit/mod.rs | 2 +- ..._outside_environment.rs => secret_without_env.rs} | 12 ++++++------ src/main.rs | 2 +- tests/integration/snapshot.rs | 2 +- ...ation__snapshot__secrets_outside_environment.snap | 4 ++-- ...utside-environment.yml => secret-without-env.yml} | 0 6 files changed, 11 insertions(+), 11 deletions(-) rename src/audit/{secrets_outside_environment.rs => secret_without_env.rs} (89%) rename tests/integration/test-data/{secrets-outside-environment.yml => secret-without-env.yml} (100%) diff --git a/src/audit/mod.rs b/src/audit/mod.rs index 04894f10c..499d2049a 100644 --- a/src/audit/mod.rs +++ b/src/audit/mod.rs @@ -30,7 +30,7 @@ pub(crate) mod obfuscation; pub(crate) mod overprovisioned_secrets; pub(crate) mod ref_confusion; pub(crate) mod secrets_inherit; -pub(crate) mod secrets_outside_environment; +pub(crate) mod secret_without_env; pub(crate) mod self_hosted_runner; pub(crate) mod stale_action_refs; pub(crate) mod template_injection; diff --git a/src/audit/secrets_outside_environment.rs b/src/audit/secret_without_env.rs similarity index 89% rename from src/audit/secrets_outside_environment.rs rename to src/audit/secret_without_env.rs index 830394e34..8a25ef242 100644 --- a/src/audit/secrets_outside_environment.rs +++ b/src/audit/secret_without_env.rs @@ -6,15 +6,15 @@ use github_actions_models::{ use super::{Audit, audit_meta}; use crate::{finding::Confidence, models::StepCommon, AuditState, AuditLoadError, Persona}; -pub(crate) struct SecretsOutsideEnvironment; +pub(crate) struct SecretWithoutEnv; audit_meta!( - SecretsOutsideEnvironment, - "secrets-outside-environment", - "secrets used without an environment to gate them" + SecretWithoutEnv, + "secret-without-env", + "secret used without an environment to gate it" ); -impl Audit for SecretsOutsideEnvironment { +impl Audit for SecretWithoutEnv { fn new(_state: &AuditState<'_>) -> Result where Self: Sized, @@ -61,7 +61,7 @@ impl Audit for SecretsOutsideEnvironment { } } -impl SecretsOutsideEnvironment { +impl SecretWithoutEnv { fn check_secrets_access<'w>( s: &str, step: &crate::models::Step<'w>, diff --git a/src/main.rs b/src/main.rs index 560dbea3f..9a5ca826e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -546,7 +546,7 @@ fn run() -> Result { register_audit!(audit::github_env::GitHubEnv); register_audit!(audit::cache_poisoning::CachePoisoning); register_audit!(audit::secrets_inherit::SecretsInherit); - register_audit!(audit::secrets_outside_environment::SecretsOutsideEnvironment); + register_audit!(audit::secret_without_env::SecretWithoutEnv); register_audit!(audit::bot_conditions::BotConditions); register_audit!(audit::overprovisioned_secrets::OverprovisionedSecrets); register_audit!(audit::unredacted_secrets::UnredactedSecrets); diff --git a/tests/integration/snapshot.rs b/tests/integration/snapshot.rs index 769ddb89c..63c16f208 100644 --- a/tests/integration/snapshot.rs +++ b/tests/integration/snapshot.rs @@ -651,7 +651,7 @@ fn unredacted_secrets() -> Result<()> { fn secrets_outside_environment() -> Result<()> { insta::assert_snapshot!( zizmor() - .input(input_under_test("secrets-outside-environment.yml")) + .input(input_under_test("secret-without-env.yml")) .args(["--persona=pedantic"]) .run()? ); diff --git a/tests/integration/snapshots/integration__snapshot__secrets_outside_environment.snap b/tests/integration/snapshots/integration__snapshot__secrets_outside_environment.snap index af0a61b06..7823f99cd 100644 --- a/tests/integration/snapshots/integration__snapshot__secrets_outside_environment.snap +++ b/tests/integration/snapshots/integration__snapshot__secrets_outside_environment.snap @@ -1,8 +1,8 @@ --- source: tests/integration/snapshot.rs -expression: "zizmor().input(input_under_test(\"secrets-outside-environment.yml\")).run()?" +expression: "zizmor().input(input_under_test(\"secret-without-env.yml\")).args([\"--persona=pedantic\"]).run()?" --- -error[secrets-outside-environment]: secrets used without an environment to gate them +error[secret-without-env]: secret used without an environment to gate it --> @@INPUT@@:7:9 | 7 | - uses: actions_repo/actions/docker@13c8cf37e54dd9488afe8c067575444cc58bf155 diff --git a/tests/integration/test-data/secrets-outside-environment.yml b/tests/integration/test-data/secret-without-env.yml similarity index 100% rename from tests/integration/test-data/secrets-outside-environment.yml rename to tests/integration/test-data/secret-without-env.yml From b5dab629bae4e65b5eae63b3308a8d828b46ec50 Mon Sep 17 00:00:00 2001 From: Adriano Di Luzio Date: Tue, 6 May 2025 09:13:18 +0200 Subject: [PATCH 12/12] Whitelist `secrets.github_token` --- src/audit/secret_without_env.rs | 17 +++++++++++++---- ...__snapshot__secrets_outside_environment.snap | 10 +++++----- .../test-data/secret-without-env.yml | 3 +++ 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/src/audit/secret_without_env.rs b/src/audit/secret_without_env.rs index 8a25ef242..f5afadde1 100644 --- a/src/audit/secret_without_env.rs +++ b/src/audit/secret_without_env.rs @@ -1,10 +1,13 @@ use github_actions_models::{ - common::{Env, EnvValue, expr::LoE}, + common::{ + Env, EnvValue, + expr::{ExplicitExpr, LoE}, + }, workflow::job::StepBody, }; use super::{Audit, audit_meta}; -use crate::{finding::Confidence, models::StepCommon, AuditState, AuditLoadError, Persona}; +use crate::{AuditLoadError, AuditState, Persona, finding::Confidence, models::StepCommon}; pub(crate) struct SecretWithoutEnv; @@ -53,7 +56,11 @@ impl Audit for SecretWithoutEnv { for v in eenv.values() { if let EnvValue::String(s) = v { - Self::check_secrets_access(s, step, &mut findings)? + if let Some(expr) = ExplicitExpr::from_curly(s) { + Self::check_secrets_access(expr.as_bare(), step, &mut findings)?; + } else { + Self::check_secrets_access(s, step, &mut findings)? + } } } @@ -67,7 +74,9 @@ impl SecretWithoutEnv { step: &crate::models::Step<'w>, findings: &mut Vec>, ) -> anyhow::Result<()> { - if s.contains("secrets") { + // See https://docs.github.com/en/actions/security-for-github-actions/security-guides/automatic-token-authentication + // for `secrets.github_token` + if s.contains("secrets") && s.trim() != "secrets.github_token" { findings.push( Self::finding() .add_location(step.location().primary()) diff --git a/tests/integration/snapshots/integration__snapshot__secrets_outside_environment.snap b/tests/integration/snapshots/integration__snapshot__secrets_outside_environment.snap index 7823f99cd..5aab4fe00 100644 --- a/tests/integration/snapshots/integration__snapshot__secrets_outside_environment.snap +++ b/tests/integration/snapshots/integration__snapshot__secrets_outside_environment.snap @@ -3,13 +3,13 @@ source: tests/integration/snapshot.rs expression: "zizmor().input(input_under_test(\"secret-without-env.yml\")).args([\"--persona=pedantic\"]).run()?" --- error[secret-without-env]: secret used without an environment to gate it - --> @@INPUT@@:7:9 + --> @@INPUT@@:10:9 | - 7 | - uses: actions_repo/actions/docker@13c8cf37e54dd9488afe8c067575444cc58bf155 +10 | - uses: actions_repo/actions/docker@13c8cf37e54dd9488afe8c067575444cc58bf155 | _________^ - 8 | | with: - 9 | | # NOT OK: Anyone with write access can exfiltrate this secret. -10 | | password: ${{ secrets.DOCKERHUB_PASSWORD }} +11 | | with: +12 | | # NOT OK: Anyone with write access can exfiltrate this secret. +13 | | password: ${{ secrets.DOCKERHUB_PASSWORD }} | |______________________________________________________^ this step | = note: audit confidence → High diff --git a/tests/integration/test-data/secret-without-env.yml b/tests/integration/test-data/secret-without-env.yml index f8f2badd1..ff5dd5c11 100644 --- a/tests/integration/test-data/secret-without-env.yml +++ b/tests/integration/test-data/secret-without-env.yml @@ -4,6 +4,9 @@ jobs: build: runs-on: ubuntu-latest steps: + - uses: actions_repo/actions/docker-login@13c8cf37e54dd9488afe8c067575444cc58bf155 + with: + password: ${{ secrets.github_token }} # This is OK. - uses: actions_repo/actions/docker@13c8cf37e54dd9488afe8c067575444cc58bf155 with: # NOT OK: Anyone with write access can exfiltrate this secret.