diff --git a/README.md b/README.md index b83908ab43..0f253555e2 100644 --- a/README.md +++ b/README.md @@ -934,6 +934,7 @@ For more, see [flake8-bandit](https://pypi.org/project/flake8-bandit/) on PyPI. | S506 | unsafe-yaml-load | Probable use of unsafe loader `{name}` with `yaml.load`. Allows instantiation of arbitrary objects. Consider `yaml.safe_load`. | | | S508 | snmp-insecure-version | The use of SNMPv1 and SNMPv2 is insecure. Use SNMPv3 if able. | | | S509 | snmp-weak-cryptography | You should not use SNMPv3 without encryption. `noAuthNoPriv` & `authNoPriv` is insecure. | | +| S608 | [hardcoded-sql-expression](https://github.com/charliermarsh/ruff/blob/main/docs/rules/hardcoded-sql-expression.md) | Possible SQL injection vector through string-based query construction: "{}" | | | S612 | logging-config-insecure-listen | Use of insecure `logging.config.listen` detected | | | S701 | jinja2-autoescape-false | Using jinja2 templates with `autoescape=False` is dangerous and can lead to XSS. Ensure `autoescape=True` or use the `select_autoescape` function. | | diff --git a/crates/ruff/resources/test/fixtures/flake8_bandit/S608.py b/crates/ruff/resources/test/fixtures/flake8_bandit/S608.py new file mode 100644 index 0000000000..4592f46e4f --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_bandit/S608.py @@ -0,0 +1,95 @@ +# single-line failures +query1 = "SELECT %s FROM table" % (var,) # bad +query2 = "SELECT var FROM " + table +query3 = "SELECT " + val + " FROM " + table +query4 = "SELECT {} FROM table;".format(var) +query5 = f"SELECT * FROM table WHERE var = {var}" + +query6 = "DELETE FROM table WHERE var = %s" % (var,) +query7 = "DELETE FROM table WHERE VAR = " + var +query8 = "DELETE FROM " + table + "WHERE var = " + var +query9 = "DELETE FROM table WHERE var = {}".format(var) +query10 = f"DELETE FROM table WHERE var = {var}" + +query11 = "INSERT INTO table VALUES (%s)" % (var,) +query12 = "INSERT INTO TABLE VALUES (" + var + ")" +query13 = "INSERT INTO {} VALUES ({})".format(table, var) +query14 = f"INSERT INTO {table} VALUES var = {var}" + +query15 = "UPDATE %s SET var = %s" % (table, var) +query16 = "UPDATE " + table + " SET var = " + var +query17 = "UPDATE {} SET var = {}".format(table, var) +query18 = f"UPDATE {table} SET var = {var}" + +query19 = "select %s from table" % (var,) +query20 = "select var from " + table +query21 = "select " + val + " from " + table +query22 = "select {} from table;".format(var) +query23 = f"select * from table where var = {var}" + +query24 = "delete from table where var = %s" % (var,) +query25 = "delete from table where var = " + var +query26 = "delete from " + table + "where var = " + var +query27 = "delete from table where var = {}".format(var) +query28 = f"delete from table where var = {var}" + +query29 = "insert into table values (%s)" % (var,) +query30 = "insert into table values (" + var + ")" +query31 = "insert into {} values ({})".format(table, var) +query32 = f"insert into {table} values var = {var}" + +query33 = "update %s set var = %s" % (table, var) +query34 = "update " + table + " set var = " + var +query35 = "update {} set var = {}".format(table, var) +query36 = f"update {table} set var = {var}" + +# multi-line failures +def query37(): + return """ + SELECT * + FROM table + WHERE var = %s + """ % var + +def query38(): + return """ + SELECT * + FROM TABLE + WHERE var = + """ + var + +def query39(): + return """ + SELECT * + FROM table + WHERE var = {} + """.format(var) + +def query40(): + return f""" + SELECT * + FROM table + WHERE var = {var} + """ + +def query41(): + return ( + "SELECT *" + "FROM table" + f"WHERE var = {var}" + ) + +# # cursor-wrapped failures +query42 = cursor.execute("SELECT * FROM table WHERE var = %s" % var) +query43 = cursor.execute(f"SELECT * FROM table WHERE var = {var}") +query44 = cursor.execute("SELECT * FROM table WHERE var = {}".format(var)) +query45 = cursor.executemany("SELECT * FROM table WHERE var = %s" % var, []) + +# # pass +query = "SELECT * FROM table WHERE id = 1" +query = "DELETE FROM table WHERE id = 1" +query = "INSERT INTO table VALUES (1)" +query = "UPDATE table SET id = 1" +cursor.execute('SELECT * FROM table WHERE id = %s', var) +cursor.execute('SELECT * FROM table WHERE id = 1') +cursor.executemany('SELECT * FROM table WHERE id = %s', [var, var2]) diff --git a/crates/ruff/src/checkers/ast.rs b/crates/ruff/src/checkers/ast.rs index 2481c2970a..5c3be34e06 100644 --- a/crates/ruff/src/checkers/ast.rs +++ b/crates/ruff/src/checkers/ast.rs @@ -2402,6 +2402,9 @@ where self.diagnostics .extend(flake8_bandit::rules::hardcoded_password_func_arg(keywords)); } + if self.settings.rules.enabled(&Rule::HardcodedSQLExpression) { + flake8_bandit::rules::hardcoded_sql_expression(self, expr); + } if self .settings .rules @@ -2809,6 +2812,9 @@ where { pyflakes::rules::f_string_missing_placeholders(expr, values, self); } + if self.settings.rules.enabled(&Rule::HardcodedSQLExpression) { + flake8_bandit::rules::hardcoded_sql_expression(self, expr); + } } ExprKind::BinOp { left, @@ -2970,6 +2976,9 @@ where if self.settings.rules.enabled(&Rule::PrintfStringFormatting) { pyupgrade::rules::printf_string_formatting(self, expr, left, right); } + if self.settings.rules.enabled(&Rule::HardcodedSQLExpression) { + flake8_bandit::rules::hardcoded_sql_expression(self, expr); + } } } ExprKind::BinOp { @@ -2991,6 +3000,9 @@ where { ruff::rules::unpack_instead_of_concatenating_to_collection_literal(self, expr); } + if self.settings.rules.enabled(&Rule::HardcodedSQLExpression) { + flake8_bandit::rules::hardcoded_sql_expression(self, expr); + } } ExprKind::UnaryOp { op, operand } => { let check_not_in = self.settings.rules.enabled(&Rule::NotInTest); diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index cfc4aff296..9a6e55bc8b 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -387,6 +387,7 @@ ruff_macros::define_rule_mapping!( S105 => rules::flake8_bandit::rules::HardcodedPasswordString, S106 => rules::flake8_bandit::rules::HardcodedPasswordFuncArg, S107 => rules::flake8_bandit::rules::HardcodedPasswordDefault, + S608 => rules::flake8_bandit::rules::HardcodedSQLExpression, S108 => rules::flake8_bandit::rules::HardcodedTempFile, S110 => rules::flake8_bandit::rules::TryExceptPass, S112 => rules::flake8_bandit::rules::TryExceptContinue, diff --git a/crates/ruff/src/rules/flake8_bandit/mod.rs b/crates/ruff/src/rules/flake8_bandit/mod.rs index 653a9cad8c..39f12625c6 100644 --- a/crates/ruff/src/rules/flake8_bandit/mod.rs +++ b/crates/ruff/src/rules/flake8_bandit/mod.rs @@ -22,6 +22,7 @@ mod tests { #[test_case(Rule::HardcodedPasswordString, Path::new("S105.py"); "S105")] #[test_case(Rule::HardcodedPasswordFuncArg, Path::new("S106.py"); "S106")] #[test_case(Rule::HardcodedPasswordDefault, Path::new("S107.py"); "S107")] + #[test_case(Rule::HardcodedSQLExpression, Path::new("S608.py"); "S608")] #[test_case(Rule::HardcodedTempFile, Path::new("S108.py"); "S108")] #[test_case(Rule::RequestWithoutTimeout, Path::new("S113.py"); "S113")] #[test_case(Rule::HashlibInsecureHashFunction, Path::new("S324.py"); "S324")] diff --git a/crates/ruff/src/rules/flake8_bandit/rules/hardcoded_sql_expression.rs b/crates/ruff/src/rules/flake8_bandit/rules/hardcoded_sql_expression.rs new file mode 100644 index 0000000000..809bc6bf38 --- /dev/null +++ b/crates/ruff/src/rules/flake8_bandit/rules/hardcoded_sql_expression.rs @@ -0,0 +1,105 @@ +use once_cell::sync::Lazy; +use regex::Regex; +use ruff_macros::{define_violation, derive_message_formats}; +use rustpython_parser::ast::{Expr, ExprKind, Operator}; + +use super::super::helpers::string_literal; +use crate::ast::helpers::{any_over_expr, unparse_expr}; +use crate::ast::types::Range; +use crate::checkers::ast::Checker; +use crate::registry::Diagnostic; +use crate::violation::Violation; + +static SQL_REGEX: Lazy = Lazy::new(|| { + Regex::new(r"(?i)(select\s.*from\s|delete\s+from\s|insert\s+into\s.*values\s|update\s.*set\s)") + .unwrap() +}); + +define_violation!( + /// ### What it does + /// Checks for strings that resemble SQL statements involved in some form + /// string building operation. + /// + /// ### Why is this bad? + /// SQL injection is a common attack vector for web applications. Unless care + /// is taken to sanitize and control the input data when building such + /// SQL statement strings, an injection attack becomes possible. + /// + /// ### Example + /// ```python + /// query = "DELETE FROM foo WHERE id = '%s'" % identifier + /// ``` + pub struct HardcodedSQLExpression { + pub string: String, + } +); +impl Violation for HardcodedSQLExpression { + #[derive_message_formats] + fn message(&self) -> String { + let HardcodedSQLExpression { string } = self; + format!( + "Possible SQL injection vector through string-based query construction: \"{}\"", + string.escape_debug() + ) + } +} + +fn has_string_literal(expr: &Expr) -> bool { + string_literal(expr).is_some() +} + +fn matches_sql_statement(string: &str) -> bool { + SQL_REGEX.is_match(string) +} + +fn unparse_string_format_expression(checker: &mut Checker, expr: &Expr) -> Option { + match &expr.node { + // "select * from table where val = " + "str" + ... + // "select * from table where val = %s" % ... + ExprKind::BinOp { + op: Operator::Add | Operator::Mod, + .. + } => { + let Some(parent) = checker.current_expr_parent() else { + if any_over_expr(expr, &has_string_literal) { + return Some(unparse_expr(expr, checker.stylist)); + } + return None; + }; + // Only evaluate the full BinOp, not the nested components. + let ExprKind::BinOp { .. } = &parent.node else { + if any_over_expr(expr, &has_string_literal) { + return Some(unparse_expr(expr, checker.stylist)); + } + return None; + }; + None + } + ExprKind::Call { func, .. } => { + let ExprKind::Attribute{ attr, value, .. } = &func.node else { + return None; + }; + // "select * from table where val = {}".format(...) + if attr == "format" && string_literal(value).is_some() { + return Some(unparse_expr(expr, checker.stylist)); + }; + None + } + // f"select * from table where val = {val}" + ExprKind::JoinedStr { .. } => Some(unparse_expr(expr, checker.stylist)), + _ => None, + } +} + +/// S608 +pub fn hardcoded_sql_expression(checker: &mut Checker, expr: &Expr) { + match unparse_string_format_expression(checker, expr) { + Some(string) if matches_sql_statement(&string) => { + checker.diagnostics.push(Diagnostic::new( + HardcodedSQLExpression { string }, + Range::from_located(expr), + )); + } + _ => (), + } +} diff --git a/crates/ruff/src/rules/flake8_bandit/rules/mod.rs b/crates/ruff/src/rules/flake8_bandit/rules/mod.rs index 5193948e9c..5b3bb51e65 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/mod.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/mod.rs @@ -9,6 +9,7 @@ pub use hardcoded_password_func_arg::{hardcoded_password_func_arg, HardcodedPass pub use hardcoded_password_string::{ assign_hardcoded_password_string, compare_to_hardcoded_password_string, HardcodedPasswordString, }; +pub use hardcoded_sql_expression::{hardcoded_sql_expression, HardcodedSQLExpression}; pub use hardcoded_tmp_directory::{hardcoded_tmp_directory, HardcodedTempFile}; pub use hashlib_insecure_hash_functions::{ hashlib_insecure_hash_functions, HashlibInsecureHashFunction, @@ -34,6 +35,7 @@ mod hardcoded_bind_all_interfaces; mod hardcoded_password_default; mod hardcoded_password_func_arg; mod hardcoded_password_string; +mod hardcoded_sql_expression; mod hardcoded_tmp_directory; mod hashlib_insecure_hash_functions; mod jinja2_autoescape_false; diff --git a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S608_S608.py.snap b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S608_S608.py.snap new file mode 100644 index 0000000000..de37453884 --- /dev/null +++ b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S608_S608.py.snap @@ -0,0 +1,500 @@ +--- +source: crates/ruff/src/rules/flake8_bandit/mod.rs +expression: diagnostics +--- +- kind: + HardcodedSQLExpression: + string: "\"SELECT %s FROM table\" % (var,)" + location: + row: 2 + column: 9 + end_location: + row: 2 + column: 40 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"SELECT var FROM \" + table" + location: + row: 3 + column: 9 + end_location: + row: 3 + column: 35 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"SELECT \" + val + \" FROM \" + table" + location: + row: 4 + column: 9 + end_location: + row: 4 + column: 43 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"SELECT {} FROM table;\".format(var)" + location: + row: 5 + column: 9 + end_location: + row: 5 + column: 44 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "f\"SELECT * FROM table WHERE var = {var}\"" + location: + row: 6 + column: 9 + end_location: + row: 6 + column: 49 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"DELETE FROM table WHERE var = %s\" % (var,)" + location: + row: 8 + column: 9 + end_location: + row: 8 + column: 52 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"DELETE FROM table WHERE VAR = \" + var" + location: + row: 9 + column: 9 + end_location: + row: 9 + column: 47 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"DELETE FROM \" + table + \"WHERE var = \" + var" + location: + row: 10 + column: 9 + end_location: + row: 10 + column: 54 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"DELETE FROM table WHERE var = {}\".format(var)" + location: + row: 11 + column: 9 + end_location: + row: 11 + column: 55 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "f\"DELETE FROM table WHERE var = {var}\"" + location: + row: 12 + column: 10 + end_location: + row: 12 + column: 48 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"INSERT INTO table VALUES (%s)\" % (var,)" + location: + row: 14 + column: 10 + end_location: + row: 14 + column: 50 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"INSERT INTO TABLE VALUES (\" + var + \")\"" + location: + row: 15 + column: 10 + end_location: + row: 15 + column: 50 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"INSERT INTO {} VALUES ({})\".format(table, var)" + location: + row: 16 + column: 10 + end_location: + row: 16 + column: 57 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "f\"INSERT INTO {table} VALUES var = {var}\"" + location: + row: 17 + column: 10 + end_location: + row: 17 + column: 51 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"UPDATE %s SET var = %s\" % (table, var)" + location: + row: 19 + column: 10 + end_location: + row: 19 + column: 49 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"UPDATE \" + table + \" SET var = \" + var" + location: + row: 20 + column: 10 + end_location: + row: 20 + column: 49 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"UPDATE {} SET var = {}\".format(table, var)" + location: + row: 21 + column: 10 + end_location: + row: 21 + column: 53 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "f\"UPDATE {table} SET var = {var}\"" + location: + row: 22 + column: 10 + end_location: + row: 22 + column: 43 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"select %s from table\" % (var,)" + location: + row: 24 + column: 10 + end_location: + row: 24 + column: 41 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"select var from \" + table" + location: + row: 25 + column: 10 + end_location: + row: 25 + column: 36 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"select \" + val + \" from \" + table" + location: + row: 26 + column: 10 + end_location: + row: 26 + column: 44 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"select {} from table;\".format(var)" + location: + row: 27 + column: 10 + end_location: + row: 27 + column: 45 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "f\"select * from table where var = {var}\"" + location: + row: 28 + column: 10 + end_location: + row: 28 + column: 50 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"delete from table where var = %s\" % (var,)" + location: + row: 30 + column: 10 + end_location: + row: 30 + column: 53 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"delete from table where var = \" + var" + location: + row: 31 + column: 10 + end_location: + row: 31 + column: 48 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"delete from \" + table + \"where var = \" + var" + location: + row: 32 + column: 10 + end_location: + row: 32 + column: 55 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"delete from table where var = {}\".format(var)" + location: + row: 33 + column: 10 + end_location: + row: 33 + column: 56 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "f\"delete from table where var = {var}\"" + location: + row: 34 + column: 10 + end_location: + row: 34 + column: 48 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"insert into table values (%s)\" % (var,)" + location: + row: 36 + column: 10 + end_location: + row: 36 + column: 50 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"insert into table values (\" + var + \")\"" + location: + row: 37 + column: 10 + end_location: + row: 37 + column: 50 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"insert into {} values ({})\".format(table, var)" + location: + row: 38 + column: 10 + end_location: + row: 38 + column: 57 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "f\"insert into {table} values var = {var}\"" + location: + row: 39 + column: 10 + end_location: + row: 39 + column: 51 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"update %s set var = %s\" % (table, var)" + location: + row: 41 + column: 10 + end_location: + row: 41 + column: 49 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"update \" + table + \" set var = \" + var" + location: + row: 42 + column: 10 + end_location: + row: 42 + column: 49 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"update {} set var = {}\".format(table, var)" + location: + row: 43 + column: 10 + end_location: + row: 43 + column: 53 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "f\"update {table} set var = {var}\"" + location: + row: 44 + column: 10 + end_location: + row: 44 + column: 43 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"\\n SELECT *\\n FROM table\\n WHERE var = %s\\n \" % var" + location: + row: 48 + column: 11 + end_location: + row: 52 + column: 13 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"\\n SELECT *\\n FROM TABLE\\n WHERE var =\\n \" + var" + location: + row: 55 + column: 11 + end_location: + row: 59 + column: 13 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"\\n SELECT *\\n FROM table\\n WHERE var = {}\\n \".format(var)" + location: + row: 62 + column: 11 + end_location: + row: 66 + column: 19 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "f\"\\n SELECT *\\n FROM table\\n WHERE var = {var}\\n \"" + location: + row: 69 + column: 11 + end_location: + row: 73 + column: 7 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "f\"SELECT *FROM tableWHERE var = {var}\"" + location: + row: 77 + column: 8 + end_location: + row: 79 + column: 28 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"SELECT * FROM table WHERE var = %s\" % var" + location: + row: 83 + column: 25 + end_location: + row: 83 + column: 67 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "f\"SELECT * FROM table WHERE var = {var}\"" + location: + row: 84 + column: 25 + end_location: + row: 84 + column: 65 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"SELECT * FROM table WHERE var = {}\".format(var)" + location: + row: 85 + column: 25 + end_location: + row: 85 + column: 73 + fix: ~ + parent: ~ +- kind: + HardcodedSQLExpression: + string: "\"SELECT * FROM table WHERE var = %s\" % var" + location: + row: 86 + column: 29 + end_location: + row: 86 + column: 71 + fix: ~ + parent: ~ + diff --git a/docs/rules/hardcoded-sql-expression.md b/docs/rules/hardcoded-sql-expression.md new file mode 100644 index 0000000000..0216454347 --- /dev/null +++ b/docs/rules/hardcoded-sql-expression.md @@ -0,0 +1,17 @@ +# hardcoded-sql-expression (S608) + +Derived from the **flake8-bandit** linter. + +### What it does +Checks for strings that resemble SQL statements involved in some form +string building operation. + +### Why is this bad? +SQL injection is a common attack vector for web applications. Unless care +is taken to sanitize and control the input data when building such +SQL statement strings, an injection attack becomes possible. + +### Example +```python +query = "DELETE FROM foo WHERE id = '%s'" % identifier +``` \ No newline at end of file diff --git a/ruff.schema.json b/ruff.schema.json index 41c30f310b..b3ab4eb1c1 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1852,6 +1852,8 @@ "S508", "S509", "S6", + "S60", + "S608", "S61", "S612", "S7",