diff --git a/src/audit/mod.rs b/src/audit/mod.rs index 483e58fe8..499d2049a 100644 --- a/src/audit/mod.rs +++ b/src/audit/mod.rs @@ -30,6 +30,7 @@ pub(crate) mod obfuscation; pub(crate) mod overprovisioned_secrets; pub(crate) mod ref_confusion; pub(crate) mod secrets_inherit; +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/secret_without_env.rs b/src/audit/secret_without_env.rs new file mode 100644 index 000000000..f5afadde1 --- /dev/null +++ b/src/audit/secret_without_env.rs @@ -0,0 +1,92 @@ +use github_actions_models::{ + common::{ + Env, EnvValue, + expr::{ExplicitExpr, LoE}, + }, + workflow::job::StepBody, +}; + +use super::{Audit, audit_meta}; +use crate::{AuditLoadError, AuditState, Persona, finding::Confidence, models::StepCommon}; + +pub(crate) struct SecretWithoutEnv; + +audit_meta!( + SecretWithoutEnv, + "secret-without-env", + "secret used without an environment to gate it" +); + +impl Audit for SecretWithoutEnv { + fn new(_state: &AuditState<'_>) -> Result + where + Self: Sized, + { + Ok(Self) + } + + fn audit_step<'w>( + &self, + step: &crate::models::Step<'w>, + ) -> anyhow::Result>> { + let mut findings = vec![]; + + if step.parent.environment().is_some() { + return Ok(findings); + } + + 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 { + 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)? + } + } + } + + Ok(findings) + } +} + +impl SecretWithoutEnv { + fn check_secrets_access<'w>( + s: &str, + step: &crate::models::Step<'w>, + findings: &mut Vec>, + ) -> anyhow::Result<()> { + // 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()) + .confidence(Confidence::High) + .severity(crate::finding::Severity::High) + .persona(Persona::Pedantic) + .build(step.workflow())?, + ); + } + + Ok(()) + } +} diff --git a/src/main.rs b/src/main.rs index ac7e0334a..9a5ca826e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,5 +1,5 @@ use std::{ - io::{Write, stdout}, + io::{stdout, Write}, process::ExitCode, str::FromStr, }; @@ -20,9 +20,9 @@ use indicatif::ProgressStyle; use owo_colors::OwoColorize; use registry::{AuditRegistry, FindingRegistry, InputKey, InputKind, 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; @@ -546,6 +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::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/src/models.rs b/src/models.rs index 1cf7fb2f1..2a23b46c1 100644 --- a/src/models.rs +++ b/src/models.rs @@ -9,8 +9,8 @@ use anyhow::{Context, bail}; use github_actions_models::common::Env; use github_actions_models::common::expr::LoE; 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; @@ -225,6 +225,10 @@ impl<'doc> NormalJob<'doc> { 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/snapshot.rs b/tests/integration/snapshot.rs index 5ce3c6976..63c16f208 100644 --- a/tests/integration/snapshot.rs +++ b/tests/integration/snapshot.rs @@ -647,6 +647,18 @@ fn unredacted_secrets() -> Result<()> { Ok(()) } +#[test] +fn secrets_outside_environment() -> Result<()> { + insta::assert_snapshot!( + zizmor() + .input(input_under_test("secret-without-env.yml")) + .args(["--persona=pedantic"]) + .run()? + ); + + Ok(()) +} + #[test] fn forbidden_uses() -> Result<()> { for config in ["allow-all", "deny-all", "allow-some", "deny-some"] { @@ -662,6 +674,7 @@ fn forbidden_uses() -> Result<()> { ); } + Ok(()) } 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 80e3c2a70..0086f0f33 100644 --- a/tests/integration/snapshots/integration__snapshot__overprovisioned_secrets.snap +++ b/tests/integration/snapshots/integration__snapshot__overprovisioned_secrets.snap @@ -3,17 +3,17 @@ source: tests/integration/snapshot.rs 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..5aab4fe00 --- /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(\"secret-without-env.yml\")).args([\"--persona=pedantic\"]).run()?" +--- +error[secret-without-env]: secret used without an environment to gate it + --> @@INPUT@@:10:9 + | +10 | - uses: actions_repo/actions/docker@13c8cf37e54dd9488afe8c067575444cc58bf155 + | _________^ +11 | | with: +12 | | # NOT OK: Anyone with write access can exfiltrate this secret. +13 | | 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 c87fae7ee..343799dfe 100644 --- a/tests/integration/snapshots/integration__snapshot__unredacted_secrets.snap +++ b/tests/integration/snapshots/integration__snapshot__unredacted_secrets.snap @@ -3,17 +3,17 @@ source: tests/integration/snapshot.rs 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/secret-without-env.yml b/tests/integration/test-data/secret-without-env.yml new file mode 100644 index 000000000..ff5dd5c11 --- /dev/null +++ b/tests/integration/test-data/secret-without-env.yml @@ -0,0 +1,13 @@ +on: push +permissions: {} +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. + 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: 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: