Skip to content

Commit

Permalink
[flake8-bandit] Implement django-raw-sql (S611) (#8651)
Browse files Browse the repository at this point in the history
See: #1646.
  • Loading branch information
ischaojie committed Nov 20, 2023
1 parent 04f0625 commit 653e51a
Show file tree
Hide file tree
Showing 8 changed files with 139 additions and 0 deletions.
13 changes: 13 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_bandit/S611.py
Original file line number Diff line number Diff line change
@@ -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')))
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::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),
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 @@ -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(
Expand Down
58 changes: 58 additions & 0 deletions crates/ruff_linter/src/rules/flake8_bandit/rules/django_raw_sql.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr};
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for uses of Django's `RawSQL` function.
///
/// ## Why is this bad?
/// 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=("%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 DjangoRawSql;

impl Violation for DjangoRawSql {
#[derive_message_formats]
fn message(&self) -> String {
format!("Use of `RawSQL` can lead to SQL injection vulnerabilities")
}
}

/// 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"]
)
})
{
if !call
.arguments
.find_argument("sql", 0)
.is_some_and(Expr::is_string_literal_expr)
{
checker
.diagnostics
.push(Diagnostic::new(DjangoRawSql, call.func.range()));
}
}
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/flake8_bandit/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -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::*;
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
---
source: crates/ruff_linter/src/rules/flake8_bandit/mod.rs
---
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', []))
| ^^^^^^ 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` can lead to SQL injection vulnerabilities
|
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` 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" --'
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` can lead to SQL injection vulnerabilities
|
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` can lead to SQL injection vulnerabilities
|
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` 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=[]))
13 | User.objects.annotate(val=RawSQL(params=[], sql='{}secure'.format('no')))
| ^^^^^^ S611
|


1 change: 1 addition & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 653e51a

Please sign in to comment.