diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S610.py b/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S610.py new file mode 100644 index 0000000000000..17844b15a16e4 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S610.py @@ -0,0 +1,34 @@ +from django.contrib.auth.models import User + +# Errors +User.objects.filter(username='admin').extra(dict(could_be='insecure')) +User.objects.filter(username='admin').extra(select=dict(could_be='insecure')) +User.objects.filter(username='admin').extra(select={'test': '%secure' % 'nos'}) +User.objects.filter(username='admin').extra(select={'test': '{}secure'.format('nos')}) +User.objects.filter(username='admin').extra(where=['%secure' % 'nos']) +User.objects.filter(username='admin').extra(where=['{}secure'.format('no')]) + +query = '"username") AS "username", * FROM "auth_user" WHERE 1=1 OR "username"=? --' +User.objects.filter(username='admin').extra(select={'test': query}) + +where_var = ['1=1) OR 1=1 AND (1=1'] +User.objects.filter(username='admin').extra(where=where_var) + +where_str = '1=1) OR 1=1 AND (1=1' +User.objects.filter(username='admin').extra(where=[where_str]) + +tables_var = ['django_content_type" WHERE "auth_user"."username"="admin'] +User.objects.all().extra(tables=tables_var).distinct() + +tables_str = 'django_content_type" WHERE "auth_user"."username"="admin' +User.objects.all().extra(tables=[tables_str]).distinct() + +# OK +User.objects.filter(username='admin').extra( + select={'test': 'secure'}, + where=['secure'], + tables=['secure'] +) +User.objects.filter(username='admin').extra({'test': 'secure'}) +User.objects.filter(username='admin').extra(select={'test': 'secure'}) +User.objects.filter(username='admin').extra(where=['secure']) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index e3b7f0a777d22..9ca9a6df71838 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -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); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 4f545d1afae88..0892a9e3992fc 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -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), diff --git a/crates/ruff_linter/src/rules/flake8_bandit/mod.rs b/crates/ruff_linter/src/rules/flake8_bandit/mod.rs index ec2a462aafbbd..08c4e96e836b7 100644 --- a/crates/ruff_linter/src/rules/flake8_bandit/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_bandit/mod.rs @@ -68,6 +68,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<()> { diff --git a/crates/ruff_linter/src/rules/flake8_bandit/rules/django_extra.rs b/crates/ruff_linter/src/rules/flake8_bandit/rules/django_extra.rs new file mode 100644 index 0000000000000..34ceaf9070830 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_bandit/rules/django_extra.rs @@ -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 +} 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 788d512e5c5d5..0c9a5953e0b5f 100644 --- a/crates/ruff_linter/src/rules/flake8_bandit/rules/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_bandit/rules/mod.rs @@ -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; diff --git a/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S610_S610.py.snap b/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S610_S610.py.snap new file mode 100644 index 0000000000000..deb3bdcf182cf --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S610_S610.py.snap @@ -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 + | diff --git a/ruff.schema.json b/ruff.schema.json index 7509cb026b855..3b45537725cf5 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3639,6 +3639,7 @@ "S608", "S609", "S61", + "S610", "S611", "S612", "S7",