From 507ce6a96271e089809394c31d28ef40d5905cdf Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Sat, 7 Sep 2024 19:25:23 -0400 Subject: [PATCH] audit: begin work on excessive_permissions (#13) --- Cargo.lock | 4 +- Cargo.toml | 2 +- src/audit/artipacked.rs | 3 +- src/audit/excessive_permissions.rs | 169 +++++++++++++++++++ src/audit/hardcoded_container_credentials.rs | 3 +- src/audit/impostor_commit.rs | 9 +- src/audit/mod.rs | 4 +- src/audit/pull_request_target.rs | 3 +- src/audit/ref_confusion.rs | 3 +- src/audit/template_injection.rs | 3 +- src/audit/use_trusted_publishing.rs | 3 +- src/finding/locate.rs | 3 +- src/finding/mod.rs | 4 +- src/main.rs | 1 + src/models.rs | 4 +- 15 files changed, 191 insertions(+), 27 deletions(-) create mode 100644 src/audit/excessive_permissions.rs diff --git a/Cargo.lock b/Cargo.lock index de6cf8fd..8c2c794a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -378,9 +378,9 @@ checksum = "40ecd4077b5ae9fd2e9e169b102c6c330d0605168eb0e8bf79952b256dbefffd" [[package]] name = "github-actions-models" -version = "0.5.0" +version = "0.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "acbbca091ff74624a2c41c37a5920aa132cfc261ea0d82000e058e3b8c768389" +checksum = "08b8739506f0d3cc7750148296dfa3b424becceec5bd5005c95c33d7474df1e9" dependencies = [ "serde", "serde_yaml", diff --git a/Cargo.toml b/Cargo.toml index c96715a4..2bffa19a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,7 +9,7 @@ homepage = "https://github.com/woodruffw/zizmor" anyhow = "1.0.86" clap = { version = "4.5.16", features = ["derive", "env"] } env_logger = "0.11.5" -github-actions-models = "0.5.0" +github-actions-models = "0.6.0" itertools = "0.13.0" log = "0.4.22" regex = "1.10.6" diff --git a/src/audit/artipacked.rs b/src/audit/artipacked.rs index e9493780..fd390ab6 100644 --- a/src/audit/artipacked.rs +++ b/src/audit/artipacked.rs @@ -7,14 +7,13 @@ use github_actions_models::{ }; use itertools::Itertools; +use super::WorkflowAudit; use crate::{ finding::{Confidence, Finding, Severity}, models::AuditConfig, }; use crate::{models::Workflow, utils::split_patterns}; -use super::WorkflowAudit; - pub(crate) struct Artipacked<'a> { pub(crate) config: AuditConfig<'a>, } diff --git a/src/audit/excessive_permissions.rs b/src/audit/excessive_permissions.rs new file mode 100644 index 00000000..7514daf1 --- /dev/null +++ b/src/audit/excessive_permissions.rs @@ -0,0 +1,169 @@ +use std::{collections::HashMap, ops::Deref, sync::LazyLock}; + +use github_actions_models::{ + common::{BasePermission, Permission, Permissions}, + workflow::Job, +}; + +use super::WorkflowAudit; +use crate::{ + finding::{Confidence, Severity}, + models::AuditConfig, +}; + +// Subjective mapping of permissions to severities, when given `write` access. +static KNOWN_PERMISSIONS: LazyLock> = LazyLock::new(|| { + [ + ("actions", Severity::High), + ("attestations", Severity::High), + ("checks", Severity::Medium), + ("contents", Severity::High), + ("deployments", Severity::High), + ("discussions", Severity::Medium), + ("id-token", Severity::High), + ("issues", Severity::High), + ("packages", Severity::High), + ("pages", Severity::High), + ("pull-requests", Severity::High), + ("repository-projects", Severity::Medium), + ("security-events", Severity::Medium), + // What does the write permission even do here? + ("statuses", Severity::Low), + ] + .into() +}); + +pub(crate) struct ExcessivePermissions<'a> { + pub(crate) _config: AuditConfig<'a>, +} + +impl<'a> WorkflowAudit<'a> for ExcessivePermissions<'a> { + fn ident() -> &'static str + where + Self: Sized, + { + "excessive-permissions" + } + + fn new(config: crate::models::AuditConfig<'a>) -> anyhow::Result + where + Self: Sized, + { + Ok(Self { _config: config }) + } + + fn audit<'w>( + &mut self, + workflow: &'w crate::models::Workflow, + ) -> anyhow::Result>> { + let mut findings = vec![]; + // Top-level permissions. + for (severity, confidence, note) in self.check_permissions(&workflow.permissions, None) { + findings.push( + Self::finding() + .severity(severity) + .confidence(confidence) + .add_location(workflow.key_location("permissions").annotated(note)) + .build(workflow)?, + ) + } + + for job in workflow.jobs() { + let Job::NormalJob(normal) = job.deref() else { + continue; + }; + + for (severity, confidence, note) in + self.check_permissions(&normal.permissions, Some(&workflow.permissions)) + { + findings.push( + Self::finding() + .severity(severity) + .confidence(confidence) + .add_location(job.key_location("permissions").annotated(note)) + .build(workflow)?, + ) + } + } + + Ok(findings) + } +} + +impl<'a> ExcessivePermissions<'a> { + fn check_permissions( + &self, + permissions: &Permissions, + parent: Option<&Permissions>, + ) -> Vec<(Severity, Confidence, String)> { + match permissions { + Permissions::Base(base) => match base { + // If no explicit permissions are specified, our behavior + // depends on the presence of a parent (workflow) permission + // specifier. + BasePermission::Default => match parent { + // If there's a parent permissions block, this job inherits + // from it and has nothing new to report. + Some(_) => vec![], + // If there's no parent permissions block, we're at the workflow + // level and should report the default permissions as potentially + // being too broad. + None => vec![( + Severity::Medium, + Confidence::Low, + "workflow uses default permissions, which may be excessive".into(), + )], + }, + BasePermission::ReadAll => vec![( + Severity::Medium, + Confidence::High, + "uses read-all permissions, which may grant read access to more resources \ + than necessary" + .into(), + )], + BasePermission::WriteAll => vec![( + Severity::High, + Confidence::High, + "uses write-all permissions, which grants destructive access to repository \ + resources" + .into(), + )], + }, + Permissions::Explicit(perms) => match parent { + // In the general case, it's impossible to tell whether a + // job-level permission block is over-scoped. + Some(_) => vec![], + // Top-level permission-blocks should almost never contain + // write permissions. + None => { + let mut results = vec![]; + + for (name, perm) in perms { + if *perm != Permission::Write { + continue; + } + + match KNOWN_PERMISSIONS.get(name.as_str()) { + Some(sev) => results.push(( + *sev, + Confidence::High, + format!("{name}: write is overly broad at the workflow level; move to the job level"), + )), + None => { + log::debug!("unknown permission: {name}"); + + results.push(( + Severity::Unknown, + Confidence::High, + format!("{name}: write is overly broad at the workflow level; move to the job level") + )) + }, + } + } + + results + } + }, + } + } +} diff --git a/src/audit/hardcoded_container_credentials.rs b/src/audit/hardcoded_container_credentials.rs index 614db737..e78121e6 100644 --- a/src/audit/hardcoded_container_credentials.rs +++ b/src/audit/hardcoded_container_credentials.rs @@ -8,13 +8,12 @@ use github_actions_models::{ }, }; +use super::WorkflowAudit; use crate::{ finding::{Confidence, Severity}, models::AuditConfig, }; -use super::WorkflowAudit; - pub(crate) struct HardcodedContainerCredentials<'a> { pub(crate) _config: AuditConfig<'a>, } diff --git a/src/audit/impostor_commit.rs b/src/audit/impostor_commit.rs index 1a1f8a28..f3437dc0 100644 --- a/src/audit/impostor_commit.rs +++ b/src/audit/impostor_commit.rs @@ -10,17 +10,16 @@ use std::{ ops::Deref, }; +use anyhow::Result; +use github_actions_models::workflow::{job::StepBody, Job}; + +use super::WorkflowAudit; use crate::{ finding::{Confidence, Finding, Severity}, github_api::{self, Branch, ComparisonStatus, Tag}, models::{AuditConfig, Uses, Workflow}, }; -use anyhow::Result; -use github_actions_models::workflow::{job::StepBody, Job}; - -use super::WorkflowAudit; - pub const IMPOSTOR_ANNOTATION: &str = "uses a commit that doesn't belong to the specified org/repo"; pub(crate) struct ImpostorCommit<'a> { diff --git a/src/audit/mod.rs b/src/audit/mod.rs index a6d80f94..599effaa 100644 --- a/src/audit/mod.rs +++ b/src/audit/mod.rs @@ -1,10 +1,12 @@ +use anyhow::Result; + use crate::{ finding::{Finding, FindingBuilder}, models::{AuditConfig, Workflow}, }; -use anyhow::Result; pub(crate) mod artipacked; +pub(crate) mod excessive_permissions; pub(crate) mod hardcoded_container_credentials; pub(crate) mod impostor_commit; pub(crate) mod pull_request_target; diff --git a/src/audit/pull_request_target.rs b/src/audit/pull_request_target.rs index 3a3153b2..dd792ae0 100644 --- a/src/audit/pull_request_target.rs +++ b/src/audit/pull_request_target.rs @@ -2,11 +2,10 @@ use anyhow::Result; use github_actions_models::workflow::event::{BareEvent, OptionalBody}; use github_actions_models::workflow::Trigger; +use super::WorkflowAudit; use crate::finding::{Confidence, Finding, Severity}; use crate::models::{AuditConfig, Workflow}; -use super::WorkflowAudit; - pub(crate) struct PullRequestTarget<'a> { pub(crate) _config: AuditConfig<'a>, } diff --git a/src/audit/ref_confusion.rs b/src/audit/ref_confusion.rs index 1464a4b5..42b27434 100644 --- a/src/audit/ref_confusion.rs +++ b/src/audit/ref_confusion.rs @@ -11,14 +11,13 @@ use std::ops::Deref; use anyhow::Result; use github_actions_models::workflow::{job::StepBody, Job}; +use super::WorkflowAudit; use crate::{ finding::{Confidence, Severity}, github_api, models::{AuditConfig, Uses}, }; -use super::WorkflowAudit; - const REF_CONFUSION_ANNOTATION: &str = "uses a ref that's provided by both the branch and tag namespaces"; diff --git a/src/audit/template_injection.rs b/src/audit/template_injection.rs index 20dada62..996704f9 100644 --- a/src/audit/template_injection.rs +++ b/src/audit/template_injection.rs @@ -9,14 +9,13 @@ use std::ops::Deref; use github_actions_models::workflow::{job::StepBody, Job}; +use super::WorkflowAudit; use crate::{ finding::{Confidence, Severity}, models::AuditConfig, utils::iter_expressions, }; -use super::WorkflowAudit; - pub(crate) struct TemplateInjection<'a> { pub(crate) _config: AuditConfig<'a>, } diff --git a/src/audit/use_trusted_publishing.rs b/src/audit/use_trusted_publishing.rs index cc99cbaf..88a9ddea 100644 --- a/src/audit/use_trusted_publishing.rs +++ b/src/audit/use_trusted_publishing.rs @@ -5,13 +5,12 @@ use github_actions_models::{ workflow::{job::StepBody, Job}, }; +use super::WorkflowAudit; use crate::{ finding::{Confidence, Severity}, models::AuditConfig, }; -use super::WorkflowAudit; - const USES_MANUAL_CREDENTIAL: &str = "uses a manually-configured credential instead of Trusted Publishing"; diff --git a/src/finding/locate.rs b/src/finding/locate.rs index 09afaf18..9938ec6d 100644 --- a/src/finding/locate.rs +++ b/src/finding/locate.rs @@ -4,9 +4,8 @@ use anyhow::{Ok, Result}; use tree_sitter::{Language, Query, QueryCursor}; -use crate::models::Workflow; - use super::{Feature, WorkflowLocation}; +use crate::models::Workflow; /// Captures an arbitrary top-level key within a YAML stream. const TOP_LEVEL_KEY: &str = r#" diff --git a/src/finding/mod.rs b/src/finding/mod.rs index 297ff1cc..9af26026 100644 --- a/src/finding/mod.rs +++ b/src/finding/mod.rs @@ -8,7 +8,7 @@ pub(crate) mod locate; // TODO: Traits + more flexible models here. -#[derive(Default, Serialize)] +#[derive(Copy, Clone, Default, Serialize)] pub(crate) enum Confidence { #[default] Unknown, @@ -17,7 +17,7 @@ pub(crate) enum Confidence { High, } -#[derive(Default, Serialize)] +#[derive(Copy, Clone, Default, Serialize)] pub(crate) enum Severity { #[default] Unknown, diff --git a/src/main.rs b/src/main.rs index 4fb9b7b9..25eec399 100644 --- a/src/main.rs +++ b/src/main.rs @@ -94,6 +94,7 @@ fn main() -> Result<()> { let mut results = vec![]; let audits: &mut [&mut dyn WorkflowAudit] = &mut [ &mut audit::artipacked::Artipacked::new(config)?, + &mut audit::excessive_permissions::ExcessivePermissions::new(config)?, &mut audit::pull_request_target::PullRequestTarget::new(config)?, &mut audit::impostor_commit::ImpostorCommit::new(config)?, &mut audit::ref_confusion::RefConfusion::new(config)?, diff --git a/src/models.rs b/src/models.rs index a91d3637..4038a708 100644 --- a/src/models.rs +++ b/src/models.rs @@ -1,8 +1,8 @@ -use anyhow::{anyhow, Context, Ok, Result}; use std::{collections::hash_map, iter::Enumerate, ops::Deref, path::Path}; -use tree_sitter::Tree; +use anyhow::{anyhow, Context, Ok, Result}; use github_actions_models::workflow; +use tree_sitter::Tree; use crate::finding::WorkflowLocation;