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
</messages/by-id/9236.1167968298@sss.pgh.pa.us> 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
Attachments:
disallow-multiple-queries-1.patchapplication/octet-stream; name=disallow-multiple-queries-1.patchDownload
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index b07d6c6..7cb498b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -887,6 +887,12 @@ exec_simple_query(const char *query_string)
bool was_logged = false;
bool isTopLevel;
char msec_str[32];
+ ListCell *parsetree_head_cell;
+ ListCell *parsetree_tail_cell;
+ const char *commandTagHead;
+ const char *commandTagTail;
+ Node *parsetreehead;
+ Node *parsetreetail;
/*
@@ -932,7 +938,29 @@ exec_simple_query(const char *query_string)
* we are in aborted transaction state!)
*/
parsetree_list = pg_parse_query(query_string);
+ /*
+ * we use head and tail cell for checking weather it is a transaction
+ * block or not
+ */
+ parsetree_head_cell = list_head(parsetree_list);
+ parsetree_tail_cell = list_tail(parsetree_list);
+ parsetreehead = (Node *) lfirst(parsetree_head_cell);
+ parsetreetail = (Node *) lfirst(parsetree_tail_cell);
+ /*
+ * figure out head and tail command type
+ */
+ commandTagHead = CreateCommandTag(parsetreehead);
+ commandTagTail = CreateCommandTag(parsetreetail);
+ /*
+ * We only allow a single user query or a transaction block per call .
+ * This is provide an additional defense against SQL-injection attacks.
+ */
+ if ((list_length(parsetree_list) > 1)
+ && (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")));
/* Log immediately if dictated by log_statement */
if (check_log_statement(parsetree_list))
{
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
</messages/by-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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
</messages/by-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 +--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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 attacksand 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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 attacksand 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.Nasby@bluetreble.com> wrote:
Show quoted text
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
</messages/by-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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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@gmail.com> wrote:
Show quoted text
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
</messages/by-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
Attachments:
disallow-multiple-queries-2.patchapplication/octet-stream; name=disallow-multiple-queries-2.patchDownload
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e02b0c8..1b165ea 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1284,6 +1284,22 @@ include_dir 'conf.d'
</listitem>
</varlistentry>
+ <varlistentry id="guc-disallow_multiple_queries" xreflabel="disallow_multiple_queries">
+ <term><varname>disallow_multiple_queries</varname> (<type>boolean</type>)
+ <indexterm>
+ <primary><varname>disallow_multiple_queries</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ When this parameter is on, the <productname>PostgreSQL</> server
+ disallow multiple SQL commands in the given string unless it is
+ transaction block in simple query protocol. It is useful for providing
+ an additional defense against SQL-injection attacks.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</sect2>
</sect1>
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 75c2d9a..70f6392 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -96,6 +96,7 @@ int max_stack_depth = 100;
/* wait N seconds to allow attach from a debugger */
int PostAuthDelay = 0;
+bool disallow_multiple_queries = false;
/* ----------------
@@ -937,6 +938,33 @@ exec_simple_query(const char *query_string)
*/
parsetree_list = pg_parse_query(query_string);
+ if (disallow_multiple_queries && (list_length(parsetree_list) > 1))
+ {
+ const char *commandTagHead;
+ const char *commandTagTail;
+ Node *parsetreehead;
+ Node *parsetreetail;
+ /*
+ * we use head and tail cell for checking weather it is a transaction
+ * block or not
+ */
+ parsetreehead = (Node *) lfirst(list_head(parsetree_list));
+ parsetreetail = (Node *) lfirst(list_tail(parsetree_list));
+ /*
+ * figure out head and tail command type
+ */
+ commandTagHead = CreateCommandTag(parsetreehead);
+ commandTagTail = CreateCommandTag(parsetreetail);
+ /*
+ * We only allow a single user query or a transaction block per call .
+ * This is provide an additional defense against SQL-injection attacks.
+ */
+
+ 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")));
+ }
+
/* Log immediately if dictated by log_statement */
if (check_log_statement(parsetree_list))
{
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index a414fb2..0f9617c 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1670,6 +1670,16 @@ static struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
+ {
+ {"disallow_multiple_queries", PGC_POSTMASTER, CLIENT_CONN_OTHER,
+ gettext_noop("Disallow multiple queries per query string."),
+ NULL
+ },
+ &disallow_multiple_queries,
+ false,
+ NULL, NULL, NULL
+ },
+
/* End-of-list marker */
{
{NULL, 0, 0, NULL, NULL}, NULL, false, NULL, NULL, NULL
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index f1a34a1..c81bbae 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -46,6 +46,7 @@ typedef enum
} LogStmtLevel;
extern int log_statement;
+extern bool disallow_multiple_queries;
extern List *pg_parse_query(const char *query_string);
extern List *pg_analyze_and_rewrite(RawStmt *parsetree,
On Thu, May 18, 2017 at 2:56 AM, Surafel Temesgen <surafel3000@gmail.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
"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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jun 12, 2017 at 5:22 PM, Daniel Verite <daniel@manitou-mail.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
Attachments:
disallow-multiple-queries-3.patchapplication/octet-stream; name=disallow-multiple-queries-3.patchDownload
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e02b0c8..1f5d50b 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1284,6 +1284,22 @@ include_dir 'conf.d'
</listitem>
</varlistentry>
+ <varlistentry id="guc-allow_multiple_queries" xreflabel="allow_multiple_queries">
+ <term><varname>allow_multiple_queries</varname> (<type>boolean</type>)
+ <indexterm>
+ <primary><varname>allow_multiple_queries</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ When this parameter is on, the <productname>PostgreSQL</> server
+ allow multiple SQL commands without being a transaction block in the given string
+ in simple query protocol. setting this parameter off is use for providing
+ an additional defense against SQL-injection attacks.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</sect2>
</sect1>
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 4bc5bf3..de48b0c 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2160,6 +2160,14 @@ PGresult *PQexec(PGconn *conn, const char *command);
of the last command executed from the string. Should one of the
commands fail, processing of the string stops with it and the returned
<structname>PGresult</structname> describes the error condition.
+ <note>
+ <para>
+ The server may be configure to disallow multiple sql commands without being
+ a transaction block to add an extra defense against SQL-injection attacks
+ so it is always a good practice to add <command>BEGIN</command>/<command>COMMIT
+ </command> commands for multiple sql commands
+ </para>
+ </note>
</para>
<para>
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 75c2d9a..3e1a2fc 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -96,7 +96,7 @@ int max_stack_depth = 100;
/* wait N seconds to allow attach from a debugger */
int PostAuthDelay = 0;
-
+bool allow_multiple_queries = true;
/* ----------------
* private variables
@@ -185,6 +185,8 @@ static void finish_xact_command(void);
static bool IsTransactionExitStmt(Node *parsetree);
static bool IsTransactionExitStmtList(List *pstmts);
static bool IsTransactionStmtList(List *pstmts);
+static bool IsTransactionStartStmt(Node *parsetree);
+static bool IsTransactionEndStmt(Node *parsetree);
static void drop_unnamed_stmt(void);
static void SigHupHandler(SIGNAL_ARGS);
static void log_disconnections(int code, Datum arg);
@@ -937,6 +939,25 @@ exec_simple_query(const char *query_string)
*/
parsetree_list = pg_parse_query(query_string);
+ if (!allow_multiple_queries && (list_length(parsetree_list) > 1))
+ {
+ /*
+ * we use head and tail cell for checking weather it is a transaction
+ * block or not
+ */
+ RawStmt *parsetreehead = lfirst_node(RawStmt, list_head(parsetree_list));
+ RawStmt *parsetreetail = lfirst_node(RawStmt, list_tail(parsetree_list));
+
+ /*
+ * We only allow a single user query or a transaction block per call .
+ * This is provide an additional defense against SQL-injection attacks.
+ */
+
+ if (!IsTransactionStartStmt(parsetreehead->stmt) || !IsTransactionEndStmt(parsetreetail->stmt) )
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR), errmsg("cannot execute multiple commands unless it is a transaction block")));
+ }
+
/* Log immediately if dictated by log_statement */
if (check_log_statement(parsetree_list))
{
@@ -2532,6 +2553,44 @@ IsTransactionStmtList(List *pstmts)
return false;
}
+/*
+ *Convenience routines for checking whether a statement is transaction Start statment.
+ */
+
+/* Test a bare parsetree */
+static bool
+IsTransactionStartStmt(Node *parsetree)
+{
+ if (parsetree && IsA(parsetree, TransactionStmt))
+ {
+ TransactionStmt *stmt = (TransactionStmt *) parsetree;
+
+ if (stmt->kind == TRANS_STMT_BEGIN ||
+ stmt->kind == TRANS_STMT_START)
+ return true;
+ }
+ return false;
+}
+
+/*
+ * Convenience routines for checking whether a statement is transaction end statment.
+ */
+
+/* Test a bare parsetree */
+static bool
+IsTransactionEndStmt(Node *parsetree)
+{
+ if (parsetree && IsA(parsetree, TransactionStmt))
+ {
+ TransactionStmt *stmt = (TransactionStmt *) parsetree;
+
+ if (stmt->kind == TRANS_STMT_COMMIT||
+ stmt->kind == TRANS_STMT_ROLLBACK)
+ return true;
+ }
+ return false;
+}
+
/* Release any existing unnamed prepared statement */
static void
drop_unnamed_stmt(void)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index a414fb2..0aada36 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1670,6 +1670,16 @@ static struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
+ {
+ {"allow_multiple_queries", PGC_SUSET, CLIENT_CONN_OTHER,
+ gettext_noop("Allow multiple queries per query string."),
+ NULL
+ },
+ &allow_multiple_queries,
+ true,
+ NULL, NULL, NULL
+ },
+
/* End-of-list marker */
{
{NULL, 0, 0, NULL, NULL}, NULL, false, NULL, NULL, NULL
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index f1a34a1..e11ea23 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -47,6 +47,8 @@ typedef enum
extern int log_statement;
+extern bool allow_multiple_queries;
+
extern List *pg_parse_query(const char *query_string);
extern List *pg_analyze_and_rewrite(RawStmt *parsetree,
const char *query_string,
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
+ allowallows
+ 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
+ attackssome type of SQL injections attacks?
+ so it is always a good practice to add
+ <command>BEGIN</command>/<command>COMMIT
+ </command> commands for multiple sql commandsI do not really agree that it is an advisable "good practice" to do
that...
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2017-06-14 19:56 GMT+02:00 Andres Freund <andres@anarazel.de>:
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 notto
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
Show quoted text
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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 restartI 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers