Is This Open To SQL Injection?

[Duncan Booth]

Ian <hobson42 at gmaiil.com> wrote:

> On 07/07/2010 19:38, Victor Subervi wrote:
>> Hi;
>> I have this code:
>>
>>     sql = 'insert into personalDataKeys values (%s, %s, %s)' %
>>     (store, 
>> user, ', %s'.join('%s' * len(col_vals))
>>     cursor.execute(sql, col_vals)
>>
>> Is this open to injection attacks? If so, how correct?
>> TIA,
>> beno
> Yes, it is trivially open to injection attacks.
> 
> What would happen if someone enters the next line into one of your
> col_vals 
> 
> x,y);DROP DATABASE personalDataKeys; ha ha
> 
> Your sql statement would be closed early by the semicolon, and the
> DROP TABLE personalDataKeys is then executed and would cause some
> unexpected data loss.
> 
> Things could be more serious - DROP DATABASE mysql;  for a mysql 
> installation for example.
> 
> You must always always every time and without any exceptions 
> what-so-ever, put all and every piece of data that comes from outside 
> the program through the appropriate routine to make whatever has been 
> entered into storable data and not part of the sql statement.
> 
> In php this is mysql_real_escape_string().  In your favourite language
> there will be an equivalent.
> 
> If you miss just one occurrence its like leaving the side window 
> unlocked! Someone will get in one day.
> 

Try reading the code more carefully. You don't need to escape the 
strings so long as you use cursor.execute properly. In this case it is 
being used properly: there is no danger of injection attacks from 
col_vals.

If `store` or `user` come from user input there is a danger as they are 
being inserted into the generated sql.

As has already been pointed out the code to generate the sql is broken, 
but the principle behind it is sound. In this case the correct thing to 
do would appear to be:

  sql = 'insert into personalDataKeys values (%%s, %%s, %s)' % (','.join
(['%s'] * len(col_vals)))
  cursor.execute(sql, (store, user) + col_vals)

which safely sanitises all of the data passed to the database.

-- 
Duncan Booth http://kupuguy.blogspot.com