From e6443de0b8eae5da554780f8ba4f8bfb155a6bdf Mon Sep 17 00:00:00 2001 From: Rob Young Date: Thu, 6 Apr 2023 22:06:28 +0100 Subject: [PATCH 1/8] Implement flak8-bandit shell injection rules This includes rules S602 - S607. --- .../test/fixtures/flake8_bandit/S602.py | 20 + .../test/fixtures/flake8_bandit/S603.py | 20 + .../test/fixtures/flake8_bandit/S604.py | 5 + .../test/fixtures/flake8_bandit/S605.py | 25 ++ .../test/fixtures/flake8_bandit/S606.py | 20 + .../test/fixtures/flake8_bandit/S607.py | 44 ++ crates/ruff/src/checkers/ast/mod.rs | 10 + crates/ruff/src/codes.rs | 6 + crates/ruff/src/registry.rs | 6 + .../ruff/src/rules/flake8_bandit/rules/mod.rs | 6 + .../flake8_bandit/rules/shell_injection.rs | 380 ++++++++++++++++++ ruff.schema.json | 6 + 12 files changed, 548 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/flake8_bandit/S602.py create mode 100644 crates/ruff/resources/test/fixtures/flake8_bandit/S603.py create mode 100644 crates/ruff/resources/test/fixtures/flake8_bandit/S604.py create mode 100644 crates/ruff/resources/test/fixtures/flake8_bandit/S605.py create mode 100644 crates/ruff/resources/test/fixtures/flake8_bandit/S606.py create mode 100644 crates/ruff/resources/test/fixtures/flake8_bandit/S607.py create mode 100644 crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs diff --git a/crates/ruff/resources/test/fixtures/flake8_bandit/S602.py b/crates/ruff/resources/test/fixtures/flake8_bandit/S602.py new file mode 100644 index 00000000000000..679a2ae667baba --- /dev/null +++ b/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) diff --git a/crates/ruff/resources/test/fixtures/flake8_bandit/S603.py b/crates/ruff/resources/test/fixtures/flake8_bandit/S603.py new file mode 100644 index 00000000000000..22c188596d04b6 --- /dev/null +++ b/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") diff --git a/crates/ruff/resources/test/fixtures/flake8_bandit/S604.py b/crates/ruff/resources/test/fixtures/flake8_bandit/S604.py new file mode 100644 index 00000000000000..3be26d5495889f --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_bandit/S604.py @@ -0,0 +1,5 @@ +def foo(shell): + pass + + +foo(shell=True) diff --git a/crates/ruff/resources/test/fixtures/flake8_bandit/S605.py b/crates/ruff/resources/test/fixtures/flake8_bandit/S605.py new file mode 100644 index 00000000000000..51c47e597f24b2 --- /dev/null +++ b/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, ""]) diff --git a/crates/ruff/resources/test/fixtures/flake8_bandit/S606.py b/crates/ruff/resources/test/fixtures/flake8_bandit/S606.py new file mode 100644 index 00000000000000..5551e8bfdc4fe8 --- /dev/null +++ b/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") diff --git a/crates/ruff/resources/test/fixtures/flake8_bandit/S607.py b/crates/ruff/resources/test/fixtures/flake8_bandit/S607.py new file mode 100644 index 00000000000000..be70c2ad05748a --- /dev/null +++ b/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") diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index c14710fb766059..ac457482427b2d 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -2615,6 +2615,16 @@ where self, func, args, keywords, ); } + if self.settings.rules.any_enabled(&[ + Rule::SubprocessWithoutShellEqualsTrue, + Rule::SubprocessPopenWithShellEqualsTrue, + Rule::AnyOtherFunctionWithShellEqualsTrue, + Rule::StartProcessWithAShell, + Rule::StartProcessWithNoShell, + Rule::StartProcessWithPartialPath, + ]) { + flake8_bandit::rules::shell_injection(self, func, args, keywords); + } // flake8-comprehensions if self.settings.rules.enabled(Rule::UnnecessaryGeneratorList) { diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 5c506835685691..7b15bbe10eae04 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -507,6 +507,12 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option { (Flake8Bandit, "506") => Rule::UnsafeYAMLLoad, (Flake8Bandit, "508") => Rule::SnmpInsecureVersion, (Flake8Bandit, "509") => Rule::SnmpWeakCryptography, + (Flake8Bandit, "602") => Rule::SubprocessPopenWithShellEqualsTrue, + (Flake8Bandit, "603") => Rule::SubprocessWithoutShellEqualsTrue, + (Flake8Bandit, "604") => Rule::AnyOtherFunctionWithShellEqualsTrue, + (Flake8Bandit, "605") => Rule::StartProcessWithAShell, + (Flake8Bandit, "606") => Rule::StartProcessWithNoShell, + (Flake8Bandit, "607") => Rule::StartProcessWithPartialPath, (Flake8Bandit, "608") => Rule::HardcodedSQLExpression, (Flake8Bandit, "612") => Rule::LoggingConfigInsecureListen, (Flake8Bandit, "701") => Rule::Jinja2AutoescapeFalse, diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index b585c733782c41..8f26f0874717dd 100644 --- a/crates/ruff/src/registry.rs +++ b/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::AnyOtherFunctionWithShellEqualsTrue, + 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, diff --git a/crates/ruff/src/rules/flake8_bandit/rules/mod.rs b/crates/ruff/src/rules/flake8_bandit/rules/mod.rs index 4809103c978893..58446a978f74bf 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/mod.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/mod.rs @@ -22,6 +22,11 @@ pub use request_with_no_cert_validation::{ request_with_no_cert_validation, RequestWithNoCertValidation, }; pub use request_without_timeout::{request_without_timeout, RequestWithoutTimeout}; +pub use shell_injection::{ + shell_injection, AnyOtherFunctionWithShellEqualsTrue, StartProcessWithAShell, + StartProcessWithNoShell, StartProcessWithPartialPath, SubprocessPopenWithShellEqualsTrue, + SubprocessWithoutShellEqualsTrue, +}; pub use snmp_insecure_version::{snmp_insecure_version, SnmpInsecureVersion}; pub use snmp_weak_cryptography::{snmp_weak_cryptography, SnmpWeakCryptography}; pub use suspicious_function_call::{ @@ -52,6 +57,7 @@ mod jinja2_autoescape_false; mod logging_config_insecure_listen; mod request_with_no_cert_validation; mod request_without_timeout; +mod shell_injection; mod snmp_insecure_version; mod snmp_weak_cryptography; mod suspicious_function_call; diff --git a/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs b/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs new file mode 100644 index 00000000000000..8219249618725e --- /dev/null +++ b/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs @@ -0,0 +1,380 @@ +//! Checks relating to shell injection + +use num_bigint::BigInt; +use once_cell::sync::Lazy; +use regex::Regex; +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::types::Range; +use rustpython_parser::ast::{Constant, Expr, ExprKind, Keyword}; + +use crate::{ + checkers::ast::Checker, registry::Rule, rules::flake8_bandit::helpers::string_literal, +}; + +static FULL_PATH_REGEX: Lazy = Lazy::new(|| Regex::new(r"^([A-Za-z]:|[\\/\.])").unwrap()); + +#[violation] +pub struct SubprocessPopenWithShellEqualsTrue { + looks_safe: bool, +} + +impl Violation for SubprocessPopenWithShellEqualsTrue { + #[derive_message_formats] + fn message(&self) -> String { + if self.looks_safe { + format!("subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell") + } else { + format!("subprocess call with shell=True identified, security issue") + } + } +} + +#[violation] +pub struct SubprocessWithoutShellEqualsTrue; + +impl Violation for SubprocessWithoutShellEqualsTrue { + #[derive_message_formats] + fn message(&self) -> String { + format!("subprocess call: check for execution of untrusted input") + } +} + +#[violation] +pub struct AnyOtherFunctionWithShellEqualsTrue; + +impl Violation for AnyOtherFunctionWithShellEqualsTrue { + #[derive_message_formats] + fn message(&self) -> String { + format!("Function call with shell=True parameter identified, possible security issue") + } +} + +#[violation] +pub struct StartProcessWithAShell { + looks_safe: bool, +} + +impl Violation for StartProcessWithAShell { + #[derive_message_formats] + fn message(&self) -> String { + if self.looks_safe { + format!("Starting a process with a shell: Seems safe, but may be changed in the future, consider rewriting without shell") + } else { + format!("Starting a process with a shell, possible injection detected, security issue.") + } + } +} + +#[violation] +pub struct StartProcessWithNoShell; + +impl Violation for StartProcessWithNoShell { + #[derive_message_formats] + fn message(&self) -> String { + format!("Starting a process without a shell.") + } +} + +#[violation] +pub struct StartProcessWithPartialPath; + +impl Violation for StartProcessWithPartialPath { + #[derive_message_formats] + fn message(&self) -> String { + format!("Starting a process with a partial executable path.") + } +} + +enum CallKind { + Subprocess, + Shell, + NoShell, +} + +struct Config<'a> { + subprocess: Vec>, + shell: Vec>, + no_shell: Vec>, +} + +static CONFIG: Lazy = Lazy::new(|| Config { + subprocess: vec![ + vec!["subprocess", "Popen"], + vec!["subprocess", "call"], + vec!["subprocess", "check_call"], + vec!["subprocess", "check_output"], + vec!["subprocess", "run"], + ], + shell: vec![ + vec!["os", "system"], + vec!["os", "popen"], + vec!["os", "popen2"], + vec!["os", "popen3"], + vec!["os", "popen4"], + vec!["popen2", "popen2"], + vec!["popen2", "popen3"], + vec!["popen2", "popen4"], + vec!["popen2", "Popen3"], + vec!["popen2", "Popen4"], + vec!["commands", "getoutput"], + vec!["commands", "getstatusoutput"], + ], + no_shell: vec![ + vec!["os", "execl"], + vec!["os", "execle"], + vec!["os", "execlp"], + vec!["os", "execlpe"], + vec!["os", "execv"], + vec!["os", "execve"], + vec!["os", "execvp"], + vec!["os", "execvpe"], + vec!["os", "spawnl"], + vec!["os", "spawnle"], + vec!["os", "spawnlp"], + vec!["os", "spawnlpe"], + vec!["os", "spawnv"], + vec!["os", "spawnve"], + vec!["os", "spawnvp"], + vec!["os", "spawnvpe"], + vec!["os", "startfile"], + ], +}); + +enum HasShell { + // The shell keyword argument is set and it evaluates to false + Falsey, + // The shell keyword argument is set and it evaluates to true + Truthy, + // The shell keyword argument is set but we cannot evaluate it + Unknown, +} + +fn has_shell(keywords: &[Keyword]) -> Option { + if let Some(keyword) = find_shell_keyword(keywords) { + match &keyword.node.value.node { + ExprKind::Constant { + value: Constant::Bool(b), + .. + } => { + if *b { + Some(HasShell::Truthy) + } else { + Some(HasShell::Falsey) + } + } + ExprKind::Constant { + value: Constant::Int(int), + .. + } => { + if int == &BigInt::from(0u8) { + Some(HasShell::Falsey) + } else { + Some(HasShell::Truthy) + } + } + ExprKind::Constant { + value: Constant::Float(float), + .. + } => { + if (float - 0.0).abs() < f64::EPSILON { + Some(HasShell::Falsey) + } else { + Some(HasShell::Truthy) + } + } + ExprKind::List { elts, .. } => { + if elts.is_empty() { + Some(HasShell::Falsey) + } else { + Some(HasShell::Truthy) + } + } + ExprKind::Dict { keys, .. } => { + if keys.is_empty() { + Some(HasShell::Falsey) + } else { + Some(HasShell::Truthy) + } + } + ExprKind::Tuple { elts, .. } => { + if elts.is_empty() { + Some(HasShell::Falsey) + } else { + Some(HasShell::Truthy) + } + } + _ => Some(HasShell::Unknown), + } + } else { + None + } +} + +fn find_shell_keyword(keywords: &[Keyword]) -> Option<&Keyword> { + keywords.iter().find(|keyword| { + keyword + .node + .arg + .as_ref() + .and_then(|arg| if arg == "shell" { Some(()) } else { None }) + .is_some() + }) +} + +fn shell_call_looks_safe(arg: &Expr) -> bool { + matches!( + arg.node, + ExprKind::Constant { + value: Constant::Str(_), + .. + } + ) +} + +fn get_call_kind(checker: &mut Checker, func: &Expr) -> Option { + checker.ctx.resolve_call_path(func).and_then(|call_path| { + if CONFIG + .subprocess + .iter() + .any(|subprocess| call_path.as_slice() == subprocess.as_slice()) + { + Some(CallKind::Subprocess) + } else if CONFIG + .shell + .iter() + .any(|shell| call_path.as_slice() == shell.as_slice()) + { + Some(CallKind::Shell) + } else if CONFIG + .no_shell + .iter() + .any(|no_shell| call_path.as_slice() == no_shell.as_slice()) + { + Some(CallKind::NoShell) + } else { + None + } + }) +} + +fn string_literal_including_list(expr: &Expr) -> Option<&str> { + match &expr.node { + ExprKind::List { elts, .. } => { + if elts.is_empty() { + None + } else { + string_literal(&elts[0]) + } + } + _ => string_literal(expr), + } +} + +/// S602, S603, S604, S605, S606, S607 +pub fn shell_injection(checker: &mut Checker, func: &Expr, args: &[Expr], keywords: &[Keyword]) { + let call_kind = get_call_kind(checker, func); + + if let Some(CallKind::Subprocess) = call_kind { + if !args.is_empty() { + match has_shell(keywords) { + // S602 + Some(HasShell::Truthy) => { + if checker + .settings + .rules + .enabled(Rule::SubprocessPopenWithShellEqualsTrue) + { + checker.diagnostics.push(Diagnostic::new( + SubprocessPopenWithShellEqualsTrue { + looks_safe: shell_call_looks_safe(&args[0]), + }, + Range::from(find_shell_keyword(keywords).unwrap()), + )); + } + } + // S603 + Some(HasShell::Falsey | HasShell::Unknown) => { + if checker + .settings + .rules + .enabled(Rule::SubprocessWithoutShellEqualsTrue) + { + checker.diagnostics.push(Diagnostic::new( + SubprocessWithoutShellEqualsTrue {}, + Range::from(find_shell_keyword(keywords).unwrap()), + )); + } + } + // S603 + None => { + if checker + .settings + .rules + .enabled(Rule::SubprocessWithoutShellEqualsTrue) + { + checker.diagnostics.push(Diagnostic::new( + SubprocessWithoutShellEqualsTrue {}, + Range::from(&args[0]), + )); + } + } + } + } + } else if let Some(HasShell::Truthy) = has_shell(keywords) { + // S604 + if checker + .settings + .rules + .enabled(Rule::AnyOtherFunctionWithShellEqualsTrue) + { + checker.diagnostics.push(Diagnostic::new( + AnyOtherFunctionWithShellEqualsTrue {}, + Range::from(find_shell_keyword(keywords).unwrap()), + )); + } + } + + // S605 + if let Some(CallKind::Shell) = call_kind { + if !args.is_empty() && checker.settings.rules.enabled(Rule::StartProcessWithAShell) { + checker.diagnostics.push(Diagnostic::new( + StartProcessWithAShell { + looks_safe: shell_call_looks_safe(&args[0]), + }, + Range::from(&args[0]), + )); + } + } + + // S606 + if let Some(CallKind::NoShell) = call_kind { + if checker + .settings + .rules + .enabled(Rule::StartProcessWithNoShell) + { + checker.diagnostics.push(Diagnostic::new( + StartProcessWithNoShell {}, + Range::from(func), + )); + } + } + + // S607 + if call_kind.is_some() && !args.is_empty() { + if let Some(value) = string_literal_including_list(&args[0]) { + if FULL_PATH_REGEX.find(value).is_none() + && checker + .settings + .rules + .enabled(Rule::StartProcessWithPartialPath) + { + checker.diagnostics.push(Diagnostic::new( + StartProcessWithPartialPath {}, + Range::from(&args[0]), + )); + } + } + } +} diff --git a/ruff.schema.json b/ruff.schema.json index dd424e6bad6658..deae29e27f61be 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2128,6 +2128,12 @@ "S509", "S6", "S60", + "S602", + "S603", + "S604", + "S605", + "S606", + "S607", "S608", "S61", "S612", From 895458644e84a6c8fec25d99a44796a90f02d131 Mon Sep 17 00:00:00 2001 From: Rob Young Date: Sun, 9 Apr 2023 22:15:23 +0100 Subject: [PATCH 2/8] Add spawnve to _typos.toml --- _typos.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/_typos.toml b/_typos.toml index d2ef48f71be21f..a14ac7083f3360 100644 --- a/_typos.toml +++ b/_typos.toml @@ -5,3 +5,4 @@ extend-exclude = ["snapshots", "black"] trivias = "trivias" hel = "hel" whos = "whos" +spawnve = "spawnve" From 68ddf146a2312d5c17c49f0699dc944334632971 Mon Sep 17 00:00:00 2001 From: Rob Young Date: Wed, 12 Apr 2023 21:01:00 +0100 Subject: [PATCH 3/8] Update crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs Co-authored-by: Micha Reiser --- crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 8219249618725e..bd4ff3010f7c32 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs @@ -217,8 +217,7 @@ fn find_shell_keyword(keywords: &[Keyword]) -> Option<&Keyword> { .node .arg .as_ref() - .and_then(|arg| if arg == "shell" { Some(()) } else { None }) - .is_some() + .map_or(false, |arg| arg == "shell") }) } From 1ede01e33cd248bde715310579974240164b253c Mon Sep 17 00:00:00 2001 From: Rob Young Date: Wed, 12 Apr 2023 21:01:12 +0100 Subject: [PATCH 4/8] Update crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs Co-authored-by: Micha Reiser --- crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs | 1 + 1 file changed, 1 insertion(+) 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 bd4ff3010f7c32..e37ebfbdd841e4 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs @@ -141,6 +141,7 @@ static CONFIG: Lazy = Lazy::new(|| Config { ], }); +#[derive(Copy, Clone, Debug)] enum HasShell { // The shell keyword argument is set and it evaluates to false Falsey, From 9d742ce3511a9c6bfb97435c31742f6d4bb315f0 Mon Sep 17 00:00:00 2001 From: Rob Young Date: Wed, 12 Apr 2023 21:34:52 +0100 Subject: [PATCH 5/8] Address review comment to remove unwraps As suggested I have refactored find_shell_keyword to return a struct that includes the has_shell and keyword fields. After applying this refactor I realised that the name of as_shell function no longer really makes sense as it was just determining whether the value for a given keyword was truthy so I've refactored that as well. --- .../flake8_bandit/rules/shell_injection.rs | 88 ++++++++++++------- 1 file changed, 54 insertions(+), 34 deletions(-) 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 e37ebfbdd841e4..1c1f58407674b6 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs @@ -142,7 +142,7 @@ static CONFIG: Lazy = Lazy::new(|| Config { }); #[derive(Copy, Clone, Debug)] -enum HasShell { +enum Truthiness { // The shell keyword argument is set and it evaluates to false Falsey, // The shell keyword argument is set and it evaluates to true @@ -151,17 +151,17 @@ enum HasShell { Unknown, } -fn has_shell(keywords: &[Keyword]) -> Option { - if let Some(keyword) = find_shell_keyword(keywords) { - match &keyword.node.value.node { +impl From<&Keyword> for Truthiness { + fn from(value: &Keyword) -> Self { + match &value.node.value.node { ExprKind::Constant { value: Constant::Bool(b), .. } => { if *b { - Some(HasShell::Truthy) + Truthiness::Truthy } else { - Some(HasShell::Falsey) + Truthiness::Falsey } } ExprKind::Constant { @@ -169,9 +169,9 @@ fn has_shell(keywords: &[Keyword]) -> Option { .. } => { if int == &BigInt::from(0u8) { - Some(HasShell::Falsey) + Truthiness::Falsey } else { - Some(HasShell::Truthy) + Truthiness::Truthy } } ExprKind::Constant { @@ -179,47 +179,57 @@ fn has_shell(keywords: &[Keyword]) -> Option { .. } => { if (float - 0.0).abs() < f64::EPSILON { - Some(HasShell::Falsey) + Truthiness::Falsey } else { - Some(HasShell::Truthy) + Truthiness::Truthy } } ExprKind::List { elts, .. } => { if elts.is_empty() { - Some(HasShell::Falsey) + Truthiness::Falsey } else { - Some(HasShell::Truthy) + Truthiness::Truthy } } ExprKind::Dict { keys, .. } => { if keys.is_empty() { - Some(HasShell::Falsey) + Truthiness::Falsey } else { - Some(HasShell::Truthy) + Truthiness::Truthy } } ExprKind::Tuple { elts, .. } => { if elts.is_empty() { - Some(HasShell::Falsey) + Truthiness::Falsey } else { - Some(HasShell::Truthy) + Truthiness::Truthy } } - _ => Some(HasShell::Unknown), + _ => Truthiness::Unknown, } - } else { - None } } -fn find_shell_keyword(keywords: &[Keyword]) -> Option<&Keyword> { - keywords.iter().find(|keyword| { - keyword - .node - .arg - .as_ref() - .map_or(false, |arg| arg == "shell") - }) +#[derive(Copy, Clone, Debug)] +struct ShellKeyword<'a> { + has_shell: Truthiness, + keyword: &'a Keyword, +} + +fn find_shell_keyword(keywords: &[Keyword]) -> Option { + keywords + .iter() + .find(|keyword| { + keyword + .node + .arg + .as_ref() + .map_or(false, |arg| arg == "shell") + }) + .map(|keyword| ShellKeyword { + has_shell: keyword.into(), + keyword, + }) } fn shell_call_looks_safe(arg: &Expr) -> bool { @@ -277,9 +287,12 @@ pub fn shell_injection(checker: &mut Checker, func: &Expr, args: &[Expr], keywor if let Some(CallKind::Subprocess) = call_kind { if !args.is_empty() { - match has_shell(keywords) { + match find_shell_keyword(keywords) { // S602 - Some(HasShell::Truthy) => { + Some(ShellKeyword { + has_shell: Truthiness::Truthy, + keyword, + }) => { if checker .settings .rules @@ -289,12 +302,15 @@ pub fn shell_injection(checker: &mut Checker, func: &Expr, args: &[Expr], keywor SubprocessPopenWithShellEqualsTrue { looks_safe: shell_call_looks_safe(&args[0]), }, - Range::from(find_shell_keyword(keywords).unwrap()), + Range::from(keyword), )); } } // S603 - Some(HasShell::Falsey | HasShell::Unknown) => { + Some(ShellKeyword { + has_shell: Truthiness::Falsey | Truthiness::Unknown, + keyword, + }) => { if checker .settings .rules @@ -302,7 +318,7 @@ pub fn shell_injection(checker: &mut Checker, func: &Expr, args: &[Expr], keywor { checker.diagnostics.push(Diagnostic::new( SubprocessWithoutShellEqualsTrue {}, - Range::from(find_shell_keyword(keywords).unwrap()), + Range::from(keyword), )); } } @@ -321,7 +337,11 @@ pub fn shell_injection(checker: &mut Checker, func: &Expr, args: &[Expr], keywor } } } - } else if let Some(HasShell::Truthy) = has_shell(keywords) { + } else if let Some(ShellKeyword { + has_shell: Truthiness::Truthy, + keyword, + }) = find_shell_keyword(keywords) + { // S604 if checker .settings @@ -330,7 +350,7 @@ pub fn shell_injection(checker: &mut Checker, func: &Expr, args: &[Expr], keywor { checker.diagnostics.push(Diagnostic::new( AnyOtherFunctionWithShellEqualsTrue {}, - Range::from(find_shell_keyword(keywords).unwrap()), + Range::from(keyword), )); } } From a0ae02977f4bdc519658567fdd3afe799b0e88e7 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 13 Apr 2023 14:01:11 -0400 Subject: [PATCH 6/8] Tweak some messages; add test fixtures --- crates/ruff/src/checkers/ast/mod.rs | 2 +- crates/ruff/src/codes.rs | 2 +- crates/ruff/src/registry.rs | 2 +- crates/ruff/src/rules/flake8_bandit/mod.rs | 6 + .../ruff/src/rules/flake8_bandit/rules/mod.rs | 4 +- .../flake8_bandit/rules/shell_injection.rs | 47 ++-- ...s__flake8_bandit__tests__S602_S602.py.snap | 117 +++++++++ ...s__flake8_bandit__tests__S603_S603.py.snap | 106 +++++++++ ...s__flake8_bandit__tests__S604_S604.py.snap | 10 + ...s__flake8_bandit__tests__S605_S605.py.snap | 147 ++++++++++++ ...s__flake8_bandit__tests__S606_S606.py.snap | 170 +++++++++++++ ...s__flake8_bandit__tests__S607_S607.py.snap | 223 ++++++++++++++++++ 12 files changed, 809 insertions(+), 27 deletions(-) create mode 100644 crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S602_S602.py.snap create mode 100644 crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S603_S603.py.snap create mode 100644 crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S604_S604.py.snap create mode 100644 crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S605_S605.py.snap create mode 100644 crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S606_S606.py.snap create mode 100644 crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S607_S607.py.snap diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index d432d1b2681f24..4fd1b04be19e5c 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -2652,7 +2652,7 @@ where if self.settings.rules.any_enabled(&[ Rule::SubprocessWithoutShellEqualsTrue, Rule::SubprocessPopenWithShellEqualsTrue, - Rule::AnyOtherFunctionWithShellEqualsTrue, + Rule::CallWithShellEqualsTrue, Rule::StartProcessWithAShell, Rule::StartProcessWithNoShell, Rule::StartProcessWithPartialPath, diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index f7f5401f46f629..ae67c325f2a039 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -509,7 +509,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option { (Flake8Bandit, "509") => Rule::SnmpWeakCryptography, (Flake8Bandit, "602") => Rule::SubprocessPopenWithShellEqualsTrue, (Flake8Bandit, "603") => Rule::SubprocessWithoutShellEqualsTrue, - (Flake8Bandit, "604") => Rule::AnyOtherFunctionWithShellEqualsTrue, + (Flake8Bandit, "604") => Rule::CallWithShellEqualsTrue, (Flake8Bandit, "605") => Rule::StartProcessWithAShell, (Flake8Bandit, "606") => Rule::StartProcessWithNoShell, (Flake8Bandit, "607") => Rule::StartProcessWithPartialPath, diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index 1cdb2c0880821d..290d9a869977a2 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -448,7 +448,7 @@ ruff_macros::register_rules!( rules::flake8_bandit::rules::SnmpWeakCryptography, rules::flake8_bandit::rules::SubprocessPopenWithShellEqualsTrue, rules::flake8_bandit::rules::SubprocessWithoutShellEqualsTrue, - rules::flake8_bandit::rules::AnyOtherFunctionWithShellEqualsTrue, + rules::flake8_bandit::rules::CallWithShellEqualsTrue, rules::flake8_bandit::rules::StartProcessWithAShell, rules::flake8_bandit::rules::StartProcessWithNoShell, rules::flake8_bandit::rules::StartProcessWithPartialPath, diff --git a/crates/ruff/src/rules/flake8_bandit/mod.rs b/crates/ruff/src/rules/flake8_bandit/mod.rs index 97ba897b301f7b..91cd61a150715c 100644 --- a/crates/ruff/src/rules/flake8_bandit/mod.rs +++ b/crates/ruff/src/rules/flake8_bandit/mod.rs @@ -18,6 +18,7 @@ mod tests { #[test_case(Rule::Assert, Path::new("S101.py"); "S101")] #[test_case(Rule::BadFilePermissions, Path::new("S103.py"); "S103")] + #[test_case(Rule::CallWithShellEqualsTrue, Path::new("S604.py"); "S604")] #[test_case(Rule::ExecBuiltin, Path::new("S102.py"); "S102")] #[test_case(Rule::HardcodedBindAllInterfaces, Path::new("S104.py"); "S104")] #[test_case(Rule::HardcodedPasswordDefault, Path::new("S107.py"); "S107")] @@ -32,6 +33,11 @@ mod tests { #[test_case(Rule::RequestWithoutTimeout, Path::new("S113.py"); "S113")] #[test_case(Rule::SnmpInsecureVersion, Path::new("S508.py"); "S508")] #[test_case(Rule::SnmpWeakCryptography, Path::new("S509.py"); "S509")] + #[test_case(Rule::StartProcessWithAShell, Path::new("S605.py"); "S605")] + #[test_case(Rule::StartProcessWithNoShell, Path::new("S606.py"); "S606")] + #[test_case(Rule::StartProcessWithPartialPath, Path::new("S607.py"); "S607")] + #[test_case(Rule::SubprocessPopenWithShellEqualsTrue, Path::new("S602.py"); "S602")] + #[test_case(Rule::SubprocessWithoutShellEqualsTrue, Path::new("S603.py"); "S603")] #[test_case(Rule::SuspiciousPickleUsage, Path::new("S301.py"); "S301")] #[test_case(Rule::SuspiciousTelnetUsage, Path::new("S312.py"); "S312")] #[test_case(Rule::TryExceptContinue, Path::new("S112.py"); "S112")] diff --git a/crates/ruff/src/rules/flake8_bandit/rules/mod.rs b/crates/ruff/src/rules/flake8_bandit/rules/mod.rs index 58446a978f74bf..aa836f6eba26ef 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/mod.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/mod.rs @@ -23,8 +23,8 @@ pub use request_with_no_cert_validation::{ }; pub use request_without_timeout::{request_without_timeout, RequestWithoutTimeout}; pub use shell_injection::{ - shell_injection, AnyOtherFunctionWithShellEqualsTrue, StartProcessWithAShell, - StartProcessWithNoShell, StartProcessWithPartialPath, SubprocessPopenWithShellEqualsTrue, + shell_injection, CallWithShellEqualsTrue, StartProcessWithAShell, StartProcessWithNoShell, + StartProcessWithPartialPath, SubprocessPopenWithShellEqualsTrue, SubprocessWithoutShellEqualsTrue, }; pub use snmp_insecure_version::{snmp_insecure_version, SnmpInsecureVersion}; 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 1c1f58407674b6..19828b681b0ac5 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs @@ -16,16 +16,18 @@ static FULL_PATH_REGEX: Lazy = Lazy::new(|| Regex::new(r"^([A-Za-z]:|[\\/ #[violation] pub struct SubprocessPopenWithShellEqualsTrue { - looks_safe: bool, + seems_safe: bool, } impl Violation for SubprocessPopenWithShellEqualsTrue { #[derive_message_formats] fn message(&self) -> String { - if self.looks_safe { - format!("subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell") + if self.seems_safe { + format!( + "`subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell`" + ) } else { - format!("subprocess call with shell=True identified, security issue") + format!("`subprocess` call with `shell=True` identified") } } } @@ -36,32 +38,32 @@ pub struct SubprocessWithoutShellEqualsTrue; impl Violation for SubprocessWithoutShellEqualsTrue { #[derive_message_formats] fn message(&self) -> String { - format!("subprocess call: check for execution of untrusted input") + format!("`subprocess` call: check for execution of untrusted input") } } #[violation] -pub struct AnyOtherFunctionWithShellEqualsTrue; +pub struct CallWithShellEqualsTrue; -impl Violation for AnyOtherFunctionWithShellEqualsTrue { +impl Violation for CallWithShellEqualsTrue { #[derive_message_formats] fn message(&self) -> String { - format!("Function call with shell=True parameter identified, possible security issue") + format!("Function call with `shell=True` parameter identified") } } #[violation] pub struct StartProcessWithAShell { - looks_safe: bool, + seems_safe: bool, } impl Violation for StartProcessWithAShell { #[derive_message_formats] fn message(&self) -> String { - if self.looks_safe { - format!("Starting a process with a shell: Seems safe, but may be changed in the future, consider rewriting without shell") + if self.seems_safe { + format!("Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell`") } else { - format!("Starting a process with a shell, possible injection detected, security issue.") + format!("Starting a process with a shell, possible injection detected") } } } @@ -72,7 +74,7 @@ pub struct StartProcessWithNoShell; impl Violation for StartProcessWithNoShell { #[derive_message_formats] fn message(&self) -> String { - format!("Starting a process without a shell.") + format!("Starting a process without a shell") } } @@ -82,10 +84,11 @@ pub struct StartProcessWithPartialPath; impl Violation for StartProcessWithPartialPath { #[derive_message_formats] fn message(&self) -> String { - format!("Starting a process with a partial executable path.") + format!("Starting a process with a partial executable path") } } +#[derive(Copy, Clone, Debug)] enum CallKind { Subprocess, Shell, @@ -143,11 +146,11 @@ static CONFIG: Lazy = Lazy::new(|| Config { #[derive(Copy, Clone, Debug)] enum Truthiness { - // The shell keyword argument is set and it evaluates to false + // The `shell` keyword argument is set and evaluates to `False`. Falsey, - // The shell keyword argument is set and it evaluates to true + // The `shell` keyword argument is set and evaluates to `True`. Truthy, - // The shell keyword argument is set but we cannot evaluate it + // The `shell` keyword argument is set, but its value is unknown. Unknown, } @@ -232,7 +235,7 @@ fn find_shell_keyword(keywords: &[Keyword]) -> Option { }) } -fn shell_call_looks_safe(arg: &Expr) -> bool { +fn shell_call_seems_safe(arg: &Expr) -> bool { matches!( arg.node, ExprKind::Constant { @@ -300,7 +303,7 @@ pub fn shell_injection(checker: &mut Checker, func: &Expr, args: &[Expr], keywor { checker.diagnostics.push(Diagnostic::new( SubprocessPopenWithShellEqualsTrue { - looks_safe: shell_call_looks_safe(&args[0]), + seems_safe: shell_call_seems_safe(&args[0]), }, Range::from(keyword), )); @@ -346,10 +349,10 @@ pub fn shell_injection(checker: &mut Checker, func: &Expr, args: &[Expr], keywor if checker .settings .rules - .enabled(Rule::AnyOtherFunctionWithShellEqualsTrue) + .enabled(Rule::CallWithShellEqualsTrue) { checker.diagnostics.push(Diagnostic::new( - AnyOtherFunctionWithShellEqualsTrue {}, + CallWithShellEqualsTrue {}, Range::from(keyword), )); } @@ -360,7 +363,7 @@ pub fn shell_injection(checker: &mut Checker, func: &Expr, args: &[Expr], keywor if !args.is_empty() && checker.settings.rules.enabled(Rule::StartProcessWithAShell) { checker.diagnostics.push(Diagnostic::new( StartProcessWithAShell { - looks_safe: shell_call_looks_safe(&args[0]), + seems_safe: shell_call_seems_safe(&args[0]), }, Range::from(&args[0]), )); diff --git a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S602_S602.py.snap b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S602_S602.py.snap new file mode 100644 index 00000000000000..f22d157d159cda --- /dev/null +++ b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S602_S602.py.snap @@ -0,0 +1,117 @@ +--- +source: crates/ruff/src/rules/flake8_bandit/mod.rs +--- +S602.py:4:15: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell` + | +4 | # Check different Popen wrappers are checked +5 | Popen("true", shell=True) + | ^^^^^^^^^^ S602 +6 | call("true", shell=True) +7 | check_call("true", shell=True) + | + +S602.py:5:14: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell` + | +5 | # Check different Popen wrappers are checked +6 | Popen("true", shell=True) +7 | call("true", shell=True) + | ^^^^^^^^^^ S602 +8 | check_call("true", shell=True) +9 | check_output("true", shell=True) + | + +S602.py:6:20: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell` + | + 6 | Popen("true", shell=True) + 7 | call("true", shell=True) + 8 | check_call("true", shell=True) + | ^^^^^^^^^^ S602 + 9 | check_output("true", shell=True) +10 | run("true", shell=True) + | + +S602.py:7:22: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell` + | + 7 | call("true", shell=True) + 8 | check_call("true", shell=True) + 9 | check_output("true", shell=True) + | ^^^^^^^^^^ S602 +10 | run("true", shell=True) + | + +S602.py:8:13: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell` + | + 8 | check_call("true", shell=True) + 9 | check_output("true", shell=True) +10 | run("true", shell=True) + | ^^^^^^^^^^ S602 +11 | +12 | # Check values that truthy values are treated as true + | + +S602.py:11:15: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell` + | +11 | # Check values that truthy values are treated as true +12 | Popen("true", shell=1) + | ^^^^^^^ S602 +13 | Popen("true", shell=[1]) +14 | Popen("true", shell={1: 1}) + | + +S602.py:12:15: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell` + | +12 | # Check values that truthy values are treated as true +13 | Popen("true", shell=1) +14 | Popen("true", shell=[1]) + | ^^^^^^^^^ S602 +15 | Popen("true", shell={1: 1}) +16 | Popen("true", shell=(1,)) + | + +S602.py:13:15: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell` + | +13 | Popen("true", shell=1) +14 | Popen("true", shell=[1]) +15 | Popen("true", shell={1: 1}) + | ^^^^^^^^^^^^ S602 +16 | Popen("true", shell=(1,)) + | + +S602.py:14:15: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell` + | +14 | Popen("true", shell=[1]) +15 | Popen("true", shell={1: 1}) +16 | Popen("true", shell=(1,)) + | ^^^^^^^^^^ S602 +17 | +18 | # Check command argument looks unsafe + | + +S602.py:18:19: S602 `subprocess` call with `shell=True` identified + | +18 | # Check command argument looks unsafe +19 | var_string = "true" +20 | Popen(var_string, shell=True) + | ^^^^^^^^^^ S602 +21 | Popen([var_string], shell=True) +22 | Popen([var_string, ""], shell=True) + | + +S602.py:19:21: S602 `subprocess` call with `shell=True` identified + | +19 | var_string = "true" +20 | Popen(var_string, shell=True) +21 | Popen([var_string], shell=True) + | ^^^^^^^^^^ S602 +22 | Popen([var_string, ""], shell=True) + | + +S602.py:20:25: S602 `subprocess` call with `shell=True` identified + | +20 | Popen(var_string, shell=True) +21 | Popen([var_string], shell=True) +22 | Popen([var_string, ""], shell=True) + | ^^^^^^^^^^ S602 + | + + diff --git a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S603_S603.py.snap b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S603_S603.py.snap new file mode 100644 index 00000000000000..257d880ed4c554 --- /dev/null +++ b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S603_S603.py.snap @@ -0,0 +1,106 @@ +--- +source: crates/ruff/src/rules/flake8_bandit/mod.rs +--- +S603.py:4:15: S603 `subprocess` call: check for execution of untrusted input + | +4 | # Different Popen wrappers are checked +5 | Popen("true", shell=False) + | ^^^^^^^^^^^ S603 +6 | call("true", shell=False) +7 | check_call("true", shell=False) + | + +S603.py:5:14: S603 `subprocess` call: check for execution of untrusted input + | +5 | # Different Popen wrappers are checked +6 | Popen("true", shell=False) +7 | call("true", shell=False) + | ^^^^^^^^^^^ S603 +8 | check_call("true", shell=False) +9 | check_output("true", shell=False) + | + +S603.py:6:20: S603 `subprocess` call: check for execution of untrusted input + | + 6 | Popen("true", shell=False) + 7 | call("true", shell=False) + 8 | check_call("true", shell=False) + | ^^^^^^^^^^^ S603 + 9 | check_output("true", shell=False) +10 | run("true", shell=False) + | + +S603.py:7:22: S603 `subprocess` call: check for execution of untrusted input + | + 7 | call("true", shell=False) + 8 | check_call("true", shell=False) + 9 | check_output("true", shell=False) + | ^^^^^^^^^^^ S603 +10 | run("true", shell=False) + | + +S603.py:8:13: S603 `subprocess` call: check for execution of untrusted input + | + 8 | check_call("true", shell=False) + 9 | check_output("true", shell=False) +10 | run("true", shell=False) + | ^^^^^^^^^^^ S603 +11 | +12 | # Values that falsey values are treated as false + | + +S603.py:11:15: S603 `subprocess` call: check for execution of untrusted input + | +11 | # Values that falsey values are treated as false +12 | Popen("true", shell=0) + | ^^^^^^^ S603 +13 | Popen("true", shell=[]) +14 | Popen("true", shell={}) + | + +S603.py:12:15: S603 `subprocess` call: check for execution of untrusted input + | +12 | # Values that falsey values are treated as false +13 | Popen("true", shell=0) +14 | Popen("true", shell=[]) + | ^^^^^^^^ S603 +15 | Popen("true", shell={}) +16 | Popen("true", shell=None) + | + +S603.py:13:15: S603 `subprocess` call: check for execution of untrusted input + | +13 | Popen("true", shell=0) +14 | Popen("true", shell=[]) +15 | Popen("true", shell={}) + | ^^^^^^^^ S603 +16 | Popen("true", shell=None) + | + +S603.py:14:15: S603 `subprocess` call: check for execution of untrusted input + | +14 | Popen("true", shell=[]) +15 | Popen("true", shell={}) +16 | Popen("true", shell=None) + | ^^^^^^^^^^ S603 +17 | +18 | # Unknown values are treated as falsey + | + +S603.py:17:15: S603 `subprocess` call: check for execution of untrusted input + | +17 | # Unknown values are treated as falsey +18 | Popen("true", shell=True if True else False) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S603 +19 | +20 | # No value is also caught + | + +S603.py:20:7: S603 `subprocess` call: check for execution of untrusted input + | +20 | # No value is also caught +21 | Popen("true") + | ^^^^^^ S603 + | + + diff --git a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S604_S604.py.snap b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S604_S604.py.snap new file mode 100644 index 00000000000000..f4bf0f4842ba2b --- /dev/null +++ b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S604_S604.py.snap @@ -0,0 +1,10 @@ +--- +source: crates/ruff/src/rules/flake8_bandit/mod.rs +--- +S604.py:5:5: S604 Function call with `shell=True` parameter identified + | +5 | foo(shell=True) + | ^^^^^^^^^^ S604 + | + + diff --git a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S605_S605.py.snap b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S605_S605.py.snap new file mode 100644 index 00000000000000..d4f3a91e3a11f5 --- /dev/null +++ b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S605_S605.py.snap @@ -0,0 +1,147 @@ +--- +source: crates/ruff/src/rules/flake8_bandit/mod.rs +--- +S605.py:7:11: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` + | + 7 | # Check all shell functions + 8 | os.system("true") + | ^^^^^^ S605 + 9 | os.popen("true") +10 | os.popen2("true") + | + +S605.py:8:10: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` + | + 8 | # Check all shell functions + 9 | os.system("true") +10 | os.popen("true") + | ^^^^^^ S605 +11 | os.popen2("true") +12 | os.popen3("true") + | + +S605.py:9:11: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` + | + 9 | os.system("true") +10 | os.popen("true") +11 | os.popen2("true") + | ^^^^^^ S605 +12 | os.popen3("true") +13 | os.popen4("true") + | + +S605.py:10:11: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` + | +10 | os.popen("true") +11 | os.popen2("true") +12 | os.popen3("true") + | ^^^^^^ S605 +13 | os.popen4("true") +14 | popen2.popen2("true") + | + +S605.py:11:11: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` + | +11 | os.popen2("true") +12 | os.popen3("true") +13 | os.popen4("true") + | ^^^^^^ S605 +14 | popen2.popen2("true") +15 | popen2.popen3("true") + | + +S605.py:12:15: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` + | +12 | os.popen3("true") +13 | os.popen4("true") +14 | popen2.popen2("true") + | ^^^^^^ S605 +15 | popen2.popen3("true") +16 | popen2.popen4("true") + | + +S605.py:13:15: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` + | +13 | os.popen4("true") +14 | popen2.popen2("true") +15 | popen2.popen3("true") + | ^^^^^^ S605 +16 | popen2.popen4("true") +17 | popen2.Popen3("true") + | + +S605.py:14:15: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` + | +14 | popen2.popen2("true") +15 | popen2.popen3("true") +16 | popen2.popen4("true") + | ^^^^^^ S605 +17 | popen2.Popen3("true") +18 | popen2.Popen4("true") + | + +S605.py:15:15: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` + | +15 | popen2.popen3("true") +16 | popen2.popen4("true") +17 | popen2.Popen3("true") + | ^^^^^^ S605 +18 | popen2.Popen4("true") +19 | commands.getoutput("true") + | + +S605.py:16:15: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` + | +16 | popen2.popen4("true") +17 | popen2.Popen3("true") +18 | popen2.Popen4("true") + | ^^^^^^ S605 +19 | commands.getoutput("true") +20 | commands.getstatusoutput("true") + | + +S605.py:17:20: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` + | +17 | popen2.Popen3("true") +18 | popen2.Popen4("true") +19 | commands.getoutput("true") + | ^^^^^^ S605 +20 | commands.getstatusoutput("true") + | + +S605.py:18:26: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` + | +18 | popen2.Popen4("true") +19 | commands.getoutput("true") +20 | commands.getstatusoutput("true") + | ^^^^^^ S605 + | + +S605.py:23:11: S605 Starting a process with a shell, possible injection detected + | +23 | # Check command argument looks unsafe +24 | var_string = "true" +25 | os.system(var_string) + | ^^^^^^^^^^ S605 +26 | os.system([var_string]) +27 | os.system([var_string, ""]) + | + +S605.py:24:11: S605 Starting a process with a shell, possible injection detected + | +24 | var_string = "true" +25 | os.system(var_string) +26 | os.system([var_string]) + | ^^^^^^^^^^^^ S605 +27 | os.system([var_string, ""]) + | + +S605.py:25:11: S605 Starting a process with a shell, possible injection detected + | +25 | os.system(var_string) +26 | os.system([var_string]) +27 | os.system([var_string, ""]) + | ^^^^^^^^^^^^^^^^ S605 + | + + diff --git a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S606_S606.py.snap b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S606_S606.py.snap new file mode 100644 index 00000000000000..724199075f7cb6 --- /dev/null +++ b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S606_S606.py.snap @@ -0,0 +1,170 @@ +--- +source: crates/ruff/src/rules/flake8_bandit/mod.rs +--- +S606.py:4:1: S606 Starting a process without a shell + | +4 | # Check all shell functions +5 | os.execl("true") + | ^^^^^^^^ S606 +6 | os.execle("true") +7 | os.execlp("true") + | + +S606.py:5:1: S606 Starting a process without a shell + | +5 | # Check all shell functions +6 | os.execl("true") +7 | os.execle("true") + | ^^^^^^^^^ S606 +8 | os.execlp("true") +9 | os.execlpe("true") + | + +S606.py:6:1: S606 Starting a process without a shell + | + 6 | os.execl("true") + 7 | os.execle("true") + 8 | os.execlp("true") + | ^^^^^^^^^ S606 + 9 | os.execlpe("true") +10 | os.execv("true") + | + +S606.py:7:1: S606 Starting a process without a shell + | + 7 | os.execle("true") + 8 | os.execlp("true") + 9 | os.execlpe("true") + | ^^^^^^^^^^ S606 +10 | os.execv("true") +11 | os.execve("true") + | + +S606.py:8:1: S606 Starting a process without a shell + | + 8 | os.execlp("true") + 9 | os.execlpe("true") +10 | os.execv("true") + | ^^^^^^^^ S606 +11 | os.execve("true") +12 | os.execvp("true") + | + +S606.py:9:1: S606 Starting a process without a shell + | + 9 | os.execlpe("true") +10 | os.execv("true") +11 | os.execve("true") + | ^^^^^^^^^ S606 +12 | os.execvp("true") +13 | os.execvpe("true") + | + +S606.py:10:1: S606 Starting a process without a shell + | +10 | os.execv("true") +11 | os.execve("true") +12 | os.execvp("true") + | ^^^^^^^^^ S606 +13 | os.execvpe("true") +14 | os.spawnl("true") + | + +S606.py:11:1: S606 Starting a process without a shell + | +11 | os.execve("true") +12 | os.execvp("true") +13 | os.execvpe("true") + | ^^^^^^^^^^ S606 +14 | os.spawnl("true") +15 | os.spawnle("true") + | + +S606.py:12:1: S606 Starting a process without a shell + | +12 | os.execvp("true") +13 | os.execvpe("true") +14 | os.spawnl("true") + | ^^^^^^^^^ S606 +15 | os.spawnle("true") +16 | os.spawnlp("true") + | + +S606.py:13:1: S606 Starting a process without a shell + | +13 | os.execvpe("true") +14 | os.spawnl("true") +15 | os.spawnle("true") + | ^^^^^^^^^^ S606 +16 | os.spawnlp("true") +17 | os.spawnlpe("true") + | + +S606.py:14:1: S606 Starting a process without a shell + | +14 | os.spawnl("true") +15 | os.spawnle("true") +16 | os.spawnlp("true") + | ^^^^^^^^^^ S606 +17 | os.spawnlpe("true") +18 | os.spawnv("true") + | + +S606.py:15:1: S606 Starting a process without a shell + | +15 | os.spawnle("true") +16 | os.spawnlp("true") +17 | os.spawnlpe("true") + | ^^^^^^^^^^^ S606 +18 | os.spawnv("true") +19 | os.spawnve("true") + | + +S606.py:16:1: S606 Starting a process without a shell + | +16 | os.spawnlp("true") +17 | os.spawnlpe("true") +18 | os.spawnv("true") + | ^^^^^^^^^ S606 +19 | os.spawnve("true") +20 | os.spawnvp("true") + | + +S606.py:17:1: S606 Starting a process without a shell + | +17 | os.spawnlpe("true") +18 | os.spawnv("true") +19 | os.spawnve("true") + | ^^^^^^^^^^ S606 +20 | os.spawnvp("true") +21 | os.spawnvpe("true") + | + +S606.py:18:1: S606 Starting a process without a shell + | +18 | os.spawnv("true") +19 | os.spawnve("true") +20 | os.spawnvp("true") + | ^^^^^^^^^^ S606 +21 | os.spawnvpe("true") +22 | os.startfile("true") + | + +S606.py:19:1: S606 Starting a process without a shell + | +19 | os.spawnve("true") +20 | os.spawnvp("true") +21 | os.spawnvpe("true") + | ^^^^^^^^^^^ S606 +22 | os.startfile("true") + | + +S606.py:20:1: S606 Starting a process without a shell + | +20 | os.spawnvp("true") +21 | os.spawnvpe("true") +22 | os.startfile("true") + | ^^^^^^^^^^^^ S606 + | + + diff --git a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S607_S607.py.snap b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S607_S607.py.snap new file mode 100644 index 00000000000000..05071f947c3e96 --- /dev/null +++ b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S607_S607.py.snap @@ -0,0 +1,223 @@ +--- +source: crates/ruff/src/rules/flake8_bandit/mod.rs +--- +S607.py:9:11: S607 Starting a process with a partial executable path + | + 9 | subprocess.check_output("true") +10 | subprocess.run("true") +11 | os.system("true") + | ^^^^^^ S607 +12 | os.popen("true") +13 | os.popen2("true") + | + +S607.py:10:10: S607 Starting a process with a partial executable path + | +10 | subprocess.run("true") +11 | os.system("true") +12 | os.popen("true") + | ^^^^^^ S607 +13 | os.popen2("true") +14 | os.popen3("true") + | + +S607.py:11:11: S607 Starting a process with a partial executable path + | +11 | os.system("true") +12 | os.popen("true") +13 | os.popen2("true") + | ^^^^^^ S607 +14 | os.popen3("true") +15 | os.popen4("true") + | + +S607.py:12:11: S607 Starting a process with a partial executable path + | +12 | os.popen("true") +13 | os.popen2("true") +14 | os.popen3("true") + | ^^^^^^ S607 +15 | os.popen4("true") +16 | popen2.popen2("true") + | + +S607.py:13:11: S607 Starting a process with a partial executable path + | +13 | os.popen2("true") +14 | os.popen3("true") +15 | os.popen4("true") + | ^^^^^^ S607 +16 | popen2.popen2("true") +17 | popen2.popen3("true") + | + +S607.py:21:10: S607 Starting a process with a partial executable path + | +21 | commands.getoutput("true") +22 | commands.getstatusoutput("true") +23 | os.execl("true") + | ^^^^^^ S607 +24 | os.execle("true") +25 | os.execlp("true") + | + +S607.py:22:11: S607 Starting a process with a partial executable path + | +22 | commands.getstatusoutput("true") +23 | os.execl("true") +24 | os.execle("true") + | ^^^^^^ S607 +25 | os.execlp("true") +26 | os.execlpe("true") + | + +S607.py:23:11: S607 Starting a process with a partial executable path + | +23 | os.execl("true") +24 | os.execle("true") +25 | os.execlp("true") + | ^^^^^^ S607 +26 | os.execlpe("true") +27 | os.execv("true") + | + +S607.py:24:12: S607 Starting a process with a partial executable path + | +24 | os.execle("true") +25 | os.execlp("true") +26 | os.execlpe("true") + | ^^^^^^ S607 +27 | os.execv("true") +28 | os.execve("true") + | + +S607.py:25:10: S607 Starting a process with a partial executable path + | +25 | os.execlp("true") +26 | os.execlpe("true") +27 | os.execv("true") + | ^^^^^^ S607 +28 | os.execve("true") +29 | os.execvp("true") + | + +S607.py:26:11: S607 Starting a process with a partial executable path + | +26 | os.execlpe("true") +27 | os.execv("true") +28 | os.execve("true") + | ^^^^^^ S607 +29 | os.execvp("true") +30 | os.execvpe("true") + | + +S607.py:27:11: S607 Starting a process with a partial executable path + | +27 | os.execv("true") +28 | os.execve("true") +29 | os.execvp("true") + | ^^^^^^ S607 +30 | os.execvpe("true") +31 | os.spawnl("true") + | + +S607.py:28:12: S607 Starting a process with a partial executable path + | +28 | os.execve("true") +29 | os.execvp("true") +30 | os.execvpe("true") + | ^^^^^^ S607 +31 | os.spawnl("true") +32 | os.spawnle("true") + | + +S607.py:29:11: S607 Starting a process with a partial executable path + | +29 | os.execvp("true") +30 | os.execvpe("true") +31 | os.spawnl("true") + | ^^^^^^ S607 +32 | os.spawnle("true") +33 | os.spawnlp("true") + | + +S607.py:30:12: S607 Starting a process with a partial executable path + | +30 | os.execvpe("true") +31 | os.spawnl("true") +32 | os.spawnle("true") + | ^^^^^^ S607 +33 | os.spawnlp("true") +34 | os.spawnlpe("true") + | + +S607.py:31:12: S607 Starting a process with a partial executable path + | +31 | os.spawnl("true") +32 | os.spawnle("true") +33 | os.spawnlp("true") + | ^^^^^^ S607 +34 | os.spawnlpe("true") +35 | os.spawnv("true") + | + +S607.py:32:13: S607 Starting a process with a partial executable path + | +32 | os.spawnle("true") +33 | os.spawnlp("true") +34 | os.spawnlpe("true") + | ^^^^^^ S607 +35 | os.spawnv("true") +36 | os.spawnve("true") + | + +S607.py:33:11: S607 Starting a process with a partial executable path + | +33 | os.spawnlp("true") +34 | os.spawnlpe("true") +35 | os.spawnv("true") + | ^^^^^^ S607 +36 | os.spawnve("true") +37 | os.spawnvp("true") + | + +S607.py:34:12: S607 Starting a process with a partial executable path + | +34 | os.spawnlpe("true") +35 | os.spawnv("true") +36 | os.spawnve("true") + | ^^^^^^ S607 +37 | os.spawnvp("true") +38 | os.spawnvpe("true") + | + +S607.py:35:12: S607 Starting a process with a partial executable path + | +35 | os.spawnv("true") +36 | os.spawnve("true") +37 | os.spawnvp("true") + | ^^^^^^ S607 +38 | os.spawnvpe("true") +39 | os.startfile("true") + | + +S607.py:36:13: S607 Starting a process with a partial executable path + | +36 | os.spawnve("true") +37 | os.spawnvp("true") +38 | os.spawnvpe("true") + | ^^^^^^ S607 +39 | os.startfile("true") + | + +S607.py:37:14: S607 Starting a process with a partial executable path + | +37 | os.spawnvp("true") +38 | os.spawnvpe("true") +39 | os.startfile("true") + | ^^^^^^ S607 +40 | +41 | # Check it does not fail for full paths + | + + From 1bda613281843d3d618601d66a65b63497dbe3fe Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 13 Apr 2023 14:14:07 -0400 Subject: [PATCH 7/8] Untweak some of those messages --- .../test/fixtures/flake8_bandit/S602.py | 6 +- .../test/fixtures/flake8_bandit/S603.py | 8 +- .../test/fixtures/flake8_bandit/S605.py | 4 +- .../test/fixtures/flake8_bandit/S606.py | 2 +- .../test/fixtures/flake8_bandit/S607.py | 4 +- .../flake8_bandit/rules/shell_injection.rs | 86 ++++++++++--------- ...s__flake8_bandit__tests__S602_S602.py.snap | 20 ++--- ...s__flake8_bandit__tests__S603_S603.py.snap | 18 ++-- ...s__flake8_bandit__tests__S604_S604.py.snap | 2 +- ...s__flake8_bandit__tests__S605_S605.py.snap | 6 +- ...s__flake8_bandit__tests__S606_S606.py.snap | 4 +- ...s__flake8_bandit__tests__S607_S607.py.snap | 2 +- 12 files changed, 83 insertions(+), 79 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_bandit/S602.py b/crates/ruff/resources/test/fixtures/flake8_bandit/S602.py index 679a2ae667baba..ef0626986e601c 100644 --- a/crates/ruff/resources/test/fixtures/flake8_bandit/S602.py +++ b/crates/ruff/resources/test/fixtures/flake8_bandit/S602.py @@ -1,19 +1,19 @@ from subprocess import Popen, call, check_call, check_output, run -# Check different Popen wrappers are checked +# 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 +# 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 +# Check command argument looks unsafe. var_string = "true" Popen(var_string, shell=True) Popen([var_string], shell=True) diff --git a/crates/ruff/resources/test/fixtures/flake8_bandit/S603.py b/crates/ruff/resources/test/fixtures/flake8_bandit/S603.py index 22c188596d04b6..da9b2dae3f0cf5 100644 --- a/crates/ruff/resources/test/fixtures/flake8_bandit/S603.py +++ b/crates/ruff/resources/test/fixtures/flake8_bandit/S603.py @@ -1,20 +1,20 @@ from subprocess import Popen, call, check_call, check_output, run -# Different Popen wrappers are checked +# 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 +# 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 +# Unknown values are treated as falsey. Popen("true", shell=True if True else False) -# No value is also caught +# No value is also caught. Popen("true") diff --git a/crates/ruff/resources/test/fixtures/flake8_bandit/S605.py b/crates/ruff/resources/test/fixtures/flake8_bandit/S605.py index 51c47e597f24b2..de9499ec54dc27 100644 --- a/crates/ruff/resources/test/fixtures/flake8_bandit/S605.py +++ b/crates/ruff/resources/test/fixtures/flake8_bandit/S605.py @@ -3,7 +3,7 @@ import commands import popen2 -# Check all shell functions +# Check all shell functions. os.system("true") os.popen("true") os.popen2("true") @@ -18,7 +18,7 @@ commands.getstatusoutput("true") -# Check command argument looks unsafe +# Check command argument looks unsafe. var_string = "true" os.system(var_string) os.system([var_string]) diff --git a/crates/ruff/resources/test/fixtures/flake8_bandit/S606.py b/crates/ruff/resources/test/fixtures/flake8_bandit/S606.py index 5551e8bfdc4fe8..e6c4bfe17d42e7 100644 --- a/crates/ruff/resources/test/fixtures/flake8_bandit/S606.py +++ b/crates/ruff/resources/test/fixtures/flake8_bandit/S606.py @@ -1,6 +1,6 @@ import os -# Check all shell functions +# Check all shell functions. os.execl("true") os.execle("true") os.execlp("true") diff --git a/crates/ruff/resources/test/fixtures/flake8_bandit/S607.py b/crates/ruff/resources/test/fixtures/flake8_bandit/S607.py index be70c2ad05748a..0bcb8cae0ab552 100644 --- a/crates/ruff/resources/test/fixtures/flake8_bandit/S607.py +++ b/crates/ruff/resources/test/fixtures/flake8_bandit/S607.py @@ -1,6 +1,6 @@ import os -# Check all functions +# Check all functions. subprocess.Popen("true") subprocess.call("true") subprocess.check_call("true") @@ -36,7 +36,7 @@ os.spawnvpe("true") os.startfile("true") -# Check it does not fail for full paths +# Check it does not fail for full paths. os.system("/bin/ls") os.system("./bin/ls") os.system(["/bin/ls"]) 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 19828b681b0ac5..d14184b94cc02e 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs @@ -1,18 +1,19 @@ -//! Checks relating to shell injection +//! Checks relating to shell injection. use num_bigint::BigInt; use once_cell::sync::Lazy; use regex::Regex; +use rustpython_parser::ast::{Constant, Expr, ExprKind, Keyword}; + use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::types::Range; -use rustpython_parser::ast::{Constant, Expr, ExprKind, Keyword}; use crate::{ checkers::ast::Checker, registry::Rule, rules::flake8_bandit::helpers::string_literal, }; -static FULL_PATH_REGEX: Lazy = Lazy::new(|| Regex::new(r"^([A-Za-z]:|[\\/\.])").unwrap()); +static FULL_PATH_REGEX: Lazy = Lazy::new(|| Regex::new(r"^([A-Za-z]:|[\\/.])").unwrap()); #[violation] pub struct SubprocessPopenWithShellEqualsTrue { @@ -27,7 +28,7 @@ impl Violation for SubprocessPopenWithShellEqualsTrue { "`subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell`" ) } else { - format!("`subprocess` call with `shell=True` identified") + format!("`subprocess` call with `shell=True` identified, security issue") } } } @@ -48,7 +49,7 @@ pub struct CallWithShellEqualsTrue; impl Violation for CallWithShellEqualsTrue { #[derive_message_formats] fn message(&self) -> String { - format!("Function call with `shell=True` parameter identified") + format!("Function call with `shell=True` parameter identified, security issue") } } @@ -187,7 +188,13 @@ impl From<&Keyword> for Truthiness { Truthiness::Truthy } } - ExprKind::List { elts, .. } => { + ExprKind::Constant { + value: Constant::None, + .. + } => Truthiness::Falsey, + ExprKind::List { elts, .. } + | ExprKind::Set { elts, .. } + | ExprKind::Tuple { elts, .. } => { if elts.is_empty() { Truthiness::Falsey } else { @@ -201,13 +208,6 @@ impl From<&Keyword> for Truthiness { Truthiness::Truthy } } - ExprKind::Tuple { elts, .. } => { - if elts.is_empty() { - Truthiness::Falsey - } else { - Truthiness::Truthy - } - } _ => Truthiness::Unknown, } } @@ -289,7 +289,7 @@ pub fn shell_injection(checker: &mut Checker, func: &Expr, args: &[Expr], keywor let call_kind = get_call_kind(checker, func); if let Some(CallKind::Subprocess) = call_kind { - if !args.is_empty() { + if let Some(arg) = args.first() { match find_shell_keyword(keywords) { // S602 Some(ShellKeyword { @@ -303,7 +303,7 @@ pub fn shell_injection(checker: &mut Checker, func: &Expr, args: &[Expr], keywor { checker.diagnostics.push(Diagnostic::new( SubprocessPopenWithShellEqualsTrue { - seems_safe: shell_call_seems_safe(&args[0]), + seems_safe: shell_call_seems_safe(arg), }, Range::from(keyword), )); @@ -320,7 +320,7 @@ pub fn shell_injection(checker: &mut Checker, func: &Expr, args: &[Expr], keywor .enabled(Rule::SubprocessWithoutShellEqualsTrue) { checker.diagnostics.push(Diagnostic::new( - SubprocessWithoutShellEqualsTrue {}, + SubprocessWithoutShellEqualsTrue, Range::from(keyword), )); } @@ -333,8 +333,8 @@ pub fn shell_injection(checker: &mut Checker, func: &Expr, args: &[Expr], keywor .enabled(Rule::SubprocessWithoutShellEqualsTrue) { checker.diagnostics.push(Diagnostic::new( - SubprocessWithoutShellEqualsTrue {}, - Range::from(&args[0]), + SubprocessWithoutShellEqualsTrue, + Range::from(arg), )); } } @@ -352,7 +352,7 @@ pub fn shell_injection(checker: &mut Checker, func: &Expr, args: &[Expr], keywor .enabled(Rule::CallWithShellEqualsTrue) { checker.diagnostics.push(Diagnostic::new( - CallWithShellEqualsTrue {}, + CallWithShellEqualsTrue, Range::from(keyword), )); } @@ -360,13 +360,15 @@ pub fn shell_injection(checker: &mut Checker, func: &Expr, args: &[Expr], keywor // S605 if let Some(CallKind::Shell) = call_kind { - if !args.is_empty() && checker.settings.rules.enabled(Rule::StartProcessWithAShell) { - checker.diagnostics.push(Diagnostic::new( - StartProcessWithAShell { - seems_safe: shell_call_seems_safe(&args[0]), - }, - Range::from(&args[0]), - )); + if let Some(arg) = args.first() { + if checker.settings.rules.enabled(Rule::StartProcessWithAShell) { + checker.diagnostics.push(Diagnostic::new( + StartProcessWithAShell { + seems_safe: shell_call_seems_safe(arg), + }, + Range::from(arg), + )); + } } } @@ -377,26 +379,28 @@ pub fn shell_injection(checker: &mut Checker, func: &Expr, args: &[Expr], keywor .rules .enabled(Rule::StartProcessWithNoShell) { - checker.diagnostics.push(Diagnostic::new( - StartProcessWithNoShell {}, - Range::from(func), - )); + checker + .diagnostics + .push(Diagnostic::new(StartProcessWithNoShell, Range::from(func))); } } // S607 - if call_kind.is_some() && !args.is_empty() { - if let Some(value) = string_literal_including_list(&args[0]) { - if FULL_PATH_REGEX.find(value).is_none() - && checker - .settings - .rules - .enabled(Rule::StartProcessWithPartialPath) + if call_kind.is_some() { + if let Some(arg) = args.first() { + if checker + .settings + .rules + .enabled(Rule::StartProcessWithPartialPath) { - checker.diagnostics.push(Diagnostic::new( - StartProcessWithPartialPath {}, - Range::from(&args[0]), - )); + if let Some(value) = string_literal_including_list(arg) { + if FULL_PATH_REGEX.find(value).is_none() { + checker.diagnostics.push(Diagnostic::new( + StartProcessWithPartialPath, + Range::from(arg), + )); + } + } } } } diff --git a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S602_S602.py.snap b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S602_S602.py.snap index f22d157d159cda..3f8121ff8983e8 100644 --- a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S602_S602.py.snap +++ b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S602_S602.py.snap @@ -3,7 +3,7 @@ source: crates/ruff/src/rules/flake8_bandit/mod.rs --- S602.py:4:15: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell` | -4 | # Check different Popen wrappers are checked +4 | # Check different Popen wrappers are checked. 5 | Popen("true", shell=True) | ^^^^^^^^^^ S602 6 | call("true", shell=True) @@ -12,7 +12,7 @@ S602.py:4:15: S602 `subprocess` call with `shell=True` seems safe, but may be ch S602.py:5:14: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell` | -5 | # Check different Popen wrappers are checked +5 | # Check different Popen wrappers are checked. 6 | Popen("true", shell=True) 7 | call("true", shell=True) | ^^^^^^^^^^ S602 @@ -46,12 +46,12 @@ S602.py:8:13: S602 `subprocess` call with `shell=True` seems safe, but may be ch 10 | run("true", shell=True) | ^^^^^^^^^^ S602 11 | -12 | # Check values that truthy values are treated as true +12 | # Check values that truthy values are treated as true. | S602.py:11:15: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell` | -11 | # Check values that truthy values are treated as true +11 | # Check values that truthy values are treated as true. 12 | Popen("true", shell=1) | ^^^^^^^ S602 13 | Popen("true", shell=[1]) @@ -60,7 +60,7 @@ S602.py:11:15: S602 `subprocess` call with `shell=True` seems safe, but may be c S602.py:12:15: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell` | -12 | # Check values that truthy values are treated as true +12 | # Check values that truthy values are treated as true. 13 | Popen("true", shell=1) 14 | Popen("true", shell=[1]) | ^^^^^^^^^ S602 @@ -84,12 +84,12 @@ S602.py:14:15: S602 `subprocess` call with `shell=True` seems safe, but may be c 16 | Popen("true", shell=(1,)) | ^^^^^^^^^^ S602 17 | -18 | # Check command argument looks unsafe +18 | # Check command argument looks unsafe. | -S602.py:18:19: S602 `subprocess` call with `shell=True` identified +S602.py:18:19: S602 `subprocess` call with `shell=True` identified, security issue | -18 | # Check command argument looks unsafe +18 | # Check command argument looks unsafe. 19 | var_string = "true" 20 | Popen(var_string, shell=True) | ^^^^^^^^^^ S602 @@ -97,7 +97,7 @@ S602.py:18:19: S602 `subprocess` call with `shell=True` identified 22 | Popen([var_string, ""], shell=True) | -S602.py:19:21: S602 `subprocess` call with `shell=True` identified +S602.py:19:21: S602 `subprocess` call with `shell=True` identified, security issue | 19 | var_string = "true" 20 | Popen(var_string, shell=True) @@ -106,7 +106,7 @@ S602.py:19:21: S602 `subprocess` call with `shell=True` identified 22 | Popen([var_string, ""], shell=True) | -S602.py:20:25: S602 `subprocess` call with `shell=True` identified +S602.py:20:25: S602 `subprocess` call with `shell=True` identified, security issue | 20 | Popen(var_string, shell=True) 21 | Popen([var_string], shell=True) diff --git a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S603_S603.py.snap b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S603_S603.py.snap index 257d880ed4c554..a258112aa67101 100644 --- a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S603_S603.py.snap +++ b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S603_S603.py.snap @@ -3,7 +3,7 @@ source: crates/ruff/src/rules/flake8_bandit/mod.rs --- S603.py:4:15: S603 `subprocess` call: check for execution of untrusted input | -4 | # Different Popen wrappers are checked +4 | # Different Popen wrappers are checked. 5 | Popen("true", shell=False) | ^^^^^^^^^^^ S603 6 | call("true", shell=False) @@ -12,7 +12,7 @@ S603.py:4:15: S603 `subprocess` call: check for execution of untrusted input S603.py:5:14: S603 `subprocess` call: check for execution of untrusted input | -5 | # Different Popen wrappers are checked +5 | # Different Popen wrappers are checked. 6 | Popen("true", shell=False) 7 | call("true", shell=False) | ^^^^^^^^^^^ S603 @@ -46,12 +46,12 @@ S603.py:8:13: S603 `subprocess` call: check for execution of untrusted input 10 | run("true", shell=False) | ^^^^^^^^^^^ S603 11 | -12 | # Values that falsey values are treated as false +12 | # Values that falsey values are treated as false. | S603.py:11:15: S603 `subprocess` call: check for execution of untrusted input | -11 | # Values that falsey values are treated as false +11 | # Values that falsey values are treated as false. 12 | Popen("true", shell=0) | ^^^^^^^ S603 13 | Popen("true", shell=[]) @@ -60,7 +60,7 @@ S603.py:11:15: S603 `subprocess` call: check for execution of untrusted input S603.py:12:15: S603 `subprocess` call: check for execution of untrusted input | -12 | # Values that falsey values are treated as false +12 | # Values that falsey values are treated as false. 13 | Popen("true", shell=0) 14 | Popen("true", shell=[]) | ^^^^^^^^ S603 @@ -84,21 +84,21 @@ S603.py:14:15: S603 `subprocess` call: check for execution of untrusted input 16 | Popen("true", shell=None) | ^^^^^^^^^^ S603 17 | -18 | # Unknown values are treated as falsey +18 | # Unknown values are treated as falsey. | S603.py:17:15: S603 `subprocess` call: check for execution of untrusted input | -17 | # Unknown values are treated as falsey +17 | # Unknown values are treated as falsey. 18 | Popen("true", shell=True if True else False) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S603 19 | -20 | # No value is also caught +20 | # No value is also caught. | S603.py:20:7: S603 `subprocess` call: check for execution of untrusted input | -20 | # No value is also caught +20 | # No value is also caught. 21 | Popen("true") | ^^^^^^ S603 | diff --git a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S604_S604.py.snap b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S604_S604.py.snap index f4bf0f4842ba2b..83e9248dfbae20 100644 --- a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S604_S604.py.snap +++ b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S604_S604.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff/src/rules/flake8_bandit/mod.rs --- -S604.py:5:5: S604 Function call with `shell=True` parameter identified +S604.py:5:5: S604 Function call with `shell=True` parameter identified, security issue | 5 | foo(shell=True) | ^^^^^^^^^^ S604 diff --git a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S605_S605.py.snap b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S605_S605.py.snap index d4f3a91e3a11f5..b0b3e45a3d84d0 100644 --- a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S605_S605.py.snap +++ b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S605_S605.py.snap @@ -3,7 +3,7 @@ source: crates/ruff/src/rules/flake8_bandit/mod.rs --- S605.py:7:11: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` | - 7 | # Check all shell functions + 7 | # Check all shell functions. 8 | os.system("true") | ^^^^^^ S605 9 | os.popen("true") @@ -12,7 +12,7 @@ S605.py:7:11: S605 Starting a process with a shell: seems safe, but may be chang S605.py:8:10: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` | - 8 | # Check all shell functions + 8 | # Check all shell functions. 9 | os.system("true") 10 | os.popen("true") | ^^^^^^ S605 @@ -119,7 +119,7 @@ S605.py:18:26: S605 Starting a process with a shell: seems safe, but may be chan S605.py:23:11: S605 Starting a process with a shell, possible injection detected | -23 | # Check command argument looks unsafe +23 | # Check command argument looks unsafe. 24 | var_string = "true" 25 | os.system(var_string) | ^^^^^^^^^^ S605 diff --git a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S606_S606.py.snap b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S606_S606.py.snap index 724199075f7cb6..f237bfd6b4d69f 100644 --- a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S606_S606.py.snap +++ b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S606_S606.py.snap @@ -3,7 +3,7 @@ source: crates/ruff/src/rules/flake8_bandit/mod.rs --- S606.py:4:1: S606 Starting a process without a shell | -4 | # Check all shell functions +4 | # Check all shell functions. 5 | os.execl("true") | ^^^^^^^^ S606 6 | os.execle("true") @@ -12,7 +12,7 @@ S606.py:4:1: S606 Starting a process without a shell S606.py:5:1: S606 Starting a process without a shell | -5 | # Check all shell functions +5 | # Check all shell functions. 6 | os.execl("true") 7 | os.execle("true") | ^^^^^^^^^ S606 diff --git a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S607_S607.py.snap b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S607_S607.py.snap index 05071f947c3e96..fde8ded402fa27 100644 --- a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S607_S607.py.snap +++ b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S607_S607.py.snap @@ -217,7 +217,7 @@ S607.py:37:14: S607 Starting a process with a partial executable path 39 | os.startfile("true") | ^^^^^^ S607 40 | -41 | # Check it does not fail for full paths +41 | # Check it does not fail for full paths. | From 1a79d2680b849d4f511bcb6d37345dbce0207def Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 13 Apr 2023 14:35:39 -0400 Subject: [PATCH 8/8] Use match --- .../flake8_bandit/rules/shell_injection.rs | 133 +++++++----------- 1 file changed, 49 insertions(+), 84 deletions(-) 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 d14184b94cc02e..618de28e86bf01 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs @@ -8,6 +8,7 @@ use rustpython_parser::ast::{Constant, Expr, ExprKind, Keyword}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::types::Range; +use ruff_python_semantic::context::Context; use crate::{ checkers::ast::Checker, registry::Rule, rules::flake8_bandit::helpers::string_literal, @@ -96,55 +97,39 @@ enum CallKind { NoShell, } -struct Config<'a> { - subprocess: Vec>, - shell: Vec>, - no_shell: Vec>, +/// Return the [`CallKind`] of the given function call. +fn get_call_kind(func: &Expr, context: &Context) -> Option { + context + .resolve_call_path(func) + .and_then(|call_path| match call_path.as_slice() { + &[module, submodule] => match module { + "os" => match submodule { + "execl" | "execle" | "execlp" | "execlpe" | "execv" | "execve" | "execvp" + | "execvpe" | "spawnl" | "spawnle" | "spawnlp" | "spawnlpe" | "spawnv" + | "spawnve" | "spawnvp" | "spawnvpe" | "startfile" => Some(CallKind::NoShell), + "system" | "popen" | "popen2" | "popen3" | "popen4" => Some(CallKind::Shell), + _ => None, + }, + "subprocess" => match submodule { + "Popen" | "call" | "check_call" | "check_output" | "run" => { + Some(CallKind::Subprocess) + } + _ => None, + }, + "popen2" => match submodule { + "popen2" | "popen3" | "popen4" | "Popen3" | "Popen4" => Some(CallKind::Shell), + _ => None, + }, + "commands" => match submodule { + "getoutput" | "getstatusoutput" => Some(CallKind::Shell), + _ => None, + }, + _ => None, + }, + _ => None, + }) } -static CONFIG: Lazy = Lazy::new(|| Config { - subprocess: vec![ - vec!["subprocess", "Popen"], - vec!["subprocess", "call"], - vec!["subprocess", "check_call"], - vec!["subprocess", "check_output"], - vec!["subprocess", "run"], - ], - shell: vec![ - vec!["os", "system"], - vec!["os", "popen"], - vec!["os", "popen2"], - vec!["os", "popen3"], - vec!["os", "popen4"], - vec!["popen2", "popen2"], - vec!["popen2", "popen3"], - vec!["popen2", "popen4"], - vec!["popen2", "Popen3"], - vec!["popen2", "Popen4"], - vec!["commands", "getoutput"], - vec!["commands", "getstatusoutput"], - ], - no_shell: vec![ - vec!["os", "execl"], - vec!["os", "execle"], - vec!["os", "execlp"], - vec!["os", "execlpe"], - vec!["os", "execv"], - vec!["os", "execve"], - vec!["os", "execvp"], - vec!["os", "execvpe"], - vec!["os", "spawnl"], - vec!["os", "spawnle"], - vec!["os", "spawnlp"], - vec!["os", "spawnlpe"], - vec!["os", "spawnv"], - vec!["os", "spawnve"], - vec!["os", "spawnvp"], - vec!["os", "spawnvpe"], - vec!["os", "startfile"], - ], -}); - #[derive(Copy, Clone, Debug)] enum Truthiness { // The `shell` keyword argument is set and evaluates to `False`. @@ -215,10 +200,13 @@ impl From<&Keyword> for Truthiness { #[derive(Copy, Clone, Debug)] struct ShellKeyword<'a> { - has_shell: Truthiness, + /// Whether the `shell` keyword argument is set and evaluates to `True`. + truthiness: Truthiness, + /// The `shell` keyword argument. keyword: &'a Keyword, } +/// Return the `shell` keyword argument to the given function call, if any. fn find_shell_keyword(keywords: &[Keyword]) -> Option { keywords .iter() @@ -230,11 +218,13 @@ fn find_shell_keyword(keywords: &[Keyword]) -> Option { .map_or(false, |arg| arg == "shell") }) .map(|keyword| ShellKeyword { - has_shell: keyword.into(), + truthiness: keyword.into(), keyword, }) } +/// Return `true` if the value provided to the `shell` call seems safe. This is based on Bandit's +/// definition: string literals are considered okay, but dynamically-computed values are not. fn shell_call_seems_safe(arg: &Expr) -> bool { matches!( arg.node, @@ -245,33 +235,8 @@ fn shell_call_seems_safe(arg: &Expr) -> bool { ) } -fn get_call_kind(checker: &mut Checker, func: &Expr) -> Option { - checker.ctx.resolve_call_path(func).and_then(|call_path| { - if CONFIG - .subprocess - .iter() - .any(|subprocess| call_path.as_slice() == subprocess.as_slice()) - { - Some(CallKind::Subprocess) - } else if CONFIG - .shell - .iter() - .any(|shell| call_path.as_slice() == shell.as_slice()) - { - Some(CallKind::Shell) - } else if CONFIG - .no_shell - .iter() - .any(|no_shell| call_path.as_slice() == no_shell.as_slice()) - { - Some(CallKind::NoShell) - } else { - None - } - }) -} - -fn string_literal_including_list(expr: &Expr) -> Option<&str> { +/// 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.node { ExprKind::List { elts, .. } => { if elts.is_empty() { @@ -286,14 +251,14 @@ fn string_literal_including_list(expr: &Expr) -> Option<&str> { /// S602, S603, S604, S605, S606, S607 pub fn shell_injection(checker: &mut Checker, func: &Expr, args: &[Expr], keywords: &[Keyword]) { - let call_kind = get_call_kind(checker, func); + let call_kind = get_call_kind(func, &checker.ctx); - if let Some(CallKind::Subprocess) = call_kind { + if matches!(call_kind, Some(CallKind::Subprocess)) { if let Some(arg) = args.first() { match find_shell_keyword(keywords) { // S602 Some(ShellKeyword { - has_shell: Truthiness::Truthy, + truthiness: Truthiness::Truthy, keyword, }) => { if checker @@ -311,7 +276,7 @@ pub fn shell_injection(checker: &mut Checker, func: &Expr, args: &[Expr], keywor } // S603 Some(ShellKeyword { - has_shell: Truthiness::Falsey | Truthiness::Unknown, + truthiness: Truthiness::Falsey | Truthiness::Unknown, keyword, }) => { if checker @@ -341,7 +306,7 @@ pub fn shell_injection(checker: &mut Checker, func: &Expr, args: &[Expr], keywor } } } else if let Some(ShellKeyword { - has_shell: Truthiness::Truthy, + truthiness: Truthiness::Truthy, keyword, }) = find_shell_keyword(keywords) { @@ -359,7 +324,7 @@ pub fn shell_injection(checker: &mut Checker, func: &Expr, args: &[Expr], keywor } // S605 - if let Some(CallKind::Shell) = call_kind { + if matches!(call_kind, Some(CallKind::Shell)) { if let Some(arg) = args.first() { if checker.settings.rules.enabled(Rule::StartProcessWithAShell) { checker.diagnostics.push(Diagnostic::new( @@ -373,7 +338,7 @@ pub fn shell_injection(checker: &mut Checker, func: &Expr, args: &[Expr], keywor } // S606 - if let Some(CallKind::NoShell) = call_kind { + if matches!(call_kind, Some(CallKind::NoShell)) { if checker .settings .rules @@ -393,7 +358,7 @@ pub fn shell_injection(checker: &mut Checker, func: &Expr, args: &[Expr], keywor .rules .enabled(Rule::StartProcessWithPartialPath) { - if let Some(value) = string_literal_including_list(arg) { + if let Some(value) = try_string_literal(arg) { if FULL_PATH_REGEX.find(value).is_none() { checker.diagnostics.push(Diagnostic::new( StartProcessWithPartialPath,