Repository: woodruffw/zizmor
Pull request #338 (2024-12-20 07:40:07)

feat: detects well-known publisher actions in cache-audit
A first follow-up from #294. also contributing to #334 

We now have a list of well-know publisher Actions (2 of them so far 😄) and the audit inspects all the steps at once alongside with the previous trigger evaluation to determine whether the Workflow/Job actually builds artifacts for publishing.

I also introduced an `PublishingArtifactsScenario` enum to easilty match the situation we are dealing with and signal properly the non-primary locations in `FindingBuilders`. 


Patch:
diff --git a/src/audit/cache_poisoning.rs b/src/audit/cache_poisoning.rs
index 1eb92b1..024dc5c 100644
--- a/src/audit/cache_poisoning.rs
+++ b/src/audit/cache_poisoning.rs
@@ -1,6 +1,6 @@
 use crate::audit::{audit_meta, WorkflowAudit};
 use crate::finding::{Confidence, Finding, Severity};
-use crate::models::{Step, Uses};
+use crate::models::{Job, Step, Steps, Uses};
 use crate::state::AuditState;
 use github_actions_models::common::expr::ExplicitExpr;
 use github_actions_models::common::Env;
@@ -110,6 +110,18 @@ static KNOWN_CACHE_AWARE_ACTIONS: LazyLock<Vec<CacheAwareAction>> = LazyLock::ne
     ]
 });
 
+/// A list of well-know publisher actions
+/// In the future we can retrieve this list from the static API
+static KNOWN_PUBLISHER_ACTIONS: LazyLock<Vec<Uses>> = LazyLock::new(|| {
+    vec![
+        Uses::from_step("pypa/gh-action-pypi-publish").unwrap(),
+        Uses::from_step("softprops/action-gh-release").unwrap(),
+        Uses::from_step("rubygems/release-gem").unwrap(),
+        Uses::from_step("goreleaser/goreleaser-action").unwrap(),
+        Uses::from_step("jreleaser/release-action").unwrap(),
+    ]
+});
+
 #[derive(PartialEq)]
 enum CacheUsage {
     ConditionalOptIn,
@@ -117,6 +129,11 @@ enum CacheUsage {
     DefaultActionBehaviour,
 }
 
+enum PublishingArtifactsScenario<'w> {
+    UsingTypicalWorkflowTrigger,
+    UsingWellKnowPublisherAction(Step<'w>),
+}
+
 pub(crate) struct CachePoisoning;
 
 audit_meta!(
@@ -137,6 +154,38 @@ impl CachePoisoning {
         }
     }
 
+    fn detected_well_known_publisher_step(steps: Steps) -> Option<Step> {
+        steps.into_iter().find(|step| {
+            let Some(Uses::Repository(target_uses)) = step.uses() else {
+                return false;
+            };
+
+            KNOWN_PUBLISHER_ACTIONS.iter().any(|publisher| {
+                let Uses::Repository(well_known_uses) = publisher else {
+                    return false;
+                };
+
+                target_uses.matches(*well_known_uses)
+            })
+        })
+    }
+
+    fn is_job_publishing_artifacts<'w>(
+        &self,
+        trigger: &Trigger,
+        steps: Steps<'w>,
+    ) -> Option<PublishingArtifactsScenario<'w>> {
+        if self.trigger_used_when_publishing_artifacts(trigger) {
+            return Some(PublishingArtifactsScenario::UsingTypicalWorkflowTrigger);
+        };
+
+        let well_know_publisher = CachePoisoning::detected_well_known_publisher_step(steps)?;
+
+        Some(PublishingArtifactsScenario::UsingWellKnowPublisherAction(
+            well_know_publisher,
+        ))
+    }
+
     fn evaluate_default_action_behaviour(action: &CacheAwareAction) -> Option<CacheUsage> {
         if action.caching_by_default {
             Some(CacheUsage::DefaultActionBehaviour)
@@ -218,32 +267,17 @@ impl CachePoisoning {
             }
         }
     }
-}
-
-impl WorkflowAudit for CachePoisoning {
-    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 trigger = &step.workflow().on;
-
-        if !self.trigger_used_when_publishing_artifacts(trigger) {
-            return Ok(findings);
-        }
 
+    fn uses_cache_aware_step<'w>(
+        &self,
+        step: &Step<'w>,
+        scenario: &PublishingArtifactsScenario<'w>,
+    ) -> Option<Finding<'w>> {
         let StepBody::Uses { ref uses, ref with } = &step.deref().body else {
-            return Ok(findings);
+            return None;
         };
 
-        let Some(cache_usage) = self.evaluate_cache_usage(uses, with) else {
-            return Ok(findings);
-        };
+        let cache_usage = self.evaluate_cache_usage(uses, with)?;
 
         let (yaml_key, annotation) = match cache_usage {
             CacheUsage::DefaultActionBehaviour => ("uses", "cache enabled by default here"),
@@ -251,8 +285,8 @@ impl WorkflowAudit for CachePoisoning {
             CacheUsage::ConditionalOptIn => ("with", "opt-in for caching might happen here"),
         };
 
-        findings.push(
-            Self::finding()
+        let finding = match scenario {
+            PublishingArtifactsScenario::UsingTypicalWorkflowTrigger => Self::finding()
                 .confidence(Confidence::Low)
                 .severity(Severity::High)
                 .add_location(
@@ -267,8 +301,51 @@ impl WorkflowAudit for CachePoisoning {
                         .with_keys(&[yaml_key.into()])
                         .annotated(annotation),
                 )
-                .build(step.workflow())?,
-        );
+                .build(step.workflow()),
+            PublishingArtifactsScenario::UsingWellKnowPublisherAction(publisher) => Self::finding()
+                .confidence(Confidence::Low)
+                .severity(Severity::High)
+                .add_location(
+                    publisher
+                        .location()
+                        .with_keys(&["uses".into()])
+                        .annotated("runtime artifacts usually published here"),
+                )
+                .add_location(
+                    step.location()
+                        .primary()
+                        .with_keys(&[yaml_key.into()])
+                        .annotated(annotation),
+                )
+                .build(step.workflow()),
+        };
+
+        finding.ok()
+    }
+}
+
+impl WorkflowAudit for CachePoisoning {
+    fn new(_: AuditState) -> anyhow::Result<Self>
+    where
+        Self: Sized,
+    {
+        Ok(Self)
+    }
+
+    fn audit_normal_job<'w>(&self, job: &Job<'w>) -> anyhow::Result<Vec<Finding<'w>>> {
+        let mut findings = vec![];
+        let steps = job.steps();
+        let trigger = &job.parent().on;
+
+        let Some(scenario) = self.is_job_publishing_artifacts(trigger, steps) else {
+            return Ok(findings);
+        };
+
+        for step in job.steps() {
+            if let Some(finding) = self.uses_cache_aware_step(&step, &scenario) {
+                findings.push(finding);
+            }
+        }
 
         Ok(findings)
     }


Test patch:
diff --git a/tests/snapshot.rs b/tests/snapshot.rs
index bcf649e..d1c6b05 100644
--- a/tests/snapshot.rs
+++ b/tests/snapshot.rs
@@ -314,6 +314,10 @@ fn cache_poisoning() -> Result<()> {
         ))
         .run()?);
 
+    insta::assert_snapshot!(zizmor()
+        .workflow(workflow_under_test("cache-poisoning/publisher-step.yml"))
+        .run()?);
+
     Ok(())
 }
 
diff --git a/tests/snapshots/snapshot__cache_poisoning-10.snap b/tests/snapshots/snapshot__cache_poisoning-10.snap
new file mode 100644
index 0000000..dd8711a
--- /dev/null
+++ b/tests/snapshots/snapshot__cache_poisoning-10.snap
@@ -0,0 +1,19 @@
+---
+source: tests/snapshot.rs
+expression: "zizmor().workflow(workflow_under_test(\"cache-poisoning/publisher-step.yml\")).run()?"
+snapshot_kind: text
+---
+error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache poisoning attack
+  --> @@INPUT@@:23:9
+   |
+16 |         uses: Swatinem/rust-cache@82a92a6e8fbeee089604da2575dc567ae9ddeaab
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cache enabled by default here
+17 |
+...
+22 |       - name: Publish draft release on Github
+23 |         uses: softprops/action-gh-release@01570a1f39cb168c169c802c3bceb9e93fb10974
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ runtime artifacts usually published here
+   |
+   = note: audit confidence → Low
+
+1 finding: 0 unknown, 0 informational, 0 low, 0 medium, 1 high
diff --git a/tests/test-data/cache-poisoning/publisher-step.yml b/tests/test-data/cache-poisoning/publisher-step.yml
new file mode 100644
index 0000000..9a9d9f4
--- /dev/null
+++ b/tests/test-data/cache-poisoning/publisher-step.yml
@@ -0,0 +1,32 @@
+on:
+  push:
+    branches:
+      - main
+
+jobs:
+  publish:
+    runs-on: macos-latest
+    steps:
+      - name: Project Checkout
+        uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683
+        with:
+          persist-credentials: false
+
+      - name: Setup CI caching
+        uses: Swatinem/rust-cache@82a92a6e8fbeee089604da2575dc567ae9ddeaab
+
+      - name: Build snapshot version of artifacts
+        id: build
+        run: cargo xtasks darwin-build
+
+      - name: Publish draft release on Github
+        uses: softprops/action-gh-release@01570a1f39cb168c169c802c3bceb9e93fb10974
+        with:
+          name: ${{ steps.build.outputs.version }}
+          tag_name: ${{ steps.build.outputs.version }}
+          token: ${{ secrets.GITHUB_RELEASES_TOKEN }}
+          generate_release_notes: false
+          draft: false
+          files: |
+            target/aarch64-apple-darwin/release/my-app
+            target/x86_64-apple-darwin/release/my-app