Prev: Error message repetition
Next: Download Microsoft C/C++ compiler for use with Python 2.6/2.7 ASAP
From: Stephen Hansen on 7 Jul 2010 14:52
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.

--

Stephen Hansen
... Also: Ixokai
... Mail: me+list/python (AT) ixokai (DOT) io
... Blog: http://meh.ixokai.io/

From: MRAB on 7 Jul 2010 15:26
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)
From: Ian on 7 Jul 2010 16:10
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.

Regards

Ian

p.s. Did I mention that there are no exceptions to the "sanitise every
piece of data" rule?

From: Kee Nethery on 7 Jul 2010 20:14
Yes, you SQL would be trivial to manipulate via SQL injection.

Not only do you need to validate each piece of data submitted by a user, you need to escape all the wildcard characters that your database uses. If the text string supplied by a user has quotes or parens or wildcard characters, the text could be interpreted as SQL and that is what you must avoid.

Kee Nethery
From: alex23 on 8 Jul 2010 01:31
Stephen Hansen <me+list/pyt...(a)ixokai.io> wrote:
> You're doing string formatting
> to construct your SQL, which is where the trouble comes from.

You're wasting your breath, this topic has been discussed ad nauseum
with Victor for well over a year now. He appears to be teaching
himself relational db based web-development within a paid project and
the pressure to produce seems to be greatly overwhelming his need to
learn.

(Yes, I am aware that I'm a bad evil man because I don't believe that
blindly restating the same answer for someone over and over and over
is really helping them)
 |  Next  |  Last
Pages: 1 2
Prev: Error message repetition
Next: Download Microsoft C/C++ compiler for use with Python 2.6/2.7 ASAP