From 85b90858bb8a31fcfa93797fd36e250e2286937c Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Sun, 24 Nov 2024 21:56:45 -0500 Subject: [PATCH 1/3] feat: improve precision for github-env --- Cargo.lock | 1 + Cargo.toml | 1 + src/audit/github_env.rs | 53 ++++++++++++++++++++++++++++++++--------- 3 files changed, 44 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9ad62f1d..b5313dad 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2318,6 +2318,7 @@ dependencies = [ "pest", "pest_derive", "pretty_assertions", + "regex", "reqwest", "serde", "serde-sarif", diff --git a/Cargo.toml b/Cargo.toml index 1223be66..57491307 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -25,6 +25,7 @@ moka = { version = "0.12.8", features = ["sync"] } owo-colors = "4.1.0" pest = "2.7.14" pest_derive = "2.7.14" +regex = "1.11.1" reqwest = { version = "0.12.9", features = ["blocking", "json"] } serde = { version = "1.0.215", features = ["derive"] } serde-sarif = "0.6.5" diff --git a/src/audit/github_env.rs b/src/audit/github_env.rs index 96a2a912..c70b9254 100644 --- a/src/audit/github_env.rs +++ b/src/audit/github_env.rs @@ -3,19 +3,20 @@ use crate::finding::{Confidence, Finding, Severity}; use crate::models::Step; use crate::state::AuditState; use github_actions_models::workflow::job::StepBody; +use regex::Regex; use std::ops::Deref; +use std::sync::LazyLock; + +static GITHUB_ENV_WRITE_SHELL: LazyLock = + LazyLock::new(|| Regex::new(r#"(?m)^.+\s*>>?\s*"?\$\{?GITHUB_ENV\}?"?.*$"#).unwrap()); pub(crate) struct GitHubEnv; -audit_meta!(GitHubEnv, "github-env", "dangerous use of $GITHUB_ENV"); +audit_meta!(GitHubEnv, "github-env", "dangerous use of GITHUB_ENV"); impl GitHubEnv { - fn uses_github_environment(&self, run_step_body: &str) -> bool { - // In the future we can improve over this implementation, - // eventually detecting how $GITHUB_ENV is being used - // and returning an Option instead - - run_step_body.contains("$GITHUB_ENV") || run_step_body.contains("${GITHUB_ENV}") + fn uses_github_environment(run_step_body: &str) -> bool { + GITHUB_ENV_WRITE_SHELL.is_match(run_step_body) } } @@ -40,14 +41,16 @@ impl WorkflowAudit for GitHubEnv { } if let StepBody::Run { run, .. } = &step.deref().body { - if self.uses_github_environment(run) { + if Self::uses_github_environment(run) { findings.push( Self::finding() .severity(Severity::High) .confidence(Confidence::Low) - .add_location(step.location().with_keys(&["run".into()]).annotated( - "GITHUB_ENV used in the context of a dangerous Workflow trigger", - )) + .add_location( + step.location() + .with_keys(&["run".into()]) + .annotated("GITHUB_ENV write may allow code execution"), + ) .build(step.workflow())?, ) } @@ -56,3 +59,31 @@ impl WorkflowAudit for GitHubEnv { Ok(findings) } } + +#[cfg(test)] +mod tests { + use crate::audit::github_env::GitHubEnv; + + #[test] + fn test_shell_patterns() { + for case in &[ + // Common cases + "echo foo >> $GITHUB_ENV", + "echo foo >> \"$GITHUB_ENV\"", + "echo foo >> ${GITHUB_ENV}", + "echo foo >> \"${GITHUB_ENV}\"", + // Single > is buggy most of the time, but still exploitable + "echo foo > $GITHUB_ENV", + "echo foo > \"$GITHUB_ENV\"", + "echo foo > ${GITHUB_ENV}", + "echo foo > \"${GITHUB_ENV}\"", + // No spaces + "echo foo>>$GITHUB_ENV", + "echo foo>>\"$GITHUB_ENV\"", + "echo foo>>${GITHUB_ENV}", + "echo foo>>\"${GITHUB_ENV}\"", + ] { + assert!(GitHubEnv::uses_github_environment(case)); + } + } +} From 601d959f80efb59a67de3ae5812e96c1e1dd9611 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Sun, 24 Nov 2024 22:12:15 -0500 Subject: [PATCH 2/3] add some `| tee` cases as well --- src/audit/github_env.rs | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/audit/github_env.rs b/src/audit/github_env.rs index c70b9254..4674e250 100644 --- a/src/audit/github_env.rs +++ b/src/audit/github_env.rs @@ -3,12 +3,19 @@ use crate::finding::{Confidence, Finding, Severity}; use crate::models::Step; use crate::state::AuditState; use github_actions_models::workflow::job::StepBody; -use regex::Regex; +use regex::RegexSet; use std::ops::Deref; use std::sync::LazyLock; -static GITHUB_ENV_WRITE_SHELL: LazyLock = - LazyLock::new(|| Regex::new(r#"(?m)^.+\s*>>?\s*"?\$\{?GITHUB_ENV\}?"?.*$"#).unwrap()); +static GITHUB_ENV_WRITE_SHELL: LazyLock = LazyLock::new(|| { + RegexSet::new(&[ + // matches the `... >> $GITHUB_ENV` pattern + r#"(?m)^.+\s*>>?\s*"?\$\{?GITHUB_ENV\}?"?.*$"#, + // matches the `... | tee $GITHUB_ENV` pattern + r#"(?m)^.*\|\s*tee\s+"?\$\{?GITHUB_ENV\}?"?.*$"#, + ]) + .unwrap() +}); pub(crate) struct GitHubEnv; @@ -82,6 +89,14 @@ mod tests { "echo foo>>\"$GITHUB_ENV\"", "echo foo>>${GITHUB_ENV}", "echo foo>>\"${GITHUB_ENV}\"", + // tee cases + "something | tee $GITHUB_ENV", + "something | tee \"$GITHUB_ENV\"", + "something | tee ${GITHUB_ENV}", + "something | tee \"${GITHUB_ENV}\"", + "something|tee $GITHUB_ENV", + "something |tee $GITHUB_ENV", + "something| tee $GITHUB_ENV", ] { assert!(GitHubEnv::uses_github_environment(case)); } From be93eda4d7be3c826bcc9ffd562fcf7b0f0e702c Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Sun, 24 Nov 2024 22:14:59 -0500 Subject: [PATCH 3/3] clippy --- src/audit/github_env.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/audit/github_env.rs b/src/audit/github_env.rs index 4674e250..a11bdbd0 100644 --- a/src/audit/github_env.rs +++ b/src/audit/github_env.rs @@ -8,7 +8,7 @@ use std::ops::Deref; use std::sync::LazyLock; static GITHUB_ENV_WRITE_SHELL: LazyLock = LazyLock::new(|| { - RegexSet::new(&[ + RegexSet::new([ // matches the `... >> $GITHUB_ENV` pattern r#"(?m)^.+\s*>>?\s*"?\$\{?GITHUB_ENV\}?"?.*$"#, // matches the `... | tee $GITHUB_ENV` pattern