search

pepkit / pepdbagent

Database for storing sample metadata
BSD 2-Clause "Simplified" License
2 stars 1 forks source link

SQL Injection Vulnerability #19

Closed nleroy917 closed 1 year ago

nleroy917 commented 1 year ago

I noticed here that we are directly building up SQL statements with f-strings. We should probably let psycopg handle parameter binding so that we aren't opening databases up to SQL injection.

https://github.com/pepkit/pephub_db/blob/70e6fa0499c1e038c2496d0d97cec46fbc42e2f0/pepagent/pepagent.py#L191

khoroshevskyi commented 1 year ago

What is the problem by using it in this way ?

There is 2 function to handle this problem: https://github.com/pepkit/pephub_db/blob/70e6fa0499c1e038c2496d0d97cec46fbc42e2f0/pepagent/pepagent.py#L307-L341

In this case we are using also this function. Do you think we should change the way we call this functions?

nleroy917 commented 1 year ago

What is the problem by using it in this way ?

Just the direct building of queries is not secure. For example, if someone supplied:

namespace = "geo; DROP TABLE projects"
sql_q = f"""select {NAME_COL}, {PROJ_COL} from {DB_TABLE_NAME} where namespace='{namespace}';"""

Would be better to parameterize the queries like so:

sql_q = f"""select {NAME_COL}, {PROJ_COL} from {DB_TABLE_NAME} where namespace=%s;"""

psygopg will sanitize things for us I believe.

... which is done for almost all the queries in PepAgent. I just found this one example of us manually formatting strings.