Thread: [HACKERS] Disallowing multiple queries per PQexec()

Surafel Temesgen <surafel3000@gmail.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/9236.1167968298@sss.pgh.pa.us>

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@gmail.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/9236.1167968298@sss.pgh.pa.us>
> 
> 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@momjian.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 ---
otherwisewe'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
preparedstatement")));
 

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)

On Tue, Feb 28, 2017 at 7:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Surafel Temesgen <surafel3000@gmail.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/9236.1167968298@sss.pgh.pa.us>
>
> 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

    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@manitou-mail.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

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@manitou-mail.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

    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