mirror of
https://github.com/astral-sh/ruff.git
synced 2025-09-29 13:25:17 +00:00
[flake8-bandit]: Implement S610 rule (#10316)
Part of https://github.com/astral-sh/ruff/issues/1646. ## Summary Implement `S610` rule from `flake8-bandit`. Upstream references: - Implementation: https://github.com/PyCQA/bandit/blob/1.7.8/bandit/plugins/django_sql_injection.py#L20-L97 - Test cases: https://github.com/PyCQA/bandit/blob/1.7.8/examples/django_sql_injection_extra.py - Test assertion: https://github.com/PyCQA/bandit/blob/1.7.8/tests/functional/test_functional.py#L517-L524 The implementation in `bandit` targets additional arguments (`params`, `order_by` and `select_params`) but doesn't seem to do anything with them in the end, so I did not include them in the implementation. Note that this rule could be prone to false positives, as ideally we would want to check if `extra()` is tied to a [Django queryset](https://docs.djangoproject.com/en/5.0/ref/models/querysets/), but AFAIK Ruff is not able to resolve classes outside of the current module. ## Test Plan Snapshot tests
This commit is contained in:
Mathieu Kniewallner
GitHub
parent
f8f56186b3
commit
fc7139d9a5
8 changed files with 228 additions and 0 deletions
|
|
@ -632,6 +632,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
|
|||
]) {
|
||||
flake8_bandit::rules::shell_injection(checker, call);
|
||||
}
|
||||
if checker.enabled(Rule::DjangoExtra) {
|
||||
flake8_bandit::rules::django_extra(checker, call);
|
||||
}
|
||||
if checker.enabled(Rule::DjangoRawSql) {
|
||||
flake8_bandit::rules::django_raw_sql(checker, call);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -680,6 +680,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
|
|||
(Flake8Bandit, "607") => (RuleGroup::Stable, rules::flake8_bandit::rules::StartProcessWithPartialPath),
|
||||
(Flake8Bandit, "608") => (RuleGroup::Stable, rules::flake8_bandit::rules::HardcodedSQLExpression),
|
||||
(Flake8Bandit, "609") => (RuleGroup::Stable, rules::flake8_bandit::rules::UnixCommandWildcardInjection),
|
||||
(Flake8Bandit, "610") => (RuleGroup::Preview, rules::flake8_bandit::rules::DjangoExtra),
|
||||
(Flake8Bandit, "611") => (RuleGroup::Stable, rules::flake8_bandit::rules::DjangoRawSql),
|
||||
(Flake8Bandit, "612") => (RuleGroup::Stable, rules::flake8_bandit::rules::LoggingConfigInsecureListen),
|
||||
(Flake8Bandit, "701") => (RuleGroup::Stable, rules::flake8_bandit::rules::Jinja2AutoescapeFalse),
|
||||
|
|
|
|||
|
|
@ -69,6 +69,7 @@ mod tests {
|
|||
#[test_case(Rule::UnixCommandWildcardInjection, Path::new("S609.py"))]
|
||||
#[test_case(Rule::UnsafeYAMLLoad, Path::new("S506.py"))]
|
||||
#[test_case(Rule::WeakCryptographicKey, Path::new("S505.py"))]
|
||||
#[test_case(Rule::DjangoExtra, Path::new("S610.py"))]
|
||||
#[test_case(Rule::DjangoRawSql, Path::new("S611.py"))]
|
||||
#[test_case(Rule::TarfileUnsafeMembers, Path::new("S202.py"))]
|
||||
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
|
||||
|
|
|
|||
|
|
@ -0,0 +1,81 @@
|
|||
use ruff_diagnostics::{Diagnostic, Violation};
|
||||
use ruff_macros::{derive_message_formats, violation};
|
||||
use ruff_python_ast::{self as ast, Expr, ExprAttribute, ExprDict, ExprList};
|
||||
use ruff_text_size::Ranged;
|
||||
|
||||
use crate::checkers::ast::Checker;
|
||||
|
||||
/// ## What it does
|
||||
/// Checks for uses of Django's `extra` function.
|
||||
///
|
||||
/// ## Why is this bad?
|
||||
/// Django's `extra` function can be used to execute arbitrary SQL queries,
|
||||
/// which can in turn lead to SQL injection vulnerabilities.
|
||||
///
|
||||
/// ## Example
|
||||
/// ```python
|
||||
/// from django.contrib.auth.models import User
|
||||
///
|
||||
/// User.objects.all().extra(select={"test": "%secure" % "nos"})
|
||||
/// ```
|
||||
///
|
||||
/// ## References
|
||||
/// - [Django documentation: SQL injection protection](https://docs.djangoproject.com/en/dev/topics/security/#sql-injection-protection)
|
||||
/// - [Common Weakness Enumeration: CWE-89](https://cwe.mitre.org/data/definitions/89.html)
|
||||
#[violation]
|
||||
pub struct DjangoExtra;
|
||||
|
||||
impl Violation for DjangoExtra {
|
||||
#[derive_message_formats]
|
||||
fn message(&self) -> String {
|
||||
format!("Use of Django `extra` can lead to SQL injection vulnerabilities")
|
||||
}
|
||||
}
|
||||
|
||||
/// S610
|
||||
pub(crate) fn django_extra(checker: &mut Checker, call: &ast::ExprCall) {
|
||||
let Expr::Attribute(ExprAttribute { attr, .. }) = call.func.as_ref() else {
|
||||
return;
|
||||
};
|
||||
|
||||
if attr.as_str() != "extra" {
|
||||
return;
|
||||
}
|
||||
|
||||
if is_call_insecure(call) {
|
||||
checker
|
||||
.diagnostics
|
||||
.push(Diagnostic::new(DjangoExtra, call.arguments.range()));
|
||||
}
|
||||
}
|
||||
|
||||
fn is_call_insecure(call: &ast::ExprCall) -> bool {
|
||||
for (argument_name, position) in [("select", 0), ("where", 1), ("tables", 3)] {
|
||||
if let Some(argument) = call.arguments.find_argument(argument_name, position) {
|
||||
match argument_name {
|
||||
"select" => match argument {
|
||||
Expr::Dict(ExprDict { keys, values, .. }) => {
|
||||
if !keys.iter().flatten().all(Expr::is_string_literal_expr) {
|
||||
return true;
|
||||
}
|
||||
if !values.iter().all(Expr::is_string_literal_expr) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
_ => return true,
|
||||
},
|
||||
"where" | "tables" => match argument {
|
||||
Expr::List(ExprList { elts, .. }) => {
|
||||
if !elts.iter().all(Expr::is_string_literal_expr) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
_ => return true,
|
||||
},
|
||||
_ => (),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
false
|
||||
}
|
||||
|
|
@ -1,5 +1,6 @@
|
|||
pub(crate) use assert_used::*;
|
||||
pub(crate) use bad_file_permissions::*;
|
||||
pub(crate) use django_extra::*;
|
||||
pub(crate) use django_raw_sql::*;
|
||||
pub(crate) use exec_used::*;
|
||||
pub(crate) use flask_debug_true::*;
|
||||
|
|
@ -33,6 +34,7 @@ pub(crate) use weak_cryptographic_key::*;
|
|||
|
||||
mod assert_used;
|
||||
mod bad_file_permissions;
|
||||
mod django_extra;
|
||||
mod django_raw_sql;
|
||||
mod exec_used;
|
||||
mod flask_debug_true;
|
||||
|
|
|
|||
|
|
@ -0,0 +1,105 @@
|
|||
---
|
||||
source: crates/ruff_linter/src/rules/flake8_bandit/mod.rs
|
||||
---
|
||||
S610.py:4:44: S610 Use of Django `extra` can lead to SQL injection vulnerabilities
|
||||
|
|
||||
3 | # Errors
|
||||
4 | User.objects.filter(username='admin').extra(dict(could_be='insecure'))
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ S610
|
||||
5 | User.objects.filter(username='admin').extra(select=dict(could_be='insecure'))
|
||||
6 | User.objects.filter(username='admin').extra(select={'test': '%secure' % 'nos'})
|
||||
|
|
||||
|
||||
S610.py:5:44: S610 Use of Django `extra` can lead to SQL injection vulnerabilities
|
||||
|
|
||||
3 | # Errors
|
||||
4 | User.objects.filter(username='admin').extra(dict(could_be='insecure'))
|
||||
5 | User.objects.filter(username='admin').extra(select=dict(could_be='insecure'))
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S610
|
||||
6 | User.objects.filter(username='admin').extra(select={'test': '%secure' % 'nos'})
|
||||
7 | User.objects.filter(username='admin').extra(select={'test': '{}secure'.format('nos')})
|
||||
|
|
||||
|
||||
S610.py:6:44: S610 Use of Django `extra` can lead to SQL injection vulnerabilities
|
||||
|
|
||||
4 | User.objects.filter(username='admin').extra(dict(could_be='insecure'))
|
||||
5 | User.objects.filter(username='admin').extra(select=dict(could_be='insecure'))
|
||||
6 | User.objects.filter(username='admin').extra(select={'test': '%secure' % 'nos'})
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S610
|
||||
7 | User.objects.filter(username='admin').extra(select={'test': '{}secure'.format('nos')})
|
||||
8 | User.objects.filter(username='admin').extra(where=['%secure' % 'nos'])
|
||||
|
|
||||
|
||||
S610.py:7:44: S610 Use of Django `extra` can lead to SQL injection vulnerabilities
|
||||
|
|
||||
5 | User.objects.filter(username='admin').extra(select=dict(could_be='insecure'))
|
||||
6 | User.objects.filter(username='admin').extra(select={'test': '%secure' % 'nos'})
|
||||
7 | User.objects.filter(username='admin').extra(select={'test': '{}secure'.format('nos')})
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S610
|
||||
8 | User.objects.filter(username='admin').extra(where=['%secure' % 'nos'])
|
||||
9 | User.objects.filter(username='admin').extra(where=['{}secure'.format('no')])
|
||||
|
|
||||
|
||||
S610.py:8:44: S610 Use of Django `extra` can lead to SQL injection vulnerabilities
|
||||
|
|
||||
6 | User.objects.filter(username='admin').extra(select={'test': '%secure' % 'nos'})
|
||||
7 | User.objects.filter(username='admin').extra(select={'test': '{}secure'.format('nos')})
|
||||
8 | User.objects.filter(username='admin').extra(where=['%secure' % 'nos'])
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ S610
|
||||
9 | User.objects.filter(username='admin').extra(where=['{}secure'.format('no')])
|
||||
|
|
||||
|
||||
S610.py:9:44: S610 Use of Django `extra` can lead to SQL injection vulnerabilities
|
||||
|
|
||||
7 | User.objects.filter(username='admin').extra(select={'test': '{}secure'.format('nos')})
|
||||
8 | User.objects.filter(username='admin').extra(where=['%secure' % 'nos'])
|
||||
9 | User.objects.filter(username='admin').extra(where=['{}secure'.format('no')])
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S610
|
||||
10 |
|
||||
11 | query = '"username") AS "username", * FROM "auth_user" WHERE 1=1 OR "username"=? --'
|
||||
|
|
||||
|
||||
S610.py:12:44: S610 Use of Django `extra` can lead to SQL injection vulnerabilities
|
||||
|
|
||||
11 | query = '"username") AS "username", * FROM "auth_user" WHERE 1=1 OR "username"=? --'
|
||||
12 | User.objects.filter(username='admin').extra(select={'test': query})
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^ S610
|
||||
13 |
|
||||
14 | where_var = ['1=1) OR 1=1 AND (1=1']
|
||||
|
|
||||
|
||||
S610.py:15:44: S610 Use of Django `extra` can lead to SQL injection vulnerabilities
|
||||
|
|
||||
14 | where_var = ['1=1) OR 1=1 AND (1=1']
|
||||
15 | User.objects.filter(username='admin').extra(where=where_var)
|
||||
| ^^^^^^^^^^^^^^^^^ S610
|
||||
16 |
|
||||
17 | where_str = '1=1) OR 1=1 AND (1=1'
|
||||
|
|
||||
|
||||
S610.py:18:44: S610 Use of Django `extra` can lead to SQL injection vulnerabilities
|
||||
|
|
||||
17 | where_str = '1=1) OR 1=1 AND (1=1'
|
||||
18 | User.objects.filter(username='admin').extra(where=[where_str])
|
||||
| ^^^^^^^^^^^^^^^^^^^ S610
|
||||
19 |
|
||||
20 | tables_var = ['django_content_type" WHERE "auth_user"."username"="admin']
|
||||
|
|
||||
|
||||
S610.py:21:25: S610 Use of Django `extra` can lead to SQL injection vulnerabilities
|
||||
|
|
||||
20 | tables_var = ['django_content_type" WHERE "auth_user"."username"="admin']
|
||||
21 | User.objects.all().extra(tables=tables_var).distinct()
|
||||
| ^^^^^^^^^^^^^^^^^^^ S610
|
||||
22 |
|
||||
23 | tables_str = 'django_content_type" WHERE "auth_user"."username"="admin'
|
||||
|
|
||||
|
||||
S610.py:24:25: S610 Use of Django `extra` can lead to SQL injection vulnerabilities
|
||||
|
|
||||
23 | tables_str = 'django_content_type" WHERE "auth_user"."username"="admin'
|
||||
24 | User.objects.all().extra(tables=[tables_str]).distinct()
|
||||
| ^^^^^^^^^^^^^^^^^^^^^ S610
|
||||
25 |
|
||||
26 | # OK
|
||||
|
|
||||
Loading…
Add table
Add a link
Reference in a new issue