Re: Protection from SQL injection - Mailing list pgsql-sql

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