Skip to content

Commit bc693ea

Browse files
mkniewallnermkniewallner
authored
[flake8-bandit] Implement upstream updates for S311, S324 and S605 (#10313)
## Summary Pick up updates made in latest [releases](https://github.com/PyCQA/bandit/releases) of `bandit`: - `S311`: PyCQA/bandit#940 and PyCQA/bandit#1096 - `S324`: PyCQA/bandit#1018 - `S605`: PyCQA/bandit#1116 ## Test Plan Snapshot tests
1 parent ad84eed commit bc693ea

10 files changed

+488
-221
lines changed

‎crates/ruff_linter/resources/test/fixtures/flake8_bandit/S311.py

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
import os
2+
import random
3+
4+
import a_lib
5+
6+
# OK
7+
random.SystemRandom()
8+
9+
# Errors
10+
random.Random()
11+
random.random()
12+
random.randrange()
13+
random.randint()
14+
random.choice()
15+
random.choices()
16+
random.uniform()
17+
random.triangular()
18+
random.randbytes()
19+
20+
# Unrelated
21+
os.urandom()
22+
a_lib.random()

‎crates/ruff_linter/resources/test/fixtures/flake8_bandit/S324.py

Lines changed: 19 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,52 +1,47 @@
1+
import crypt
12
import hashlib
23
from hashlib import new as hashlib_new
34
from hashlib import sha1 as hashlib_sha1
45

5-
# Invalid
6-
6+
# Errors
77
hashlib.new('md5')
8-
98
hashlib.new('md4', b'test')
10-
119
hashlib.new(name='md5', data=b'test')
12-
1310
hashlib.new('MD4', data=b'test')
14-
1511
hashlib.new('sha1')
16-
1712
hashlib.new('sha1', data=b'test')
18-
1913
hashlib.new('sha', data=b'test')
20-
2114
hashlib.new(name='SHA', data=b'test')
22-
2315
hashlib.sha(data=b'test')
24-
2516
hashlib.md5()
26-
2717
hashlib_new('sha1')
28-
2918
hashlib_sha1('sha1')
30-
3119
# usedforsecurity arg only available in Python 3.9+
3220
hashlib.new('sha1', usedforsecurity=True)
3321

34-
# Valid
22+
crypt.crypt("test", salt=crypt.METHOD_CRYPT)
23+
crypt.crypt("test", salt=crypt.METHOD_MD5)
24+
crypt.crypt("test", salt=crypt.METHOD_BLOWFISH)
25+
crypt.crypt("test", crypt.METHOD_BLOWFISH)
3526

36-
hashlib.new('sha256')
27+
crypt.mksalt(crypt.METHOD_CRYPT)
28+
crypt.mksalt(crypt.METHOD_MD5)
29+
crypt.mksalt(crypt.METHOD_BLOWFISH)
3730

31+
# OK
32+
hashlib.new('sha256')
3833
hashlib.new('SHA512')
39-
4034
hashlib.sha256(data=b'test')
41-
4235
# usedforsecurity arg only available in Python 3.9+
4336
hashlib_new(name='sha1', usedforsecurity=False)
44-
45-
# usedforsecurity arg only available in Python 3.9+
4637
hashlib_sha1(name='sha1', usedforsecurity=False)
47-
48-
# usedforsecurity arg only available in Python 3.9+
4938
hashlib.md4(usedforsecurity=False)
50-
51-
# usedforsecurity arg only available in Python 3.9+
5239
hashlib.new(name='sha256', usedforsecurity=False)
40+
41+
crypt.crypt("test")
42+
crypt.crypt("test", salt=crypt.METHOD_SHA256)
43+
crypt.crypt("test", salt=crypt.METHOD_SHA512)
44+
45+
crypt.mksalt()
46+
crypt.mksalt(crypt.METHOD_SHA256)
47+
crypt.mksalt(crypt.METHOD_SHA512)

‎crates/ruff_linter/resources/test/fixtures/flake8_bandit/S605.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import os
2+
import subprocess
23

34
import commands
45
import popen2
@@ -16,6 +17,8 @@
1617
popen2.Popen4("true")
1718
commands.getoutput("true")
1819
commands.getstatusoutput("true")
20+
subprocess.getoutput("true")
21+
subprocess.getstatusoutput("true")
1922

2023

2124
# Check command argument looks unsafe.

‎crates/ruff_linter/src/rules/flake8_bandit/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ mod tests {
4848
#[test_case(Rule::SuspiciousEvalUsage, Path::new("S307.py"))]
4949
#[test_case(Rule::SuspiciousMarkSafeUsage, Path::new("S308.py"))]
5050
#[test_case(Rule::SuspiciousURLOpenUsage, Path::new("S310.py"))]
51+
#[test_case(Rule::SuspiciousNonCryptographicRandomUsage, Path::new("S311.py"))]
5152
#[test_case(Rule::SuspiciousTelnetUsage, Path::new("S312.py"))]
5253
#[test_case(Rule::SuspiciousTelnetlibImport, Path::new("S401.py"))]
5354
#[test_case(Rule::SuspiciousFtplibImport, Path::new("S402.py"))]

‎crates/ruff_linter/src/rules/flake8_bandit/rules/hashlib_insecure_hash_functions.rs

Lines changed: 108 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,8 @@ use crate::checkers::ast::Checker;
99
use super::super::helpers::string_literal;
1010

1111
/// ## What it does
12-
/// Checks for uses of weak or broken cryptographic hash functions.
12+
/// Checks for uses of weak or broken cryptographic hash functions in
13+
/// `hashlib` and `crypt` libraries.
1314
///
1415
/// ## Why is this bad?
1516
/// Weak or broken cryptographic hash functions may be susceptible to
@@ -43,68 +44,134 @@ use super::super::helpers::string_literal;
4344
///
4445
/// ## References
4546
/// - [Python documentation: `hashlib` — Secure hashes and message digests](https://docs.python.org/3/library/hashlib.html)
47+
/// - [Python documentation: `crypt` — Function to check Unix passwords](https://docs.python.org/3/library/crypt.html)
4648
/// - [Common Weakness Enumeration: CWE-327](https://cwe.mitre.org/data/definitions/327.html)
4749
/// - [Common Weakness Enumeration: CWE-328](https://cwe.mitre.org/data/definitions/328.html)
4850
/// - [Common Weakness Enumeration: CWE-916](https://cwe.mitre.org/data/definitions/916.html)
4951
#[violation]
5052
pub struct HashlibInsecureHashFunction {
53+
library: String,
5154
string: String,
5255
}
5356

5457
impl Violation for HashlibInsecureHashFunction {
5558
#[derive_message_formats]
5659
fn message(&self) -> String {
57-
let HashlibInsecureHashFunction { string } = self;
58-
format!("Probable use of insecure hash functions in `hashlib`: `{string}`")
60+
let HashlibInsecureHashFunction { library, string } = self;
61+
format!("Probable use of insecure hash functions in `{library}`: `{string}`")
5962
}
6063
}
6164

6265
/// S324
6366
pub(crate) fn hashlib_insecure_hash_functions(checker: &mut Checker, call: &ast::ExprCall) {
64-
if let Some(hashlib_call) = checker
67+
if let Some(weak_hash_call) = checker
6568
.semantic()
6669
.resolve_qualified_name(&call.func)
6770
.and_then(|qualified_name| match qualified_name.segments() {
68-
["hashlib", "new"] => Some(HashlibCall::New),
69-
["hashlib", "md4"] => Some(HashlibCall::WeakHash("md4")),
70-
["hashlib", "md5"] => Some(HashlibCall::WeakHash("md5")),
71-
["hashlib", "sha"] => Some(HashlibCall::WeakHash("sha")),
72-
["hashlib", "sha1"] => Some(HashlibCall::WeakHash("sha1")),
71+
["hashlib", "new"] => Some(WeakHashCall::Hashlib {
72+
call: HashlibCall::New,
73+
}),
74+
["hashlib", "md4"] => Some(WeakHashCall::Hashlib {
75+
call: HashlibCall::WeakHash("md4"),
76+
}),
77+
["hashlib", "md5"] => Some(WeakHashCall::Hashlib {
78+
call: HashlibCall::WeakHash("md5"),
79+
}),
80+
["hashlib", "sha"] => Some(WeakHashCall::Hashlib {
81+
call: HashlibCall::WeakHash("sha"),
82+
}),
83+
["hashlib", "sha1"] => Some(WeakHashCall::Hashlib {
84+
call: HashlibCall::WeakHash("sha1"),
85+
}),
86+
["crypt", "crypt" | "mksalt"] => Some(WeakHashCall::Crypt),
7387
_ => None,
7488
})
7589
{
76-
if !is_used_for_security(&call.arguments) {
77-
return;
78-
}
79-
match hashlib_call {
80-
HashlibCall::New => {
81-
if let Some(name_arg) = call.arguments.find_argument("name", 0) {
82-
if let Some(hash_func_name) = string_literal(name_arg) {
83-
// `hashlib.new` accepts both lowercase and uppercase names for hash
84-
// functions.
85-
if matches!(
86-
hash_func_name,
87-
"md4" | "md5" | "sha" | "sha1" | "MD4" | "MD5" | "SHA" | "SHA1"
88-
) {
89-
checker.diagnostics.push(Diagnostic::new(
90-
HashlibInsecureHashFunction {
91-
string: hash_func_name.to_string(),
92-
},
93-
name_arg.range(),
94-
));
95-
}
96-
}
97-
}
90+
match weak_hash_call {
91+
WeakHashCall::Hashlib { call: hashlib_call } => {
92+
detect_insecure_hashlib_calls(checker, call, hashlib_call);
9893
}
99-
HashlibCall::WeakHash(func_name) => {
94+
WeakHashCall::Crypt => detect_insecure_crypt_calls(checker, call),
95+
}
96+
}
97+
}
98+
99+
fn detect_insecure_hashlib_calls(
100+
checker: &mut Checker,
101+
call: &ast::ExprCall,
102+
hashlib_call: HashlibCall,
103+
) {
104+
if !is_used_for_security(&call.arguments) {
105+
return;
106+
}
107+
108+
match hashlib_call {
109+
HashlibCall::New => {
110+
let Some(name_arg) = call.arguments.find_argument("name", 0) else {
111+
return;
112+
};
113+
let Some(hash_func_name) = string_literal(name_arg) else {
114+
return;
115+
};
116+
117+
// `hashlib.new` accepts both lowercase and uppercase names for hash
118+
// functions.
119+
if matches!(
120+
hash_func_name,
121+
"md4" | "md5" | "sha" | "sha1" | "MD4" | "MD5" | "SHA" | "SHA1"
122+
) {
100123
checker.diagnostics.push(Diagnostic::new(
101124
HashlibInsecureHashFunction {
102-
string: (*func_name).to_string(),
125+
library: "hashlib".to_string(),
126+
string: hash_func_name.to_string(),
103127
},
104-
call.func.range(),
128+
name_arg.range(),
105129
));
106130
}
107131
}
132+
HashlibCall::WeakHash(func_name) => {
133+
checker.diagnostics.push(Diagnostic::new(
134+
HashlibInsecureHashFunction {
135+
library: "hashlib".to_string(),
136+
string: (*func_name).to_string(),
137+
},
138+
call.func.range(),
139+
));
140+
}
141+
}
142+
}
143+
144+
fn detect_insecure_crypt_calls(checker: &mut Checker, call: &ast::ExprCall) {
145+
let Some(method) = checker
146+
.semantic()
147+
.resolve_qualified_name(&call.func)
148+
.and_then(|qualified_name| match qualified_name.segments() {
149+
["crypt", "crypt"] => Some(("salt", 1)),
150+
["crypt", "mksalt"] => Some(("method", 0)),
151+
_ => None,
152+
})
153+
.and_then(|(argument_name, position)| {
154+
call.arguments.find_argument(argument_name, position)
155+
})
156+
else {
157+
return;
158+
};
159+
160+
let Some(qualified_name) = checker.semantic().resolve_qualified_name(method) else {
161+
return;
162+
};
163+
164+
if matches!(
165+
qualified_name.segments(),
166+
["crypt", "METHOD_CRYPT" | "METHOD_MD5" | "METHOD_BLOWFISH"]
167+
) {
168+
checker.diagnostics.push(Diagnostic::new(
169+
HashlibInsecureHashFunction {
170+
library: "crypt".to_string(),
171+
string: qualified_name.to_string(),
172+
},
173+
method.range(),
174+
));
108175
}
109176
}
110177

@@ -114,7 +181,13 @@ fn is_used_for_security(arguments: &Arguments) -> bool {
114181
.map_or(true, |keyword| !is_const_false(&keyword.value))
115182
}
116183

117-
#[derive(Debug)]
184+
#[derive(Debug, Copy, Clone)]
185+
enum WeakHashCall {
186+
Hashlib { call: HashlibCall },
187+
Crypt,
188+
}
189+
190+
#[derive(Debug, Copy, Clone)]
118191
enum HashlibCall {
119192
New,
120193
WeakHash(&'static str),

‎crates/ruff_linter/src/rules/flake8_bandit/rules/shell_injection.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -433,6 +433,7 @@ fn get_call_kind(func: &Expr, semantic: &SemanticModel) -> Option<CallKind> {
433433
"Popen" | "call" | "check_call" | "check_output" | "run" => {
434434
Some(CallKind::Subprocess)
435435
}
436+
"getoutput" | "getstatusoutput" => Some(CallKind::Shell),
436437
_ => None,
437438
},
438439
"popen2" => match submodule {

‎crates/ruff_linter/src/rules/flake8_bandit/rules/suspicious_function_call.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -867,7 +867,7 @@ pub(crate) fn suspicious_function_call(checker: &mut Checker, call: &ExprCall) {
867867
["urllib", "request", "URLopener" | "FancyURLopener"] |
868868
["six", "moves", "urllib", "request", "URLopener" | "FancyURLopener"] => Some(SuspiciousURLOpenUsage.into()),
869869
// NonCryptographicRandom
870-
["random", "random" | "randrange" | "randint" | "choice" | "choices" | "uniform" | "triangular"] => Some(SuspiciousNonCryptographicRandomUsage.into()),
870+
["random", "Random" | "random" | "randrange" | "randint" | "choice" | "choices" | "uniform" | "triangular" | "randbytes"] => Some(SuspiciousNonCryptographicRandomUsage.into()),
871871
// UnverifiedContext
872872
["ssl", "_create_unverified_context"] => Some(SuspiciousUnverifiedContextUsage.into()),
873873
// XMLCElementTree

0 commit comments

Comments
 (0)