Flag str.replace as possible sql injection (#1044)

* 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

* Revert cosmetic change

* Fix lint

* Reduce str.replace to LOW confidence in all cases

Since the rate of false positives may be higher for
str.replace over other string constructions like str.format,
we should reduce to LOW confidence to compensate for this.

* Update bandit/plugins/injection_sql.py

Correct version in versionchanged directive

Co-authored-by: Eric Brown <[email protected]>

* Fix typo in comment

---------

Co-authored-by: Ian Stapleton Cordasco <[email protected]>
Co-authored-by: Eric Brown <[email protected]>

Author: 3 people

bandit/plugins/injection_sql.py

@@ -27,6 +27,13 @@
 
  - 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 @@
 .. 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.",

examples/sql_statements.py

@@ -10,6 +10,7 @@
 # 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 @@
 # 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")

tests/functional/test_functional.py

@@ -439,12 +439,12 @@ def test_sql_statements(self):
             "SEVERITY": {
                 "UNDEFINED": 0,
                 "LOW": 0,
-                "MEDIUM": 18,
+                "MEDIUM": 20,
                 "HIGH": 0,
             },
             "CONFIDENCE": {
                 "UNDEFINED": 0,
-                "LOW": 8,
+                "LOW": 10,
                 "MEDIUM": 10,
                 "HIGH": 0,
             },