Skip to content

Commit

Permalink
feat(rules): add flake8-bandit S610
Browse files Browse the repository at this point in the history
  • Loading branch information
mkniewallner committed Mar 9, 2024
1 parent 0c84fbb commit eb10243
Show file tree
Hide file tree
Showing 8 changed files with 234 additions and 1 deletion.
34 changes: 34 additions & 0 deletions 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'])
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Expand Up @@ -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);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Expand Up @@ -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),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/flake8_bandit/mod.rs
Expand Up @@ -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<()> {
Expand Down
86 changes: 86 additions & 0 deletions crates/ruff_linter/src/rules/flake8_bandit/rules/django_extra.rs
@@ -0,0 +1,86 @@
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_python_semantic::Modules;
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 `extra` can lead to SQL injection vulnerabilities")
}
}

/// S610
pub(crate) fn django_extra(checker: &mut Checker, call: &ast::ExprCall) {
if !checker.semantic().seen_module(Modules::DJANGO) {
return;
}

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
}
2 changes: 2 additions & 0 deletions 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::*;
Expand Down Expand Up @@ -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;
Expand Down
@@ -0,0 +1,105 @@
---
source: crates/ruff_linter/src/rules/flake8_bandit/mod.rs
---
S610.py:4:44: S610 Use of `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 `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 `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 `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 `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 `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 `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 `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 `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 `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 `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
|
3 changes: 2 additions & 1 deletion ruff.schema.json

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

0 comments on commit eb10243

Please sign in to comment.