Skip to content

Commit

Permalink
Reduce str.replace to LOW confidence in all cases
Browse files Browse the repository at this point in the history
Since the rate of false positives may be higher for
str.replace over other string constructions like str.format,
we should reduce to LOW confidence to compensate for this.
  • Loading branch information
costaparas committed Aug 27, 2023
1 parent f45f89f commit 5a1b6e5
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 7 deletions.
20 changes: 15 additions & 5 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 Down Expand Up @@ -80,6 +87,7 @@ 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)
Expand All @@ -91,6 +99,8 @@ def _evaluate_ast(node):
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 @@ -110,19 +120,19 @@ 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
4 changes: 2 additions & 2 deletions tests/functional/test_functional.py
Original file line number Diff line number Diff line change
Expand Up @@ -444,8 +444,8 @@ def test_sql_statements(self):
},
"CONFIDENCE": {
"UNDEFINED": 0,
"LOW": 9,
"MEDIUM": 11,
"LOW": 10,
"MEDIUM": 10,
"HIGH": 0,
},
}
Expand Down

0 comments on commit 5a1b6e5

Please sign in to comment.