Improve detecting SQL injections in f-strings (#917)

* 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.

Resolves: #916

* Add tests for multiline strings

Author: kfrydel

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"]

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)

examples/sql_statements.py

@@ -20,6 +20,12 @@
 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)

tests/functional/test_functional.py

@@ -439,18 +439,40 @@ def test_sql_statements(self):
             "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 = {