feat: artipacked now supported composite actions (#896)

This commit is contained in:
William Woodruff 2025-06-05 13:38:59 -04:00 committed by GitHub
parent 73dfa03a41
commit 2f4c874499
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
19 changed files with 218 additions and 153 deletions

View file

@ -1,20 +1,11 @@
use std::ops::Deref as _;
use anyhow::Result;
use github_actions_models::{
common::{EnvValue, Uses, expr::ExplicitExpr},
workflow::job::StepBody,
};
use github_actions_models::common::{EnvValue, Uses, expr::ExplicitExpr};
use itertools::Itertools as _;
use super::{Audit, AuditLoadError, audit_meta};
use crate::{
finding::{
Confidence, Finding, Fix, Persona, Severity,
location::{Locatable as _, Route, RouteComponent},
},
models::{JobExt, Step, uses::RepositoryUsesExt as _},
route,
finding::{Confidence, Finding, Fix, Persona, Severity, location::Routable as _},
models::{StepBodyCommon, StepCommon, uses::RepositoryUsesExt as _},
state::AuditState,
utils::split_patterns,
yaml_patch::{Op, Patch},
@ -29,74 +20,20 @@ audit_meta!(
);
impl Artipacked {
fn dangerous_artifact_patterns<'b>(&self, path: &'b str) -> Vec<&'b str> {
let mut patterns = vec![];
for path in split_patterns(path) {
match path {
// TODO: this could be even more generic.
"." | "./" | ".." | "../" => patterns.push(path),
path => match ExplicitExpr::from_curly(path) {
Some(expr) if expr.as_bare().contains("github.workspace") => {
patterns.push(path)
}
// TODO: Other expressions worth flagging here?
Some(_) => continue,
_ => continue,
},
}
}
patterns
}
/// Create a Fix for setting persist-credentials: false
fn create_persist_credentials_fix<'doc>(step: &Step<'doc>) -> Fix<'doc> {
let job_id = step.parent.id();
let checkout_index = step.index;
Fix {
title: "Set persist-credentials: false".to_string(),
description: "To prevent credential persistence, set 'persist-credentials: false' in this checkout step. \
When 'persist-credentials' is true (the default), the GITHUB_TOKEN persists in the local git config \
after checkout, which may be inadvertently leaked through subsequent actions like artifact uploads. \
Setting 'persist-credentials: false' ensures that credentials don't persist beyond the checkout step itself.".to_string(),
_key: step.location().key,
patches: vec![
Patch {
route: route!("jobs", job_id, "steps", checkout_index),
operation: Op::MergeInto {
key: "with".to_string(),
value: {
let mut with_map = serde_yaml::Mapping::new();
with_map.insert(
serde_yaml::Value::String("persist-credentials".to_string()),
serde_yaml::Value::Bool(false),
);
serde_yaml::Value::Mapping(with_map)
},
},
}
],
}
}
}
impl Audit for Artipacked {
fn new(_state: &AuditState<'_>) -> Result<Self, AuditLoadError> {
Ok(Self)
}
fn audit_normal_job<'doc>(&self, job: &super::NormalJob<'doc>) -> Result<Vec<Finding<'doc>>> {
fn process_steps<'doc>(
&self,
steps: impl Iterator<Item = impl StepCommon<'doc>>,
) -> anyhow::Result<Vec<Finding<'doc>>> {
let mut findings = vec![];
// First, collect all vulnerable checkouts and upload steps independently.
let mut vulnerable_checkouts = vec![];
let mut vulnerable_uploads = vec![];
for step in job.steps() {
let StepBody::Uses {
for step in steps {
let StepBodyCommon::Uses {
uses: Uses::Repository(uses),
with,
} = &step.deref().body
} = &step.body()
else {
continue;
};
@ -133,20 +70,20 @@ impl Audit for 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) in &vulnerable_checkouts {
findings.push(
Self::finding()
.severity(Severity::Medium)
.confidence(Confidence::Low)
.persona(persona)
.persona(*persona)
.add_location(
checkout
.location()
.primary()
.annotated("does not set persist-credentials: false"),
)
.fix(Self::create_persist_credentials_fix(&checkout))
.build(job.parent())?,
.fix(Self::create_persist_credentials_fix(checkout))
.build(checkout)?,
);
}
} else {
@ -154,15 +91,15 @@ impl Audit for Artipacked {
// 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
.into_iter()
.cartesian_product(vulnerable_uploads.into_iter())
.iter()
.cartesian_product(vulnerable_uploads.iter())
{
if checkout.index < upload.index {
if checkout.index() < upload.index() {
findings.push(
Self::finding()
.severity(Severity::High)
.confidence(Confidence::High)
.persona(persona)
.persona(*persona)
.add_location(
checkout
.location()
@ -174,8 +111,8 @@ impl Audit for Artipacked {
.location()
.annotated("may leak the credentials persisted above"),
)
.fix(Self::create_persist_credentials_fix(&checkout))
.build(job.parent())?,
.fix(Self::create_persist_credentials_fix(checkout))
.build(checkout)?,
);
}
}
@ -183,6 +120,72 @@ impl Audit for Artipacked {
Ok(findings)
}
fn dangerous_artifact_patterns<'b>(&self, path: &'b str) -> Vec<&'b str> {
let mut patterns = vec![];
for path in split_patterns(path) {
match path {
// TODO: this could be even more generic.
"." | "./" | ".." | "../" => patterns.push(path),
path => match ExplicitExpr::from_curly(path) {
Some(expr) if expr.as_bare().contains("github.workspace") => {
patterns.push(path)
}
// TODO: Other expressions worth flagging here?
Some(_) => continue,
_ => continue,
},
}
}
patterns
}
/// Create a Fix for setting persist-credentials: false
fn create_persist_credentials_fix<'doc>(step: &impl StepCommon<'doc>) -> Fix<'doc> {
Fix {
title: "Set persist-credentials: false".to_string(),
description: "To prevent credential persistence, set 'persist-credentials: false' in this checkout step. \
When 'persist-credentials' is true (the default), the GITHUB_TOKEN persists in the local git config \
after checkout, which may be inadvertently leaked through subsequent actions like artifact uploads. \
Setting 'persist-credentials: false' ensures that credentials don't persist beyond the checkout step itself.".to_string(),
_key: step.location().key,
patches: vec![
Patch {
route: step.route(),
operation: Op::MergeInto {
key: "with".to_string(),
value: {
let mut with_map = serde_yaml::Mapping::new();
with_map.insert(
serde_yaml::Value::String("persist-credentials".to_string()),
serde_yaml::Value::Bool(false),
);
serde_yaml::Value::Mapping(with_map)
},
},
}
],
}
}
}
impl Audit for Artipacked {
fn new(_state: &AuditState<'_>) -> Result<Self, AuditLoadError> {
Ok(Self)
}
fn audit_action<'doc>(
&self,
action: &'doc crate::models::Action,
) -> anyhow::Result<Vec<Finding<'doc>>> {
let steps = action.steps();
self.process_steps(steps)
}
fn audit_normal_job<'doc>(&self, job: &super::NormalJob<'doc>) -> Result<Vec<Finding<'doc>>> {
self.process_steps(job.steps())
}
}
#[cfg(test)]

View file

@ -277,7 +277,7 @@ impl CachePoisoning {
))
}
fn evaluate_cache_usage<'a, 'doc>(&self, step: &impl StepCommon<'a, 'doc>) -> Option<Usage> {
fn evaluate_cache_usage<'doc>(&self, step: &impl StepCommon<'doc>) -> Option<Usage> {
KNOWN_CACHE_AWARE_ACTIONS
.iter()
.find_map(|coord| coord.usage(step))

View file

@ -1,7 +1,6 @@
use anyhow::Result;
use super::{Audit, AuditLoadError, audit_meta};
use crate::finding::location::Locatable as _;
use crate::finding::{Confidence, Finding, Severity};
use crate::models::Workflow;
use crate::state::AuditState;

View file

@ -36,9 +36,9 @@ impl ForbiddenUses {
}
}
fn process_step<'a, 'doc>(
fn process_step<'doc>(
&self,
step: &'a impl StepCommon<'a, 'doc>,
step: &impl StepCommon<'doc>,
) -> anyhow::Result<Vec<Finding<'doc>>> {
let mut findings = vec![];

View file

@ -116,10 +116,7 @@ impl KnownVulnerableActions {
Ok(results)
}
fn process_step<'a, 'doc>(
&self,
step: &'a impl StepCommon<'a, 'doc>,
) -> Result<Vec<Finding<'doc>>> {
fn process_step<'doc>(&self, step: &impl StepCommon<'doc>) -> Result<Vec<Finding<'doc>>> {
let mut findings = vec![];
let Some(Uses::Repository(uses)) = step.uses() else {

View file

@ -7,10 +7,7 @@ use tracing::instrument;
use yamlpath::Document;
use crate::{
finding::{
Finding, FindingBuilder,
location::{Locatable, SymbolicLocation},
},
finding::{Finding, FindingBuilder, location::SymbolicLocation},
models::{
Action, AsDocument, CompositeStep, Job, NormalJob, ReusableWorkflowCallJob, Step, Workflow,
},
@ -69,10 +66,8 @@ impl AuditInput {
AuditInput::Action(action) => action.link.as_deref(),
}
}
}
impl<'a> Locatable<'a, 'a> for AuditInput {
fn location(&'a self) -> SymbolicLocation<'a> {
pub(crate) fn location(&self) -> SymbolicLocation<'_> {
match self {
AuditInput::Workflow(workflow) => workflow.location(),
AuditInput::Action(action) => action.location(),

View file

@ -5,7 +5,7 @@ use crate::{
Confidence, Severity,
finding::{
Finding,
location::{Feature, Locatable as _, Location},
location::{Feature, Location},
},
models::{CompositeStep, Step, StepCommon},
utils::parse_expressions_from_input,
@ -72,9 +72,9 @@ impl Obfuscation {
annotations
}
fn process_step<'a, 'doc>(
fn process_step<'doc>(
&self,
step: &'a impl StepCommon<'a, 'doc>,
step: &impl StepCommon<'doc>,
) -> anyhow::Result<Vec<Finding<'doc>>> {
let mut findings = vec![];

View file

@ -3,7 +3,7 @@ use github_actions_expressions::Expr;
use crate::{
finding::{
Confidence, Severity,
location::{Feature, Locatable as _, Location},
location::{Feature, Location},
},
utils::parse_expressions_from_input,
};

View file

@ -34,10 +34,7 @@ impl StaleActionRefs {
Ok(tag.is_none())
}
fn process_step<'a, 'doc>(
&self,
step: &'a impl StepCommon<'a, 'doc>,
) -> Result<Vec<Finding<'doc>>> {
fn process_step<'doc>(&self, step: &impl StepCommon<'doc>) -> Result<Vec<Finding<'doc>>> {
let mut findings = vec![];
let Some(Uses::Repository(uses)) = step.uses() else {

View file

@ -104,8 +104,8 @@ impl TemplateInjection {
.unwrap_or(&[])
}
fn scripts_with_location<'a, 'doc>(
step: &'a impl StepCommon<'a, 'doc>,
fn scripts_with_location<'doc>(
step: &impl StepCommon<'doc>,
) -> Vec<(String, SymbolicLocation<'doc>)> {
match step.body() {
models::StepBodyCommon::Uses {
@ -196,11 +196,11 @@ impl TemplateInjection {
}
/// Attempts to produce a `Fix` for a given expression.
fn attempt_fix<'a, 'doc>(
fn attempt_fix<'doc>(
&self,
raw: &ExplicitExpr,
parsed: &Expr,
step: &'a impl StepCommon<'a, 'doc>,
step: &impl StepCommon<'doc>,
) -> Option<Fix<'doc>> {
// We can only fix `run:` steps for now.
if !matches!(step.body(), models::StepBodyCommon::Run { .. }) {
@ -260,10 +260,10 @@ impl TemplateInjection {
})
}
fn injectable_template_expressions<'a, 'doc>(
fn injectable_template_expressions<'doc>(
&self,
script: &str,
step: &'a impl StepCommon<'a, 'doc>,
step: &impl StepCommon<'doc>,
) -> Vec<(String, Option<Fix<'doc>>, Severity, Confidence, Persona)> {
let mut bad_expressions = vec![];
for (expr, _) in extract_expressions(script) {
@ -412,9 +412,9 @@ impl TemplateInjection {
bad_expressions
}
fn process_step<'a, 'doc>(
fn process_step<'doc>(
&self,
step: &'a impl StepCommon<'a, 'doc>,
step: &impl StepCommon<'doc>,
) -> anyhow::Result<Vec<Finding<'doc>>> {
let mut findings = vec![];

View file

@ -87,9 +87,9 @@ impl UnpinnedUses {
}
}
fn process_step<'a, 'doc>(
fn process_step<'doc>(
&self,
step: &'a impl StepCommon<'a, 'doc>,
step: &impl StepCommon<'doc>,
) -> anyhow::Result<Vec<Finding<'doc>>> {
let mut findings = vec![];

View file

@ -2,7 +2,7 @@ use github_actions_expressions::{Expr, context::Context};
use crate::{
Confidence, Severity,
finding::location::{Feature, Locatable as _, Location},
finding::location::{Feature, Location},
utils::parse_expressions_from_input,
};

View file

@ -214,9 +214,9 @@ impl<'doc> SymbolicLocation<'doc> {
}
/// Gives models (e.g. workflow steps) the ability to express their symbolic location.
pub(crate) trait Locatable<'a, 'doc> {
pub(crate) trait Locatable<'doc> {
/// Returns the symbolic location of this model.
fn location(&'a self) -> SymbolicLocation<'doc>;
fn location(&self) -> SymbolicLocation<'doc>;
/// Returns an "enriched" symbolic location of this model,
/// when the model is of a type that has a name. Otherwise,
@ -224,7 +224,7 @@ pub(crate) trait Locatable<'a, 'doc> {
///
/// For example, a GitHub Actions workflow step has an optional name,
/// which is included in this symbolic location if present.
fn location_with_name(&'a self) -> SymbolicLocation<'doc> {
fn location_with_name(&self) -> SymbolicLocation<'doc> {
self.location()
}
}
@ -233,7 +233,7 @@ pub(crate) trait Routable<'a, 'doc> {
fn route(&'a self) -> Route<'doc>;
}
impl<'a, 'doc, T: Locatable<'a, 'doc>> Routable<'a, 'doc> for T {
impl<'a, 'doc, T: Locatable<'doc>> Routable<'a, 'doc> for T {
fn route(&'a self) -> Route<'doc> {
self.location().route
}

View file

@ -44,7 +44,10 @@ pub(crate) enum StepBodyCommon<'s> {
}
/// Common interfaces between workflow and action steps.
pub(crate) trait StepCommon<'a, 'doc>: Locatable<'a, 'doc> {
pub(crate) trait StepCommon<'doc>: Locatable<'doc> {
/// Returns the step's index within its parent job or action.
fn index(&self) -> usize;
/// Returns whether the given `env.name` environment access is "static,"
/// i.e. is not influenced by another expression.
fn env_is_static(&self, name: &str) -> bool;
@ -64,7 +67,7 @@ pub(crate) trait StepCommon<'a, 'doc>: Locatable<'a, 'doc> {
fn document(&self) -> &'doc yamlpath::Document;
}
impl<'a, 'doc, T: StepCommon<'a, 'doc>> AsDocument<'a, 'doc> for T {
impl<'a, 'doc, T: StepCommon<'doc>> AsDocument<'a, 'doc> for T {
fn as_document(&'a self) -> &'doc yamlpath::Document {
self.document()
}
@ -171,11 +174,15 @@ impl Workflow {
Trigger::Events(events) => events.count() == 1,
}
}
}
impl<'a> Locatable<'a, 'a> for Workflow {
/// This workflow's [`SymbolicLocation`].
fn location(&'a self) -> SymbolicLocation<'a> {
/// Returns this workflow's [`SymbolicLocation`].
///
/// NOTE: This is intentionally implemented directly on the `Workflow` type
/// rather than through the [`Locatable`] trait, since introducing
/// this through [`Locatable`] would require a split lifetime between
/// `'self` and `'doc` for just this and [`Action`], i.e. the owning
/// container types rather than the borrowing subtypes.
pub fn location(&self) -> SymbolicLocation<'_> {
SymbolicLocation {
key: &self.key,
annotation: "this workflow".to_string(),
@ -195,9 +202,9 @@ pub(crate) trait JobExt<'doc> {
fn parent(&self) -> &'doc Workflow;
}
impl<'a, 'doc, T: JobExt<'doc>> Locatable<'a, 'doc> for T {
impl<'doc, T: JobExt<'doc>> Locatable<'doc> for T {
/// Returns this job's [`SymbolicLocation`].
fn location(&'a self) -> SymbolicLocation<'doc> {
fn location(&self) -> SymbolicLocation<'doc> {
self.parent()
.location()
.annotated("this job")
@ -542,16 +549,16 @@ impl<'doc> Deref for Step<'doc> {
}
}
impl<'a, 'doc> Locatable<'a, 'doc> for Step<'doc> {
impl<'doc> Locatable<'doc> for Step<'doc> {
/// This step's [`SymbolicLocation`].
fn location(&'a self) -> SymbolicLocation<'doc> {
fn location(&self) -> SymbolicLocation<'doc> {
self.parent
.location()
.with_keys(&["steps".into(), self.index.into()])
.annotated("this step")
}
fn location_with_name(&'a self) -> SymbolicLocation<'doc> {
fn location_with_name(&self) -> SymbolicLocation<'doc> {
match self.inner.name {
Some(_) => self.location().with_keys(&["name".into()]),
None => self.location(),
@ -560,7 +567,11 @@ impl<'a, 'doc> Locatable<'a, 'doc> for Step<'doc> {
}
}
impl<'doc> StepCommon<'_, 'doc> for Step<'doc> {
impl<'doc> StepCommon<'doc> for Step<'doc> {
fn index(&self) -> usize {
self.index
}
fn env_is_static(&self, name: &str) -> bool {
utils::env_is_static(name, &[&self.env, &self.job().env, &self.workflow().env])
}
@ -699,19 +710,6 @@ impl<'a> AsDocument<'a, 'a> for Action {
}
}
impl<'a> Locatable<'a, 'a> for Action {
/// This actions's [`SymbolicLocation`].
fn location(&'a self) -> SymbolicLocation<'a> {
SymbolicLocation {
key: &self.key,
annotation: "this action".to_string(),
link: None,
route: Route::new(),
kind: Default::default(),
}
}
}
impl Deref for Action {
type Target = action::Action;
@ -757,6 +755,20 @@ impl Action {
pub(crate) fn steps(&self) -> CompositeSteps<'_> {
CompositeSteps::new(self)
}
/// Returns this action's [`SymbolicLocation`].
///
/// See [`Workflow::location`] for an explanation of why this isn't
/// implemented through the [`Locatable`] trait.
pub(crate) fn location(&self) -> SymbolicLocation<'_> {
SymbolicLocation {
key: &self.key,
annotation: "this action".to_string(),
link: None,
route: Route::new(),
kind: Default::default(),
}
}
}
/// An iterable container for steps within a [`Job`].
@ -810,14 +822,14 @@ impl<'a> Deref for CompositeStep<'a> {
}
}
impl<'a, 'doc> Locatable<'a, 'doc> for CompositeStep<'doc> {
fn location(&'a self) -> SymbolicLocation<'doc> {
impl<'doc> Locatable<'doc> for CompositeStep<'doc> {
fn location(&self) -> SymbolicLocation<'doc> {
self.parent
.location()
.with_keys(&["runs".into(), "steps".into(), self.index.into()])
}
fn location_with_name(&'a self) -> SymbolicLocation<'doc> {
fn location_with_name(&self) -> SymbolicLocation<'doc> {
match self.inner.name {
Some(_) => self.location().with_keys(&["name".into()]),
None => self.location(),
@ -826,7 +838,11 @@ impl<'a, 'doc> Locatable<'a, 'doc> for CompositeStep<'doc> {
}
}
impl<'doc> StepCommon<'_, 'doc> for CompositeStep<'doc> {
impl<'doc> StepCommon<'doc> for CompositeStep<'doc> {
fn index(&self) -> usize {
self.index
}
fn env_is_static(&self, name: &str) -> bool {
utils::env_is_static(name, &[&self.env])
}

View file

@ -44,7 +44,7 @@ impl ActionCoordinate {
/// while the `Some(_)` variants indicate various (potential) usages (such as being implicitly
/// enabled, or explicitly enabled, or potentially enabled by a template expansion that
/// can't be directly analyzed).
pub(crate) fn usage<'a, 'doc>(&self, step: &impl StepCommon<'a, 'doc>) -> Option<Usage> {
pub(crate) fn usage<'doc>(&self, step: &impl StepCommon<'doc>) -> Option<Usage> {
let uses_pattern = self.uses_pattern();
let StepBodyCommon::Uses {
@ -336,7 +336,7 @@ mod tests {
};
// Test-only trait impls.
impl<'a, 'doc> Locatable<'a, 'doc> for Step {
impl<'doc> Locatable<'doc> for Step {
fn location(&self) -> crate::models::SymbolicLocation<'doc> {
unreachable!()
}
@ -346,7 +346,11 @@ mod tests {
}
}
impl<'a, 'doc> StepCommon<'a, 'doc> for Step {
impl<'doc> StepCommon<'doc> for Step {
fn index(&self) -> usize {
unreachable!()
}
fn env_is_static(&self, _name: &str) -> bool {
unreachable!()
}

View file

@ -58,6 +58,13 @@ fn artipacked() -> Result<()> {
.run()?
);
insta::assert_snapshot!(
zizmor()
.input(input_under_test("artipacked/demo-action/action.yml"))
.args(["--persona=auditor"])
.run()?
);
Ok(())
}

View file

@ -0,0 +1,27 @@
---
source: crates/zizmor/tests/integration/snapshot.rs
expression: "zizmor().input(input_under_test(\"artipacked/demo-action/action.yml\")).args([\"--persona=auditor\"]).run()?"
---
warning[artipacked]: credential persistence through GitHub Actions artifacts
--> @@INPUT@@:9:7
|
9 | - name: true-positive-1
| _______-
10 | | uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # tag=v4.2.2
| |__________________________________________________________________________________- does not set persist-credentials: false
|
= note: audit confidence → Low
warning[artipacked]: credential persistence through GitHub Actions artifacts
--> @@INPUT@@:12:7
|
12 | - name: true-positive-2-pedantic
| _______-
13 | | uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # tag=v4.2.2
14 | | with:
15 | | persist-credentials: true
| |__________________________________- does not set persist-credentials: false
|
= note: audit confidence → Low
2 findings: 0 unknown, 0 informational, 0 low, 2 medium, 0 high

View file

@ -0,0 +1,15 @@
# demo of a composite action being flagged by artipacked
name: artipacked-composite-action
description: artipacked-composite-action
runs:
using: composite
steps:
- name: true-positive-1
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # tag=v4.2.2
- name: true-positive-2-pedantic
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # tag=v4.2.2
with:
persist-credentials: true

View file

@ -9,6 +9,11 @@ of `zizmor`.
## Next (UNRELEASED)
### Enhancements 🌱
* The [artipacked] audit now produces findings on composite action definitions,
rather than just workflow definitions (#896)
### Bug Fixes 🐛
* The [template-injection] audit no longer crashes when attempting to