On Sun, 27 Apr 2008 11:55:18 -0400
Joe <dev@freedomcircle.net> wrote:

> Ivan Sergio Borgonovo wrote:
> > It'd be nice to have a wrapper that let you write prepared
> > statements this way:
> >
> > "select a.id, b.name from a join b on a.id=b.id where
> > a.status=$variable1 and b.id>$variable2 etc... but that's a pretty
> > good change to any language parser.

> Python already supports something like that. See PEP 249
> (http://www.python.org/dev/peps/pep-0249/), under Module Interface,
> the description of the paramstyle parameter. Psycopg2 supports both
> the "format" (C printf) and "pyformat" styles. See the last section
> on this page for an example using the pyformat style:
> http://www.devx.com/opensource/Article/29071/0/page/3.

That's better than nothing but it is still a lot of code duplication.
You've to write column names in the sql statement and in the array
and... column values are not contextual to the statement.
That's easy... while what I wrote above does look as requiring a
really special parser.

Furthermore from the example it looks as if all this is going to
miss the scope to prevent sql injection since it doesn't support
prepared statements.


--
Ivan Sergio Borgonovo
http://www.webthatworks.it


--
Sent via pgsql-sql mailing list (pgsql-sql@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-sql

Hi,

> The problem may be legacy code.

Of course. There is a lot of legacy code that needs to be made secure
('hardened'). The best solution is to use parameterized queries. To
find out what statements are still not doing that, first disable text
literals only. If some places are incorrect, the relevant statements
will be in the log file. After text literals are fixed, number
literals can be disabled.

> Since it is going to make dev nervous because it adds code bloat
> that's going to cause more bugs than the SQL injections it may
> prevent.

I don't think it will really bloat the code. In fact, the code will be
cleaner afterwards.

> "select a.id, b.name from a join b on b.id=a.id where
> a.status='pending' and b.id>7 and b.status='logged'

I even saw code like this in a VIEW: ... and b.type = 10430001 ... The
10430001 had a special meaning. It was documented somewhere, but not
in the code of course ;-) 'pending' and 'logged' are better, but in
regular programming languages usually constants are defined. This also
solves the 'disable literals' pain you have described. The code would
look like this:

select a.id, b.name from a join b on b.id=a.id where
a.status=STATUS_PENDING and b.id>? and b.status=STATUS_LOGGED.

So parameters only need to be used for the actual user input.

> Prepared statements force you to match input with position and it is
> definitively error prone.

The correctness of the application in the normal case is usually
tested. But I didn't so far see any SQL injection tests in regular
application.

> Everything is still in the hands of the dev.

Not always, specially not in banking applications.

> you want to make your dev life a hell

Using parameterized queries is not 'hell', it's best practice.

Regards,
Thomas

On Sun, Apr 27, 2008 at 12:38 PM, Ivan Sergio Borgonovo
<mail@webthatworks.it> wrote:
> On Sat, 26 Apr 2008 21:50:10 -0600
> "Scott Marlowe" <scott.marlowe@gmail.com> wrote:
>
> > Agreed. My point was that to do what the OP wants, wouldn't it make
> > more sense to just lobotomize libpq so it doesn't understand
> > anything but prepared queries. Doesn't obviate the need for a
> > client side language based solution. Just seems to make WAY more
> > sense than trying to make the change at the server level in pgsql.
>
> The problem may be legacy code.
>
> You'd like to statically point out places where multiple statements
> could get injected.
> All calls to your "query" function get routed to a wrapper that
> actually call prepare/execute logic.
> You do a BIG search&replace and see where your code fail cos you
> actually needed more than one statement in a query.
> Now you just have to grep your code for direct call to "plain"
> queries during commit of your rcs.
>
> My proposal was to add a switch that force routing to prepared
> statement logic in libpq.
>
> I'm thinking about situation in which you're using a library that
> already wrap your query call.
> You don't want to change the wrapper, so you don't want to take the
> responsibility, sync troubles etc... of the library maintainer but
> still you'd like to add a safety net to your code.
>
> People dealing with your code would still see the familiar library
> wrapper (you're not wrapping the wrapper) but you'd be able to switch
> to "single statement mode".
>
>
> Still ALLOW_LITERALS is a nice feature even if I think it won't fix
> the notorious SQL injection problem forever.
> Since it is going to make dev nervous because it adds code bloat
> that's going to cause more bugs than the SQL injections it may
> prevent.
> Once you've developers that are so patient to write stuff like:
>
> "select a.id, b.name from a join b on b.id=a.id where
> a.status='pending' and b.id>7 and b.status='logged'"
>
> into
>
> "select a.id, b.name from a join b on b.id=a.id where
> a.status=? and b.id>? and b.status=?", 'pending', 7, 'logged'
>
> there are high chances they will prefer to spend some of their time
> actually thinking about what they are writing.
>
> I do know that thinking can't be taken for granted and that habits
> and automatic methods may be generally preferable to "thinking", but
> automatic methods works when they don't look painful.
> Prepared statements force you to match input with position and it is
> definitively error prone.
>
> It is a tool... you may have some section of your code where that
> parameter can't be changed, but most of the time you'll find it
> useful if its default is set to NONE and dev *can* change it.
>
> Now... let's think at the poor programmer...
>
> He is writing a SQL statement that is static. He has to disable
> ALLOW_LITERALS.
> He is writing dynamic SQL that DON'T take user input.
> ALLOW_LITERALS may still have some sense as a debugging tool but
> there are high chances he will disable it to avoid other errors and
> make coding simpler.
> He is writing dynamic SQL that does take user input. He should be
> forced to use ALLOW_LITTERALS NONE. But how can he be forced in the
> middle of a program?
> He is writing a "mixed" statement where some input is actually static
> but not all... he may think it is easier to allow literals.
>
> Everything is still in the hands of the dev.
> Such setting may help you in static code evaluation since you may
> spot easier the places where there could be breach of policy... but
> still unless you want to make your dev life a hell... it is not going
> to solve the SQL injection problem.
>
> "mixed" statements that use external input and static input are quite
> common and writing them avoiding literals may be a pain that your dev
> won't be willing to suffer.
>
> Queued statements in one query are far less common.
>
> Still I do think that ALLOW_LITERAL is a valuable tool.
> Same problems for legacy code apply.
>
>
> --
> Ivan Sergio Borgonovo
> http://www.webthatworks.it
>
>
> --
>
>
> Sent via pgsql-sql mailing list (pgsql-sql@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-sql
>

--
Sent via pgsql-sql mailing list (pgsql-sql@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-sql

Ivan Sergio Borgonovo wrote:
> That's better than nothing but it is still a lot of code duplication.
> You've to write column names in the sql statement and in the array
> and... column values are not contextual to the statement.
>

The apparent duplication in the example stems for its tutorial nature.
In a real program, the namedict "array" (it's actually a Python
tuple--an immutable array) would normally be constructed
programmatically from user or other input. Note also that although
Joshua chose to use dictionary keys named identical to the PG column
names, they could be named differently, like "first" and "last".
> That's easy... while what I wrote above does look as requiring a
> really special parser.
>
> Furthermore from the example it looks as if all this is going to
> miss the scope to prevent sql injection since it doesn't support
> prepared statements.
>

I assume you didn't check the PEP 249
(http://www.python.org/dev/peps/pep-0249/). The execute() and
executemany() Cursor object methods are precisely to prepare and execute
database operations.

Joe

--
Sent via pgsql-sql mailing list (pgsql-sql@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-sql