Is This Open To SQL Injection?

[MRAB]

Stephen Hansen wrote:
> On 7/7/10 11:38 AM, 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)
> 
> First, its always best to be explicit with insert statements. Meaning,
> don't rely on the underlining structure of a table, as in:
> 
> INSERT INTO YourRandomTable VALUES ("my", "value", "here");
> 
> Instead, do:
> 
> INSERT INTO YourRandomTable (field1, field2, field3) VALUES ("my",
> "value", "here");
> 
>> Is this open to injection attacks? If so, how correct?
> 
> Secondly, I'd say: probably yes. Maybe. You're doing string formatting
> to construct your SQL, which is where the trouble comes from. Its
> possible to do safely, but takes exquisite care -- which is why we've
> been nudging you away from it.
> 
> But I can't be a whole lot more specific because I can't for the life of
> me figure out what you're actually doing with it.
> 
> I can't figure out why you're passing the store and user variables into
> the SQL statement, instead of just passing them into the .execute as you
> are supposed to. I.e., cursor.execute(sql, [store, user] + col_vals) or
> something.
> 
> It looks like you're sort of trying to get one generic SQL statement
> which can set some arbitrary number of random columns-- if so, why? I
> can't picture just what this table layout is or what kind of data it
> holds to advise better.
> 
Not only that, there's a missing ")" and the .join is wrong. For
example, if 'store' is "STORE", 'user' is "USER" and 'col_vals' has,
say, 3 members, then what you get is:

     insert into personalDataKeys values (STORE, USER, %, %ss, %s%, %ss, 
%s%, %ss)