From fbc86657a0a9338d29bdca79d8eed2c5752a9e5b Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Thu, 4 Jan 2024 17:52:22 +0100 Subject: [PATCH 1/7] Add boilerplate --- .../test/fixtures/flake8_bandit/S503.py | 23 +++++++++ .../src/checkers/ast/analyze/expression.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 | 47 +++++++++++++++++++ ruff.schema.json | 1 + 7 files changed, 78 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 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..c8e1f8c12212c --- /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_1_METHOD): # OK + pass diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 8cc25e8128b9e..913503a10f3f7 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -968,6 +968,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::SslWithNoVersion) { flake8_bandit::rules::ssl_with_no_version(checker, call); } + if checker.enabled(Rule::SslWithBadDefaults) { + flake8_bandit::rules::ssl_with_bad_defaults(checker, call); + } } Expr::Dict(dict) => { if checker.any_enabled(&[ diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 8b735b18e70ca..62ce1caae4658 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -642,6 +642,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Bandit, "413") => (RuleGroup::Preview, rules::flake8_bandit::rules::SuspiciousPycryptoImport), (Flake8Bandit, "415") => (RuleGroup::Preview, rules::flake8_bandit::rules::SuspiciousPyghmiImport), (Flake8Bandit, "501") => (RuleGroup::Stable, rules::flake8_bandit::rules::RequestWithNoCertValidation), + (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 5976d02af5f87..55286ae4d83fe 100644 --- a/crates/ruff_linter/src/rules/flake8_bandit/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_bandit/mod.rs @@ -36,6 +36,7 @@ mod tests { #[test_case(Rule::SSHNoHostKeyVerification, Path::new("S507.py"))] #[test_case(Rule::SnmpInsecureVersion, Path::new("S508.py"))] #[test_case(Rule::SnmpWeakCryptography, Path::new("S509.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 672e5f6e48f45..65ea967a3c05e 100644 --- a/crates/ruff_linter/src/rules/flake8_bandit/rules/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_bandit/rules/mod.rs @@ -20,6 +20,7 @@ pub(crate) use shell_injection::*; pub(crate) use snmp_insecure_version::*; pub(crate) use snmp_weak_cryptography::*; pub(crate) use ssh_no_host_key_verification::*; +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::*; @@ -51,6 +52,7 @@ mod shell_injection; mod snmp_insecure_version; mod snmp_weak_cryptography; mod ssh_no_host_key_verification; +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..3b9c2c592ec48 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_bandit/rules/ssl_with_bad_defaults.rs @@ -0,0 +1,47 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::ExprCall; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for function definitions which have default arguments set to broken SSL/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. It is strongly recommended that use of the following known broken +/// protocol versions 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 { + format!("Argument default set to insecure SSL protocol: {}", self.protocol) + } +} + +/// S503 +pub(crate) fn ssl_with_bad_defaults(checker: &mut Checker, call: &ExprCall) {} diff --git a/ruff.schema.json b/ruff.schema.json index c747417ed2656..6ebf1aa2acb4c 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3512,6 +3512,7 @@ "S5", "S50", "S501", + "S503", "S504", "S505", "S506", From 011ed1c4128656ee626e00e3004d1f5e3305f7fd Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Thu, 4 Jan 2024 17:53:55 +0100 Subject: [PATCH 2/7] Move check to right place, fix signature --- crates/ruff_linter/src/checkers/ast/analyze/expression.rs | 3 --- crates/ruff_linter/src/checkers/ast/analyze/statement.rs | 3 +++ .../src/rules/flake8_bandit/rules/ssl_with_bad_defaults.rs | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 913503a10f3f7..8cc25e8128b9e 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -968,9 +968,6 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::SslWithNoVersion) { flake8_bandit::rules::ssl_with_no_version(checker, call); } - if checker.enabled(Rule::SslWithBadDefaults) { - flake8_bandit::rules::ssl_with_bad_defaults(checker, call); - } } Expr::Dict(dict) => { if checker.any_enabled(&[ diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 4c5360beb250e..80ddbbe88c30a 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/rules/flake8_bandit/rules/ssl_with_bad_defaults.rs b/crates/ruff_linter/src/rules/flake8_bandit/rules/ssl_with_bad_defaults.rs index 3b9c2c592ec48..815016d4edfa4 100644 --- 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 @@ -1,6 +1,6 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::ExprCall; +use ruff_python_ast::{ExprCall, StmtFunctionDef}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -44,4 +44,4 @@ impl Violation for SslWithBadDefaults { } /// S503 -pub(crate) fn ssl_with_bad_defaults(checker: &mut Checker, call: &ExprCall) {} +pub(crate) fn ssl_with_bad_defaults(checker: &mut Checker, call: &StmtFunctionDef) {} From 06871df09aec55a2b53c0dff1c6aa46495aff68a Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Thu, 4 Jan 2024 18:15:28 +0100 Subject: [PATCH 3/7] Implement rule logic, generate snapshots --- .../test/fixtures/flake8_bandit/S503.py | 2 +- .../src/checkers/ast/analyze/statement.rs | 2 +- .../rules/ssl_with_bad_defaults.rs | 66 +++++++++++++++++-- ...s__flake8_bandit__tests__S503_S503.py.snap | 32 +++++++++ 4 files changed, 96 insertions(+), 6 deletions(-) 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 index c8e1f8c12212c..a2f653ffe144b 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S503.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S503.py @@ -19,5 +19,5 @@ def func(protocol=PROTOCOL_TLSv1): # S503 pass -def func(version=SSL.TLSv1_1_METHOD): # OK +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 80ddbbe88c30a..0165e431bc422 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -375,7 +375,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { refurb::rules::reimplemented_operator(checker, &function_def.into()); } if checker.enabled(Rule::SslWithBadDefaults) { - flake8_bandit::rules::ssl_with_bad_defaults(checker, function_def); + flake8_bandit::rules::ssl_with_bad_defaults(checker, function_def); } } Stmt::Return(_) => { 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 index 815016d4edfa4..e37e879763c86 100644 --- 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 @@ -1,6 +1,6 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::{ExprCall, StmtFunctionDef}; +use ruff_python_ast::{self as ast, Expr, StmtFunctionDef}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -33,15 +33,73 @@ use crate::checkers::ast::Checker; /// ``` #[violation] pub struct SslWithBadDefaults { - protocol: String + protocol: String, } impl Violation for SslWithBadDefaults { #[derive_message_formats] fn message(&self) -> String { - format!("Argument default set to insecure SSL protocol: {}", self.protocol) + format!( + "Argument default set to insecure SSL protocol: {}", + self.protocol + ) } } +const INSECURE_SSL_PROTOCOLS: &[&str] = &[ + "PROTOCOL_SSLv2", + "PROTOCOL_SSLv3", + "PROTOCOL_TLSv1", + "PROTOCOL_TLSv1_1", + "SSLv2_METHOD", + "SSLv23_METHOD", + "SSLv3_METHOD", + "TLSv1_METHOD", + "TLSv1_1_METHOD", +]; + /// S503 -pub(crate) fn ssl_with_bad_defaults(checker: &mut Checker, call: &StmtFunctionDef) {} +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 { + check_default_arg_for_insecure_ssl_violation(default, checker); + } + }); +} + +fn check_default_arg_for_insecure_ssl_violation(expr: &Expr, checker: &mut Checker) { + match expr { + Expr::Name(ast::ExprName { id, .. }) => { + if INSECURE_SSL_PROTOCOLS.contains(&id.as_str()) { + checker.diagnostics.push(Diagnostic::new( + SslWithBadDefaults { + protocol: id.to_string(), + }, + expr.range(), + )); + } + } + Expr::Attribute(ast::ExprAttribute { attr, .. }) => { + if INSECURE_SSL_PROTOCOLS.contains(&attr.as_str()) { + checker.diagnostics.push(Diagnostic::new( + SslWithBadDefaults { + protocol: attr.to_string(), + }, + expr.range(), + )); + } + } + _ => {} + } +} 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..bdcc759227eab --- /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 + | + + From 903cc0094c73c6a2a8f82d9a1ce34663f47ae62b Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Thu, 4 Jan 2024 18:21:21 +0100 Subject: [PATCH 4/7] Fix docs --- .../src/rules/flake8_bandit/rules/ssl_with_bad_defaults.rs | 2 ++ 1 file changed, 2 insertions(+) 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 index e37e879763c86..1bc6cbfe61b8f 100644 --- 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 @@ -23,6 +23,7 @@ use crate::checkers::ast::Checker; /// import ssl /// /// def func(version=ssl.PROTOCOL_TLSv1): +/// ... /// ``` /// /// Use instead: @@ -30,6 +31,7 @@ use crate::checkers::ast::Checker; /// import ssl /// /// def func(version=ssl.PROTOCOL_TLSv1_2): +/// ... /// ``` #[violation] pub struct SslWithBadDefaults { From cdb5244bf7e8fd4143619bbba3b7ab7f96e2d875 Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Thu, 4 Jan 2024 18:32:41 +0100 Subject: [PATCH 5/7] Refactor diagnostic push --- .../rules/ssl_with_bad_defaults.rs | 47 +++++++++---------- 1 file changed, 22 insertions(+), 25 deletions(-) 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 index 1bc6cbfe61b8f..27272d2b8c02f 100644 --- 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 @@ -1,7 +1,7 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::{self as ast, Expr, StmtFunctionDef}; -use ruff_text_size::Ranged; +use ruff_text_size::TextRange; use crate::checkers::ast::Checker; @@ -75,33 +75,30 @@ pub(crate) fn ssl_with_bad_defaults(checker: &mut Checker, function_def: &StmtFu ) .for_each(|param| { if let Some(default) = ¶m.default { - check_default_arg_for_insecure_ssl_violation(default, checker); + match default.as_ref() { + Expr::Name(ast::ExprName { id, range, .. }) => { + check_default_arg_for_insecure_ssl_violation(id.as_str(), checker, range) + } + Expr::Attribute(ast::ExprAttribute { attr, range, .. }) => { + check_default_arg_for_insecure_ssl_violation(attr.as_str(), checker, range) + } + _ => {} + } } }); } -fn check_default_arg_for_insecure_ssl_violation(expr: &Expr, checker: &mut Checker) { - match expr { - Expr::Name(ast::ExprName { id, .. }) => { - if INSECURE_SSL_PROTOCOLS.contains(&id.as_str()) { - checker.diagnostics.push(Diagnostic::new( - SslWithBadDefaults { - protocol: id.to_string(), - }, - expr.range(), - )); - } - } - Expr::Attribute(ast::ExprAttribute { attr, .. }) => { - if INSECURE_SSL_PROTOCOLS.contains(&attr.as_str()) { - checker.diagnostics.push(Diagnostic::new( - SslWithBadDefaults { - protocol: attr.to_string(), - }, - expr.range(), - )); - } - } - _ => {} +fn check_default_arg_for_insecure_ssl_violation( + id: &str, + checker: &mut Checker, + range: &TextRange, +) { + if INSECURE_SSL_PROTOCOLS.contains(&id) { + checker.diagnostics.push(Diagnostic::new( + SslWithBadDefaults { + protocol: id.to_string(), + }, + *range, + )); } } From 490d4e318d58a6187ab1b3fd24236ea82f9c5a50 Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Thu, 4 Jan 2024 18:37:39 +0100 Subject: [PATCH 6/7] Fix docs some more, clippy --- .../rules/ssl_with_bad_defaults.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) 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 index 27272d2b8c02f..9e03ecefeb079 100644 --- 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 @@ -22,6 +22,7 @@ use crate::checkers::ast::Checker; /// ```python /// import ssl /// +/// /// def func(version=ssl.PROTOCOL_TLSv1): /// ... /// ``` @@ -30,6 +31,7 @@ use crate::checkers::ast::Checker; /// ```python /// import ssl /// +/// /// def func(version=ssl.PROTOCOL_TLSv1_2): /// ... /// ``` @@ -77,10 +79,14 @@ pub(crate) fn ssl_with_bad_defaults(checker: &mut Checker, function_def: &StmtFu if let Some(default) = ¶m.default { match default.as_ref() { Expr::Name(ast::ExprName { id, range, .. }) => { - check_default_arg_for_insecure_ssl_violation(id.as_str(), checker, range) + check_default_arg_for_insecure_ssl_violation(id.as_str(), checker, *range); } Expr::Attribute(ast::ExprAttribute { attr, range, .. }) => { - check_default_arg_for_insecure_ssl_violation(attr.as_str(), checker, range) + check_default_arg_for_insecure_ssl_violation( + attr.as_str(), + checker, + *range, + ); } _ => {} } @@ -88,17 +94,13 @@ pub(crate) fn ssl_with_bad_defaults(checker: &mut Checker, function_def: &StmtFu }); } -fn check_default_arg_for_insecure_ssl_violation( - id: &str, - checker: &mut Checker, - range: &TextRange, -) { +fn check_default_arg_for_insecure_ssl_violation(id: &str, checker: &mut Checker, range: TextRange) { if INSECURE_SSL_PROTOCOLS.contains(&id) { checker.diagnostics.push(Diagnostic::new( SslWithBadDefaults { protocol: id.to_string(), }, - *range, + range, )); } } From e832dd735a80f3332dabfff77b2bebc83ee2a9b3 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 4 Jan 2024 20:32:26 -0500 Subject: [PATCH 7/7] Tweaks --- .../rules/ssl_with_bad_defaults.rs | 82 +++++++++---------- ...s__flake8_bandit__tests__S503_S503.py.snap | 8 +- 2 files changed, 45 insertions(+), 45 deletions(-) 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 index 9e03ecefeb079..67f4033e6022b 100644 --- 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 @@ -1,22 +1,21 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::{self as ast, Expr, StmtFunctionDef}; -use ruff_text_size::TextRange; use crate::checkers::ast::Checker; /// ## What it does -/// Checks for function definitions which have default arguments set to broken SSL/TLS -/// protocol versions. +/// 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. It is strongly recommended that use of the following known broken -/// protocol versions be avoided: -/// - SSL v2 -/// - SSL v3 -/// - TLS v1 -/// - TLS v1.1 +/// 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 @@ -43,25 +42,11 @@ pub struct SslWithBadDefaults { impl Violation for SslWithBadDefaults { #[derive_message_formats] fn message(&self) -> String { - format!( - "Argument default set to insecure SSL protocol: {}", - self.protocol - ) + let SslWithBadDefaults { protocol } = self; + format!("Argument default set to insecure SSL protocol: `{protocol}`") } } -const INSECURE_SSL_PROTOCOLS: &[&str] = &[ - "PROTOCOL_SSLv2", - "PROTOCOL_SSLv3", - "PROTOCOL_TLSv1", - "PROTOCOL_TLSv1_1", - "SSLv2_METHOD", - "SSLv23_METHOD", - "SSLv3_METHOD", - "TLSv1_METHOD", - "TLSv1_1_METHOD", -]; - /// S503 pub(crate) fn ssl_with_bad_defaults(checker: &mut Checker, function_def: &StmtFunctionDef) { function_def @@ -79,14 +64,24 @@ pub(crate) fn ssl_with_bad_defaults(checker: &mut Checker, function_def: &StmtFu if let Some(default) = ¶m.default { match default.as_ref() { Expr::Name(ast::ExprName { id, range, .. }) => { - check_default_arg_for_insecure_ssl_violation(id.as_str(), checker, *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, .. }) => { - check_default_arg_for_insecure_ssl_violation( - attr.as_str(), - checker, - *range, - ); + if is_insecure_protocol(attr.as_str()) { + checker.diagnostics.push(Diagnostic::new( + SslWithBadDefaults { + protocol: attr.to_string(), + }, + *range, + )); + } } _ => {} } @@ -94,13 +89,18 @@ pub(crate) fn ssl_with_bad_defaults(checker: &mut Checker, function_def: &StmtFu }); } -fn check_default_arg_for_insecure_ssl_violation(id: &str, checker: &mut Checker, range: TextRange) { - if INSECURE_SSL_PROTOCOLS.contains(&id) { - checker.diagnostics.push(Diagnostic::new( - SslWithBadDefaults { - protocol: id.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 index bdcc759227eab..93932f9a6b0bf 100644 --- 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 @@ -1,28 +1,28 @@ --- source: crates/ruff_linter/src/rules/flake8_bandit/mod.rs --- -S503.py:6:18: S503 Argument default set to insecure SSL protocol: PROTOCOL_SSLv2 +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 +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 +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 +S503.py:18:19: S503 Argument default set to insecure SSL protocol: `PROTOCOL_TLSv1` | 18 | def func(protocol=PROTOCOL_TLSv1): # S503 | ^^^^^^^^^^^^^^ S503