Allows admins to query barcodes with sql where clauses #362
Conversation
|
Thanks! The public API doesn't use SQL behind the scenes. Is it possible to use psycopg2's literals or other constructs so we can rely on its SQL injection machinery? |
|
...if you merge commits from master, it should resolve the dependency issue now |
…a sql condition. We now convert the json rules to sql server side, restricting access. Note that while sql injection is prevented by escaping table names and condition parameters, we are still exposing a conditional read oracle. You may assume that anyone with access to this api (any administrator user) is given the equivalent of table level read access to the project_barcode, ag_kit_barcode and barcode_scans tables
|
I've moved the conversion from QueryBuilder json to sql conditions server side using psycopg2. This removes an attack vector where a bad actor with administrator credentials passes up improperly escaped sql. These routes still expose a conditional query oracle against a few tables in our database by design. Think of this as giving full table level read access to administrator users. |
| with self._transaction.cursor() as cur: | ||
| # Appending the sql_cond like this | ||
| # is extremely dangerous! This must ONLY | ||
| # be called with administrator privileges |
There was a problem hiding this comment.
...if psycopg2 provides a way to clean way to filter this then we should do that. Otherwise, we should consider testing the string for danger.
There was a problem hiding this comment.
I think this comment is outdated, with the change to parse the querybuilder rule json, the sql_cond is now fully escaped.
| 'type': 'string', | ||
| 'input': 'select', | ||
| 'values': { | ||
| "Blood (skin prick)": "Blood (skin prick)", |
There was a problem hiding this comment.
...for a separate time, but it really sucks having these strings replicated through the codebase
There was a problem hiding this comment.
Worst part is, I thought I found them in another file, but it was a different list than was in the yaml.
| ) | ||
| self.assertListEqual( | ||
| params, | ||
| ['bobby" or 1=1"\\; DROP TABLES;'] |
There was a problem hiding this comment.
Is this asserting that the sql injection will work...?
There was a problem hiding this comment.
Hahaha, it asserts that the parameter list generated is the string specified, meaning the injection failed since it couldn't close the quotes to escape out.
You can try running the exact query this generates, it will tell you that "1"="1" or "name" isn't a valid column name, and prove that little bobby drop tables is fully escaped.
This is the private api side of using jquery's querybuilder to let admin users pick barcode search criteria.
This side takes the where clause from the user as is, which is a huge red flag for sql injection. May be worth asking how George handled this on the public api side, as taking in the full json query object and parsing it into sql in python might be safer (but will require some extra libraries it looks like).
Depending on our threat model, this may or may not be an issue-
Queries are only allowed to be performed by admin users (and an adversary achieving admin access generally grants substantial access to the database anyway)
Transactions are rolled back, so an admin user shouldn't be able to cause lasting damage by mistake
Queries are limited to the where clause of specific tables (assuming they can't escape out, which I can't guarantee)
The only usage of the route comes from microsetta-interface where all user data is correctly escaped in the generation of the where clause.