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

Flag str.replace as possible sql injection #1044

Merged
Merged
Show file tree
Hide file tree
Changes from 4 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
32 changes: 23 additions & 9 deletions bandit/plugins/injection_sql.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 ues of str.replace can be common.

:Example:

Expand All @@ -52,6 +59,9 @@
.. versionchanged:: 1.7.3
CWE information added

.. versionchanged:: 1.7.6
sigmavirus24 marked this conversation as resolved.
Show resolved Hide resolved
Flag when str.replace is used in the string construction

""" # noqa: E501
import ast
import re
Expand All @@ -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
):
Expand All @@ -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.",
Expand Down
2 changes: 2 additions & 0 deletions examples/sql_statements.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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")
Expand Down
4 changes: 2 additions & 2 deletions tests/functional/test_functional.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand Down