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

Detecting misuse of the psycopg2.sql module #412

Open
Changaco opened this issue Oct 26, 2018 · 4 comments · May be fixed by #608
Open

Detecting misuse of the psycopg2.sql module #412

Changaco opened this issue Oct 26, 2018 · 4 comments · May be fixed by #608
Labels
enhancement New feature or request
Milestone

Comments

@Changaco
Copy link

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".

@ericwb ericwb added the enhancement New feature or request label Nov 12, 2018
@ericwb ericwb added this to the Near Future milestone May 9, 2019
@wtkm11
Copy link

wtkm11 commented Apr 26, 2020

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
Copy link
Author

@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 added a commit to wtkm11/bandit that referenced this issue May 1, 2020
Add a plugin test to detect when something other than a string
literal is passed to the constructor of the `psycopg2.sql.SQL`
composable object.

Resolves: PyCQA#412
wtkm11 added a commit to wtkm11/bandit that referenced this issue May 1, 2020
Add a plugin test to detect when something other than a string
literal is passed to the constructor of the `psycopg2.sql.SQL`
composable object.

Resolves: PyCQA#412
@wtkm11 wtkm11 linked a pull request May 1, 2020 that will close this issue
@wtkm11
Copy link

wtkm11 commented May 1, 2020

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
Copy link
Author

Changaco commented May 2, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants