Skip to content

fix: don't flag parameterized cursor.execute() as SQL injection #39

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.

Sign up for GitHub

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

Open
mvanhorn wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: don't flag parameterized cursor.execute() as SQL injection #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion rules/python/sql_injection.ron
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@
mode: "search",
patterns: Some([
"execute_raw_query(",
"*cursor.execute(*%*",
// Match the % operator after a complete query expression, not DB-API placeholders.
"regex:(?s)cursor\\.execute\\(\\s*\\(*\\s*(?:(?:[rRuUbBfF]{0,2})?(?:\"\"\".*?\"\"\"|'''.*?'''|\"(?:\\\\.|[^\"\\\\])*\"|'(?:\\\\.|[^'\\\\])*')|[A-Za-z_][A-Za-z0-9_.]*)\\s*\\)*\\s*%\\s*[^\\s,)]",

@yhoztak yhoztak Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We try to avoid very complex regex like this as it may become too specific and hard to understand it.
We could have separate AST rule here instead.

As an example:

(
    rules: [
        // Covers `"..." % x` and `"..." + x` passed to cursor.execute.
        // Parameterized execute("...%s", params) has a string as arg0, not
        // a binary_operator, so it is not matched.
        (
            id: Some("python-sql-raw-binop"),
            name: Some("Raw SQL Execution (binary-built query)"),
            category: Some("database"),
            mode: "search",
            patterns: Some([
                "cursor.execute",
            ]),
            conditions: Some([
                (
                    field: "argument",
                    operator: "matches",
                    value: "",
                    condition_type: Some("has_argument"),
                    argument_position: Some(0),
                    node_type: Some("binary_operator"),
                ),
            ]),
            finding_type: Some("SQL Injection"),
            severity: Some("Critical"),
            confidence: Some("Medium"),
            cwe_id: Some("cwe-89"),
            description: Some("Raw SQL query executed from a dynamically-built string (string formatting/concatenation)"),
            file_types: Some((
                extensions: Some([".py"])
            )),
            tags: Some(["sql", "injection", "database"])
        ),

        // Remaining inline string-building shapes the binary-operator rule
        // does not cover (f-strings, str.format, raw-query helpers).
        (
            id: Some("python-sql-raw-001"),
            name: Some("Raw SQL Execution"),
            category: Some("database"),
            mode: "search",
            patterns: Some([
                "execute_raw_query(",
                "*cursor.execute(*.format(*",
                "*cursor.execute(f\"*",
                "*cursor.execute(f'*",
                "*.executescript(*",
            ]),
            finding_type: Some("SQL Injection"),
            severity: Some("Critical"),
            confidence: Some("Medium"),
            cwe_id: Some("cwe-89"),
            description: Some("Raw SQL query executed from a dynamically-built string (string formatting/concatenation)"),
            file_types: Some((
                extensions: Some([".py"])
            )),
            tags: Some(["sql", "injection", "database"])
        ),

        // Taint: user input flowing into a SQL execute sink (catches variable-built
        // queries that aren't visible as a literal at the call site).
        (
            id: Some("python-sql-taint-001"),
            name: Some("SQL Injection Taint Flow"),
            category: Some("database"),
            mode: "taint",
            sources: Some([
                "request.",
                "input(",
                "raw_input(",
                "flask.request",
            ]),
            sinks: Some([
                "execute(",
                "executemany(",
                "executescript(",
            ]),
            sanitizers: Some([
                "parameterized",
            ]),
            finding_type: Some("SQL Injection"),
            severity: Some("Critical"),
            confidence: Some("High"),
            cwe_id: Some("cwe-89"),
            description: Some("User input flows to SQL execution without parameterization"),
            file_types: Some((
                extensions: Some([".py"])
            )),
            tags: Some(["sql", "injection", "taint", "database"])
        ),
    ]
)

@juangaitanv juangaitanv Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trailing %\s*[^\s,)] excludes (, so tuple formatting such as cursor.execute("SELECT ... %s" % (user_id,)) no longer matches. This leaves a common SQL-injection pattern undetected; allow ( and add a tuple-RHS fixture.

"*cursor.execute(*.format(*",
"*cursor.execute(f\"*",
"*cursor.execute(f'*",
Expand Down
5 changes: 5 additions & 0 deletions tests/features/false_positive_floor.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,8 @@ Feature: False-positive floor
Given a staged benign Python module "calculator.py"
When I scan the staging directory as "python" with the production rules
Then no findings should be reported

Scenario: Parameterized cursor execution produces zero findings
Given a staged copy of the fixture "tests/test_files/python/sql_parameterized_execute.py" as "parameterized_query.py"
When I scan the staging directory as "python" with the production rules
Then no findings should be reported
14 changes: 14 additions & 0 deletions tests/features/python_injection.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,17 @@ Feature: Python injection detection
When I scan the staging directory as "python" with the production rules
Then the findings should include a "Command Injection" finding in "mixed_case.py"
And the findings should include a "SQL Injection" finding in "mixed_case.py"

Scenario Outline: Dynamically formatted cursor execution is detected
Given a staged copy of the fixture "<fixture>" as "unsafe_query.py"
When I scan the staging directory as "python" with the production rules
Then the findings should include a "SQL Injection" finding in "unsafe_query.py"

Examples:
| fixture |
| tests/test_files/python/sql_percent_spaced.py |
| tests/test_files/python/sql_percent_unspaced.py |
| tests/test_files/python/sql_percent_variable.py |
| tests/test_files/python/sql_fstring_interpolation.py |
| tests/test_files/python/sql_concatenation.py |
| tests/test_files/python/sql_format_interpolation.py |
2 changes: 2 additions & 0 deletions tests/test_files/python/sql_concatenation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
def load_user(cursor, user_id):
cursor.execute("SELECT * FROM users WHERE id = " + user_id)
2 changes: 2 additions & 0 deletions tests/test_files/python/sql_format_interpolation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
def load_user(cursor, user_id):
cursor.execute("SELECT * FROM users WHERE id = {}".format(user_id))
2 changes: 2 additions & 0 deletions tests/test_files/python/sql_fstring_interpolation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
def load_user(cursor, user_id):
cursor.execute(f"SELECT * FROM users WHERE id = {user_id}")
13 changes: 13 additions & 0 deletions tests/test_files/python/sql_parameterized_execute.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
def load_firms(cursor, firm_id, firm_name):
cursor.execute(
"SELECT * FROM firms WHERE firm_id = %s",
[firm_id],
)
cursor.execute(
"SELECT * FROM firms WHERE name = %(name)s",
{"name": firm_name},
)
cursor.execute(
"SELECT * FROM firms WHERE name LIKE %s",
[f"%{firm_name}%"],
)
2 changes: 2 additions & 0 deletions tests/test_files/python/sql_percent_spaced.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
def load_user(cursor, user_id):
cursor.execute("SELECT * FROM users WHERE id = %s" % user_id)
2 changes: 2 additions & 0 deletions tests/test_files/python/sql_percent_unspaced.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
def load_user(cursor, user_id):

@yhoztak yhoztak Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add these to the same sql test file for readability and coverages?

like:

def percent_spaced(cursor, user_id):
    cursor.execute("SELECT * FROM users WHERE id = %s" % user_id)


def percent_unspaced(cursor, user_id):
    cursor.execute("SELECT * FROM users WHERE id = %s"%user_id)


def percent_variable(cursor, query, user_id):
    cursor.execute(query % user_id)


def fstring_interpolation(cursor, user_id):
    cursor.execute(f"SELECT * FROM users WHERE id = {user_id}")


def string_concatenation(cursor, user_id):
    cursor.execute("SELECT * FROM users WHERE id = " + user_id)


def format_interpolation(cursor, user_id):
    cursor.execute("SELECT * FROM users WHERE id = {}".format(user_id))

@yhoztak yhoztak Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are cleaning up test folder structures starting from python and PR is up here fyi #43
My apology for not having a clean structure to follow

@yhoztak yhoztak Jul 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#43 is now merged. If there's a new fixture files for test cases, can you move them under https://github.com/Corgea/Sighthound/tree/main/tests/test_files/python/fixtures ?
As an example for SQL Injection, add please add new functions to https://github.com/Corgea/Sighthound/blob/main/tests/test_files/python/fixtures/sql_injection_variants.py

cursor.execute("SELECT * FROM users WHERE id = %s"%user_id)
2 changes: 2 additions & 0 deletions tests/test_files/python/sql_percent_variable.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
def load_user(cursor, query, user_id):
cursor.execute(query % user_id)
Loading