From eb993ea0bb9b51294dea54636ccd91400497b6a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Thu, 18 May 2023 22:20:51 +0300 Subject: [PATCH] Implement S609, linux_commands_wildcard_injection --- .../test/fixtures/flake8_bandit/S609.py | 8 ++++ crates/ruff/src/checkers/ast/mod.rs | 1 + crates/ruff/src/codes.rs | 1 + crates/ruff/src/registry.rs | 1 + crates/ruff/src/rules/flake8_bandit/mod.rs | 1 + .../ruff/src/rules/flake8_bandit/rules/mod.rs | 2 +- .../flake8_bandit/rules/shell_injection.rs | 41 ++++++++++++++++++- ...s__flake8_bandit__tests__S609_S609.py.snap | 41 +++++++++++++++++++ ruff.schema.json | 1 + 9 files changed, 95 insertions(+), 2 deletions(-) create mode 100644 crates/ruff/resources/test/fixtures/flake8_bandit/S609.py create mode 100644 crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S609_S609.py.snap diff --git a/crates/ruff/resources/test/fixtures/flake8_bandit/S609.py b/crates/ruff/resources/test/fixtures/flake8_bandit/S609.py new file mode 100644 index 00000000000000..848eb4a2fce976 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_bandit/S609.py @@ -0,0 +1,8 @@ +import os +import subprocess + +os.popen("chmod +w foo*") +subprocess.Popen("/bin/chown root: *", shell=True) +subprocess.Popen(["/usr/local/bin/rsync", "*", "some_where:"], shell=True) +subprocess.Popen("/usr/local/bin/rsync * no_injection_here:") +os.system("tar cf foo.tar bar/*") diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index c6fa2d148b0311..fe81fd95a5448b 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -2862,6 +2862,7 @@ where Rule::StartProcessWithAShell, Rule::StartProcessWithNoShell, Rule::StartProcessWithPartialPath, + Rule::UnixCommandWildcardInjection, ]) { flake8_bandit::rules::shell_injection(self, func, args, keywords); } diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 064fd722f4318e..66a0c0c34612f8 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -514,6 +514,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Bandit, "606") => (RuleGroup::Unspecified, Rule::StartProcessWithNoShell), (Flake8Bandit, "607") => (RuleGroup::Unspecified, Rule::StartProcessWithPartialPath), (Flake8Bandit, "608") => (RuleGroup::Unspecified, Rule::HardcodedSQLExpression), + (Flake8Bandit, "609") => (RuleGroup::Unspecified, Rule::UnixCommandWildcardInjection), (Flake8Bandit, "612") => (RuleGroup::Unspecified, Rule::LoggingConfigInsecureListen), (Flake8Bandit, "701") => (RuleGroup::Unspecified, Rule::Jinja2AutoescapeFalse), diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index 2de02bde300205..77fb22bd2d3c6b 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -433,6 +433,7 @@ ruff_macros::register_rules!( rules::flake8_bandit::rules::StartProcessWithAShell, rules::flake8_bandit::rules::StartProcessWithNoShell, rules::flake8_bandit::rules::StartProcessWithPartialPath, + rules::flake8_bandit::rules::UnixCommandWildcardInjection, rules::flake8_bandit::rules::SuspiciousEvalUsage, rules::flake8_bandit::rules::SuspiciousFTPLibUsage, rules::flake8_bandit::rules::SuspiciousInsecureCipherUsage, diff --git a/crates/ruff/src/rules/flake8_bandit/mod.rs b/crates/ruff/src/rules/flake8_bandit/mod.rs index 91cd61a150715c..d9c6f1b135a584 100644 --- a/crates/ruff/src/rules/flake8_bandit/mod.rs +++ b/crates/ruff/src/rules/flake8_bandit/mod.rs @@ -42,6 +42,7 @@ mod tests { #[test_case(Rule::SuspiciousTelnetUsage, Path::new("S312.py"); "S312")] #[test_case(Rule::TryExceptContinue, Path::new("S112.py"); "S112")] #[test_case(Rule::TryExceptPass, Path::new("S110.py"); "S110")] + #[test_case(Rule::UnixCommandWildcardInjection, Path::new("S609.py"); "S609")] #[test_case(Rule::UnsafeYAMLLoad, Path::new("S506.py"); "S506")] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); diff --git a/crates/ruff/src/rules/flake8_bandit/rules/mod.rs b/crates/ruff/src/rules/flake8_bandit/rules/mod.rs index a7a3f4f92e8b50..b124b79b386d0f 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/mod.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/mod.rs @@ -27,7 +27,7 @@ pub(crate) use request_without_timeout::{request_without_timeout, RequestWithout pub(crate) use shell_injection::{ shell_injection, CallWithShellEqualsTrue, StartProcessWithAShell, StartProcessWithNoShell, StartProcessWithPartialPath, SubprocessPopenWithShellEqualsTrue, - SubprocessWithoutShellEqualsTrue, + SubprocessWithoutShellEqualsTrue, UnixCommandWildcardInjection, }; pub(crate) use snmp_insecure_version::{snmp_insecure_version, SnmpInsecureVersion}; pub(crate) use snmp_weak_cryptography::{snmp_weak_cryptography, SnmpWeakCryptography}; diff --git a/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs b/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs index b571173c197452..82f49fea3236f1 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs @@ -1,5 +1,6 @@ //! Checks relating to shell injection. +use itertools::Itertools; use once_cell::sync::Lazy; use regex::Regex; use rustpython_parser::ast::{self, Constant, Expr, Keyword, Ranged}; @@ -14,6 +15,8 @@ use crate::{ }; static FULL_PATH_REGEX: Lazy = Lazy::new(|| Regex::new(r"^([A-Za-z]:|[\\/.])").unwrap()); +static WILDCARD_COMMAND_REGEX: Lazy = + Lazy::new(|| Regex::new(r"(^|[\s/])(chown|chmod|tar|rsync)\s.*\*").unwrap()); #[violation] pub struct SubprocessPopenWithShellEqualsTrue { @@ -89,6 +92,16 @@ impl Violation for StartProcessWithPartialPath { } } +#[violation] +pub struct UnixCommandWildcardInjection; + +impl Violation for UnixCommandWildcardInjection { + #[derive_message_formats] + fn message(&self) -> String { + format!("Possible wildcard injection in call") + } +} + #[derive(Copy, Clone, Debug)] enum CallKind { Subprocess, @@ -174,7 +187,7 @@ fn try_string_literal(expr: &Expr) -> Option<&str> { } } -/// S602, S603, S604, S605, S606, S607 +/// S602, S603, S604, S605, S606, S607, S609 pub(crate) fn shell_injection( checker: &mut Checker, func: &Expr, @@ -182,6 +195,7 @@ pub(crate) fn shell_injection( keywords: &[Keyword], ) { let call_kind = get_call_kind(func, &checker.ctx); + let mut subprocess_with_shell = false; if matches!(call_kind, Some(CallKind::Subprocess)) { if let Some(arg) = args.first() { @@ -191,6 +205,7 @@ pub(crate) fn shell_injection( truthiness: Truthiness::Truthy, keyword, }) => { + subprocess_with_shell = true; if checker .settings .rules @@ -297,4 +312,28 @@ pub(crate) fn shell_injection( } } } + + // S609 + if checker + .settings + .rules + .enabled(Rule::UnixCommandWildcardInjection) + && args.len() != 0 + && (matches!(call_kind, Some(CallKind::Shell)) || subprocess_with_shell) + { + if let Some(cmd) = args.first() { + let cmd_string = match cmd { + Expr::List(ast::ExprList { elts, .. }) => elts + .iter() + .map(|elt| string_literal(elt).unwrap_or("")) + .join(" "), + _ => String::from(string_literal(cmd).unwrap_or("")), + }; + if WILDCARD_COMMAND_REGEX.is_match(cmd_string.as_str()) { + checker + .diagnostics + .push(Diagnostic::new(UnixCommandWildcardInjection, func.range())); + } + } + } } diff --git a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S609_S609.py.snap b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S609_S609.py.snap new file mode 100644 index 00000000000000..46bd1733e230dd --- /dev/null +++ b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S609_S609.py.snap @@ -0,0 +1,41 @@ +--- +source: crates/ruff/src/rules/flake8_bandit/mod.rs +--- +S609.py:4:1: S609 Possible wildcard injection in call + | +4 | import subprocess +5 | +6 | os.popen("chmod +w foo*") + | ^^^^^^^^ S609 +7 | subprocess.Popen("/bin/chown root: *", shell=True) +8 | subprocess.Popen(["/usr/local/bin/rsync", "*", "some_where:"], shell=True) + | + +S609.py:5:1: S609 Possible wildcard injection in call + | +5 | os.popen("chmod +w foo*") +6 | subprocess.Popen("/bin/chown root: *", shell=True) + | ^^^^^^^^^^^^^^^^ S609 +7 | subprocess.Popen(["/usr/local/bin/rsync", "*", "some_where:"], shell=True) +8 | subprocess.Popen("/usr/local/bin/rsync * no_injection_here:") + | + +S609.py:6:1: S609 Possible wildcard injection in call + | + 6 | os.popen("chmod +w foo*") + 7 | subprocess.Popen("/bin/chown root: *", shell=True) + 8 | subprocess.Popen(["/usr/local/bin/rsync", "*", "some_where:"], shell=True) + | ^^^^^^^^^^^^^^^^ S609 + 9 | subprocess.Popen("/usr/local/bin/rsync * no_injection_here:") +10 | os.system("tar cf foo.tar bar/*") + | + +S609.py:8:1: S609 Possible wildcard injection in call + | + 8 | subprocess.Popen(["/usr/local/bin/rsync", "*", "some_where:"], shell=True) + 9 | subprocess.Popen("/usr/local/bin/rsync * no_injection_here:") +10 | os.system("tar cf foo.tar bar/*") + | ^^^^^^^^^ S609 + | + + diff --git a/ruff.schema.json b/ruff.schema.json index 3e0e357729d141..6cd554738ebab5 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2254,6 +2254,7 @@ "S606", "S607", "S608", + "S609", "S61", "S612", "S7",