diff --git a/crates/zizmor/src/audit/artipacked.rs b/crates/zizmor/src/audit/artipacked.rs index a831f1cf9..7513400de 100644 --- a/crates/zizmor/src/audit/artipacked.rs +++ b/crates/zizmor/src/audit/artipacked.rs @@ -1,3 +1,5 @@ +use std::sync::LazyLock; + use github_actions_models::common::{EnvValue, Uses, expr::ExplicitExpr}; use itertools::Itertools as _; @@ -5,13 +7,19 @@ use super::{Audit, AuditLoadError, audit_meta}; use crate::{ audit::AuditError, finding::{Confidence, Finding, Fix, Persona, Severity, location::Routable as _}, - models::{StepBodyCommon, StepCommon, uses::RepositoryUsesExt as _}, + github::{Client, ClientError}, + models::{StepBodyCommon, StepCommon, uses::RepositoryUsesExt, version::Version}, state::AuditState, utils::split_patterns, }; use yamlpatch::{Op, Patch}; -pub(crate) struct Artipacked; +#[allow(clippy::unwrap_used)] +static V6: LazyLock = LazyLock::new(|| Version::parse("v6").unwrap()); + +pub(crate) struct Artipacked { + client: Option, +} audit_meta!( Artipacked, @@ -20,7 +28,68 @@ audit_meta!( ); impl Artipacked { - fn process_steps<'doc>( + /// Determines if an `actions/checkout` usage is version 6 or higher. + /// + /// This takes two different paths: + /// 1. If the ref is a symbolic ref (tag/branch), we use it for a direct version comparison. + /// 2. If the ref is a commit SHA *and* we have a GitHub client, we match the commit + /// to its longest tag. We then use that tag for the version comparison. + /// + /// If we can't determine the version (e.g., commit SHA without client), + /// we return `None`. + async fn is_checkout_v6_or_higher( + &self, + uses: &github_actions_models::common::RepositoryUses, + ) -> Result, ClientError> { + let version = if !uses.ref_is_commit() { + uses.git_ref.clone() + } else { + match self.client { + Some(ref client) => { + let tag = client + .longest_tag_for_commit(&uses.owner, &uses.repo, &uses.git_ref) + .await?; + + match tag { + Some(tag) => tag.name, + None => return Ok(None), + } + } + None => return Ok(None), + } + }; + + // Try to parse the ref as a version + let Ok(version) = Version::parse(&version) else { + // If we can't parse it as a version, assume it's not v6+ + return Ok(None); + }; + + Ok(Some(version >= *V6)) + } + + /// Determine the severity for an artipacked finding based on checkout version + /// and whether there are vulnerable uploads. + fn determine_severity( + is_v6_or_higher: Option, + has_no_vulnerable_uploads: bool, + ) -> Severity { + if is_v6_or_higher == Some(true) { + // For checkout@v6+, downgrade severity since credentials are stored + // in $RUNNER_TEMP instead of .git/config, reducing leakage risk. + if has_no_vulnerable_uploads { + Severity::Low + } else { + Severity::Medium + } + } else if has_no_vulnerable_uploads { + Severity::Medium + } else { + Severity::High + } + } + + async fn process_steps<'doc>( &self, steps: impl Iterator>, ) -> Result>, AuditError> { @@ -39,6 +108,10 @@ impl Artipacked { }; if uses.matches("actions/checkout") { + let is_v6_or_higher = self + .is_checkout_v6_or_higher(uses) + .await + .map_err(Self::err)?; match with .get("persist-credentials") .map(|v| v.to_string()) @@ -48,11 +121,11 @@ impl Artipacked { Some("true") => { // If a user explicitly sets `persist-credentials: true`, // they probably mean it. Only report if in auditor mode. - vulnerable_checkouts.push((step, Persona::Auditor)) + vulnerable_checkouts.push((step, Persona::Auditor, is_v6_or_higher)) } // TODO: handle expressions here. // persist-credentials is true by default. - _ => vulnerable_checkouts.push((step, Persona::default())), + _ => vulnerable_checkouts.push((step, Persona::default(), is_v6_or_higher)), } } else if uses.matches("actions/upload-artifact") { let Some(EnvValue::String(path)) = with.get("path") else { @@ -70,10 +143,13 @@ impl Artipacked { if vulnerable_uploads.is_empty() { // If we have no vulnerable uploads, then emit lower-confidence // findings for just the checkout steps. - for (checkout, persona) in &vulnerable_checkouts { + for (checkout, persona, is_v6_or_higher) in &vulnerable_checkouts { + let severity = + Self::determine_severity(*is_v6_or_higher, vulnerable_uploads.is_empty()); + findings.push( Self::finding() - .severity(Severity::Medium) + .severity(severity) .confidence(Confidence::Low) .persona(*persona) .add_location( @@ -90,14 +166,17 @@ impl Artipacked { // Select only pairs where the vulnerable checkout precedes the // vulnerable upload. There are more efficient ways to do this than // a cartesian product, but this way is simple. - for ((checkout, persona), upload) in vulnerable_checkouts + for ((checkout, persona, is_v6_or_higher), upload) in vulnerable_checkouts .iter() .cartesian_product(vulnerable_uploads.iter()) { if checkout.index() < upload.index() { + let severity = + Self::determine_severity(*is_v6_or_higher, vulnerable_uploads.is_empty()); + findings.push( Self::finding() - .severity(Severity::High) + .severity(severity) .confidence(Confidence::High) .persona(*persona) .add_location( @@ -163,8 +242,10 @@ impl Artipacked { #[async_trait::async_trait] impl Audit for Artipacked { - fn new(_state: &AuditState) -> Result { - Ok(Self) + fn new(state: &AuditState) -> Result { + Ok(Self { + client: state.gh_client.clone(), + }) } async fn audit_action<'doc>( @@ -176,7 +257,7 @@ impl Audit for Artipacked { return Ok(vec![]); }; - self.process_steps(steps) + self.process_steps(steps).await } async fn audit_normal_job<'doc>( @@ -184,12 +265,16 @@ impl Audit for Artipacked { job: &super::NormalJob<'doc>, _config: &crate::config::Config, ) -> Result>, AuditError> { - self.process_steps(job.steps()) + self.process_steps(job.steps()).await } } #[cfg(test)] mod tests { + use std::str::FromStr; + + use github_actions_models::common::RepositoryUses; + use super::*; use crate::{ config::Config, @@ -237,6 +322,164 @@ mod tests { fix.apply(document).unwrap() } + #[tokio::test] + async fn test_is_checkout_v6_or_higher_offline() { + // Test v6 and higher versions + let v6 = RepositoryUses::from_str("actions/checkout@v6").unwrap(); + let v6_0 = RepositoryUses::from_str("actions/checkout@v6.0").unwrap(); + let v6_1_0 = RepositoryUses::from_str("actions/checkout@v6.1.0").unwrap(); + let v7 = RepositoryUses::from_str("actions/checkout@v7").unwrap(); + let v10 = RepositoryUses::from_str("actions/checkout@v10").unwrap(); + + let artipacked = Artipacked { client: None }; + + assert_eq!( + artipacked.is_checkout_v6_or_higher(&v6).await.unwrap(), + Some(true) + ); + assert_eq!( + artipacked.is_checkout_v6_or_higher(&v6_0).await.unwrap(), + Some(true) + ); + assert_eq!( + artipacked.is_checkout_v6_or_higher(&v6_1_0).await.unwrap(), + Some(true) + ); + assert_eq!( + artipacked.is_checkout_v6_or_higher(&v7).await.unwrap(), + Some(true) + ); + assert_eq!( + artipacked.is_checkout_v6_or_higher(&v10).await.unwrap(), + Some(true) + ); + + // Test versions below v6 + let v4 = RepositoryUses::from_str("actions/checkout@v4").unwrap(); + let v5 = RepositoryUses::from_str("actions/checkout@v5").unwrap(); + let v5_9 = RepositoryUses::from_str("actions/checkout@v5.9").unwrap(); + + assert_eq!( + artipacked.is_checkout_v6_or_higher(&v4).await.unwrap(), + Some(false) + ); + assert_eq!( + artipacked.is_checkout_v6_or_higher(&v5).await.unwrap(), + Some(false) + ); + assert_eq!( + artipacked.is_checkout_v6_or_higher(&v5_9).await.unwrap(), + Some(false) + ); + + // Test commit SHA (should return None when offline) + let commit_sha = + RepositoryUses::from_str("actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683") + .unwrap(); + assert_eq!( + artipacked + .is_checkout_v6_or_higher(&commit_sha) + .await + .unwrap(), + None + ); + + // Test invalid/unparseable refs (should return None) + let invalid = RepositoryUses::from_str("actions/checkout@main").unwrap(); + assert_eq!( + artipacked.is_checkout_v6_or_higher(&invalid).await.unwrap(), + None + ); + } + + #[cfg(feature = "online-tests")] + #[tokio::test] + async fn test_is_checkout_v6_or_higher_online() { + use crate::github; + + let artipacked = Artipacked { + client: Some( + Client::new( + &github::GitHubHost::default(), + &github::GitHubToken::new(&std::env::var("GH_TOKEN").unwrap()).unwrap(), + "/tmp".into(), + ) + .unwrap(), + ), + }; + + // Points to v6.0.0. + let commit_sha_v6 = + RepositoryUses::from_str("actions/checkout@1af3b93b6815bc44a9784bd300feb67ff0d1eeb3") + .unwrap(); + + assert_eq!( + artipacked + .is_checkout_v6_or_higher(&commit_sha_v6) + .await + .unwrap(), + Some(true) + ); + + // Points to v5.0.1. + let commit_sha_v5 = + RepositoryUses::from_str("actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd") + .unwrap(); + + assert_eq!( + artipacked + .is_checkout_v6_or_higher(&commit_sha_v5) + .await + .unwrap(), + Some(false) + ); + } + + #[test] + fn test_determine_severity() { + const IS_V6_OR_HIGHER: Option = Some(true); + const IS_OLDER_VERSION: Option = Some(false); + const UNKNOWN_VERSION: Option = None; + const HAS_NO_VULNERABLE_UPLOADS: bool = true; + const HAS_VULNERABLE_UPLOADS: bool = false; + + // checkout@v6+ with no vulnerable uploads -> Low + assert_eq!( + Artipacked::determine_severity(IS_V6_OR_HIGHER, HAS_NO_VULNERABLE_UPLOADS), + Severity::Low + ); + + // checkout@v6+ with vulnerable uploads -> Medium + assert_eq!( + Artipacked::determine_severity(IS_V6_OR_HIGHER, HAS_VULNERABLE_UPLOADS), + Severity::Medium + ); + + // Older checkout versions with no vulnerable uploads -> Medium + assert_eq!( + Artipacked::determine_severity(IS_OLDER_VERSION, HAS_NO_VULNERABLE_UPLOADS), + Severity::Medium + ); + + // Older checkout versions with vulnerable uploads -> High + assert_eq!( + Artipacked::determine_severity(IS_OLDER_VERSION, HAS_VULNERABLE_UPLOADS), + Severity::High + ); + + // Unknown version (None) with no vulnerable uploads -> Medium (treated as older) + assert_eq!( + Artipacked::determine_severity(UNKNOWN_VERSION, HAS_NO_VULNERABLE_UPLOADS), + Severity::Medium + ); + + // Unknown version (None) with vulnerable uploads -> High (treated as older) + assert_eq!( + Artipacked::determine_severity(UNKNOWN_VERSION, HAS_VULNERABLE_UPLOADS), + Severity::High + ); + } + #[test] fn test_fix_title_and_description() { // Test that the fix has the expected title and description format diff --git a/docs/audits.md b/docs/audits.md index f955a3552..1ebf888b3 100644 --- a/docs/audits.md +++ b/docs/audits.md @@ -71,15 +71,24 @@ Add a `name:` field to your workflow or action. Detects local filesystem `git` credential storage on GitHub Actions, as well as potential avenues for unintentional persistence of credentials in artifacts. -By default, using @actions/checkout causes a credential to be persisted -in the checked-out repo's `.git/config`, so that subsequent `git` operations -can be authenticated. +By default, using @actions/checkout causes a credential to be persisted on disk. +Versions below v6.0.0 store the credential directly in the checked-out repo's +`.git/config`, while v6.0.0 and later store it under `$RUNNER_TEMP`. -Subsequent steps may accidentally publicly persist `.git/config`, e.g. by +Subsequent steps may accidentally publicly persist the credential, e.g. by including it in a publicly accessible artifact via @actions/upload-artifact. -However, even without this, persisting the credential in the `.git/config` -is non-ideal unless actually needed. +However, even without this, persisting the credential on disk is non-ideal +unless actually needed. + +!!! note "Behavior change" + + Starting with zizmor v1.17.0, this audit produces lower-severity findings + when v6.0.0 or higher of @actions/checkout is used. This reflects a + change in v6.0.0's credential persistence behavior towards a more + misuse-resistant location. + + See orgs/community?179107 for additional information. Other resources: diff --git a/docs/release-notes.md b/docs/release-notes.md index f9332a2b9..64cc2daa0 100644 --- a/docs/release-notes.md +++ b/docs/release-notes.md @@ -13,6 +13,11 @@ of `zizmor`. * `zizmor` now produces a more useful error message when asked to collect only workflows from a remote input that contains no workflows (#1324) + +* `zizmor` now produces more precise severities on @actions/checkout versions + that have more misuse-resistant credentials persistence behavior (#1353) + + Many thanks to @ManuelLerchnerQC for proposing and implementing this improvement! ### Performance Improvements 🚄