From 0a85ad5eafc84d09191ad7b6ff1bba8492575709 Mon Sep 17 00:00:00 2001 From: Chaojie Date: Mon, 13 Nov 2023 18:45:38 +0800 Subject: [PATCH 1/2] [flake8-bandit] Implement django-raw-sql (S611) --- .../test/fixtures/flake8_bandit/S611.py | 13 ++++ .../src/checkers/ast/analyze/expression.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + .../src/rules/flake8_bandit/mod.rs | 1 + .../flake8_bandit/rules/django_raw_sql.rs | 60 +++++++++++++++++++ .../src/rules/flake8_bandit/rules/mod.rs | 2 + ...s__flake8_bandit__tests__S611_S611.py.snap | 60 +++++++++++++++++++ ruff.schema.json | 1 + 8 files changed, 141 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_bandit/S611.py create mode 100644 crates/ruff_linter/src/rules/flake8_bandit/rules/django_raw_sql.rs create mode 100644 crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S611_S611.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S611.py b/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S611.py new file mode 100644 index 0000000000000..ee4230273582f --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S611.py @@ -0,0 +1,13 @@ +from django.db.models.expressions import RawSQL +from django.contrib.auth.models import User + +User.objects.annotate(val=RawSQL('secure', [])) +User.objects.annotate(val=RawSQL('%secure' % 'nos', [])) +User.objects.annotate(val=RawSQL('{}secure'.format('no'), [])) +raw = '"username") AS "val" FROM "auth_user" WHERE "username"="admin" --' +User.objects.annotate(val=RawSQL(raw, [])) +raw = '"username") AS "val" FROM "auth_user"' \ + ' WHERE "username"="admin" OR 1=%s --' +User.objects.annotate(val=RawSQL(raw, [0])) +User.objects.annotate(val=RawSQL(sql='{}secure'.format('no'), params=[])) +User.objects.annotate(val=RawSQL(params=[], sql='{}secure'.format('no'))) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 8ce687e0f6a73..313218cca2ded 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -612,6 +612,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { ]) { flake8_bandit::rules::shell_injection(checker, call); } + if checker.enabled(Rule::DjangoRawSql) { + flake8_bandit::rules::django_raw_sql(checker, call); + } if checker.enabled(Rule::UnnecessaryGeneratorList) { flake8_comprehensions::rules::unnecessary_generator_list( checker, expr, func, args, keywords, diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 5e23a2e13887e..7a71f6b5bbbc9 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -632,6 +632,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, "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), (Flake8Bandit, "702") => (RuleGroup::Preview, rules::flake8_bandit::rules::MakoTemplates), diff --git a/crates/ruff_linter/src/rules/flake8_bandit/mod.rs b/crates/ruff_linter/src/rules/flake8_bandit/mod.rs index 3ad7a8e421b41..feb2dcb507651 100644 --- a/crates/ruff_linter/src/rules/flake8_bandit/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_bandit/mod.rs @@ -50,6 +50,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::DjangoRawSql, Path::new("S611.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/flake8_bandit/rules/django_raw_sql.rs b/crates/ruff_linter/src/rules/flake8_bandit/rules/django_raw_sql.rs new file mode 100644 index 0000000000000..33bc08c0edc6b --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_bandit/rules/django_raw_sql.rs @@ -0,0 +1,60 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{self as ast}; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for Django that use `RawSQL` function. +/// +/// ## Why is this bad? +/// Django `RawSQL` function can cause SQL injection attack. +/// +/// ## Example +/// ```python +/// from django.db.models.expressions import RawSQL +/// from django.contrib.auth.models import User +/// +/// User.objects.annotate(val=RawSQL("%secure" % "nos", [])) +/// ``` +/// +/// ## References +/// - [Django documentation: API](https://docs.djangoproject.com/en/dev/ref/models/expressions/#django.db.models.expressions.RawSQL) +/// - [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 DjangoRawSql; + +impl Violation for DjangoRawSql { + #[derive_message_formats] + fn message(&self) -> String { + format!("Use of RawSQL potential SQL attack vector.") + } +} + +/// S611 +pub(crate) fn django_raw_sql(checker: &mut Checker, call: &ast::ExprCall) { + if checker + .semantic() + .resolve_call_path(&call.func) + .is_some_and(|call_path| { + matches!( + call_path.as_slice(), + ["django", "db", "models", "expressions", "RawSQL"] + ) + }) + { + let sql = if let Some(arg) = call.arguments.find_argument("sql", 0) { + arg + } else { + &call.arguments.find_keyword("sql").unwrap().value + }; + + if !sql.is_string_literal_expr() { + checker + .diagnostics + .push(Diagnostic::new(DjangoRawSql, call.func.range())); + } + } +} 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 b1ebad53e41ca..b7a655366876f 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_raw_sql::*; pub(crate) use exec_used::*; pub(crate) use flask_debug_true::*; pub(crate) use hardcoded_bind_all_interfaces::*; @@ -27,6 +28,7 @@ pub(crate) use weak_cryptographic_key::*; mod assert_used; mod bad_file_permissions; +mod django_raw_sql; mod exec_used; mod flask_debug_true; mod hardcoded_bind_all_interfaces; diff --git a/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S611_S611.py.snap b/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S611_S611.py.snap new file mode 100644 index 0000000000000..447a83a7536ba --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S611_S611.py.snap @@ -0,0 +1,60 @@ +--- +source: crates/ruff_linter/src/rules/flake8_bandit/mod.rs +--- +S611.py:5:27: S611 Use of RawSQL potential SQL attack vector. + | +4 | User.objects.annotate(val=RawSQL('secure', [])) +5 | User.objects.annotate(val=RawSQL('%secure' % 'nos', [])) + | ^^^^^^ S611 +6 | User.objects.annotate(val=RawSQL('{}secure'.format('no'), [])) +7 | raw = '"username") AS "val" FROM "auth_user" WHERE "username"="admin" --' + | + +S611.py:6:27: S611 Use of RawSQL potential SQL attack vector. + | +4 | User.objects.annotate(val=RawSQL('secure', [])) +5 | User.objects.annotate(val=RawSQL('%secure' % 'nos', [])) +6 | User.objects.annotate(val=RawSQL('{}secure'.format('no'), [])) + | ^^^^^^ S611 +7 | raw = '"username") AS "val" FROM "auth_user" WHERE "username"="admin" --' +8 | User.objects.annotate(val=RawSQL(raw, [])) + | + +S611.py:8:27: S611 Use of RawSQL potential SQL attack vector. + | + 6 | User.objects.annotate(val=RawSQL('{}secure'.format('no'), [])) + 7 | raw = '"username") AS "val" FROM "auth_user" WHERE "username"="admin" --' + 8 | User.objects.annotate(val=RawSQL(raw, [])) + | ^^^^^^ S611 + 9 | raw = '"username") AS "val" FROM "auth_user"' \ +10 | ' WHERE "username"="admin" OR 1=%s --' + | + +S611.py:11:27: S611 Use of RawSQL potential SQL attack vector. + | + 9 | raw = '"username") AS "val" FROM "auth_user"' \ +10 | ' WHERE "username"="admin" OR 1=%s --' +11 | User.objects.annotate(val=RawSQL(raw, [0])) + | ^^^^^^ S611 +12 | User.objects.annotate(val=RawSQL(sql='{}secure'.format('no'), params=[])) +13 | User.objects.annotate(val=RawSQL(params=[], sql='{}secure'.format('no'))) + | + +S611.py:12:27: S611 Use of RawSQL potential SQL attack vector. + | +10 | ' WHERE "username"="admin" OR 1=%s --' +11 | User.objects.annotate(val=RawSQL(raw, [0])) +12 | User.objects.annotate(val=RawSQL(sql='{}secure'.format('no'), params=[])) + | ^^^^^^ S611 +13 | User.objects.annotate(val=RawSQL(params=[], sql='{}secure'.format('no'))) + | + +S611.py:13:27: S611 Use of RawSQL potential SQL attack vector. + | +11 | User.objects.annotate(val=RawSQL(raw, [0])) +12 | User.objects.annotate(val=RawSQL(sql='{}secure'.format('no'), params=[])) +13 | User.objects.annotate(val=RawSQL(params=[], sql='{}secure'.format('no'))) + | ^^^^^^ S611 + | + + diff --git a/ruff.schema.json b/ruff.schema.json index a3c30890c6f16..8ac638a4c11d1 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3396,6 +3396,7 @@ "S608", "S609", "S61", + "S611", "S612", "S7", "S70", From 2ab2876bdedadf9749cb11381a79646bb5df1f67 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 20 Nov 2023 12:09:49 +0000 Subject: [PATCH 2/2] Tweak logic --- crates/ruff_linter/src/codes.rs | 2 +- .../flake8_bandit/rules/django_raw_sql.rs | 26 +++++++++---------- ...s__flake8_bandit__tests__S611_S611.py.snap | 12 ++++----- 3 files changed, 19 insertions(+), 21 deletions(-) diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 7a71f6b5bbbc9..759ccc54ac49e 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -632,7 +632,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, "611") => (RuleGroup::Stable, rules::flake8_bandit::rules::DjangoRawSql), + (Flake8Bandit, "611") => (RuleGroup::Preview, rules::flake8_bandit::rules::DjangoRawSql), (Flake8Bandit, "612") => (RuleGroup::Stable, rules::flake8_bandit::rules::LoggingConfigInsecureListen), (Flake8Bandit, "701") => (RuleGroup::Stable, rules::flake8_bandit::rules::Jinja2AutoescapeFalse), (Flake8Bandit, "702") => (RuleGroup::Preview, rules::flake8_bandit::rules::MakoTemplates), diff --git a/crates/ruff_linter/src/rules/flake8_bandit/rules/django_raw_sql.rs b/crates/ruff_linter/src/rules/flake8_bandit/rules/django_raw_sql.rs index 33bc08c0edc6b..1491894d5b3ee 100644 --- a/crates/ruff_linter/src/rules/flake8_bandit/rules/django_raw_sql.rs +++ b/crates/ruff_linter/src/rules/flake8_bandit/rules/django_raw_sql.rs @@ -1,27 +1,27 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::{self as ast}; +use ruff_python_ast::{self as ast, Expr}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; /// ## What it does -/// Checks for Django that use `RawSQL` function. +/// Checks for uses of Django's `RawSQL` function. /// /// ## Why is this bad? -/// Django `RawSQL` function can cause SQL injection attack. +/// Django's `RawSQL` function can be used to execute arbitrary SQL queries, +/// which can in turn lead to SQL injection vulnerabilities. /// /// ## Example /// ```python /// from django.db.models.expressions import RawSQL /// from django.contrib.auth.models import User /// -/// User.objects.annotate(val=RawSQL("%secure" % "nos", [])) +/// User.objects.annotate(val=("%secure" % "nos", [])) /// ``` /// /// ## References -/// - [Django documentation: API](https://docs.djangoproject.com/en/dev/ref/models/expressions/#django.db.models.expressions.RawSQL) -/// - [Django documentation: sql injection protection](https://docs.djangoproject.com/en/dev/topics/security/#sql-injection-protection) +/// - [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 DjangoRawSql; @@ -29,7 +29,7 @@ pub struct DjangoRawSql; impl Violation for DjangoRawSql { #[derive_message_formats] fn message(&self) -> String { - format!("Use of RawSQL potential SQL attack vector.") + format!("Use of `RawSQL` can lead to SQL injection vulnerabilities") } } @@ -45,13 +45,11 @@ pub(crate) fn django_raw_sql(checker: &mut Checker, call: &ast::ExprCall) { ) }) { - let sql = if let Some(arg) = call.arguments.find_argument("sql", 0) { - arg - } else { - &call.arguments.find_keyword("sql").unwrap().value - }; - - if !sql.is_string_literal_expr() { + if !call + .arguments + .find_argument("sql", 0) + .is_some_and(Expr::is_string_literal_expr) + { checker .diagnostics .push(Diagnostic::new(DjangoRawSql, call.func.range())); diff --git a/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S611_S611.py.snap b/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S611_S611.py.snap index 447a83a7536ba..026360e756ff1 100644 --- a/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S611_S611.py.snap +++ b/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S611_S611.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff_linter/src/rules/flake8_bandit/mod.rs --- -S611.py:5:27: S611 Use of RawSQL potential SQL attack vector. +S611.py:5:27: S611 Use of `RawSQL` can lead to SQL injection vulnerabilities | 4 | User.objects.annotate(val=RawSQL('secure', [])) 5 | User.objects.annotate(val=RawSQL('%secure' % 'nos', [])) @@ -10,7 +10,7 @@ S611.py:5:27: S611 Use of RawSQL potential SQL attack vector. 7 | raw = '"username") AS "val" FROM "auth_user" WHERE "username"="admin" --' | -S611.py:6:27: S611 Use of RawSQL potential SQL attack vector. +S611.py:6:27: S611 Use of `RawSQL` can lead to SQL injection vulnerabilities | 4 | User.objects.annotate(val=RawSQL('secure', [])) 5 | User.objects.annotate(val=RawSQL('%secure' % 'nos', [])) @@ -20,7 +20,7 @@ S611.py:6:27: S611 Use of RawSQL potential SQL attack vector. 8 | User.objects.annotate(val=RawSQL(raw, [])) | -S611.py:8:27: S611 Use of RawSQL potential SQL attack vector. +S611.py:8:27: S611 Use of `RawSQL` can lead to SQL injection vulnerabilities | 6 | User.objects.annotate(val=RawSQL('{}secure'.format('no'), [])) 7 | raw = '"username") AS "val" FROM "auth_user" WHERE "username"="admin" --' @@ -30,7 +30,7 @@ S611.py:8:27: S611 Use of RawSQL potential SQL attack vector. 10 | ' WHERE "username"="admin" OR 1=%s --' | -S611.py:11:27: S611 Use of RawSQL potential SQL attack vector. +S611.py:11:27: S611 Use of `RawSQL` can lead to SQL injection vulnerabilities | 9 | raw = '"username") AS "val" FROM "auth_user"' \ 10 | ' WHERE "username"="admin" OR 1=%s --' @@ -40,7 +40,7 @@ S611.py:11:27: S611 Use of RawSQL potential SQL attack vector. 13 | User.objects.annotate(val=RawSQL(params=[], sql='{}secure'.format('no'))) | -S611.py:12:27: S611 Use of RawSQL potential SQL attack vector. +S611.py:12:27: S611 Use of `RawSQL` can lead to SQL injection vulnerabilities | 10 | ' WHERE "username"="admin" OR 1=%s --' 11 | User.objects.annotate(val=RawSQL(raw, [0])) @@ -49,7 +49,7 @@ S611.py:12:27: S611 Use of RawSQL potential SQL attack vector. 13 | User.objects.annotate(val=RawSQL(params=[], sql='{}secure'.format('no'))) | -S611.py:13:27: S611 Use of RawSQL potential SQL attack vector. +S611.py:13:27: S611 Use of `RawSQL` can lead to SQL injection vulnerabilities | 11 | User.objects.annotate(val=RawSQL(raw, [0])) 12 | User.objects.annotate(val=RawSQL(sql='{}secure'.format('no'), params=[]))