From 1c0376a72d8599b49251d9dcaf8c8cd279a5b20d Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Mon, 17 Jul 2023 03:12:57 +0100 Subject: [PATCH] Add documentation to the `S5XX` rules (#5805) ## Summary Add documentation to the `S5XX` rules (the `flake8-bandit` ['cryptography'](https://bandit.readthedocs.io/en/latest/plugins/index.html#plugin-id-groupings) rule group). Related to #2646. ## Test Plan `python scripts/check_docs_formatted.py` --- crates/ruff/src/checkers/ast/mod.rs | 6 +- .../rules/request_with_no_cert_validation.rs | 39 ++++++++-- .../rules/snmp_insecure_version.rs | 45 +++++++++--- .../rules/snmp_weak_cryptography.rs | 23 ++++++ .../flake8_bandit/rules/unsafe_yaml_load.rs | 29 ++++++++ ...s__flake8_bandit__tests__S501_S501.py.snap | 72 +++++++++---------- ...s__flake8_bandit__tests__S508_S508.py.snap | 8 +-- 7 files changed, 161 insertions(+), 61 deletions(-) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index a168fcd248..33a16e339c 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -2635,15 +2635,13 @@ where flake8_bandit::rules::bad_file_permissions(self, func, args, keywords); } if self.enabled(Rule::RequestWithNoCertValidation) { - flake8_bandit::rules::request_with_no_cert_validation( - self, func, args, keywords, - ); + flake8_bandit::rules::request_with_no_cert_validation(self, func, keywords); } if self.enabled(Rule::UnsafeYAMLLoad) { flake8_bandit::rules::unsafe_yaml_load(self, func, args, keywords); } if self.enabled(Rule::SnmpInsecureVersion) { - flake8_bandit::rules::snmp_insecure_version(self, func, args, keywords); + flake8_bandit::rules::snmp_insecure_version(self, func, keywords); } if self.enabled(Rule::SnmpWeakCryptography) { flake8_bandit::rules::snmp_weak_cryptography(self, func, args, keywords); diff --git a/crates/ruff/src/rules/flake8_bandit/rules/request_with_no_cert_validation.rs b/crates/ruff/src/rules/flake8_bandit/rules/request_with_no_cert_validation.rs index 27c22af441..aea68dfef8 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/request_with_no_cert_validation.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/request_with_no_cert_validation.rs @@ -2,10 +2,34 @@ use rustpython_parser::ast::{Expr, Keyword, Ranged}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::helpers::{is_const_false, SimpleCallArgs}; +use ruff_python_ast::helpers::is_const_false; use crate::checkers::ast::Checker; +/// ## What it does +/// Checks for HTTPS requests that disable SSL certificate checks. +/// +/// ## Why is this bad? +/// If SSL certificates are not verified, an attacker could perform a "man in +/// the middle" attack by intercepting and modifying traffic between the client +/// and server. +/// +/// ## Example +/// ```python +/// import requests +/// +/// requests.get("https://www.example.com", verify=False) +/// ``` +/// +/// Use instead: +/// ```python +/// import requests +/// +/// requests.get("https://www.example.com") # By default, `verify=True`. +/// ``` +/// +/// ## References +/// - [Common Weakness Enumeration: CWE-295](https://cwe.mitre.org/data/definitions/295.html) #[violation] pub struct RequestWithNoCertValidation { string: String, @@ -25,7 +49,6 @@ impl Violation for RequestWithNoCertValidation { pub(crate) fn request_with_no_cert_validation( checker: &mut Checker, func: &Expr, - args: &[Expr], keywords: &[Keyword], ) { if let Some(target) = checker @@ -40,14 +63,18 @@ pub(crate) fn request_with_no_cert_validation( _ => None, }) { - let call_args = SimpleCallArgs::new(args, keywords); - if let Some(verify_arg) = call_args.keyword_argument("verify") { - if is_const_false(verify_arg) { + if let Some(arg) = keywords.iter().find(|keyword| { + keyword + .arg + .as_ref() + .map_or(false, |arg| arg.as_str() == "verify") + }) { + if is_const_false(&arg.value) { checker.diagnostics.push(Diagnostic::new( RequestWithNoCertValidation { string: target.to_string(), }, - verify_arg.range(), + arg.range(), )); } } diff --git a/crates/ruff/src/rules/flake8_bandit/rules/snmp_insecure_version.rs b/crates/ruff/src/rules/flake8_bandit/rules/snmp_insecure_version.rs index 1c4b032f97..b4c6434d72 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/snmp_insecure_version.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/snmp_insecure_version.rs @@ -3,10 +3,34 @@ use rustpython_parser::ast::{self, Constant, Expr, Keyword, Ranged}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::helpers::SimpleCallArgs; use crate::checkers::ast::Checker; +/// ## What it does +/// Checks for uses of SNMPv1 or SNMPv2. +/// +/// ## Why is this bad? +/// The SNMPv1 and SNMPv2 protocols are considered insecure as they do +/// not support encryption. Instead, prefer SNMPv3, which supports +/// encryption. +/// +/// ## Example +/// ```python +/// from pysnmp.hlapi import CommunityData +/// +/// CommunityData("public", mpModel=0) +/// ``` +/// +/// Use instead: +/// ```python +/// from pysnmp.hlapi import CommunityData +/// +/// CommunityData("public", mpModel=2) +/// ``` +/// +/// ## References +/// - [Cybersecurity and Infrastructure Security Agency (CISA): Alert TA17-156A](https://www.cisa.gov/news-events/alerts/2017/06/05/reducing-risk-snmp-abuse) +/// - [Common Weakness Enumeration: CWE-319](https://cwe.mitre.org/data/definitions/319.html) #[violation] pub struct SnmpInsecureVersion; @@ -18,12 +42,7 @@ impl Violation for SnmpInsecureVersion { } /// S508 -pub(crate) fn snmp_insecure_version( - checker: &mut Checker, - func: &Expr, - args: &[Expr], - keywords: &[Keyword], -) { +pub(crate) fn snmp_insecure_version(checker: &mut Checker, func: &Expr, keywords: &[Keyword]) { if checker .semantic() .resolve_call_path(func) @@ -31,17 +50,21 @@ pub(crate) fn snmp_insecure_version( matches!(call_path.as_slice(), ["pysnmp", "hlapi", "CommunityData"]) }) { - let call_args = SimpleCallArgs::new(args, keywords); - if let Some(mp_model_arg) = call_args.keyword_argument("mpModel") { + if let Some(arg) = keywords.iter().find(|keyword| { + keyword + .arg + .as_ref() + .map_or(false, |arg| arg.as_str() == "mpModel") + }) { if let Expr::Constant(ast::ExprConstant { value: Constant::Int(value), .. - }) = &mp_model_arg + }) = &arg.value { if value.is_zero() || value.is_one() { checker .diagnostics - .push(Diagnostic::new(SnmpInsecureVersion, mp_model_arg.range())); + .push(Diagnostic::new(SnmpInsecureVersion, arg.range())); } } } diff --git a/crates/ruff/src/rules/flake8_bandit/rules/snmp_weak_cryptography.rs b/crates/ruff/src/rules/flake8_bandit/rules/snmp_weak_cryptography.rs index f654c2550e..97093dd6cb 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/snmp_weak_cryptography.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/snmp_weak_cryptography.rs @@ -6,6 +6,29 @@ use ruff_python_ast::helpers::SimpleCallArgs; use crate::checkers::ast::Checker; +/// ## What it does +/// Checks for uses of the SNMPv3 protocol without encryption. +/// +/// ## Why is this bad? +/// Unencrypted SNMPv3 communication can be intercepted and read by +/// unauthorized parties. Instead, enable encryption when using SNMPv3. +/// +/// ## Example +/// ```python +/// from pysnmp.hlapi import UsmUserData +/// +/// UsmUserData("user") +/// ``` +/// +/// Use instead: +/// ```python +/// from pysnmp.hlapi import UsmUserData +/// +/// UsmUserData("user", "authkey", "privkey") +/// ``` +/// +/// ## References +/// - [Common Weakness Enumeration: CWE-319](https://cwe.mitre.org/data/definitions/319.html) #[violation] pub struct SnmpWeakCryptography; diff --git a/crates/ruff/src/rules/flake8_bandit/rules/unsafe_yaml_load.rs b/crates/ruff/src/rules/flake8_bandit/rules/unsafe_yaml_load.rs index 9d5274e523..20face98c8 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/unsafe_yaml_load.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/unsafe_yaml_load.rs @@ -6,6 +6,35 @@ use ruff_python_ast::helpers::SimpleCallArgs; use crate::checkers::ast::Checker; +/// ## What it does +/// Checks for uses of the `yaml.load` function. +/// +/// ## Why is this bad? +/// Running the `yaml.load` function over untrusted YAML files is insecure, as +/// `yaml.load` allows for the creation of arbitrary Python objects, which can +/// then be used to execute arbitrary code. +/// +/// Instead, consider using `yaml.safe_load`, which allows for the creation of +/// simple Python objects like integers and lists, but prohibits the creation of +/// more complex objects like functions and classes. +/// +/// ## Example +/// ```python +/// import yaml +/// +/// yaml.load(untrusted_yaml) +/// ``` +/// +/// Use instead: +/// ```python +/// import yaml +/// +/// yaml.safe_load(untrusted_yaml) +/// ``` +/// +/// ## References +/// - [PyYAML documentation: Loading YAML](https://pyyaml.org/wiki/PyYAMLDocumentation) +/// - [Common Weakness Enumeration: CWE-20](https://cwe.mitre.org/data/definitions/20.html) #[violation] pub struct UnsafeYAMLLoad { pub loader: Option, diff --git a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S501_S501.py.snap b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S501_S501.py.snap index 318f21de04..4875cf2e1f 100644 --- a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S501_S501.py.snap +++ b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S501_S501.py.snap @@ -1,180 +1,180 @@ --- source: crates/ruff/src/rules/flake8_bandit/mod.rs --- -S501.py:5:54: S501 Probable use of `requests` call with `verify=False` disabling SSL certificate checks +S501.py:5:47: S501 Probable use of `requests` call with `verify=False` disabling SSL certificate checks | 4 | requests.get('https://gmail.com', timeout=30, verify=True) 5 | requests.get('https://gmail.com', timeout=30, verify=False) - | ^^^^^ S501 + | ^^^^^^^^^^^^ S501 6 | requests.post('https://gmail.com', timeout=30, verify=True) 7 | requests.post('https://gmail.com', timeout=30, verify=False) | -S501.py:7:55: S501 Probable use of `requests` call with `verify=False` disabling SSL certificate checks +S501.py:7:48: S501 Probable use of `requests` call with `verify=False` disabling SSL certificate checks | 5 | requests.get('https://gmail.com', timeout=30, verify=False) 6 | requests.post('https://gmail.com', timeout=30, verify=True) 7 | requests.post('https://gmail.com', timeout=30, verify=False) - | ^^^^^ S501 + | ^^^^^^^^^^^^ S501 8 | requests.put('https://gmail.com', timeout=30, verify=True) 9 | requests.put('https://gmail.com', timeout=30, verify=False) | -S501.py:9:54: S501 Probable use of `requests` call with `verify=False` disabling SSL certificate checks +S501.py:9:47: S501 Probable use of `requests` call with `verify=False` disabling SSL certificate checks | 7 | requests.post('https://gmail.com', timeout=30, verify=False) 8 | requests.put('https://gmail.com', timeout=30, verify=True) 9 | requests.put('https://gmail.com', timeout=30, verify=False) - | ^^^^^ S501 + | ^^^^^^^^^^^^ S501 10 | requests.delete('https://gmail.com', timeout=30, verify=True) 11 | requests.delete('https://gmail.com', timeout=30, verify=False) | -S501.py:11:57: S501 Probable use of `requests` call with `verify=False` disabling SSL certificate checks +S501.py:11:50: S501 Probable use of `requests` call with `verify=False` disabling SSL certificate checks | 9 | requests.put('https://gmail.com', timeout=30, verify=False) 10 | requests.delete('https://gmail.com', timeout=30, verify=True) 11 | requests.delete('https://gmail.com', timeout=30, verify=False) - | ^^^^^ S501 + | ^^^^^^^^^^^^ S501 12 | requests.patch('https://gmail.com', timeout=30, verify=True) 13 | requests.patch('https://gmail.com', timeout=30, verify=False) | -S501.py:13:56: S501 Probable use of `requests` call with `verify=False` disabling SSL certificate checks +S501.py:13:49: S501 Probable use of `requests` call with `verify=False` disabling SSL certificate checks | 11 | requests.delete('https://gmail.com', timeout=30, verify=False) 12 | requests.patch('https://gmail.com', timeout=30, verify=True) 13 | requests.patch('https://gmail.com', timeout=30, verify=False) - | ^^^^^ S501 + | ^^^^^^^^^^^^ S501 14 | requests.options('https://gmail.com', timeout=30, verify=True) 15 | requests.options('https://gmail.com', timeout=30, verify=False) | -S501.py:15:58: S501 Probable use of `requests` call with `verify=False` disabling SSL certificate checks +S501.py:15:51: S501 Probable use of `requests` call with `verify=False` disabling SSL certificate checks | 13 | requests.patch('https://gmail.com', timeout=30, verify=False) 14 | requests.options('https://gmail.com', timeout=30, verify=True) 15 | requests.options('https://gmail.com', timeout=30, verify=False) - | ^^^^^ S501 + | ^^^^^^^^^^^^ S501 16 | requests.head('https://gmail.com', timeout=30, verify=True) 17 | requests.head('https://gmail.com', timeout=30, verify=False) | -S501.py:17:55: S501 Probable use of `requests` call with `verify=False` disabling SSL certificate checks +S501.py:17:48: S501 Probable use of `requests` call with `verify=False` disabling SSL certificate checks | 15 | requests.options('https://gmail.com', timeout=30, verify=False) 16 | requests.head('https://gmail.com', timeout=30, verify=True) 17 | requests.head('https://gmail.com', timeout=30, verify=False) - | ^^^^^ S501 + | ^^^^^^^^^^^^ S501 18 | 19 | httpx.request('GET', 'https://gmail.com', verify=True) | -S501.py:20:50: S501 Probable use of `httpx` call with `verify=False` disabling SSL certificate checks +S501.py:20:43: S501 Probable use of `httpx` call with `verify=False` disabling SSL certificate checks | 19 | httpx.request('GET', 'https://gmail.com', verify=True) 20 | httpx.request('GET', 'https://gmail.com', verify=False) - | ^^^^^ S501 + | ^^^^^^^^^^^^ S501 21 | httpx.get('https://gmail.com', verify=True) 22 | httpx.get('https://gmail.com', verify=False) | -S501.py:22:39: S501 Probable use of `httpx` call with `verify=False` disabling SSL certificate checks +S501.py:22:32: S501 Probable use of `httpx` call with `verify=False` disabling SSL certificate checks | 20 | httpx.request('GET', 'https://gmail.com', verify=False) 21 | httpx.get('https://gmail.com', verify=True) 22 | httpx.get('https://gmail.com', verify=False) - | ^^^^^ S501 + | ^^^^^^^^^^^^ S501 23 | httpx.options('https://gmail.com', verify=True) 24 | httpx.options('https://gmail.com', verify=False) | -S501.py:24:43: S501 Probable use of `httpx` call with `verify=False` disabling SSL certificate checks +S501.py:24:36: S501 Probable use of `httpx` call with `verify=False` disabling SSL certificate checks | 22 | httpx.get('https://gmail.com', verify=False) 23 | httpx.options('https://gmail.com', verify=True) 24 | httpx.options('https://gmail.com', verify=False) - | ^^^^^ S501 + | ^^^^^^^^^^^^ S501 25 | httpx.head('https://gmail.com', verify=True) 26 | httpx.head('https://gmail.com', verify=False) | -S501.py:26:40: S501 Probable use of `httpx` call with `verify=False` disabling SSL certificate checks +S501.py:26:33: S501 Probable use of `httpx` call with `verify=False` disabling SSL certificate checks | 24 | httpx.options('https://gmail.com', verify=False) 25 | httpx.head('https://gmail.com', verify=True) 26 | httpx.head('https://gmail.com', verify=False) - | ^^^^^ S501 + | ^^^^^^^^^^^^ S501 27 | httpx.post('https://gmail.com', verify=True) 28 | httpx.post('https://gmail.com', verify=False) | -S501.py:28:40: S501 Probable use of `httpx` call with `verify=False` disabling SSL certificate checks +S501.py:28:33: S501 Probable use of `httpx` call with `verify=False` disabling SSL certificate checks | 26 | httpx.head('https://gmail.com', verify=False) 27 | httpx.post('https://gmail.com', verify=True) 28 | httpx.post('https://gmail.com', verify=False) - | ^^^^^ S501 + | ^^^^^^^^^^^^ S501 29 | httpx.put('https://gmail.com', verify=True) 30 | httpx.put('https://gmail.com', verify=False) | -S501.py:30:39: S501 Probable use of `httpx` call with `verify=False` disabling SSL certificate checks +S501.py:30:32: S501 Probable use of `httpx` call with `verify=False` disabling SSL certificate checks | 28 | httpx.post('https://gmail.com', verify=False) 29 | httpx.put('https://gmail.com', verify=True) 30 | httpx.put('https://gmail.com', verify=False) - | ^^^^^ S501 + | ^^^^^^^^^^^^ S501 31 | httpx.patch('https://gmail.com', verify=True) 32 | httpx.patch('https://gmail.com', verify=False) | -S501.py:32:41: S501 Probable use of `httpx` call with `verify=False` disabling SSL certificate checks +S501.py:32:34: S501 Probable use of `httpx` call with `verify=False` disabling SSL certificate checks | 30 | httpx.put('https://gmail.com', verify=False) 31 | httpx.patch('https://gmail.com', verify=True) 32 | httpx.patch('https://gmail.com', verify=False) - | ^^^^^ S501 + | ^^^^^^^^^^^^ S501 33 | httpx.delete('https://gmail.com', verify=True) 34 | httpx.delete('https://gmail.com', verify=False) | -S501.py:34:42: S501 Probable use of `httpx` call with `verify=False` disabling SSL certificate checks +S501.py:34:35: S501 Probable use of `httpx` call with `verify=False` disabling SSL certificate checks | 32 | httpx.patch('https://gmail.com', verify=False) 33 | httpx.delete('https://gmail.com', verify=True) 34 | httpx.delete('https://gmail.com', verify=False) - | ^^^^^ S501 + | ^^^^^^^^^^^^ S501 35 | httpx.stream('https://gmail.com', verify=True) 36 | httpx.stream('https://gmail.com', verify=False) | -S501.py:36:42: S501 Probable use of `httpx` call with `verify=False` disabling SSL certificate checks +S501.py:36:35: S501 Probable use of `httpx` call with `verify=False` disabling SSL certificate checks | 34 | httpx.delete('https://gmail.com', verify=False) 35 | httpx.stream('https://gmail.com', verify=True) 36 | httpx.stream('https://gmail.com', verify=False) - | ^^^^^ S501 + | ^^^^^^^^^^^^ S501 37 | httpx.Client() 38 | httpx.Client(verify=False) | -S501.py:38:21: S501 Probable use of `httpx` call with `verify=False` disabling SSL certificate checks +S501.py:38:14: S501 Probable use of `httpx` call with `verify=False` disabling SSL certificate checks | 36 | httpx.stream('https://gmail.com', verify=False) 37 | httpx.Client() 38 | httpx.Client(verify=False) - | ^^^^^ S501 + | ^^^^^^^^^^^^ S501 39 | httpx.AsyncClient() 40 | httpx.AsyncClient(verify=False) | -S501.py:40:26: S501 Probable use of `httpx` call with `verify=False` disabling SSL certificate checks +S501.py:40:19: S501 Probable use of `httpx` call with `verify=False` disabling SSL certificate checks | 38 | httpx.Client(verify=False) 39 | httpx.AsyncClient() 40 | httpx.AsyncClient(verify=False) - | ^^^^^ S501 + | ^^^^^^^^^^^^ S501 | diff --git a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S508_S508.py.snap b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S508_S508.py.snap index 8fd040ddd4..b166212ea0 100644 --- a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S508_S508.py.snap +++ b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S508_S508.py.snap @@ -1,20 +1,20 @@ --- source: crates/ruff/src/rules/flake8_bandit/mod.rs --- -S508.py:3:33: S508 The use of SNMPv1 and SNMPv2 is insecure. Use SNMPv3 if able. +S508.py:3:25: S508 The use of SNMPv1 and SNMPv2 is insecure. Use SNMPv3 if able. | 1 | from pysnmp.hlapi import CommunityData 2 | 3 | CommunityData("public", mpModel=0) # S508 - | ^ S508 + | ^^^^^^^^^ S508 4 | CommunityData("public", mpModel=1) # S508 | -S508.py:4:33: S508 The use of SNMPv1 and SNMPv2 is insecure. Use SNMPv3 if able. +S508.py:4:25: S508 The use of SNMPv1 and SNMPv2 is insecure. Use SNMPv3 if able. | 3 | CommunityData("public", mpModel=0) # S508 4 | CommunityData("public", mpModel=1) # S508 - | ^ S508 + | ^^^^^^^^^ S508 5 | 6 | CommunityData("public", mpModel=2) # OK |