Repository: PyCQA/bandit
Pull request #1044 (2023-08-16 05:51:56)

Flag str.replace as possible sql injection
This extends the existing implementation for detecting possible cases of SQL injection to account for `str.replace` used in the string construction.

Use of `str.replace` can lead to SQL injection in much the same way as `str.format` can, and that is already considered in the pre-existing implementation, along with other common string constructions.

Resolves #878

costaparas: > While this can happen, I don't believe it's nearly as common as format or % formatting and can potentially lead to a higher rate of false positives. This check should have a lower confidence if we accept it

Switched to LOW confidence to compensate for possibly higher rate of false positives.

Patch:
diff --git a/bandit/plugins/injection_sql.py b/bandit/plugins/injection_sql.py
index dd9b5c0..eed3e52 100644
--- a/bandit/plugins/injection_sql.py
+++ b/bandit/plugins/injection_sql.py
@@ -27,6 +27,13 @@ If so, a MEDIUM issue is reported. For example:
 
  - cursor.execute("SELECT %s FROM derp;" % var)
 
+Use of str.replace in the string construction can also be dangerous.
+For example:
+
+- "SELECT * FROM foo WHERE id = '[VALUE]'".replace("[VALUE]", identifier)
+
+However, such cases are always reported with LOW confidence to compensate
+for false positives, since valid uses of str.replace can be common.
 
 :Example:
 
@@ -52,6 +59,9 @@ If so, a MEDIUM issue is reported. For example:
 .. versionchanged:: 1.7.3
     CWE information added
 
+.. versionchanged:: 1.7.7
+    Flag when str.replace is used in the string construction
+
 """  # noqa: E501
 import ast
 import re
@@ -77,18 +87,20 @@ def _check_string(data):
 def _evaluate_ast(node):
     wrapper = None
     statement = ""
+    str_replace = False
 
     if isinstance(node._bandit_parent, ast.BinOp):
         out = utils.concat_string(node, node._bandit_parent)
         wrapper = out[0]._bandit_parent
         statement = out[1]
-    elif (
-        isinstance(node._bandit_parent, ast.Attribute)
-        and node._bandit_parent.attr == "format"
-    ):
+    elif isinstance(
+        node._bandit_parent, ast.Attribute
+    ) and node._bandit_parent.attr in ("format", "replace"):
         statement = node.s
         # Hierarchy for "".format() is Wrapper -> Call -> Attribute -> Str
         wrapper = node._bandit_parent._bandit_parent._bandit_parent
+        if node._bandit_parent.attr == "replace":
+            str_replace = True
     elif hasattr(ast, "JoinedStr") and isinstance(
         node._bandit_parent, ast.JoinedStr
     ):
@@ -108,19 +120,21 @@ def _evaluate_ast(node):
     if isinstance(wrapper, ast.Call):  # wrapped in "execute" call?
         names = ["execute", "executemany"]
         name = utils.get_called_name(wrapper)
-        return (name in names, statement)
+        return (name in names, statement, str_replace)
     else:
-        return (False, statement)
+        return (False, statement, str_replace)
 
 
 @test.checks("Str")
 @test.test_id("B608")
 def hardcoded_sql_expressions(context):
-    val = _evaluate_ast(context.node)
-    if _check_string(val[1]):
+    execute_call, statement, str_replace = _evaluate_ast(context.node)
+    if _check_string(statement):
         return bandit.Issue(
             severity=bandit.MEDIUM,
-            confidence=bandit.MEDIUM if val[0] else bandit.LOW,
+            confidence=bandit.MEDIUM
+            if execute_call and not str_replace
+            else bandit.LOW,
             cwe=issue.Cwe.SQL_INJECTION,
             text="Possible SQL injection vector through string-based "
             "query construction.",
diff --git a/examples/sql_statements.py b/examples/sql_statements.py
index ff88a92..53a5306 100644
--- a/examples/sql_statements.py
+++ b/examples/sql_statements.py
@@ -10,6 +10,7 @@ SELECT x FROM cte WHERE x = '%s'""" % identifier
 # bad alternate forms
 query = "SELECT * FROM foo WHERE id = '" + identifier + "'"
 query = "SELECT * FROM foo WHERE id = '{}'".format(identifier)
+query = "SELECT * FROM foo WHERE id = '[VALUE]'".replace("[VALUE]", identifier)
 
 # bad
 cur.execute("SELECT * FROM foo WHERE id = '%s'" % identifier)
@@ -19,6 +20,7 @@ cur.execute("UPDATE foo SET value = 'b' WHERE id = '%s'" % identifier)
 # bad alternate forms
 cur.execute("SELECT * FROM foo WHERE id = '" + identifier + "'")
 cur.execute("SELECT * FROM foo WHERE id = '{}'".format(identifier))
+cur.execute("SELECT * FROM foo WHERE id = '[VALUE]'".replace("[VALUE]", identifier))
 
 # bad f-strings
 cur.execute(f"SELECT {column_name} FROM foo WHERE id = 1")


Test patch:
diff --git a/tests/functional/test_functional.py b/tests/functional/test_functional.py
index 6bc047e..8469943 100644
--- a/tests/functional/test_functional.py
+++ b/tests/functional/test_functional.py
@@ -439,12 +439,12 @@ class FunctionalTests(testtools.TestCase):
             "SEVERITY": {
                 "UNDEFINED": 0,
                 "LOW": 0,
-                "MEDIUM": 18,
+                "MEDIUM": 20,
                 "HIGH": 0,
             },
             "CONFIDENCE": {
                 "UNDEFINED": 0,
-                "LOW": 8,
+                "LOW": 10,
                 "MEDIUM": 10,
                 "HIGH": 0,
             },