From c11f65381fe0c9ba7e79711862223b75c888e970 Mon Sep 17 00:00:00 2001 From: qdegraaf <34540841+qdegraaf@users.noreply.github.com> Date: Fri, 5 Jan 2024 02:38:41 +0100 Subject: [PATCH] [`flake8-bandit`] Implement `S503` `SslWithBadDefaults` rule (#9391) ## Summary Adds S503 rule for the [flake8-bandit](https://github.com/tylerwince/flake8-bandit) plugin port. Checks for function defs argument defaults which have an insecure ssl_version value. See also https://bandit.readthedocs.io/en/latest/_modules/bandit/plugins/insecure_ssl_tls.html#ssl_with_bad_defaults Some logic and the `const` can be shared with https://github.com/astral-sh/ruff/pull/9390. When one of the two is merged. ## Test Plan Fixture added ## Issue Link Refers: https://github.com/astral-sh/ruff/issues/1646 --- .../test/fixtures/flake8_bandit/S503.py | 23 ++++ .../src/checkers/ast/analyze/statement.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + .../src/rules/flake8_bandit/mod.rs | 1 + .../src/rules/flake8_bandit/rules/mod.rs | 2 + .../rules/ssl_with_bad_defaults.rs | 106 ++++++++++++++++++ ...s__flake8_bandit__tests__S503_S503.py.snap | 32 ++++++ ruff.schema.json | 1 + 8 files changed, 169 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_bandit/S503.py create mode 100644 crates/ruff_linter/src/rules/flake8_bandit/rules/ssl_with_bad_defaults.rs create mode 100644 crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S503_S503.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S503.py b/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S503.py new file mode 100644 index 0000000000000..a2f653ffe144b --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S503.py @@ -0,0 +1,23 @@ +import ssl +from OpenSSL import SSL +from ssl import PROTOCOL_TLSv1 + + +def func(version=ssl.PROTOCOL_SSLv2): # S503 + pass + + +def func(protocol=SSL.SSLv2_METHOD): # S503 + pass + + +def func(version=SSL.SSLv23_METHOD): # S503 + pass + + +def func(protocol=PROTOCOL_TLSv1): # S503 + pass + + +def func(version=SSL.TLSv1_2_METHOD): # OK + pass diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 4c5360beb250e..0165e431bc422 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -374,6 +374,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::ReimplementedOperator) { refurb::rules::reimplemented_operator(checker, &function_def.into()); } + if checker.enabled(Rule::SslWithBadDefaults) { + flake8_bandit::rules::ssl_with_bad_defaults(checker, function_def); + } } Stmt::Return(_) => { if checker.enabled(Rule::ReturnOutsideFunction) { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 56e51ed65fbbf..e3322155ed184 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -643,6 +643,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Bandit, "415") => (RuleGroup::Preview, rules::flake8_bandit::rules::SuspiciousPyghmiImport), (Flake8Bandit, "501") => (RuleGroup::Stable, rules::flake8_bandit::rules::RequestWithNoCertValidation), (Flake8Bandit, "502") => (RuleGroup::Preview, rules::flake8_bandit::rules::SslInsecureVersion), + (Flake8Bandit, "503") => (RuleGroup::Preview, rules::flake8_bandit::rules::SslWithBadDefaults), (Flake8Bandit, "504") => (RuleGroup::Preview, rules::flake8_bandit::rules::SslWithNoVersion), (Flake8Bandit, "505") => (RuleGroup::Preview, rules::flake8_bandit::rules::WeakCryptographicKey), (Flake8Bandit, "506") => (RuleGroup::Stable, rules::flake8_bandit::rules::UnsafeYAMLLoad), diff --git a/crates/ruff_linter/src/rules/flake8_bandit/mod.rs b/crates/ruff_linter/src/rules/flake8_bandit/mod.rs index 0f79d2dbddb1b..f8922655313be 100644 --- a/crates/ruff_linter/src/rules/flake8_bandit/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_bandit/mod.rs @@ -37,6 +37,7 @@ mod tests { #[test_case(Rule::SnmpInsecureVersion, Path::new("S508.py"))] #[test_case(Rule::SnmpWeakCryptography, Path::new("S509.py"))] #[test_case(Rule::SslInsecureVersion, Path::new("S502.py"))] + #[test_case(Rule::SslWithBadDefaults, Path::new("S503.py"))] #[test_case(Rule::SslWithNoVersion, Path::new("S504.py"))] #[test_case(Rule::StartProcessWithAShell, Path::new("S605.py"))] #[test_case(Rule::StartProcessWithNoShell, Path::new("S606.py"))] diff --git a/crates/ruff_linter/src/rules/flake8_bandit/rules/mod.rs b/crates/ruff_linter/src/rules/flake8_bandit/rules/mod.rs index cd622042f33ee..788d512e5c5d5 100644 --- a/crates/ruff_linter/src/rules/flake8_bandit/rules/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_bandit/rules/mod.rs @@ -21,6 +21,7 @@ pub(crate) use snmp_insecure_version::*; pub(crate) use snmp_weak_cryptography::*; pub(crate) use ssh_no_host_key_verification::*; pub(crate) use ssl_insecure_version::*; +pub(crate) use ssl_with_bad_defaults::*; pub(crate) use ssl_with_no_version::*; pub(crate) use suspicious_function_call::*; pub(crate) use suspicious_imports::*; @@ -53,6 +54,7 @@ mod snmp_insecure_version; mod snmp_weak_cryptography; mod ssh_no_host_key_verification; mod ssl_insecure_version; +mod ssl_with_bad_defaults; mod ssl_with_no_version; mod suspicious_function_call; mod suspicious_imports; diff --git a/crates/ruff_linter/src/rules/flake8_bandit/rules/ssl_with_bad_defaults.rs b/crates/ruff_linter/src/rules/flake8_bandit/rules/ssl_with_bad_defaults.rs new file mode 100644 index 0000000000000..67f4033e6022b --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_bandit/rules/ssl_with_bad_defaults.rs @@ -0,0 +1,106 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{self as ast, Expr, StmtFunctionDef}; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for function definitions with default arguments set to insecure SSL +/// and TLS protocol versions. +/// +/// ## Why is this bad? +/// Several highly publicized exploitable flaws have been discovered in all +/// versions of SSL and early versions of TLS. The following versions are +/// considered insecure, and should be avoided: +/// - SSL v2 +/// - SSL v3 +/// - TLS v1 +/// - TLS v1.1 +/// +/// ## Example +/// ```python +/// import ssl +/// +/// +/// def func(version=ssl.PROTOCOL_TLSv1): +/// ... +/// ``` +/// +/// Use instead: +/// ```python +/// import ssl +/// +/// +/// def func(version=ssl.PROTOCOL_TLSv1_2): +/// ... +/// ``` +#[violation] +pub struct SslWithBadDefaults { + protocol: String, +} + +impl Violation for SslWithBadDefaults { + #[derive_message_formats] + fn message(&self) -> String { + let SslWithBadDefaults { protocol } = self; + format!("Argument default set to insecure SSL protocol: `{protocol}`") + } +} + +/// S503 +pub(crate) fn ssl_with_bad_defaults(checker: &mut Checker, function_def: &StmtFunctionDef) { + function_def + .parameters + .posonlyargs + .iter() + .chain( + function_def + .parameters + .args + .iter() + .chain(function_def.parameters.kwonlyargs.iter()), + ) + .for_each(|param| { + if let Some(default) = ¶m.default { + match default.as_ref() { + Expr::Name(ast::ExprName { id, range, .. }) => { + if is_insecure_protocol(id.as_str()) { + checker.diagnostics.push(Diagnostic::new( + SslWithBadDefaults { + protocol: id.to_string(), + }, + *range, + )); + } + } + Expr::Attribute(ast::ExprAttribute { attr, range, .. }) => { + if is_insecure_protocol(attr.as_str()) { + checker.diagnostics.push(Diagnostic::new( + SslWithBadDefaults { + protocol: attr.to_string(), + }, + *range, + )); + } + } + _ => {} + } + } + }); +} + +/// Returns `true` if the given protocol name is insecure. +fn is_insecure_protocol(name: &str) -> bool { + matches!( + name, + "PROTOCOL_SSLv2" + | "PROTOCOL_SSLv3" + | "PROTOCOL_TLSv1" + | "PROTOCOL_TLSv1_1" + | "SSLv2_METHOD" + | "SSLv23_METHOD" + | "SSLv3_METHOD" + | "TLSv1_METHOD" + | "TLSv1_1_METHOD" + ) +} diff --git a/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S503_S503.py.snap b/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S503_S503.py.snap new file mode 100644 index 0000000000000..93932f9a6b0bf --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S503_S503.py.snap @@ -0,0 +1,32 @@ +--- +source: crates/ruff_linter/src/rules/flake8_bandit/mod.rs +--- +S503.py:6:18: S503 Argument default set to insecure SSL protocol: `PROTOCOL_SSLv2` + | +6 | def func(version=ssl.PROTOCOL_SSLv2): # S503 + | ^^^^^^^^^^^^^^^^^^ S503 +7 | pass + | + +S503.py:10:19: S503 Argument default set to insecure SSL protocol: `SSLv2_METHOD` + | +10 | def func(protocol=SSL.SSLv2_METHOD): # S503 + | ^^^^^^^^^^^^^^^^ S503 +11 | pass + | + +S503.py:14:18: S503 Argument default set to insecure SSL protocol: `SSLv23_METHOD` + | +14 | def func(version=SSL.SSLv23_METHOD): # S503 + | ^^^^^^^^^^^^^^^^^ S503 +15 | pass + | + +S503.py:18:19: S503 Argument default set to insecure SSL protocol: `PROTOCOL_TLSv1` + | +18 | def func(protocol=PROTOCOL_TLSv1): # S503 + | ^^^^^^^^^^^^^^ S503 +19 | pass + | + + diff --git a/ruff.schema.json b/ruff.schema.json index 4a2f88dd35372..48e5dd16cf175 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3513,6 +3513,7 @@ "S50", "S501", "S502", + "S503", "S504", "S505", "S506",