Disallowing multiple queries per PQexec()

This assignment is on todo list and has a benefit of providing an
additional defense against SQL-injection attacks. Previous mailing list
discussion is here
<https://www.postgresql.org/message-id/[email protected]> and I
attach a small patch that fix the issue by checking whether query string
contains multiple sql commands without being a transaction block or not and
emits appropriate error message in the case of non-transaction block
multiple query string.

This patch tests using psql –c option

i.e. if it’s not a transaction block and have multiple query string ,it
emits appropriate error message.

psql -c 'DECLARE myportal CURSOR FOR select * from pg_database;FETCH ALL in
myportal;CLOSE myportal' postgres

ERROR: cannot execute multiple commands unless it is a transaction block

In a case of transaction block and single command query string it continue
with normal execution

psql -c 'BEGIN;DECLARE myportal CURSOR FOR select * from pg_database;FETCH
ALL in myportal;CLOSE myportal;END' postgres

COMMIT

psql -c 'CREATE TABLE foo();' postgres

CREATE TABLE

Comments?

Regards

Surafel

Surafel Temesgen <surafel3000(at)gmail(dot)com> writes:
> This assignment is on todo list and has a benefit of providing an
> additional defense against SQL-injection attacks.

This is on the todo list? Really? It seems unlikely to be worth the
backwards-compatibility breakage. I certainly doubt that we could
get away with unconditionally rejecting such cases with no "off" switch,
as you have here.

> Previous mailing list discussion is here
> <https://www.postgresql.org/message-id/[email protected]>

That message points out specifically that we *didn't* plan to do this.
Perhaps back then (ten years ago) we could have gotten away with the
compatibility breakage, but now I doubt it.

regards, tom lane

On Tue, Feb 28, 2017 at 09:04:29AM -0500, Tom Lane wrote:
> Surafel Temesgen <surafel3000(at)gmail(dot)com> writes:
> > This assignment is on todo list and has a benefit of providing an
> > additional defense against SQL-injection attacks.
>
> This is on the todo list? Really? It seems unlikely to be worth the
> backwards-compatibility breakage. I certainly doubt that we could
> get away with unconditionally rejecting such cases with no "off" switch,
> as you have here.
>
> > Previous mailing list discussion is here
> > <https://www.postgresql.org/message-id/[email protected]>
>
> That message points out specifically that we *didn't* plan to do this.
> Perhaps back then (ten years ago) we could have gotten away with the
> compatibility breakage, but now I doubt it.

I might have added that one; the text is:

Consider disallowing multiple queries in PQexec()
as an additional barrier to SQL injection attacks

and it is a "consider" item. Should it be moved to the Wire Protocol
Changes / v4 Protocol section or removed?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +

On 02/28/2017 03:13 PM, Bruce Momjian wrote:
> I might have added that one; the text is:
>
> Consider disallowing multiple queries in PQexec()
> as an additional barrier to SQL injection attacks
>
> and it is a "consider" item. Should it be moved to the Wire Protocol
> Changes / v4 Protocol section or removed?

A new protocol version wont solve the breakage of the C API, so I am not
sure we can ever drop this feature other than by adding a new function
something in the protocol to support this.

Andreas

On 2017-02-28 15:59:08 +0100, Andreas Karlsson wrote:
> On 02/28/2017 03:13 PM, Bruce Momjian wrote:
> > I might have added that one; the text is:
> >
> > Consider disallowing multiple queries in PQexec()
> > as an additional barrier to SQL injection attacks
> >
> > and it is a "consider" item. Should it be moved to the Wire Protocol
> > Changes / v4 Protocol section or removed?
>
> A new protocol version wont solve the breakage of the C API, so I am not
> sure we can ever drop this feature other than by adding a new function
> something in the protocol to support this.

The protocol and C APIs to enforce this are already available, no? The
extended protocol (and thus PQexecParam/PQExecPrepared/...) don't allow
multiple statements:

/*
* We only allow a single user statement in a prepared statement. This is
* mainly to keep the protocol simple --- otherwise we'd need to worry
* about multiple result tupdescs and things like that.
*/
if (list_length(parsetree_list) > 1)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("cannot insert multiple commands into a prepared statement")));

So if you don't want to allow multiple statements, use PQexecParams et
al.

- Andres

On 2/28/17 2:45 PM, Andres Freund wrote:
> So if you don't want to allow multiple statements, use PQexecParams et
> al.

That does leave most application authors out in the cold though, since
they're using a higher level connection manager.

If the maintenance burden isn't terribly high it would be nice to allow
disabling multiple statements via a GUC.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)

As far as my understanding the issue at that time was inability to process
creation

of a database and connecting to it with one query string and that can be
solved by

fixing transaction restriction checks for CREATE DATABASE or disallowing
multiple

queries in PQexe.

If the issue solved and allowing multiple queries in PQexec doesn’t result
in SQL injection

attacks that worth backwards-compatibility breakage by itself the item can
be drop or

included to v4 Protocol section if it contains items that break
backwards-compatibility already

regards

surafel

On Thu, Mar 2, 2017 at 1:02 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:

> On 2/28/17 2:45 PM, Andres Freund wrote:
>
>> So if you don't want to allow multiple statements, use PQexecParams et
>> al.
>>
>
> That does leave most application authors out in the cold though, since
> they're using a higher level connection manager.
>
> If the maintenance burden isn't terribly high it would be nice to allow
> disabling multiple statements via a GUC.
> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
> Experts in Analytics, Data Architecture and PostgreSQL
> Data in Trouble? Get it in Treble! http://BlueTreble.com
> 855-TREBLE2 (855-873-2532)
>

On Tue, Feb 28, 2017 at 7:34 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Surafel Temesgen <surafel3000(at)gmail(dot)com> writes:
>> This assignment is on todo list and has a benefit of providing an
>> additional defense against SQL-injection attacks.
>
> This is on the todo list? Really? It seems unlikely to be worth the
> backwards-compatibility breakage. I certainly doubt that we could
> get away with unconditionally rejecting such cases with no "off" switch,
> as you have here.
>
>> Previous mailing list discussion is here
>> <https://www.postgresql.org/message-id/[email protected]>
>
> That message points out specifically that we *didn't* plan to do this.
> Perhaps back then (ten years ago) we could have gotten away with the
> compatibility breakage, but now I doubt it.

Probably true, but I bet it would be OK to add this as an optional
behavior, controlled by a GUC. I know behavior-changing GUCs aren't
good, but this seems like a sufficiently-peripheral behavior that it
would be OK. Extensions, for example, wouldn't break, because
they're executing inside the database, not through libpq. Stored
procedures wouldn't break either. The only real risk is that the
user's application itself might break, but there's an easy solution to
that problem.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Sorry for being very late. I also think guc version of the patch can be
acceptable and useful.

I modified the patch as such and added to commitfest 2017-07.

Regards

Surafel

On Sat, Mar 4, 2017 at 10:24 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Tue, Feb 28, 2017 at 7:34 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Surafel Temesgen <surafel3000(at)gmail(dot)com> writes:
> >> This assignment is on todo list and has a benefit of providing an
> >> additional defense against SQL-injection attacks.
> >
> > This is on the todo list? Really? It seems unlikely to be worth the
> > backwards-compatibility breakage. I certainly doubt that we could
> > get away with unconditionally rejecting such cases with no "off" switch,
> > as you have here.
> >
> >> Previous mailing list discussion is here
> >> <https://www.postgresql.org/message-id/[email protected]>
> >
> > That message points out specifically that we *didn't* plan to do this.
> > Perhaps back then (ten years ago) we could have gotten away with the
> > compatibility breakage, but now I doubt it.
>
> Probably true, but I bet it would be OK to add this as an optional
> behavior, controlled by a GUC. I know behavior-changing GUCs aren't
> good, but this seems like a sufficiently-peripheral behavior that it
> would be OK. Extensions, for example, wouldn't break, because
> they're executing inside the database, not through libpq. Stored
> procedures wouldn't break either. The only real risk is that the
> user's application itself might break, but there's an easy solution to
> that problem.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

On Thu, May 18, 2017 at 2:56 AM, Surafel Temesgen <surafel3000(at)gmail(dot)com>
wrote:

> Sorry for being very late. I also think guc version of the patch can be
> acceptable and useful.
>
> I modified the patch as such and added to commitfest 2017-07.
>
>
>
You need documentation changes in "libpq - C Library" chapter's PQexec
section, where it is mentioned that command string can contain multiple SQL
commands and explain how it behaves.

I think GUC's name can be something like "multiple_query_execution" and
setting it ON/OFF will be better. I think others will also come up with
some suggestions here as the current name doesn't go well with other
existing GUCs.

Also, consider adding this new GUC in postgresql.conf.sample file.

Regards,
Vaishnavi,
Fujitsu Australia.

hey Vaishnavi

>
> I think GUC's name can be something like "multiple_query_execution" and
> setting it ON/OFF will be better. I think others will also come up with
> some suggestions here as the current name doesn't go well with other
> existing GUCs.
>
Thank you very much for the suggestion multiple_query_execution is better
and can be set using ON/OFF or true/false as documented in
https://www.postgresql.org/docs/9.5/static/config-setting.html

Regards
Surafel

Surafel Temesgen wrote:

> I modified the patch as such and added to commitfest 2017-07.

A couple comments:

+ {"disallow_multiple_queries", PGC_POSTMASTER,
CLIENT_CONN_OTHER,
+ gettext_noop("Disallow multiple queries per query
string."),
+ NULL
+ },

PGC_POSTMASTER implies that it's an instance-wide setting.
Is is intentional? I can understand that it's more secure for this not to
be changeable in an existing session, but it's also much less usable if you
can't set it per-database and per-user.
Maybe it should be PGC_SUSET ?

+ if ((strcmp(commandTagHead, "BEGIN") != 0) ||
(strcmp(commandTagTail, "COMMIT") != 0) )
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
errmsg("cannot execute multiple commands unless it is a transaction
block")));

Shouldn't ROLLBACK be considered too as ending a transaction block?
Also, can it use a more specific code than ERRCODE_SYNTAX_ERROR?
It feels more like a rule violation than a syntax error.

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

"Daniel Verite" <daniel(at)manitou-mail(dot)org> writes:
> PGC_POSTMASTER implies that it's an instance-wide setting.
> Is is intentional? I can understand that it's more secure for this not to
> be changeable in an existing session, but it's also much less usable if you
> can't set it per-database and per-user.
> Maybe it should be PGC_SUSET ?

Bearing in mind that I'm not really for this at all... why shouldn't
it be plain old USERSET? AFAICS, the only argument for this restriction
is to make SQL injection harder. But if an attacker is able to inject
a SET command, he's already found a way around it. So there's no real
point in locking down the GUC to prevent that.

Also, generally speaking, GUCs should be phrased positively, ie this
should be named something more like "allow_multiple_queries" (with
opposite sense & default of course).

> + if ((strcmp(commandTagHead, "BEGIN") != 0) ||
> (strcmp(commandTagTail, "COMMIT") != 0) )
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("cannot execute multiple commands unless it is a transaction
> block")));

I haven't read the patch, but surely looking at command tags is not
an appropriate implementation of anything in this line.

regards, tom lane

Tom Lane wrote:

> Bearing in mind that I'm not really for this at all...

It's a band-aid, but certainly there are cases
where a DBA confronted to a badly written website
would just want to be able to:
ALTER USER webuser SET allow_multiple_queries TO off;

> But if an attacker is able to inject a SET command,
> he's already found a way around it. So there's no real
> point in locking down the GUC to prevent that.

I can think of the following case, where given the SQL-injectable
select id from users where email='$email';
an attacker would submit this string in $email:
foo' AND set_config('allow_multiple_queries', 'on', false)='on
which opens the rest of the session for a second injection, this
time in the form of several colon-separated commands that
would do the actual damage.

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

On Mon, Jun 12, 2017 at 5:22 PM, Daniel Verite <daniel(at)manitou-mail(dot)org>
wrote:
>
>
> PGC_POSTMASTER implies that it's an instance-wide setting.
> Is is intentional? I can understand that it's more secure for this not to
> be changeable in an existing session, but it's also much less usable if you
> can't set it per-database and per-user.
> Maybe it should be PGC_SUSET ?
>
It’s my misunderstanding I thought PGC_POSTMASTER set only by
superuser and changed with a hard restart

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

Regards,

Surafel

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.

On 2017-06-12 10:32:57 -0400, Tom Lane wrote:
> "Daniel Verite" <daniel(at)manitou-mail(dot)org> writes:
> > PGC_POSTMASTER implies that it's an instance-wide setting.
> > Is is intentional? I can understand that it's more secure for this not to
> > be changeable in an existing session, but it's also much less usable if you
> > can't set it per-database and per-user.
> > Maybe it should be PGC_SUSET ?
>
> Bearing in mind that I'm not really for this at all...

FWIW, I agree that this isn't something we should do. For one the GUC
would really have to be GUC_REPORT, which'll cost everyone, and will
break things like pgbouncer. I also don't think it's a good solution to
the problem at hand - there *are* cases where application
*intentionally* use PQexec() with multiple statements, namely when
aggregate latency is an issue. Since it's an application writer's choice
whether to use it, it seems to make not that much sense to have a
serverside guc - it can't really be sensible set. If you want to do
something here, you should probably work on convincing ORM etc. writers
to use PQexecParams().

Greetings,

Andres Freund

2017-06-14 19:56 GMT+02:00 Andres Freund <andres(at)anarazel(dot)de>:

> On 2017-06-12 10:32:57 -0400, Tom Lane wrote:
> > "Daniel Verite" <daniel(at)manitou-mail(dot)org> writes:
> > > PGC_POSTMASTER implies that it's an instance-wide setting.
> > > Is is intentional? I can understand that it's more secure for this not
> to
> > > be changeable in an existing session, but it's also much less usable
> if you
> > > can't set it per-database and per-user.
> > > Maybe it should be PGC_SUSET ?
> >
> > Bearing in mind that I'm not really for this at all...
>
> FWIW, I agree that this isn't something we should do. For one the GUC
> would really have to be GUC_REPORT, which'll cost everyone, and will
> break things like pgbouncer. I also don't think it's a good solution to
> the problem at hand - there *are* cases where application
> *intentionally* use PQexec() with multiple statements, namely when
> aggregate latency is an issue. Since it's an application writer's choice
> whether to use it, it seems to make not that much sense to have a
> serverside guc - it can't really be sensible set. If you want to do
> something here, you should probably work on convincing ORM etc. writers
> to use PQexecParams().
>

sometimes you are without possibility to check a control what application
does. The tools on server side is one possibility.

Regards

Pavel

>
> Greetings,
>
> Andres Freund
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Fabien COELHO wrote:

> 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.

It's proposed as an option. For apps that intentionally put multiple
commands in single PQexec calls, or for apps for which the deployer doesn't
know or care whether they do that, the setting should be kept to its default
value that let such calls pass, as they pass today.

In my understanding of the proposal, there is no implication that
intentionally using such multiple commands is bad, or that
it should be obsoleted in the future.

It's only bad in the specific case when this possibility is not used
normally by the app, but it's available to an attacker to make an
attack both easier to mount and more potent than a single-query injection.
This reasoning is also based on the assumption that the app has
bugs concerning quoting of parameters, but it's a longstanding class of
bugs that plague tons of low-quality code in production, and it shows no
sign of going away.

> 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.

It looks more like the "training wheel" patch that
was discussed in https://commitfest.postgresql.org/13/948/
rather than something that should be in this patch.
This is a different direction because allowing or disallowing
compound statements is a clear-cut thing, whereas making
a list of SQL constructs to cripple to mitigate bugs is more like opening
a Pandora box.

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

Andres Freund wrote:

> Since it's an application writer's choice whether to use it,
> it seems to make not that much sense to have a
> serverside guc - it can't really be sensible set.

The application writers who are concerned by this wouldn't
know that they have a choice. If there were informed,
supposedly they would grok the SQL syntax to begin with,
understanding the necessity and workings of proper quoting, and
the problem would not exist.

What is proposed AFAIU is an optional policy to be set on already
developed client apps, not a setting that is meant to be played with
by them.

An analogy I can find in existing GUCs, and that incidentally is
actually relevant to me as an app writer, is lo_compat_privileges
It's SUSET, it's not GUC_REPORT. Either it's on and the app
is not subject to permission checking for large objects,
or it's off and it is subject to them.
It's something that is relevant at deployment time, and not really besides
that, and it's the DBA's problem to set the policy depending on the app
requirements and the security requirements, rather than the app's problem
to adjust to whatever value there is in there.
As an example of app requirement, if the app has to let a user create a
large object and a different user to delete it, this GUC must be on,
otherwise such a scenario is not allowed, as unlinking is not a grantable
privilege, just like drop table.

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

On 6/14/17 10:05, Surafel Temesgen wrote:
> PGC_POSTMASTER implies that it's an instance-wide setting.
> Is is intentional? I can understand that it's more secure for this
> not to
> be changeable in an existing session, but it's also much less usable
> if you
> can't set it per-database and per-user.
> Maybe it should be PGC_SUSET ?
>
> It’s my misunderstanding I thought PGC_POSTMASTER set only by 
> superuser and changed with a hard restart 
>
> I attach a patch that incorporate the comments and uses similar routines
> with the rest of the file rather than using command tag

After reviewing the discussion, I'm inclined to reject this patch.
Several people have spoken out strongly against this patch. It's clear
that this feature wouldn't actually offer any absolute protection; it
just closes one particular hole. On the other hand, it introduces a
maintenance and management burden.

We already have libpq APIs that offer a more comprehensive protection
against SQL injection, so we can encourage users to use those, instead
of relying on uncertain measures such as this.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services