Implement S609, linux_commands_wildcard_injection (#4504)

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/*")

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);
                 }

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),
 

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(

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};

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.
+/// Return `true` if the [`Expr`] is a string literal or list of string literals that starts with a
-fn try_string_literal(expr: &Expr) -> Option<&str> {
+/// partial path.
-    match expr {
+fn is_partial_path(expr: &Expr) -> bool {
-        Expr::List(ast::ExprList { elts, .. }) => {
+    let string_literal = match expr {
-            if elts.is_empty() {
+        Expr::List(ast::ExprList { elts, .. }) => elts.first().and_then(string_literal),
-                None
-            } else {
-                string_literal(&elts[0])
-            }
-        }
         _ => string_literal(expr),
-    }
+    };
+    string_literal.map_or(false, |text| !FULL_PATH_REGEX.is_match(text))
 }
 
-/// S602, S603, S604, S605, S606, S607
+/// Return `true` if the [`Expr`] is a wildcard command.
-pub(crate) fn shell_injection(
+///
-    checker: &mut Checker,
+/// ## Examples
-    func: &Expr,
+/// ```python
-    args: &[Expr],
+/// import subprocess
-    keywords: &[Keyword],
+///
-) {
+/// subprocess.Popen("/bin/chown root: *", shell=True)
-    let call_kind = get_call_kind(func, checker.semantic_model());
+/// subprocess.Popen(["/usr/local/bin/rsync", "*", "some_where:"], shell=True)
-
+/// ```
-    if matches!(call_kind, Some(CallKind::Subprocess)) {
+fn is_wildcard_command(expr: &Expr) -> bool {
-        if let Some(arg) = args.first() {
+    if let Expr::List(ast::ExprList { elts, .. }) = expr {
-            match find_shell_keyword(checker.semantic_model(), keywords) {
+        let mut has_star = false;
-                // S602
+        let mut has_command = false;
-                Some(ShellKeyword {
+        for elt in elts.iter() {
-                    truthiness: Truthiness::Truthy,
+            if let Some(text) = string_literal(elt) {
-                    keyword,
+                has_star |= text.contains('*');
-                }) => {
+                has_command |= text.contains("chown")
-                    if checker.enabled(Rule::SubprocessPopenWithShellEqualsTrue) {
+                    || text.contains("chmod")
-                        checker.diagnostics.push(Diagnostic::new(
+                    || text.contains("tar")
-                            SubprocessPopenWithShellEqualsTrue {
+                    || text.contains("rsync");
-                                seems_safe: shell_call_seems_safe(arg),
+            }
-                            },
+            if has_star && has_command {
-                            keyword.range(),
+                break;
-                        ));
-                    }
-                }
-                // 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()));
-                    }
-                }
             }
         }
+        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"))
+        })
     }
 }

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
+   |
+
+

ruff.schema.json

@@ -2352,6 +2352,7 @@
         "S606",
         "S607",
         "S608",
+        "S609",
         "S61",
         "S612",
         "S7",