Re: [HACKERS] Disallowing multiple queries per PQexec() - Mailing list pgsql-hackers

Hello Surafel,

My 0.02€:

> I attach a patch that incorporate the comments and uses similar routines
> with the rest of the file rather than using command tag

I'm not fully convinced by this feature: using multiple queries is a 
useful trick to reduce network-related latency by combining several 
queries in one packet. Devs and even ORMs could use this trick.

Now I do also understand the point of trying to shield against some types 
of SQL injection at the database level, because the other levels have 
shown weaknesses in the past...

However the protection offered here is quite partial: it really helps if 
say a SELECT operation is turned into something else which modifies the 
database. Many SQL injection attacks focus on retrieving sensitive data, 
in which case "' UNION SELECT ... --" would still work nicely. Should we 
allow to forbid UNION? And/or disallow comments as well, which are 
unlikely to be used from app code, and would make this harder? If pg is to 
provide SQL injection guards by removing some features on some 
connections, maybe it could be more complete about it.

About the documentation:
 +        When this parameter is on, the <productname>PostgreSQL</> server +        allow

allows
 +        multiple SQL commands without being a transaction block in the +        given string in simple query
protocol.

This sentence is not very clear.

I'm unsure about this "transaction block" exception, because (1) the name 
of the setting becomes misleading, it should really be: 
"allow_multiple_queries_outside_transaction_block", and maybe it should 
also say that it is only for the simple protocol (if it is the case), (2) 
there may be SQL injections in a transaction block anyway and they are not 
prevented nor preventable (3) if I'm combining queries for latency 
optimization, it does not mean that I do want a single transaction block 
anyway in the packet, so it does not help me all the way there.
 +        setting

Setting

+ this parameter off is use for  providing an

One useless space between "for" & "providing".

Maybe be more direct "... off provides ...". Otherwise "is used for"

+       additional defense against SQL-injection attacks.

I would add something about the feature cost: ", at the price of rejecting 
combined queries."

+       The server may be configure to disallow multiple sql

SQL ?

+       commands without being a transaction block to add an extra defense against SQL-injection 
+       attacks

some type of SQL injections attacks?

+       so it is always a good practice to add 
+       <command>BEGIN</command>/<command>COMMIT
+       </command> commands for multiple sql commands

I do not really agree that it is an advisable "good practice" to do 
that...

-- 
Fabien.