Thread: Willing to fix a PQexec() in libpq module

Hello.

At Tue, 19 Mar 2019 08:18:23 +0000, "Wu, Fei" <wufei.fnst@cn.fujitsu.com> wrote in
<52E6E0843B9D774C8C73D6CF64402F05621F0FFC@G08CNEXMBPEKD02.g08.fujitsu.local>
> Hi,all
> 
> On website: https://wiki.postgresql.org/wiki/Todo#libpq
> I found that in libpq module,there is a TODO case:
> -------------------------------------------------------------------------------
> Consider disallowing multiple queries in PQexec() as an additional barrier to SQL injection attacks
> -------------------------------------------------------------------------------
> I am interested in this one. So ,Had it be fixed?
> If not, I am willing to do so.
> In manual, I found that:
> -----------------------------------------------------------------------------
> Unlike PQexec, PQexecParams allows at most one SQL command in the given string. (There can be
> semicolons in it, but not more than one nonempty command.) This is a limitation of the underlying
> protocol, but has some usefulness as an extra defense against SQL-injection attacks.
> 
> -------------------------------------------------------------------------------
> Maybe we can fix PQexec() just likes PQexecParams()?
> 
> I will try to fix it~

I don't oppose that, but as the discussion linked from there [1],
psql already has a feature that sends multiple statements by one
PQexec() in two ways. Fixing it means making the features
obsolete.

psql db -c 'select 1; select 1;'

bash> psql db
db=> select 1\; select 1;


I couldn't find the documentation about the behavior..

[1] https://www.postgresql.org/message-id/9236.1167968298@sss.pgh.pa.us

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> At Tue, 19 Mar 2019 08:18:23 +0000, "Wu, Fei" <wufei.fnst@cn.fujitsu.com> wrote in
<52E6E0843B9D774C8C73D6CF64402F05621F0FFC@G08CNEXMBPEKD02.g08.fujitsu.local>
>> I will try to fix it~

> I don't oppose that, but as the discussion linked from there [1],
> psql already has a feature that sends multiple statements by one
> PQexec() in two ways. Fixing it means making the features
> obsolete.

Yeah, the problem here is that a lot of people think that that's
a feature not a bug.  You certainly can't get away with just summarily
changing the behavior of PQexec without any recourse.  Maybe there
would be acceptance for either of

(1) a different function that is like PQexec but restricts the
query string

(2) a connection option or state variable that affects PQexec's
behavior --- but it probably still has to default to permissive.

Unfortunately, if the default behavior doesn't change, then there's little
argument for doing this at all.  The security reasoning behind doing
anything in this area would be to provide an extra measure of protection
against SQL-injection attacks on carelessly-written clients, and of course
it's unlikely that a carelessly-written client would get changed to make
use of a non-default feature.

So that's why nothing has been done about this for umpteen years.
If somebody can think of a way to resolve this tension, maybe the
item will get finished; but it's not just a matter of writing some
code.

            regards, tom lane

On Tue, Mar 19, 2019 at 10:30:45AM -0400, Tom Lane wrote:
> Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> > At Tue, 19 Mar 2019 08:18:23 +0000, "Wu, Fei" <wufei.fnst@cn.fujitsu.com> wrote in
<52E6E0843B9D774C8C73D6CF64402F05621F0FFC@G08CNEXMBPEKD02.g08.fujitsu.local>
> >> I will try to fix it~
> 
> > I don't oppose that, but as the discussion linked from there [1],
> > psql already has a feature that sends multiple statements by one
> > PQexec() in two ways. Fixing it means making the features
> > obsolete.
> 
> Yeah, the problem here is that a lot of people think that that's
> a feature not a bug.  You certainly can't get away with just summarily
> changing the behavior of PQexec without any recourse.  Maybe there
> would be acceptance for either of
> 
> (1) a different function that is like PQexec but restricts the
> query string
> 
> (2) a connection option or state variable that affects PQexec's
> behavior --- but it probably still has to default to permissive.
> 
> Unfortunately, if the default behavior doesn't change, then there's little
> argument for doing this at all.  The security reasoning behind doing
> anything in this area would be to provide an extra measure of protection
> against SQL-injection attacks on carelessly-written clients, and of course
> it's unlikely that a carelessly-written client would get changed to make
> use of a non-default feature.

It's also unlikely that writers and maintainers of carelessly-written
clients are our main user base. Quite the opposite, in fact. Do we
really need to set their failure to make an effort as a higher
priority than getting this fixed?

I think the answer is "no," and we should deprecate this misfeature.
It's bad enough that we'll be supporting it for five years after
deprecating it, but it's worse to leave it hanging around our necks
forever. https://en.wikipedia.org/wiki/Albatross_(metaphor)

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter <david@fetter.org> writes:
> I think the answer is "no," and we should deprecate this misfeature.
> It's bad enough that we'll be supporting it for five years after
> deprecating it, but it's worse to leave it hanging around our necks
> forever. https://en.wikipedia.org/wiki/Albatross_(metaphor)

The problem with that approach is that not everybody agrees that
it's a misfeature.

            regards, tom lane

Hi,

On 2019-03-19 12:51:39 -0400, Tom Lane wrote:
> David Fetter <david@fetter.org> writes:
> > I think the answer is "no," and we should deprecate this misfeature.
> > It's bad enough that we'll be supporting it for five years after
> > deprecating it, but it's worse to leave it hanging around our necks
> > forever. https://en.wikipedia.org/wiki/Albatross_(metaphor)
> 
> The problem with that approach is that not everybody agrees that
> it's a misfeature.

Yea, it's extremely useful to just be able to send a whole script to the
server. Otherwise every application wanting to do so needs to be able to
split SQL statements, not exactly a trivial task. And the result will be
slower, due to increased rountrips.

Greetings,

Andres Freund

On 2019-Mar-19, Andres Freund wrote:

> Hi,
> 
> On 2019-03-19 12:51:39 -0400, Tom Lane wrote:
> > David Fetter <david@fetter.org> writes:
> > > I think the answer is "no," and we should deprecate this misfeature.
> > > It's bad enough that we'll be supporting it for five years after
> > > deprecating it, but it's worse to leave it hanging around our necks
> > > forever. https://en.wikipedia.org/wiki/Albatross_(metaphor)
> > 
> > The problem with that approach is that not everybody agrees that
> > it's a misfeature.
> 
> Yea, it's extremely useful to just be able to send a whole script to the
> server. Otherwise every application wanting to do so needs to be able to
> split SQL statements, not exactly a trivial task. And the result will be
> slower, due to increased rountrips.

I suppose it can be argued that for the cases where they want that, it
is not entirely ridiculous to have it be done with a different API call,
say PQexecMultiple.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Mar 19, 2019 at 01:59:34PM -0300, Alvaro Herrera wrote:
> On 2019-Mar-19, Andres Freund wrote:
> 
> > Hi,
> > 
> > On 2019-03-19 12:51:39 -0400, Tom Lane wrote:
> > > David Fetter <david@fetter.org> writes:
> > > > I think the answer is "no," and we should deprecate this misfeature.
> > > > It's bad enough that we'll be supporting it for five years after
> > > > deprecating it, but it's worse to leave it hanging around our necks
> > > > forever. https://en.wikipedia.org/wiki/Albatross_(metaphor)
> > > 
> > > The problem with that approach is that not everybody agrees that
> > > it's a misfeature.
> > 
> > Yea, it's extremely useful to just be able to send a whole script to the
> > server. Otherwise every application wanting to do so needs to be able to
> > split SQL statements, not exactly a trivial task. And the result will be
> > slower, due to increased rountrips.
> 
> I suppose it can be argued that for the cases where they want that, it
> is not entirely ridiculous to have it be done with a different API call,
> say PQexecMultiple.

Renaming it to emphasize that it's a non-default choice seems like a
large step in the right direction.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Hi,

On 2019-03-19 13:59:34 -0300, Alvaro Herrera wrote:
> On 2019-Mar-19, Andres Freund wrote:
> > On 2019-03-19 12:51:39 -0400, Tom Lane wrote:
> > > David Fetter <david@fetter.org> writes:
> > > > I think the answer is "no," and we should deprecate this misfeature.
> > > > It's bad enough that we'll be supporting it for five years after
> > > > deprecating it, but it's worse to leave it hanging around our necks
> > > > forever. https://en.wikipedia.org/wiki/Albatross_(metaphor)
> > > 
> > > The problem with that approach is that not everybody agrees that
> > > it's a misfeature.
> > 
> > Yea, it's extremely useful to just be able to send a whole script to the
> > server. Otherwise every application wanting to do so needs to be able to
> > split SQL statements, not exactly a trivial task. And the result will be
> > slower, due to increased rountrips.
> 
> I suppose it can be argued that for the cases where they want that, it
> is not entirely ridiculous to have it be done with a different API call,
> say PQexecMultiple.

Sure, but what'd the gain be? Using PQexecParams() already enforces that
there's only a single command. Sure, explicit is better than implicit
and all that, but is that justification for breaking a significant
number of applications?

Greetings,

Andres Freund

On 2019-03-19 10:02:33 -0700, Andres Freund wrote:
> Hi,
> 
> On 2019-03-19 13:59:34 -0300, Alvaro Herrera wrote:
> > On 2019-Mar-19, Andres Freund wrote:
> > > On 2019-03-19 12:51:39 -0400, Tom Lane wrote:
> > > > David Fetter <david@fetter.org> writes:
> > > > > I think the answer is "no," and we should deprecate this misfeature.
> > > > > It's bad enough that we'll be supporting it for five years after
> > > > > deprecating it, but it's worse to leave it hanging around our necks
> > > > > forever. https://en.wikipedia.org/wiki/Albatross_(metaphor)
> > > > 
> > > > The problem with that approach is that not everybody agrees that
> > > > it's a misfeature.
> > > 
> > > Yea, it's extremely useful to just be able to send a whole script to the
> > > server. Otherwise every application wanting to do so needs to be able to
> > > split SQL statements, not exactly a trivial task. And the result will be
> > > slower, due to increased rountrips.
> > 
> > I suppose it can be argued that for the cases where they want that, it
> > is not entirely ridiculous to have it be done with a different API call,
> > say PQexecMultiple.
> 
> Sure, but what'd the gain be? Using PQexecParams() already enforces that
> there's only a single command. Sure, explicit is better than implicit
> and all that, but is that justification for breaking a significant
> number of applications?

In short: I think we should just remove this todo entry.  If somebody
feels like we should do something, I guess making the dangers of
PQexec() vs PQexecPrepared() even clearer would be the best thing to
do. Although I actually find it easy enough, it's not like we're holding
back:

https://www.postgresql.org/docs/devel/libpq-exec.html

PQexec():

The command string can include multiple SQL commands (separated by semicolons). Multiple queries sent in a single
PQexeccall are processed in a single transaction, unless there are explicit BEGIN/COMMIT commands included in the query
stringto divide it into multiple transactions. (See Section 52.2.2.1 for more details about how the server handles
multi-querystrings.) Note however that the returned PGresult structure describes only the result of the last command
executedfrom the string. Should one of the commands fail, processing of the string stops with it and the returned
PGresultdescribes the error condition.
 

PQexecParams():
Unlike PQexec, PQexecParams allows at most one SQL command in the given string. (There can be semicolons in it, but not
morethan one nonempty command.) This is a limitation of the underlying protocol, but has some usefulness as an extra
defenseagainst SQL-injection attacks.
 

Andres Freund <andres@anarazel.de> writes:
> On 2019-03-19 13:59:34 -0300, Alvaro Herrera wrote:
>> I suppose it can be argued that for the cases where they want that, it
>> is not entirely ridiculous to have it be done with a different API call,
>> say PQexecMultiple.

> Sure, but what'd the gain be? Using PQexecParams() already enforces that
> there's only a single command. Sure, explicit is better than implicit
> and all that, but is that justification for breaking a significant
> number of applications?

Right, the tradeoff here comes down to breaking existing apps vs.
adding security for poorly-written apps.  Whether you think it's
worthwhile to break stuff depends on your estimate of how common
poorly-written apps are.  To that point, I'd be inclined to throw
David's previous comment back at him: they're likely not that
common.  A well-written app should probably be treating insecure
inputs as parameters in PQexecParams anyhow, making this whole
discussion moot.

Having said that ... a better argument for a new API is that it
could be explicitly designed to handle multiple queries, and in
particular make some provision for returning multiple PGresults.
Maybe if we had that there would be more support for deprecating
the ability to send multiple queries in plain PQexec.  It'd still
be a long time before we could turn it off though, at least by
default.

            regards, tom lane

Hi,

On 2019-03-19 13:18:25 -0400, Tom Lane wrote:
> Having said that ... a better argument for a new API is that it
> could be explicitly designed to handle multiple queries, and in
> particular make some provision for returning multiple PGresults.

Oh, I completely agree, that'd be hugely useful.

Greetings,

Andres Freund

On Tue, Mar 19, 2019 at 01:18:25PM -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2019-03-19 13:59:34 -0300, Alvaro Herrera wrote:
> >> I suppose it can be argued that for the cases where they want that, it
> >> is not entirely ridiculous to have it be done with a different API call,
> >> say PQexecMultiple.
> 
> > Sure, but what'd the gain be? Using PQexecParams() already enforces that
> > there's only a single command. Sure, explicit is better than implicit
> > and all that, but is that justification for breaking a significant
> > number of applications?
> 
> Right, the tradeoff here comes down to breaking existing apps vs.
> adding security for poorly-written apps.  Whether you think it's
> worthwhile to break stuff depends on your estimate of how common
> poorly-written apps are.  To that point, I'd be inclined to throw
> David's previous comment back at him: they're likely not that
> common.  A well-written app should probably be treating insecure
> inputs as parameters in PQexecParams anyhow, making this whole
> discussion moot.
> 
> Having said that ... a better argument for a new API is that it
> could be explicitly designed to handle multiple queries, and in
> particular make some provision for returning multiple PGresults.

That sounds like it'd be *really* handy if one were building a
client-side retry framework. People will be doing (the equivalent of)
this as the vulnerabilities inherent in isolation levels lower than
SERIALIZABLE become better known.
https://www.cockroachlabs.com/blog/acid-rain/

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Andres Freund <andres@anarazel.de> writes:
> On 2019-03-19 13:18:25 -0400, Tom Lane wrote:
>> Having said that ... a better argument for a new API is that it
>> could be explicitly designed to handle multiple queries, and in
>> particular make some provision for returning multiple PGresults.

> Oh, I completely agree, that'd be hugely useful.

Of course, you can do that already with PQsendQuery + a loop
around PQgetResult.  So the question here is whether that can
be wrapped up into something easier-to-use.  I'm not entirely
sure what that might look like.

We should also keep in mind that there's a perfectly valid
use-case for wanting to send a big script of commands and
just check for overall success or failure.  So it's not like
PQexec's current behavior has *no* valid uses.

            regards, tom lane

    Tom Lane wrote:

> Unfortunately, if the default behavior doesn't change, then there's little
> argument for doing this at all.  The security reasoning behind doing
> anything in this area would be to provide an extra measure of protection
> against SQL-injection attacks on carelessly-written clients, and of course
> it's unlikely that a carelessly-written client would get changed to make
> use of a non-default feature.

A patch introducing an "allow_multiple_queries" GUC to
control this was proposed and eventually rejected for lack of
consensus some time ago (also there were some concerns about
the implementation that might have played against it too):

https://www.postgresql.org/message-id/CALAY4q_eHUx%3D3p1QUOvabibwBvxEWGm-bzORrHA-itB7MBtd5Q%40mail.gmail.com

About the effectiveness of this feature, there's a valid use case in
which it's not the developers who decide to set this GUC, but the DBA
or the organization deploying the application. That applies to
applications that of course do not intentionally use multiple queries
per command.
That would provide a certain level a protection against SQL
injections, without changing the application or libpq or breaking
backward compatibility, being optional.

But both in this thread and the other thread, the reasoning about the
GUC seems to make the premise that applications would
need to be updated or developpers need to be aware of it,
as if they _had_ to issue SET allow_multiple_queries TO off/on,
rather than being submitted to it, as imposed upon them by
postgresql.conf or the database settings.

If we compare this to, say, lo_compat_privileges. An application
typically doesn't get to decide whether it's "on". It's for a
superuser to decide which databases or which users must operate with
this setting to "on".
Why wouldn't that model work for disallowing multiple queries per command?


Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

Hi, thanks for all replies.
According to all your discussions, Maybe the problems is that
1) keep modifications just in client side;
2) modifications VS client current applications

Maybe we could create a new function(May called PQexecSafe() ) just likes PQexec() but with additional input
argument(Maycalled issafe) to switch whether allowing at most one SQL command.
 
In that way, clients who want the safe feature just use the new function PQexecSafe() with issafe set true,
The others can choose:
1) just use the old version PQexec(),
2) using PQexecSafe() with issafe set false

Then, we strongly recommended using PQexecSafe(),and PQexec() keep in use but labeled deprecated in documents. In other
word,give client the time to choose and modify their applications if then want use the safe feature.
 

Of course, we should admit that it is not just a coding problem.


-----Original Message-----
From: Daniel Verite [mailto:daniel@manitou-mail.org] 
Sent: Wednesday, March 20, 2019 3:08 AM
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>; Wu, Fei/吴 非 <wufei.fnst@cn.fujitsu.com>;
pgsql-hackers@postgresql.org
Subject: Re: Willing to fix a PQexec() in libpq module

    Tom Lane wrote:

> Unfortunately, if the default behavior doesn't change, then there's 
> little argument for doing this at all.  The security reasoning behind 
> doing anything in this area would be to provide an extra measure of 
> protection against SQL-injection attacks on carelessly-written 
> clients, and of course it's unlikely that a carelessly-written client 
> would get changed to make use of a non-default feature.

A patch introducing an "allow_multiple_queries" GUC to control this was proposed and eventually rejected for lack of
consensussome time ago (also there were some concerns about the implementation that might have played against it too):
 

https://www.postgresql.org/message-id/CALAY4q_eHUx%3D3p1QUOvabibwBvxEWGm-bzORrHA-itB7MBtd5Q%40mail.gmail.com

About the effectiveness of this feature, there's a valid use case in which it's not the developers who decide to set
thisGUC, but the DBA or the organization deploying the application. That applies to applications that of course do not
intentionallyuse multiple queries per command.
 
That would provide a certain level a protection against SQL injections, without changing the application or libpq or
breakingbackward compatibility, being optional.
 

But both in this thread and the other thread, the reasoning about the GUC seems to make the premise that applications
wouldneed to be updated or developpers need to be aware of it, as if they _had_ to issue SET allow_multiple_queries TO
off/on,rather than being submitted to it, as imposed upon them by postgresql.conf or the database settings.
 

If we compare this to, say, lo_compat_privileges. An application typically doesn't get to decide whether it's "on".
It'sfor a superuser to decide which databases or which users must operate with this setting to "on".
 
Why wouldn't that model work for disallowing multiple queries per command?


Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

Hi,

On 2019-03-20 02:19:54 +0000, Wu, Fei wrote:
> Hi, thanks for all replies.
> According to all your discussions, Maybe the problems is that
> 1) keep modifications just in client side;
> 2) modifications VS client current applications
> 
> Maybe we could create a new function(May called PQexecSafe() ) just likes PQexec() but with additional input
argument(Maycalled issafe) to switch whether allowing at most one SQL command.
 
> In that way, clients who want the safe feature just use the new function PQexecSafe() with issafe set true,
> The others can choose:
> 1) just use the old version PQexec(),
> 2) using PQexecSafe() with issafe set false
> 
> Then, we strongly recommended using PQexecSafe(),and PQexec() keep in use but labeled deprecated in documents. In
otherword, give client the time to choose and modify their applications if then want use the safe feature.
 

We already have PQexecParams(). And there's already comments explaining
the multi-statement behaviour in the docs.  Do you see an additional
advantage in your proposal?

Greetings,

Andres Freund