From 27ac34d6833fb3dc90c152d0e186e7bc5d59e93f Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Mon, 7 Oct 2024 13:31:01 +0100 Subject: [PATCH] Rework `S606` (`start-process-with-no-shell`) docs to make clear the security motivations (#13658) Helps with #13614. This docs rewrite draws on the [documentation for the original bandit rule](https://bandit.readthedocs.io/en/latest/plugins/b606_start_process_with_no_shell.html). --- .../flake8_bandit/rules/shell_injection.rs | 30 +++++++++++-------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_bandit/rules/shell_injection.rs b/crates/ruff_linter/src/rules/flake8_bandit/rules/shell_injection.rs index ff2330b623..eeb2f894c3 100644 --- a/crates/ruff_linter/src/rules/flake8_bandit/rules/shell_injection.rs +++ b/crates/ruff_linter/src/rules/flake8_bandit/rules/shell_injection.rs @@ -191,22 +191,28 @@ impl Violation for StartProcessWithAShell { /// Checks for functions that start a process without a shell. /// /// ## Why is this bad? -/// The `subprocess` module provides more powerful facilities for spawning new -/// processes and retrieving their results; using that module is preferable to -/// using these functions. +/// Invoking any kind of external executable via a function call can pose +/// security risks if arbitrary variables are passed to the executable, or if +/// the input is otherwise unsanitised or unvalidated. +/// +/// This rule specifically flags functions in the `os` module that spawn +/// subprocesses *without* the use of a shell. Note that these typically pose a +/// much smaller security risk than subprocesses that are started *with* a +/// shell, which are flagged by [`start-process-with-a-shell`] (`S605`). This +/// gives you the option of enabling one rule while disabling the other if you +/// decide that the security risk from these functions is acceptable for your +/// use case. /// /// ## Example /// ```python -/// os.spawnlp(os.P_NOWAIT, "/bin/mycmd", "mycmd", "myarg") +/// import os +/// +/// +/// def insecure_function(arbitrary_user_input: str): +/// os.spawnlp(os.P_NOWAIT, "/bin/mycmd", "mycmd", arbitrary_user_input) /// ``` /// -/// Use instead: -/// ```python -/// subprocess.Popen(["/bin/mycmd", "myarg"]) -/// ``` -/// -/// ## References -/// - [Python documentation: Replacing the `os.spawn` family](https://docs.python.org/3/library/subprocess.html#replacing-the-os-spawn-family) +/// [start-process-with-a-shell]: https://docs.astral.sh/ruff/rules/start-process-with-a-shell/#start-process-with-a-shell-s605 #[violation] pub struct StartProcessWithNoShell; @@ -480,7 +486,7 @@ impl From<&Expr> for Safety { /// /// ## Examples /// ```python -/// import subprocess +/// import os /// /// os.system("/bin/ls") /// os.system("./bin/ls")