12
\$\begingroup\$

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)
\$\endgroup\$
  • 7
    \$\begingroup\$ You need to add more information to your question. What is database.records_table_name? If it's controlled by the (malicious) user, then of course you have a huge gaping SQL injection vulnerability. If it's not controlled by the user, then you should be okay. \$\endgroup\$ – Quuxplusone Jul 11 '16 at 7:33
  • 2
    \$\begingroup\$ Why do you want the table name to be configurable? \$\endgroup\$ – 200_success Jul 11 '16 at 10:02
  • 3
    \$\begingroup\$ Having the table name be a runtime value is almost always poor design. Whatever value was being used for the table name should instead be a column in a single table. \$\endgroup\$ – Colonel Thirty Two Jul 11 '16 at 12:52
  • 2
    \$\begingroup\$ Is database.records_table_name being supplied by the user or other potential source of malicious/id10t intent? No? You should be good. Yes, kiss your DB bye-bye \$\endgroup\$ – MonkeyZeus Jul 11 '16 at 13:22
  • 2
    \$\begingroup\$ Everybody seems to be going nuts over the string interpolation while this wouldn't by far be the first program that lets the administrator (i.e. trusted person, has access to the DB anyways) specify table names or prefixes. \$\endgroup\$ – Matti Virkkunen Jul 12 '16 at 6:03
36
\$\begingroup\$

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.

\$\endgroup\$
19
\$\begingroup\$

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.

enter image description here

\$\endgroup\$
  • 1
    \$\begingroup\$ You say "yes it is" to "is it safe" but then explain why it isn't safe \$\endgroup\$ – Mego Jul 11 '16 at 10:30
13
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ I was hoping to use different tables for different scenarios, since there's going to be a lot of data, and I didn't want to have to search through one large database for every query as that seems inefficient to me. Or are you saying there's no efficiency benefit for having different tables? \$\endgroup\$ – JasTonAChair Jul 11 '16 at 9:00
  • \$\begingroup\$ Also, no the records are not identical. \$\endgroup\$ – JasTonAChair Jul 11 '16 at 9:05
  • \$\begingroup\$ when you say 'a lot of data' do you mean multiple billions of rows? proper indexing will do far more than splitting out into tens or hundreds of tables \$\endgroup\$ – Caleth Jul 11 '16 at 9:13
  • \$\begingroup\$ There's a couple of million rows. This is my first attempt at reinforcement learning, and I want to tear down agents' memories if I need to, so having several tables is not so much an efficiency thing, it's more about ease of creating new agents. \$\endgroup\$ – JasTonAChair Jul 11 '16 at 9:37
  • 1
    \$\begingroup\$ Yeah, I really have been thinking about it the wrong way. Thanks. \$\endgroup\$ – JasTonAChair Jul 11 '16 at 11:24
3
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Hi, I considered the second option a little while ago. When I looked at that option though it seemed messy. But, as Gareth pointed out, I don't actually need dynamic table names at all. I just had to re-frame the problem really! \$\endgroup\$ – JasTonAChair Jul 11 '16 at 15:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.