Detecting misuse of the `psycopg2.sql` module

[Changaco]

The psycopg2.sql module is meant to provide a safe way to compose SQL queries dynamically, however it is possible to misuse it in a way that would result in an SQL injection vulnerability, and bandit currently doesn't support detecting this.

Solution: create a new test to detect when a psycopg2.sql.SQL object is being created from a non-literal, e.g. SQL(foo) or SQL('%s AND %s' % (foo, bar)). The severity and confidence levels of this new test would both be at least "medium".

[wtkm11]

Hi @Changaco! I was considering picking up this issue as a way to contribute but it looks like these cases would be covered by B608:hardcoded_sql_expressions (see injection_sql.py). Two instances of hardcoded sql expressions were detected in this snippet of code:

from psycopg2 import sql

sql.SQL('insert into %s values (%s, %s)' % ('albatross', 1, 2)) # detected

moa = 'insert into %s values (%s, %s)' % ('ostrich', 3, 4)
sql.SQL(moa) # detected

sql.SQL("insert into {} values (%s, %s)") # OK

Is there another variation of this issue involving psycopg2.sql.SQL that bandit is still not detecting?

[Changaco]

@wtkm11 bandit still doesn't detect calling the SQL constructor with a non-literal. For example in a function that receives a table name as an argument, using sql.SQL(table_name) would be unsafe, it should be sql.Identifier(table_name) instead.

from psycopg2 import sql

def count_rows(db, table):
    print(db.one(sql.SQL('select count(*) from {}').format(sql.Identifier(table))))  # safe
    print(db.one(sql.SQL('select count(*) from {}').format(sql.SQL(table))))  # unsafe

[wtkm11]

I wrote a plugin test (#608) to detect situations like this:

from psycopg2 import sql

table = 'users; drop table users; --'
sql.SQL('select * from {}').format(sql.SQL(table))

The non-literal being passed in sql.SQL(table) would be detected. However, I feel that in this case the confidence should be LOW rather than MEDIUM. What do you think?

[Changaco]

I don't see why it should be low confidence, and I think it would make the test useless because in my experience low confidence alerts are numerous and have to be silenced by default.