Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sql injection check for f-strings #434

Merged
merged 5 commits into from Dec 2, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions bandit/plugins/injection_sql.py
Expand Up @@ -28,6 +28,7 @@
- "SELECT thing FROM " + tab
- "SELECT " + val + " FROM " + tab + ...
- "SELECT {} FROM derp;".format(var)
- f"SELECT foo FROM bar WHERE id = {product}"

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
Expand Down Expand Up @@ -93,6 +94,9 @@ def _evaluate_ast(node):
statement = node.s
# Hierarchy for "".format() is Wrapper -> Call -> Attribute -> Str
wrapper = node.parent.parent.parent
elif isinstance(node.parent, ast.JoinedStr):
statement = node.s
wrapper = node.parent.parent

if isinstance(wrapper, ast.Call): # wrapped in "execute" call?
names = ['execute', 'executemany']
Expand Down
41 changes: 41 additions & 0 deletions examples/sql_statements-py36.py
@@ -0,0 +1,41 @@
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 = {tmp}"

# 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))
cur.execute(f"SELECT * FROM foo WHERE id {tmp}")

# 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)

# real world false positives
choices=[('server_list', _("Select from active instances"))]
print("delete from the cache as the first argument")
25 changes: 20 additions & 5 deletions tests/functional/test_functional.py
Expand Up @@ -15,6 +15,7 @@
# under the License.

import os
import sys

import six
import testtools
Expand Down Expand Up @@ -404,11 +405,25 @@ def test_ignore_skip(self):

def test_sql_statements(self):
'''Test for SQL injection through string building.'''
expect = {
'SEVERITY': {'UNDEFINED': 0, 'LOW': 0, 'MEDIUM': 14, 'HIGH': 0},
'CONFIDENCE': {'UNDEFINED': 0, 'LOW': 8, 'MEDIUM': 6, 'HIGH': 0}
}
self.check_example('sql_statements.py', expect)
filename = 'sql_statements{}.py'
if sys.version_info <= (3, 6):
filename = filename.format('')
expect = {
'SEVERITY': {'UNDEFINED': 0, 'LOW': 0, 'MEDIUM': 14,
'HIGH': 0},
'CONFIDENCE': {'UNDEFINED': 0, 'LOW': 8, 'MEDIUM': 6,
'HIGH': 0}
}
else:
filename = filename.format('-py36')
expect = {
'SEVERITY': {'UNDEFINED': 0, 'LOW': 0, 'MEDIUM': 16,
'HIGH': 0},
'CONFIDENCE': {'UNDEFINED': 0, 'LOW': 9, 'MEDIUM': 7,
'HIGH': 0}
}

self.check_example(filename, expect)

def test_ssl_insecure_version(self):
'''Test for insecure SSL protocol versions.'''
Expand Down