Skip to content

Commit

Permalink
[flake8-bandit] Implement upstream updates for S311, S324 and `…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
mkniewallner committed Mar 11, 2024
1 parent ad84eed commit bc693ea
Show file tree
Hide file tree
Showing 10 changed files with 488 additions and 221 deletions.
22 changes: 22 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_bandit/S311.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import os
import random

import a_lib

# OK
random.SystemRandom()

# Errors
random.Random()
random.random()
random.randrange()
random.randint()
random.choice()
random.choices()
random.uniform()
random.triangular()
random.randbytes()

# Unrelated
os.urandom()
a_lib.random()
43 changes: 19 additions & 24 deletions crates/ruff_linter/resources/test/fixtures/flake8_bandit/S324.py
Original file line number Diff line number Diff line change
@@ -1,52 +1,47 @@
import crypt
import hashlib
from hashlib import new as hashlib_new
from hashlib import sha1 as hashlib_sha1

# Invalid

# Errors
hashlib.new('md5')

hashlib.new('md4', b'test')

hashlib.new(name='md5', data=b'test')

hashlib.new('MD4', data=b'test')

hashlib.new('sha1')

hashlib.new('sha1', data=b'test')

hashlib.new('sha', data=b'test')

hashlib.new(name='SHA', data=b'test')

hashlib.sha(data=b'test')

hashlib.md5()

hashlib_new('sha1')

hashlib_sha1('sha1')

# usedforsecurity arg only available in Python 3.9+
hashlib.new('sha1', usedforsecurity=True)

# Valid
crypt.crypt("test", salt=crypt.METHOD_CRYPT)
crypt.crypt("test", salt=crypt.METHOD_MD5)
crypt.crypt("test", salt=crypt.METHOD_BLOWFISH)
crypt.crypt("test", crypt.METHOD_BLOWFISH)

hashlib.new('sha256')
crypt.mksalt(crypt.METHOD_CRYPT)
crypt.mksalt(crypt.METHOD_MD5)
crypt.mksalt(crypt.METHOD_BLOWFISH)

# OK
hashlib.new('sha256')
hashlib.new('SHA512')

hashlib.sha256(data=b'test')

# usedforsecurity arg only available in Python 3.9+
hashlib_new(name='sha1', usedforsecurity=False)

# usedforsecurity arg only available in Python 3.9+
hashlib_sha1(name='sha1', usedforsecurity=False)

# usedforsecurity arg only available in Python 3.9+
hashlib.md4(usedforsecurity=False)

# usedforsecurity arg only available in Python 3.9+
hashlib.new(name='sha256', usedforsecurity=False)

crypt.crypt("test")
crypt.crypt("test", salt=crypt.METHOD_SHA256)
crypt.crypt("test", salt=crypt.METHOD_SHA512)

crypt.mksalt()
crypt.mksalt(crypt.METHOD_SHA256)
crypt.mksalt(crypt.METHOD_SHA512)
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import os
import subprocess

import commands
import popen2
Expand All @@ -16,6 +17,8 @@
popen2.Popen4("true")
commands.getoutput("true")
commands.getstatusoutput("true")
subprocess.getoutput("true")
subprocess.getstatusoutput("true")


# Check command argument looks unsafe.
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/flake8_bandit/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ mod tests {
#[test_case(Rule::SuspiciousEvalUsage, Path::new("S307.py"))]
#[test_case(Rule::SuspiciousMarkSafeUsage, Path::new("S308.py"))]
#[test_case(Rule::SuspiciousURLOpenUsage, Path::new("S310.py"))]
#[test_case(Rule::SuspiciousNonCryptographicRandomUsage, Path::new("S311.py"))]
#[test_case(Rule::SuspiciousTelnetUsage, Path::new("S312.py"))]
#[test_case(Rule::SuspiciousTelnetlibImport, Path::new("S401.py"))]
#[test_case(Rule::SuspiciousFtplibImport, Path::new("S402.py"))]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ use crate::checkers::ast::Checker;
use super::super::helpers::string_literal;

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

impl Violation for HashlibInsecureHashFunction {
#[derive_message_formats]
fn message(&self) -> String {
let HashlibInsecureHashFunction { string } = self;
format!("Probable use of insecure hash functions in `hashlib`: `{string}`")
let HashlibInsecureHashFunction { library, string } = self;
format!("Probable use of insecure hash functions in `{library}`: `{string}`")
}
}

/// S324
pub(crate) fn hashlib_insecure_hash_functions(checker: &mut Checker, call: &ast::ExprCall) {
if let Some(hashlib_call) = checker
if let Some(weak_hash_call) = checker
.semantic()
.resolve_qualified_name(&call.func)
.and_then(|qualified_name| match qualified_name.segments() {
["hashlib", "new"] => Some(HashlibCall::New),
["hashlib", "md4"] => Some(HashlibCall::WeakHash("md4")),
["hashlib", "md5"] => Some(HashlibCall::WeakHash("md5")),
["hashlib", "sha"] => Some(HashlibCall::WeakHash("sha")),
["hashlib", "sha1"] => Some(HashlibCall::WeakHash("sha1")),
["hashlib", "new"] => Some(WeakHashCall::Hashlib {
call: HashlibCall::New,
}),
["hashlib", "md4"] => Some(WeakHashCall::Hashlib {
call: HashlibCall::WeakHash("md4"),
}),
["hashlib", "md5"] => Some(WeakHashCall::Hashlib {
call: HashlibCall::WeakHash("md5"),
}),
["hashlib", "sha"] => Some(WeakHashCall::Hashlib {
call: HashlibCall::WeakHash("sha"),
}),
["hashlib", "sha1"] => Some(WeakHashCall::Hashlib {
call: HashlibCall::WeakHash("sha1"),
}),
["crypt", "crypt" | "mksalt"] => Some(WeakHashCall::Crypt),
_ => None,
})
{
if !is_used_for_security(&call.arguments) {
return;
}
match hashlib_call {
HashlibCall::New => {
if let Some(name_arg) = call.arguments.find_argument("name", 0) {
if let Some(hash_func_name) = string_literal(name_arg) {
// `hashlib.new` accepts both lowercase and uppercase names for hash
// functions.
if matches!(
hash_func_name,
"md4" | "md5" | "sha" | "sha1" | "MD4" | "MD5" | "SHA" | "SHA1"
) {
checker.diagnostics.push(Diagnostic::new(
HashlibInsecureHashFunction {
string: hash_func_name.to_string(),
},
name_arg.range(),
));
}
}
}
match weak_hash_call {
WeakHashCall::Hashlib { call: hashlib_call } => {
detect_insecure_hashlib_calls(checker, call, hashlib_call);
}
HashlibCall::WeakHash(func_name) => {
WeakHashCall::Crypt => detect_insecure_crypt_calls(checker, call),
}
}
}

fn detect_insecure_hashlib_calls(
checker: &mut Checker,
call: &ast::ExprCall,
hashlib_call: HashlibCall,
) {
if !is_used_for_security(&call.arguments) {
return;
}

match hashlib_call {
HashlibCall::New => {
let Some(name_arg) = call.arguments.find_argument("name", 0) else {
return;
};
let Some(hash_func_name) = string_literal(name_arg) else {
return;
};

// `hashlib.new` accepts both lowercase and uppercase names for hash
// functions.
if matches!(
hash_func_name,
"md4" | "md5" | "sha" | "sha1" | "MD4" | "MD5" | "SHA" | "SHA1"
) {
checker.diagnostics.push(Diagnostic::new(
HashlibInsecureHashFunction {
string: (*func_name).to_string(),
library: "hashlib".to_string(),
string: hash_func_name.to_string(),
},
call.func.range(),
name_arg.range(),
));
}
}
HashlibCall::WeakHash(func_name) => {
checker.diagnostics.push(Diagnostic::new(
HashlibInsecureHashFunction {
library: "hashlib".to_string(),
string: (*func_name).to_string(),
},
call.func.range(),
));
}
}
}

fn detect_insecure_crypt_calls(checker: &mut Checker, call: &ast::ExprCall) {
let Some(method) = checker
.semantic()
.resolve_qualified_name(&call.func)
.and_then(|qualified_name| match qualified_name.segments() {
["crypt", "crypt"] => Some(("salt", 1)),
["crypt", "mksalt"] => Some(("method", 0)),
_ => None,
})
.and_then(|(argument_name, position)| {
call.arguments.find_argument(argument_name, position)
})
else {
return;
};

let Some(qualified_name) = checker.semantic().resolve_qualified_name(method) else {
return;
};

if matches!(
qualified_name.segments(),
["crypt", "METHOD_CRYPT" | "METHOD_MD5" | "METHOD_BLOWFISH"]
) {
checker.diagnostics.push(Diagnostic::new(
HashlibInsecureHashFunction {
library: "crypt".to_string(),
string: qualified_name.to_string(),
},
method.range(),
));
}
}

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

#[derive(Debug)]
#[derive(Debug, Copy, Clone)]
enum WeakHashCall {
Hashlib { call: HashlibCall },
Crypt,
}

#[derive(Debug, Copy, Clone)]
enum HashlibCall {
New,
WeakHash(&'static str),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,7 @@ fn get_call_kind(func: &Expr, semantic: &SemanticModel) -> Option<CallKind> {
"Popen" | "call" | "check_call" | "check_output" | "run" => {
Some(CallKind::Subprocess)
}
"getoutput" | "getstatusoutput" => Some(CallKind::Shell),
_ => None,
},
"popen2" => match submodule {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -867,7 +867,7 @@ pub(crate) fn suspicious_function_call(checker: &mut Checker, call: &ExprCall) {
["urllib", "request", "URLopener" | "FancyURLopener"] |
["six", "moves", "urllib", "request", "URLopener" | "FancyURLopener"] => Some(SuspiciousURLOpenUsage.into()),
// NonCryptographicRandom
["random", "random" | "randrange" | "randint" | "choice" | "choices" | "uniform" | "triangular"] => Some(SuspiciousNonCryptographicRandomUsage.into()),
["random", "Random" | "random" | "randrange" | "randint" | "choice" | "choices" | "uniform" | "triangular" | "randbytes"] => Some(SuspiciousNonCryptographicRandomUsage.into()),
// UnverifiedContext
["ssl", "_create_unverified_context"] => Some(SuspiciousUnverifiedContextUsage.into()),
// XMLCElementTree
Expand Down

0 comments on commit bc693ea

Please sign in to comment.