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..278c008303d07a --- /dev/null +++ b/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs @@ -0,0 +1,381 @@ +//! 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]) { + println!("> {}", value); + 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",