Implement flake8-bandit shell injection rules (astral-sh#3924)

Author: robyoung

_typos.toml

@@ -5,3 +5,4 @@ extend-exclude = ["snapshots", "black"]
 trivias = "trivias"
 hel = "hel"
 whos = "whos"
+spawnve = "spawnve"

crates/ruff/resources/test/fixtures/flake8_bandit/S602.py

@@ -0,0 +1,20 @@
+from subprocess import Popen, call, check_call, check_output, run
+
+# Check different Popen wrappers are checked.
+Popen("true", shell=True)
+call("true", shell=True)
+check_call("true", shell=True)
+check_output("true", shell=True)
+run("true", shell=True)
+
+# Check values that truthy values are treated as true.
+Popen("true", shell=1)
+Popen("true", shell=[1])
+Popen("true", shell={1: 1})
+Popen("true", shell=(1,))
+
+# Check command argument looks unsafe.
+var_string = "true"
+Popen(var_string, shell=True)
+Popen([var_string], shell=True)
+Popen([var_string, ""], shell=True)

crates/ruff/resources/test/fixtures/flake8_bandit/S603.py

@@ -0,0 +1,20 @@
+from subprocess import Popen, call, check_call, check_output, run
+
+# Different Popen wrappers are checked.
+Popen("true", shell=False)
+call("true", shell=False)
+check_call("true", shell=False)
+check_output("true", shell=False)
+run("true", shell=False)
+
+# Values that falsey values are treated as false.
+Popen("true", shell=0)
+Popen("true", shell=[])
+Popen("true", shell={})
+Popen("true", shell=None)
+
+# Unknown values are treated as falsey.
+Popen("true", shell=True if True else False)
+
+# No value is also caught.
+Popen("true")

crates/ruff/resources/test/fixtures/flake8_bandit/S604.py

@@ -0,0 +1,5 @@
+def foo(shell):
+    pass
+
+
+foo(shell=True)

crates/ruff/resources/test/fixtures/flake8_bandit/S605.py

@@ -0,0 +1,25 @@
+import os
+
+import commands
+import popen2
+
+# Check all shell functions.
+os.system("true")
+os.popen("true")
+os.popen2("true")
+os.popen3("true")
+os.popen4("true")
+popen2.popen2("true")
+popen2.popen3("true")
+popen2.popen4("true")
+popen2.Popen3("true")
+popen2.Popen4("true")
+commands.getoutput("true")
+commands.getstatusoutput("true")
+
+
+# Check command argument looks unsafe.
+var_string = "true"
+os.system(var_string)
+os.system([var_string])
+os.system([var_string, ""])

crates/ruff/resources/test/fixtures/flake8_bandit/S606.py

@@ -0,0 +1,20 @@
+import os
+
+# Check all shell functions.
+os.execl("true")
+os.execle("true")
+os.execlp("true")
+os.execlpe("true")
+os.execv("true")
+os.execve("true")
+os.execvp("true")
+os.execvpe("true")
+os.spawnl("true")
+os.spawnle("true")
+os.spawnlp("true")
+os.spawnlpe("true")
+os.spawnv("true")
+os.spawnve("true")
+os.spawnvp("true")
+os.spawnvpe("true")
+os.startfile("true")

crates/ruff/resources/test/fixtures/flake8_bandit/S607.py

@@ -0,0 +1,44 @@
+import os
+
+# Check all functions.
+subprocess.Popen("true")
+subprocess.call("true")
+subprocess.check_call("true")
+subprocess.check_output("true")
+subprocess.run("true")
+os.system("true")
+os.popen("true")
+os.popen2("true")
+os.popen3("true")
+os.popen4("true")
+popen2.popen2("true")
+popen2.popen3("true")
+popen2.popen4("true")
+popen2.Popen3("true")
+popen2.Popen4("true")
+commands.getoutput("true")
+commands.getstatusoutput("true")
+os.execl("true")
+os.execle("true")
+os.execlp("true")
+os.execlpe("true")
+os.execv("true")
+os.execve("true")
+os.execvp("true")
+os.execvpe("true")
+os.spawnl("true")
+os.spawnle("true")
+os.spawnlp("true")
+os.spawnlpe("true")
+os.spawnv("true")
+os.spawnve("true")
+os.spawnvp("true")
+os.spawnvpe("true")
+os.startfile("true")
+
+# Check it does not fail for full paths.
+os.system("/bin/ls")
+os.system("./bin/ls")
+os.system(["/bin/ls"])
+os.system(["/bin/ls", "/tmp"])
+os.system(r"C:\\bin\ls")

crates/ruff/src/checkers/ast/mod.rs

@@ -2651,6 +2651,16 @@ where
                         self, func, args, keywords,
                     );
                 }
+                if self.settings.rules.any_enabled(&[
+                    Rule::SubprocessWithoutShellEqualsTrue,
+                    Rule::SubprocessPopenWithShellEqualsTrue,
+                    Rule::CallWithShellEqualsTrue,
+                    Rule::StartProcessWithAShell,
+                    Rule::StartProcessWithNoShell,
+                    Rule::StartProcessWithPartialPath,
+                ]) {
+                    flake8_bandit::rules::shell_injection(self, func, args, keywords);
+                }
 
                 // flake8-comprehensions
                 if self.settings.rules.enabled(Rule::UnnecessaryGeneratorList) {

crates/ruff/src/codes.rs

@@ -507,6 +507,12 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {
         (Flake8Bandit, "506") => Rule::UnsafeYAMLLoad,
         (Flake8Bandit, "508") => Rule::SnmpInsecureVersion,
         (Flake8Bandit, "509") => Rule::SnmpWeakCryptography,
+        (Flake8Bandit, "602") => Rule::SubprocessPopenWithShellEqualsTrue,
+        (Flake8Bandit, "603") => Rule::SubprocessWithoutShellEqualsTrue,
+        (Flake8Bandit, "604") => Rule::CallWithShellEqualsTrue,
+        (Flake8Bandit, "605") => Rule::StartProcessWithAShell,
+        (Flake8Bandit, "606") => Rule::StartProcessWithNoShell,
+        (Flake8Bandit, "607") => Rule::StartProcessWithPartialPath,
         (Flake8Bandit, "608") => Rule::HardcodedSQLExpression,
         (Flake8Bandit, "612") => Rule::LoggingConfigInsecureListen,
         (Flake8Bandit, "701") => Rule::Jinja2AutoescapeFalse,

crates/ruff/src/registry.rs

@@ -446,6 +446,12 @@ ruff_macros::register_rules!(
     rules::flake8_bandit::rules::RequestWithoutTimeout,
     rules::flake8_bandit::rules::SnmpInsecureVersion,
     rules::flake8_bandit::rules::SnmpWeakCryptography,
+    rules::flake8_bandit::rules::SubprocessPopenWithShellEqualsTrue,
+    rules::flake8_bandit::rules::SubprocessWithoutShellEqualsTrue,
+    rules::flake8_bandit::rules::CallWithShellEqualsTrue,
+    rules::flake8_bandit::rules::StartProcessWithAShell,
+    rules::flake8_bandit::rules::StartProcessWithNoShell,
+    rules::flake8_bandit::rules::StartProcessWithPartialPath,
     rules::flake8_bandit::rules::SuspiciousEvalUsage,
     rules::flake8_bandit::rules::SuspiciousFTPLibUsage,
     rules::flake8_bandit::rules::SuspiciousInsecureCipherUsage,