Repository: PyCQA/bandit
Pull request #917 (2022-06-28 11:19:17)

Improve detecting SQL injections in f-strings
This commit fixes detecting SQL injection
in statements like:
```
f"SELECT {column_name} FROM foo WHERE id = 1"
f"INSERT INTO {table_name} VALUES (1)"
f"UPDATE {table_name} SET id = 1"
```
Before this change, the bandit was analyzing statements
by parts, especially, in the case of:
`"SELECT {column_name} FROM foo WHERE id = 1"`
it was firstly checking `"SELECT "` for being
an SQL statement and then `" FROM foo WHERE id = 1"`.
Neither of these parts match to defined
regular expressions:
```
    r"(select\s.*from\s|"
    r"delete\s+from\s|"
    r"insert\s+into\s.*values\s|"
    r"update\s.*set\s)",
```
Thus SQL injection was not detected.

This commit makes bandit checking the whole SQL
statement for matching the above regexps.
However, since it uses `ast.unparse` it works
only for python >= 3.9.

Resolves: #916

kfrydel: We do not have tests for multiline SQL strings at all in the current code as far as I can see. I added some in my second PR: https://github.com/PyCQA/bandit/pull/915. I can move them here (assuming that you will merge this PR as the first one) and add tests for multiline f-strings. I will get back to you when done.
sigmavirus24: No i mean implicit concatenation. Think something like

```
s = (
 'a'
'b'
'c'
)
```

Note no commas. This turns into `'abc'`
kfrydel: Yes, I didn't know that name but I googled it. I added tests for multiline strings (as in the second PR but without nosec cases) and some for implicit concatenation: https://github.com/PyCQA/bandit/pull/917/files#diff-a691abc0c51d7e1432ef4486311a653dc950d5f440f71f7093f6f4f0892fe077R89

Patch:
diff --git a/bandit/plugins/injection_sql.py b/bandit/plugins/injection_sql.py
index c69750c..dd9b5c0 100644
--- a/bandit/plugins/injection_sql.py
+++ b/bandit/plugins/injection_sql.py
@@ -92,8 +92,18 @@ def _evaluate_ast(node):
     elif hasattr(ast, "JoinedStr") and isinstance(
         node._bandit_parent, ast.JoinedStr
     ):
-        statement = node.s
-        wrapper = node._bandit_parent._bandit_parent
+        substrings = [
+            child
+            for child in node._bandit_parent.values
+            if isinstance(child, ast.Str)
+        ]
+        # JoinedStr consists of list of Constant and FormattedValue
+        # instances. Let's perform one test for the whole string
+        # and abandon all parts except the first one to raise one
+        # failed test instead of many for the same SQL statement.
+        if substrings and node == substrings[0]:
+            statement = "".join([str(child.s) for child in substrings])
+            wrapper = node._bandit_parent._bandit_parent
 
     if isinstance(wrapper, ast.Call):  # wrapped in "execute" call?
         names = ["execute", "executemany"]
diff --git a/examples/sql_multiline_statements.py b/examples/sql_multiline_statements.py
new file mode 100644
index 0000000..1f07809
--- /dev/null
+++ b/examples/sql_multiline_statements.py
@@ -0,0 +1,123 @@
+import sqlalchemy
+
+# bad
+query = """SELECT *
+FROM foo WHERE id = '%s'""" % identifier
+query = """INSERT INTO foo
+VALUES ('a', 'b', '%s')""" % value
+query = """DELETE FROM foo
+WHERE id = '%s'""" % identifier
+query = """UPDATE foo
+SET value = 'b'
+WHERE id = '%s'""" % identifier
+query = """WITH cte AS (SELECT x FROM foo)
+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 = f"""
+SELECT *
+FROM foo
+WHERE id = {identifier}
+"""
+
+# bad
+cur.execute("""SELECT *
+FROM foo
+WHERE id = '%s'""" % identifier)
+cur.execute("""INSERT INTO foo
+VALUES ('a', 'b', '%s')""" % value)
+cur.execute("""DELETE FROM foo
+WHERE id = '%s'""" % identifier)
+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))
+
+# bad with f-string
+query = f"""
+SELECT *
+FROM foo
+WHERE id = {identifier}
+"""
+query = f"""
+SELECT *
+FROM foo
+WHERE id = {identifier}
+"""
+
+query = f"""
+SELECT *
+FROM foo
+WHERE id = {identifier}"""
+query = f"""
+SELECT *
+FROM foo
+WHERE id = {identifier}"""
+
+cur.execute(f"""
+SELECT
+    {column_name}
+FROM foo
+WHERE id = 1""")
+
+cur.execute(f"""
+SELECT
+    {a + b}
+FROM foo
+WHERE id = 1""")
+
+cur.execute(f"""
+INSERT INTO
+    {table_name}
+VALUES (1)""")
+cur.execute(f"""
+UPDATE {table_name}
+SET id = 1""")
+
+# implicit concatenation mixed with f-strings
+cur.execute("SELECT "
+            f"{column_name} "
+            "FROM foo "
+            "WHERE id = 1"
+            )
+cur.execute("INSERT INTO "
+            f"{table_name} "
+            "VALUES (1)")
+cur.execute(f"UPDATE {table_name} "
+            "SET id = 1")
+
+# good
+cur.execute("""SELECT *
+FROM foo
+WHERE id = '%s'""", identifier)
+cur.execute("""INSERT INTO foo
+VALUES ('a', 'b', '%s')""", value)
+cur.execute("""DELETE FROM foo
+WHERE id = '%s'""", identifier)
+cur.execute("""UPDATE foo
+SET value = 'b'
+WHERE id = '%s'""", identifier)
+
+
+# bug: https://bugs.launchpad.net/bandit/+bug/1479625
+def a():
+    def b():
+        pass
+
+    return b
+
+
+a()("""SELECT %s
+FROM foo""" % val)
diff --git a/examples/sql_statements.py b/examples/sql_statements.py
index 1fabb70..ff88a92 100644
--- a/examples/sql_statements.py
+++ b/examples/sql_statements.py
@@ -20,6 +20,12 @@ cur.execute("UPDATE foo SET value = 'b' WHERE id = '%s'" % identifier)
 cur.execute("SELECT * FROM foo WHERE id = '" + identifier + "'")
 cur.execute("SELECT * FROM foo WHERE id = '{}'".format(identifier))
 
+# bad f-strings
+cur.execute(f"SELECT {column_name} FROM foo WHERE id = 1")
+cur.execute(f"SELECT {a + b} FROM foo WHERE id = 1")
+cur.execute(f"INSERT INTO {table_name} VALUES (1)")
+cur.execute(f"UPDATE {table_name} SET id = 1")
+
 # good
 cur.execute("SELECT * FROM foo WHERE id = '%s'", identifier)
 cur.execute("INSERT INTO foo VALUES ('a', 'b', '%s')", value)


Test patch:
diff --git a/tests/functional/test_functional.py b/tests/functional/test_functional.py
index f30ca25..e1d10a9 100644
--- a/tests/functional/test_functional.py
+++ b/tests/functional/test_functional.py
@@ -439,18 +439,40 @@ class FunctionalTests(testtools.TestCase):
             "SEVERITY": {
                 "UNDEFINED": 0,
                 "LOW": 0,
-                "MEDIUM": 14,
+                "MEDIUM": 18,
                 "HIGH": 0,
             },
             "CONFIDENCE": {
                 "UNDEFINED": 0,
                 "LOW": 8,
-                "MEDIUM": 6,
+                "MEDIUM": 10,
                 "HIGH": 0,
             },
         }
         self.check_example("sql_statements.py", expect)
 
+    def test_multiline_sql_statements(self):
+        """
+        Test for SQL injection through string building using
+        multi-line strings.
+        """
+        example_file = "sql_multiline_statements.py"
+        expect = {
+            "SEVERITY": {
+                "UNDEFINED": 0,
+                "LOW": 0,
+                "MEDIUM": 26,
+                "HIGH": 0,
+            },
+            "CONFIDENCE": {
+                "UNDEFINED": 0,
+                "LOW": 13,
+                "MEDIUM": 13,
+                "HIGH": 0,
+            },
+        }
+        self.check_example(example_file, expect)
+
     def test_ssl_insecure_version(self):
         """Test for insecure SSL protocol versions."""
         expect = {