Alter SQL injection plugin to consider .format strings

This considers `"{}".format()` style alongside `"%s" % ` string
formatting for possible SQL injection vulnerabilities.

Change-Id: If7b09083bd2cc5e48e5d3fd3e8d5e6142fdb67ed

bandit/plugins/injection_sql.py

@@ -27,6 +27,7 @@ some form of string building operation. For example:
  - "SELECT %s FROM derp;" % var
  - "SELECT thing FROM " + tab
  - "SELECT " + val + " FROM " + tab + ...
+ - "SELECT {} FROM derp;".format(var)
 
 Unless care is taken to sanitize and control the input data when building such
 SQL statement strings, an injection attack becomes possible. If strings of this
@@ -80,15 +81,25 @@ def _check_string(data):
 
 
 def _evaluate_ast(node):
-    if not isinstance(node.parent, ast.BinOp):
-        return (False, "")
+    wrapper = None
+    statement = ''
 
-    out = utils.concat_string(node, node.parent)
-    if isinstance(out[0].parent, ast.Call):  # wrapped in "execute" call?
+    if isinstance(node.parent, ast.BinOp):
+        out = utils.concat_string(node, node.parent)
+        wrapper = out[0].parent
+        statement = out[1]
+    elif (isinstance(node.parent, ast.Attribute)
+          and node.parent.attr == 'format'):
+        statement = node.s
+        # Hierarchy for "".format() is Wrapper -> Call -> Attribute -> Str
+        wrapper = node.parent.parent.parent
+
+    if isinstance(wrapper, ast.Call):  # wrapped in "execute" call?
         names = ['execute', 'executemany']
-        name = utils.get_called_name(out[0].parent)
-        return (name in names, out[1])
-    return (False, out[1])
+        name = utils.get_called_name(wrapper)
+        return (name in names, statement)
+    else:
+        return (False, statement)
 
 
 @test.checks('Str')

examples/sql_statements.py

@@ -7,12 +7,18 @@ 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)
 
 # 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))
 
 # good
 cur.execute("SELECT * FROM foo WHERE id = '%s'", identifier)
@@ -20,13 +26,6 @@ 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
-query = "SELECT " + val + " FROM " + val +" WHERE id = " + val
-
-# bad
-cur.execute("SELECT " + val + " FROM " + val +" WHERE id = " + val)
-
-
 # bug: https://bugs.launchpad.net/bandit/+bug/1479625
 def a():
     def b():

tests/functional/test_functional.py

@@ -292,8 +292,8 @@ class FunctionalTests(testtools.TestCase):
     def test_sql_statements(self):
         '''Test for SQL injection through string building.'''
         expect = {
-            'SEVERITY': {'MEDIUM': 12},
-            'CONFIDENCE': {'LOW': 7, 'MEDIUM': 5}}
+            'SEVERITY': {'MEDIUM': 14},
+            'CONFIDENCE': {'LOW': 8, 'MEDIUM': 6}}
         self.check_example('sql_statements.py', expect)
 
     def test_ssl_insecure_version(self):