Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement S609, linux_commands_wildcard_injection #4504

Merged
merged 7 commits into from Jun 2, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_bandit/S609.py
@@ -0,0 +1,8 @@
import os
import subprocess

os.popen("chmod +w foo*")
subprocess.Popen("/bin/chown root: *", shell=True)
subprocess.Popen(["/usr/local/bin/rsync", "*", "some_where:"], shell=True)
subprocess.Popen("/usr/local/bin/rsync * no_injection_here:")
os.system("tar cf foo.tar bar/*")
1 change: 1 addition & 0 deletions crates/ruff/src/checkers/ast/mod.rs
Expand Up @@ -2862,6 +2862,7 @@ where
Rule::StartProcessWithAShell,
Rule::StartProcessWithNoShell,
Rule::StartProcessWithPartialPath,
Rule::UnixCommandWildcardInjection,
charliermarsh marked this conversation as resolved.
Show resolved Hide resolved
]) {
flake8_bandit::rules::shell_injection(self, func, args, keywords);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/codes.rs
Expand Up @@ -514,6 +514,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8Bandit, "606") => (RuleGroup::Unspecified, Rule::StartProcessWithNoShell),
(Flake8Bandit, "607") => (RuleGroup::Unspecified, Rule::StartProcessWithPartialPath),
(Flake8Bandit, "608") => (RuleGroup::Unspecified, Rule::HardcodedSQLExpression),
(Flake8Bandit, "609") => (RuleGroup::Unspecified, Rule::UnixCommandWildcardInjection),
(Flake8Bandit, "612") => (RuleGroup::Unspecified, Rule::LoggingConfigInsecureListen),
(Flake8Bandit, "701") => (RuleGroup::Unspecified, Rule::Jinja2AutoescapeFalse),

Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/registry.rs
Expand Up @@ -433,6 +433,7 @@ ruff_macros::register_rules!(
rules::flake8_bandit::rules::StartProcessWithAShell,
rules::flake8_bandit::rules::StartProcessWithNoShell,
rules::flake8_bandit::rules::StartProcessWithPartialPath,
rules::flake8_bandit::rules::UnixCommandWildcardInjection,
rules::flake8_bandit::rules::SuspiciousEvalUsage,
rules::flake8_bandit::rules::SuspiciousFTPLibUsage,
rules::flake8_bandit::rules::SuspiciousInsecureCipherUsage,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/rules/flake8_bandit/mod.rs
Expand Up @@ -42,6 +42,7 @@ mod tests {
#[test_case(Rule::SuspiciousTelnetUsage, Path::new("S312.py"); "S312")]
#[test_case(Rule::TryExceptContinue, Path::new("S112.py"); "S112")]
#[test_case(Rule::TryExceptPass, Path::new("S110.py"); "S110")]
#[test_case(Rule::UnixCommandWildcardInjection, Path::new("S609.py"); "S609")]
#[test_case(Rule::UnsafeYAMLLoad, Path::new("S506.py"); "S506")]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff/src/rules/flake8_bandit/rules/mod.rs
Expand Up @@ -27,7 +27,7 @@ pub(crate) use request_without_timeout::{request_without_timeout, RequestWithout
pub(crate) use shell_injection::{
shell_injection, CallWithShellEqualsTrue, StartProcessWithAShell, StartProcessWithNoShell,
StartProcessWithPartialPath, SubprocessPopenWithShellEqualsTrue,
SubprocessWithoutShellEqualsTrue,
SubprocessWithoutShellEqualsTrue, UnixCommandWildcardInjection,
};
pub(crate) use snmp_insecure_version::{snmp_insecure_version, SnmpInsecureVersion};
pub(crate) use snmp_weak_cryptography::{snmp_weak_cryptography, SnmpWeakCryptography};
Expand Down
41 changes: 40 additions & 1 deletion crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs
@@ -1,5 +1,6 @@
//! Checks relating to shell injection.

use itertools::Itertools;
use once_cell::sync::Lazy;
use regex::Regex;
use rustpython_parser::ast::{self, Constant, Expr, Keyword, Ranged};
Expand All @@ -14,6 +15,8 @@ use crate::{
};

static FULL_PATH_REGEX: Lazy<Regex> = Lazy::new(|| Regex::new(r"^([A-Za-z]:|[\\/.])").unwrap());
static WILDCARD_COMMAND_REGEX: Lazy<Regex> =
Lazy::new(|| Regex::new(r"(^|[\s/])(chown|chmod|tar|rsync)\s.*\*").unwrap());

#[violation]
pub struct SubprocessPopenWithShellEqualsTrue {
Expand Down Expand Up @@ -89,6 +92,16 @@ impl Violation for StartProcessWithPartialPath {
}
}

#[violation]
pub struct UnixCommandWildcardInjection;

impl Violation for UnixCommandWildcardInjection {
#[derive_message_formats]
fn message(&self) -> String {
format!("Possible wildcard injection in call")
}
}

#[derive(Copy, Clone, Debug)]
enum CallKind {
Subprocess,
Expand Down Expand Up @@ -174,14 +187,15 @@ fn try_string_literal(expr: &Expr) -> Option<&str> {
}
}

/// S602, S603, S604, S605, S606, S607
/// S602, S603, S604, S605, S606, S607, S609
pub(crate) fn shell_injection(
checker: &mut Checker,
func: &Expr,
args: &[Expr],
keywords: &[Keyword],
) {
let call_kind = get_call_kind(func, &checker.ctx);
let mut subprocess_with_shell = false;

if matches!(call_kind, Some(CallKind::Subprocess)) {
if let Some(arg) = args.first() {
Expand All @@ -191,6 +205,7 @@ pub(crate) fn shell_injection(
truthiness: Truthiness::Truthy,
keyword,
}) => {
subprocess_with_shell = true;
if checker
.settings
.rules
Expand Down Expand Up @@ -297,4 +312,28 @@ pub(crate) fn shell_injection(
}
}
}

// S609
if checker
.settings
.rules
.enabled(Rule::UnixCommandWildcardInjection)
&& !args.is_empty()
scop marked this conversation as resolved.
Show resolved Hide resolved
&& (matches!(call_kind, Some(CallKind::Shell)) || subprocess_with_shell)
{
if let Some(cmd) = args.first() {
let cmd_string = match cmd {
Expr::List(ast::ExprList { elts, .. }) => elts
.iter()
.map(|elt| string_literal(elt).unwrap_or(""))
.join(" "),
_ => String::from(string_literal(cmd).unwrap_or("")),
scop marked this conversation as resolved.
Show resolved Hide resolved
};
if WILDCARD_COMMAND_REGEX.is_match(cmd_string.as_str()) {
checker
.diagnostics
.push(Diagnostic::new(UnixCommandWildcardInjection, func.range()));
}
}
}
}
@@ -0,0 +1,41 @@
---
source: crates/ruff/src/rules/flake8_bandit/mod.rs
---
S609.py:4:1: S609 Possible wildcard injection in call
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you copied this message from the upstream plugin? I struggle to understand the message. Which part of my code is triggering the wildcard injection? Unfortunately, I also don't know how to improve the message much without our Diagnostic's supporting an advice that is shown to explain the rule in more detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the message comes from Bandit: https://bandit.readthedocs.io/en/latest/plugins/b609_linux_commands_wildcard_injection.html

I did not bother to include the called function name in the message, but I don't think doing so would make it any clearer. Anyway I don't think it's a particularly good one either. Something like "Potentially dangerous use of wildcard in shell call" could be better, but then again I'm not sure how close to upstream Ruff wants to stick.

|
4 | import subprocess
5 |
6 | os.popen("chmod +w foo*")
| ^^^^^^^^ S609
7 | subprocess.Popen("/bin/chown root: *", shell=True)
8 | subprocess.Popen(["/usr/local/bin/rsync", "*", "some_where:"], shell=True)
|

S609.py:5:1: S609 Possible wildcard injection in call
|
5 | os.popen("chmod +w foo*")
6 | subprocess.Popen("/bin/chown root: *", shell=True)
| ^^^^^^^^^^^^^^^^ S609
7 | subprocess.Popen(["/usr/local/bin/rsync", "*", "some_where:"], shell=True)
8 | subprocess.Popen("/usr/local/bin/rsync * no_injection_here:")
|

S609.py:6:1: S609 Possible wildcard injection in call
|
6 | os.popen("chmod +w foo*")
7 | subprocess.Popen("/bin/chown root: *", shell=True)
8 | subprocess.Popen(["/usr/local/bin/rsync", "*", "some_where:"], shell=True)
| ^^^^^^^^^^^^^^^^ S609
9 | subprocess.Popen("/usr/local/bin/rsync * no_injection_here:")
10 | os.system("tar cf foo.tar bar/*")
|

S609.py:8:1: S609 Possible wildcard injection in call
|
8 | subprocess.Popen(["/usr/local/bin/rsync", "*", "some_where:"], shell=True)
9 | subprocess.Popen("/usr/local/bin/rsync * no_injection_here:")
10 | os.system("tar cf foo.tar bar/*")
| ^^^^^^^^^ S609
|


1 change: 1 addition & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.