diff --git a/crates/zizmor/src/audit/obfuscation.rs b/crates/zizmor/src/audit/obfuscation.rs index c2cd766e..bd82fb73 100644 --- a/crates/zizmor/src/audit/obfuscation.rs +++ b/crates/zizmor/src/audit/obfuscation.rs @@ -11,7 +11,7 @@ use crate::{ location::{Feature, Location, Routable}, }, models::{StepCommon, action::CompositeStep, workflow::Step}, - utils::parse_fenced_expressions_from_input, + utils::{self, parse_fenced_expressions_from_input}, }; use subfeature::Subfeature; @@ -190,30 +190,58 @@ impl Obfuscation { ) -> Result>, AuditError> { let mut findings = vec![]; - if let Some(Uses::Repository(uses)) = step.uses() { - let obfuscated_annotations = self.obfuscated_repo_uses(uses); - if !obfuscated_annotations.is_empty() { - let mut finding_builder = Self::finding() - .confidence(Confidence::High) - .severity(Severity::Low); + match step.body() { + crate::models::StepBodyCommon::Uses { + uses: Uses::Repository(uses), + .. + } => { + let obfuscated_annotations = self.obfuscated_repo_uses(uses); + if !obfuscated_annotations.is_empty() { + let mut finding_builder = Self::finding() + .confidence(Confidence::High) + .severity(Severity::Low); - // Add all annotations as locations - for annotation in &obfuscated_annotations { - finding_builder = finding_builder.add_location( - step.location() - .primary() - .with_keys(["uses".into()]) - .annotated(*annotation), + // Add all annotations as locations + for annotation in &obfuscated_annotations { + finding_builder = finding_builder.add_location( + step.location() + .primary() + .with_keys(["uses".into()]) + .annotated(*annotation), + ); + } + + // Try to create a fix for the obfuscated uses path + if let Some(fix) = self.create_uses_fix(uses, step) { + finding_builder = finding_builder.fix(fix); + } + + findings.push(finding_builder.build(step).map_err(Self::err)?); + } + } + crate::models::StepBodyCommon::Run { .. } => { + if let Some("cmd" | "cmd.exe") = step.shell().map(utils::normalize_shell) { + // `shell: cmd` is basically impossible to analyze: it has no formal + // grammar and has several line continuation mechanisms that stymie + // naive matching. It also hasn't been the default shell on Windows + // runners since 2019. + findings.push( + Self::finding() + .confidence(Confidence::High) + .severity(Severity::Low) + .add_location( + step.location() + .primary() + .with_keys(["shell".into()]) + .annotated("Windows CMD shell limits analysis"), + ) + .tip("use 'shell: pwsh' or 'shell: bash' for improved analysis") + .build(step) + .map_err(Self::err)?, ); } - - // Try to create a fix for the obfuscated uses path - if let Some(fix) = self.create_uses_fix(uses, step) { - finding_builder = finding_builder.fix(fix); - } - - findings.push(finding_builder.build(step).map_err(Self::err)?); } + _ => {} } Ok(findings) diff --git a/crates/zizmor/src/finding.rs b/crates/zizmor/src/finding.rs index 0f432471..fcc1dba9 100644 --- a/crates/zizmor/src/finding.rs +++ b/crates/zizmor/src/finding.rs @@ -126,6 +126,8 @@ pub(crate) struct Finding<'doc> { /// and carries metadata about how an output layer might choose to /// present it. pub(crate) locations: Vec>, + /// A tip or recommendation associated with this finding. + pub(crate) tip: Option, /// Whether this finding is ignored, either via inline comments or /// through a user's configuration. pub(crate) ignored: bool, @@ -176,6 +178,7 @@ pub(crate) struct FindingBuilder<'doc> { persona: Persona, raw_locations: Vec>, locations: Vec>, + tip: Option, fixes: Vec>, } @@ -190,6 +193,7 @@ impl<'doc> FindingBuilder<'doc> { persona: Default::default(), raw_locations: vec![], locations: vec![], + tip: None, fixes: vec![], } } @@ -219,6 +223,11 @@ impl<'doc> FindingBuilder<'doc> { self } + pub(crate) fn tip(mut self, tip: impl Into) -> Self { + self.tip = Some(tip.into()); + self + } + pub(crate) fn fix(mut self, fix: Fix<'doc>) -> Self { self.fixes.push(fix); self @@ -256,6 +265,7 @@ impl<'doc> FindingBuilder<'doc> { persona: self.persona, }, locations, + tip: self.tip, ignored: should_ignore, fixes: self.fixes, }) diff --git a/crates/zizmor/src/output/plain.rs b/crates/zizmor/src/output/plain.rs index 7a06c2c8..da7a0e7a 100644 --- a/crates/zizmor/src/output/plain.rs +++ b/crates/zizmor/src/output/plain.rs @@ -205,6 +205,10 @@ fn render_finding(registry: &InputRegistry, finding: &Finding) { .elements(finding_snippets(registry, finding)) .element(Level::NOTE.message(confidence)); + if let Some(tip) = &finding.tip { + group = group.element(Level::HELP.with_name("tip").message(tip)); + } + if !finding.fixes.is_empty() { group = group.element(Level::NOTE.message("this finding has an auto-fix")); } diff --git a/crates/zizmor/tests/integration/snapshots/integration__snapshot__github_env.snap b/crates/zizmor/tests/integration/snapshots/integration__snapshot__github_env.snap index 0b45cc53..06b975c6 100644 --- a/crates/zizmor/tests/integration/snapshots/integration__snapshot__github_env.snap +++ b/crates/zizmor/tests/integration/snapshots/integration__snapshot__github_env.snap @@ -29,4 +29,13 @@ error[github-env]: dangerous use of environment file | = note: audit confidence → Low -3 findings: 0 informational, 0 low, 0 medium, 3 high +help[obfuscation]: obfuscated usage of GitHub Actions features + --> @@INPUT@@:22:7 + | +22 | shell: cmd + | ^^^^^^^^^^ Windows CMD shell limits analysis + | + = note: audit confidence → High + = tip: use 'shell: pwsh' or 'shell: bash' for improved analysis + +4 findings: 0 informational, 1 low, 0 medium, 3 high diff --git a/docs/audits.md b/docs/audits.md index 1ebf888b..c891587c 100644 --- a/docs/audits.md +++ b/docs/audits.md @@ -948,6 +948,9 @@ This audit detects a variety of obfuscated usages, including: * Obfuscated GitHub expressions, including no-op patterns like `fromJSON(toJSON(...))` and calls to `format(...)` where all arguments are literal values. +* Use of the Windows CMD shell, i.e. `#!yaml shell: cmd` and similar. + The CMD shell has no formal grammar, making it impossible to accurately + analyze for security issues. ### Remediation diff --git a/docs/release-notes.md b/docs/release-notes.md index df48475c..5a4c03bf 100644 --- a/docs/release-notes.md +++ b/docs/release-notes.md @@ -19,8 +19,12 @@ of `zizmor`. Many thanks to @ManuelLerchnerQC for proposing and implementing this improvement! -* `zizmor` now correctly detects many more "dry-run" patterns in - the [use-trusted-publishing] audit, making it significantly more accurate (#1357) +* The [use-trusted-publishing] audit now correctly detecting more "dry-run" + patterns, making it significantly more accurate (#1357) + +* The [obfuscation] audit now detects usages of `#!yaml shell: cmd` and similar, + as the Windows CMD shell lacks a formal grammar and limits analysis of `#!yaml run:` blocks + in other audits (#1361) ### Performance Improvements 🚄