23

As the title suggests, I would like to know if this code is vulnerable to SQL Injection? And if so, is there a better, more secure, way of achieving the same thing? 

def add(table,*args):
    statement="INSERT INTO %s VALUES %s" % (table,args)
    cursor.execute(statement)
PearsonArtPhoto
  • 38,970
  • 17
  • 111
  • 142
Sheldon
  • 9,639
  • 20
  • 59
  • 96

1 Answers1

41

Yes, it is. Use something like this to prevent it:

cursor.execute("INSERT INTO table VALUES ?", args)

Note that you cannot enter the table in like this. Ideally the table should be hard coded, in no circumstance 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.

Essentially, you want to put the parameters in the cursor command, because it will make sure to make the data database safe. With your first command, it would be relatively easy to make a special table or args that put something into your SQL code that wasn't safe. See the python pages, and the referenced http://xkcd.com/327/ . Specifically, the python pages quote:

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 (see http://xkcd.com/327/ for humorous example of what can go wrong).

Instead, use the DB-API’s parameter substitution. Put ? as a placeholder wherever you want to use a value, and then provide a tuple of values as the second argument to the cursor’s execute() method. (Other database modules may use a different placeholder, such as %s or :1.)

Basically, someone could set an args that executed another command, something like this:

args="name; DELETE table"

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

Community
  • 1
  • 1
PearsonArtPhoto
  • 38,970
  • 17
  • 111
  • 142
  • 1
    Thanks. Can you expand a bit as to why it is vulnerable? – Sheldon Nov 28 '12 at 19:43
  • 2
    It might be quiet obvious, but just to be safe: Do this for ALL statements, not just INSERT. A SELECT statement is just as vulnerable. – Niclas Nilsson Nov 28 '12 at 19:46
  • @PearsonArtPhoto I get "TypeError: function takes at most 2 arguments (3 given)" – Sheldon Nov 28 '12 at 19:46
  • Still seem to be getting an error - "sqlite3.OperationalError: near "?": syntax error" – Sheldon Nov 28 '12 at 19:54
  • 2
    5 upvotes, accepted and no one bothered to check? (http://stackoverflow.com/questions/5870284/can-i-use-parameters-for-the-table-name-in-sqlite3) – lqc Nov 28 '12 at 20:05
  • 3
    Can you explain in more detail why/how using parameters (?) will not return the same executable SQL code as (%s) given the same input (table,args) strings are provided? – RyanKDalton Nov 28 '12 at 20:17
  • 5
    @RyanDalton: Using parameters means sqlite3 (or your other DB library) can bind the parameters however it wants. It might just make sure everything is properly quoted and escaped (possibly using features specific to that DBMS, or otherwise hard for you to get right). Or it might actually bind the params after parsing the SQL statement into an internal format (which is likely to be faster, as well as safer). The point is that any DB interface is required to do _something_ safe with parameters, while with strings you have to figure out and do the same thing yourself (and will do it wrong). – abarnert Nov 28 '12 at 20:36