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 0000000000..848eb4a2fc --- /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 4e779154a6..fb1856db54 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -2735,6 +2735,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 920c74e568..1475aadc38 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -529,6 +529,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Bandit, "606") => (RuleGroup::Unspecified, rules::flake8_bandit::rules::StartProcessWithNoShell), (Flake8Bandit, "607") => (RuleGroup::Unspecified, rules::flake8_bandit::rules::StartProcessWithPartialPath), (Flake8Bandit, "608") => (RuleGroup::Unspecified, rules::flake8_bandit::rules::HardcodedSQLExpression), + (Flake8Bandit, "609") => (RuleGroup::Unspecified, rules::flake8_bandit::rules::UnixCommandWildcardInjection), (Flake8Bandit, "612") => (RuleGroup::Unspecified, rules::flake8_bandit::rules::LoggingConfigInsecureListen), (Flake8Bandit, "701") => (RuleGroup::Unspecified, rules::flake8_bandit::rules::Jinja2AutoescapeFalse), diff --git a/crates/ruff/src/rules/flake8_bandit/mod.rs b/crates/ruff/src/rules/flake8_bandit/mod.rs index d75bf471dd..4abd69f58a 100644 --- a/crates/ruff/src/rules/flake8_bandit/mod.rs +++ b/crates/ruff/src/rules/flake8_bandit/mod.rs @@ -28,6 +28,7 @@ mod tests { #[test_case(Rule::HashlibInsecureHashFunction, Path::new("S324.py"))] #[test_case(Rule::Jinja2AutoescapeFalse, Path::new("S701.py"))] #[test_case(Rule::LoggingConfigInsecureListen, Path::new("S612.py"))] + #[test_case(Rule::ParamikoCall, Path::new("S601.py"))] #[test_case(Rule::RequestWithNoCertValidation, Path::new("S501.py"))] #[test_case(Rule::RequestWithoutTimeout, Path::new("S113.py"))] #[test_case(Rule::SnmpInsecureVersion, Path::new("S508.py"))] @@ -41,8 +42,8 @@ mod tests { #[test_case(Rule::SuspiciousTelnetUsage, Path::new("S312.py"))] #[test_case(Rule::TryExceptContinue, Path::new("S112.py"))] #[test_case(Rule::TryExceptPass, Path::new("S110.py"))] + #[test_case(Rule::UnixCommandWildcardInjection, Path::new("S609.py"))] #[test_case(Rule::UnsafeYAMLLoad, Path::new("S506.py"))] - #[test_case(Rule::ParamikoCall, Path::new("S601.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff/src/rules/flake8_bandit/rules/mod.rs b/crates/ruff/src/rules/flake8_bandit/rules/mod.rs index 5653a946e6..90f1266e42 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/mod.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/mod.rs @@ -28,7 +28,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 5be6818183..a7c3d63a9d 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs @@ -89,6 +89,140 @@ 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 due to `*` usage") + } +} + +/// S602, S603, S604, S605, S606, S607, S609 +pub(crate) fn shell_injection( + checker: &mut Checker, + func: &Expr, + args: &[Expr], + keywords: &[Keyword], +) { + let call_kind = get_call_kind(func, checker.semantic_model()); + let shell_keyword = find_shell_keyword(checker.semantic_model(), keywords); + + if matches!(call_kind, Some(CallKind::Subprocess)) { + if let Some(arg) = args.first() { + match shell_keyword { + // S602 + Some(ShellKeyword { + truthiness: Truthiness::Truthy, + keyword, + }) => { + if checker.enabled(Rule::SubprocessPopenWithShellEqualsTrue) { + checker.diagnostics.push(Diagnostic::new( + SubprocessPopenWithShellEqualsTrue { + seems_safe: shell_call_seems_safe(arg), + }, + keyword.range(), + )); + } + } + // S603 + Some(ShellKeyword { + truthiness: Truthiness::Falsey | Truthiness::Unknown, + keyword, + }) => { + if checker.enabled(Rule::SubprocessWithoutShellEqualsTrue) { + checker.diagnostics.push(Diagnostic::new( + SubprocessWithoutShellEqualsTrue, + keyword.range(), + )); + } + } + // S603 + None => { + if checker.enabled(Rule::SubprocessWithoutShellEqualsTrue) { + checker.diagnostics.push(Diagnostic::new( + SubprocessWithoutShellEqualsTrue, + arg.range(), + )); + } + } + } + } + } else if let Some(ShellKeyword { + truthiness: Truthiness::Truthy, + keyword, + }) = shell_keyword + { + // S604 + if checker.enabled(Rule::CallWithShellEqualsTrue) { + checker + .diagnostics + .push(Diagnostic::new(CallWithShellEqualsTrue, keyword.range())); + } + } + + // S605 + if checker.enabled(Rule::StartProcessWithAShell) { + if matches!(call_kind, Some(CallKind::Shell)) { + if let Some(arg) = args.first() { + checker.diagnostics.push(Diagnostic::new( + StartProcessWithAShell { + seems_safe: shell_call_seems_safe(arg), + }, + arg.range(), + )); + } + } + } + + // S606 + if checker.enabled(Rule::StartProcessWithNoShell) { + if matches!(call_kind, Some(CallKind::NoShell)) { + checker + .diagnostics + .push(Diagnostic::new(StartProcessWithNoShell, func.range())); + } + } + + // S607 + if checker.enabled(Rule::StartProcessWithPartialPath) { + if call_kind.is_some() { + if let Some(arg) = args.first() { + if is_partial_path(arg) { + checker + .diagnostics + .push(Diagnostic::new(StartProcessWithPartialPath, arg.range())); + } + } + } + } + + // S609 + if checker.enabled(Rule::UnixCommandWildcardInjection) { + if matches!(call_kind, Some(CallKind::Shell)) + || matches!( + (call_kind, shell_keyword), + ( + Some(CallKind::Subprocess), + Some(ShellKeyword { + truthiness: Truthiness::Truthy, + keyword: _, + }) + ) + ) + { + if let Some(arg) = args.first() { + if is_wildcard_command(arg) { + checker + .diagnostics + .push(Diagnostic::new(UnixCommandWildcardInjection, func.range())); + } + } + } + } +} + #[derive(Copy, Clone, Debug)] enum CallKind { Subprocess, @@ -163,117 +297,50 @@ fn shell_call_seems_safe(arg: &Expr) -> bool { ) } -/// Return the [`Expr`] as a string literal, if it's a string or a list of strings. -fn try_string_literal(expr: &Expr) -> Option<&str> { - match expr { - Expr::List(ast::ExprList { elts, .. }) => { - if elts.is_empty() { - None - } else { - string_literal(&elts[0]) - } - } +/// Return `true` if the [`Expr`] is a string literal or list of string literals that starts with a +/// partial path. +fn is_partial_path(expr: &Expr) -> bool { + let string_literal = match expr { + Expr::List(ast::ExprList { elts, .. }) => elts.first().and_then(string_literal), _ => string_literal(expr), - } + }; + string_literal.map_or(false, |text| !FULL_PATH_REGEX.is_match(text)) } -/// S602, S603, S604, S605, S606, S607 -pub(crate) fn shell_injection( - checker: &mut Checker, - func: &Expr, - args: &[Expr], - keywords: &[Keyword], -) { - let call_kind = get_call_kind(func, checker.semantic_model()); - - if matches!(call_kind, Some(CallKind::Subprocess)) { - if let Some(arg) = args.first() { - match find_shell_keyword(checker.semantic_model(), keywords) { - // S602 - Some(ShellKeyword { - truthiness: Truthiness::Truthy, - keyword, - }) => { - if checker.enabled(Rule::SubprocessPopenWithShellEqualsTrue) { - checker.diagnostics.push(Diagnostic::new( - SubprocessPopenWithShellEqualsTrue { - seems_safe: shell_call_seems_safe(arg), - }, - keyword.range(), - )); - } - } - // S603 - Some(ShellKeyword { - truthiness: Truthiness::Falsey | Truthiness::Unknown, - keyword, - }) => { - if checker.enabled(Rule::SubprocessWithoutShellEqualsTrue) { - checker.diagnostics.push(Diagnostic::new( - SubprocessWithoutShellEqualsTrue, - keyword.range(), - )); - } - } - // S603 - None => { - if checker.enabled(Rule::SubprocessWithoutShellEqualsTrue) { - checker.diagnostics.push(Diagnostic::new( - SubprocessWithoutShellEqualsTrue, - arg.range(), - )); - } - } - } - } - } else if let Some(ShellKeyword { - truthiness: Truthiness::Truthy, - keyword, - }) = find_shell_keyword(checker.semantic_model(), keywords) - { - // S604 - if checker.enabled(Rule::CallWithShellEqualsTrue) { - checker - .diagnostics - .push(Diagnostic::new(CallWithShellEqualsTrue, keyword.range())); - } - } - - // S605 - if matches!(call_kind, Some(CallKind::Shell)) { - if let Some(arg) = args.first() { - if checker.enabled(Rule::StartProcessWithAShell) { - checker.diagnostics.push(Diagnostic::new( - StartProcessWithAShell { - seems_safe: shell_call_seems_safe(arg), - }, - arg.range(), - )); - } - } - } - - // S606 - if matches!(call_kind, Some(CallKind::NoShell)) { - if checker.enabled(Rule::StartProcessWithNoShell) { - checker - .diagnostics - .push(Diagnostic::new(StartProcessWithNoShell, func.range())); - } - } - - // S607 - if call_kind.is_some() { - if let Some(arg) = args.first() { - if checker.enabled(Rule::StartProcessWithPartialPath) { - if let Some(value) = try_string_literal(arg) { - if FULL_PATH_REGEX.find(value).is_none() { - checker - .diagnostics - .push(Diagnostic::new(StartProcessWithPartialPath, arg.range())); - } - } +/// Return `true` if the [`Expr`] is a wildcard command. +/// +/// ## Examples +/// ```python +/// import subprocess +/// +/// subprocess.Popen("/bin/chown root: *", shell=True) +/// subprocess.Popen(["/usr/local/bin/rsync", "*", "some_where:"], shell=True) +/// ``` +fn is_wildcard_command(expr: &Expr) -> bool { + if let Expr::List(ast::ExprList { elts, .. }) = expr { + let mut has_star = false; + let mut has_command = false; + for elt in elts.iter() { + if let Some(text) = string_literal(elt) { + has_star |= text.contains('*'); + has_command |= text.contains("chown") + || text.contains("chmod") + || text.contains("tar") + || text.contains("rsync"); + } + if has_star && has_command { + break; } } + has_star && has_command + } else { + let string_literal = string_literal(expr); + string_literal.map_or(false, |text| { + text.contains('*') + && (text.contains("chown") + || text.contains("chmod") + || text.contains("tar") + || text.contains("rsync")) + }) } } 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 0000000000..d22cefbae2 --- /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 due to `*` usage + | +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 due to `*` usage + | +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 due to `*` usage + | + 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 due to `*` usage + | + 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 bae8b2012c..038af86c1c 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2352,6 +2352,7 @@ "S606", "S607", "S608", + "S609", "S61", "S612", "S7",