Re: Disallowing multiple queries per PQexec()

From: Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>
To: Surafel Temesgen <surafel3000(at)gmail(dot)com>
Cc: Daniel Verite <daniel(at)manitou-mail(dot)org>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Disallowing multiple queries per PQexec()
Date: 2017-06-14 15:17:03
Message-ID: alpine.DEB.2.20.1706141630390.4158@lancre
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: 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.

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Bruce Momjian 2017-06-14 15:26:03 Re: WIP: Data at rest encryption
Previous Message Bruce Momjian 2017-06-14 15:14:09 Re: Broken hint bits (freeze)