Inserting robot moves into an SQLite3 database

Question

I'm just wondering if you would consider this to be safe from SQL injection.

A peculiarity of my program is I need to dynamically access table names depending on my scenario.

This is not for publishing online, but I wish to know if this approach is any good.

I'm using Python3 and SQLite3. My database object contains the table name, and is not controlled by any user. It comes from another class in a config.cfg file.

def save_record(state_int, database):
    query = '''INSERT INTO %s (state_int, turn_left, turn_right, move_forward) VALUES (?,?,?,?)''' %(database.records_table_name)
    database.cursor.execute(query, (state_int, 0, 0, 0))
    database.robot_memory.commit()

save_record(10012, database)

Answer 1

Just a philosophical answer here.

I think you're asking the wrong question. You shouldn't ask yourself "is my SQL code safe or vulnerable?" This is too hard to answer in any individual case, and coming up with the right answer could depend on a lot of contextual things such as the system configuration, the whole program structure, etc.

You don't want to have to consider all these things when writing code. People have already considered them a lot, and based on their experience they have come up with best practices.

Instead you should ask the question,

Does my code follow the standard best practices for preventing SQL injection?

As alluded to in another answer, using string interpolation to build queries is against best practices. So, while it doesn't seem like this creates a vulnerability in your code, we can't rule out that there is some edge case based on the rest of your program structure that would allow an attacker to put something into this string interpolation.

Unless you have a very good reason for doing so, follow the best practices, even if you don't need to. Then you can sleep well and ignore pathological edge cases.

Answer 2

Your code is vulnerable to SQL injection!

Use something like this to prevent it:

cursor.execute("INSERT INTO table (state_int, turn_left, turn_right, move_forward) VALUES (?,?,?,?)",(database.records_table_name))

Note that you cannot (or you shouldn't) enter the table in like this:

query = '''INSERT INTO %s...'''

Ideally the table should be hard coded. Under no circumstances should it come from a user input of any kind. You can use a string similar to what you did for the table, but you'd better make 100% certain that a user can't change it somehow... See (Can I use parameters for the table name in sqlite3 ?) for more details.

The python pages quote say something like:

Usually your SQL operations will need to use values from Python variables. You shouldn’t assemble your query using Python’s string operations because doing so is insecure; it makes your program vulnerable to an SQL injection attack

Using cursor.execute will stuff the value given, so that the argument could be as listed, and when you do a query on it, that is exactly what you will get out. XKCD explains this humorously as well.

Answer 3

There's no way for us tell, from reading this fragment of code, whether there is a SQL injection problem. On the one hand, the query is constructed using string interpolation, which is risky, but the interpolated string is database.records_table_name and we can't tell whether this comes from an untrusted source. (You say in the post that it doesn't, but there's no way for us to check that you're right about that.)

But it is a bad sign that you have to use string interpolation here. Why don't you know the name of the table statically? Is the problem that there are several different tables with identical fields (but different table names) that you might need to insert records into? If so, your database is not properly normalized. These tables should be combined into one table with a field that indicates whatever it is that the table name indicates in your current schema. Then there would be no need to construct the query by string interpolation, and so no risk of SQL injection.

Response to comments

1. You say that you adopted the current design (multiple tables instead of a single table with one extra field) because you had a concern about performance. Did you carry out a performance comparison? If not, how do you know that your design addresses the concern? If you did, did you remember to add indexes on the fields you are querying on? Databases can easily query millions of records if you create a good set of indexes.

2. You also say that you adopted the current design because you had a concern about the ease of creating new agents. But in a normalized database, creating a new agent would be easy (you'd insert a record into the AGENT table), and so would destroying an agent (you'd declare the agent field in the RECORD table with REFERENCES AGENT ON DELETE CASCADE and so all you'd have to do would be to delete the agent record from the AGENT table, and all the deletions from the RECORD table would follow automatically).

Answer 4

Given the restrictions that you can't change your database structure (and as Gareth Rees points out, that would be better), the way I would resolve this is using an Enum or Class. Basically, while you can have multiple databases with the same structure, you won't have an unbounded number of those databases.

Based on the type of data. I would switch the entire SQL statement with no string injection in it, so that you basically tie the SQL statement to the class. e.g.

if(is_X(my_obj))
   query = '''INSERT INTO X_table (state_int, turn_left, turn_right, move_forward) VALUES (?,?,?,?)'''
else if(is_Y(my_obj))
   query = '''INSERT INTO Y_table (state_int, turn_left, turn_right, move_forward) VALUES (?,?,?,?)'''
...
...

or

query = my_obj.get_insert_query();

While not ideal, this is a better way of ensuring that you're safe from injection issues without restructuring your entire data layer.

WITH THAT SAID, it's important to be aware that the persistent data layer is the worst layer to have bad architecture in, because it's almost impossible to change once it's in use, and any architectural problems cause problems all the way up the application stack (because you need to write clunky code in order to massage the bad data structure into a clean UI, and the fact that the UI doesn't mirror the data well makes it hard to link visible bug to underlying cause etc).

So while you're still at the stage where you can change the data structure to something more sensible, you should.

Get the data right, everything else is easy. Get the data wrong, everything else is hard.