Failed assertion due to procedure created with SECURITY DEFINER option
Hi,
I am getting assertion failure in StartTransaction() on the latest
master when procedure having SECURITY DEFINER called, here is the
script to reproducible this crash:
--create procedure
CREATE PROCEDURE transaction_test1() LANGUAGE plpgsql SECURITY DEFINER
AS $$ BEGIN
COMMIT;
END $$;
-- calling this procedure will have the assertion
CALL transaction_test1();
This happens because of in fmgr_security_definer() function we are
changing global variable SecurityRestrictionContext and in the
StartTransaction() insisting it should be zero, which is the problem.
TWIW, here is the backtrace:
(gdb) bt 15
#0 0x00007f2db45fb277 in raise () from /lib64/libc.so.6
#1 0x00007f2db45fc968 in abort () from /lib64/libc.so.6
#2 0x0000000000a1f456 in ExceptionalCondition (conditionName=0xadf94f
"!(s->prevSecContext == 0)", errorType=0xadf467 "FailedAsse
rtion", fileName=0xadf460 "xact.c", lineNumber=1908) at assert.c:54
#3 0x0000000000542f98 in StartTransaction () at xact.c:1908
#4 0x0000000000543c54 in StartTransactionCommand () at xact.c:2684
#5 0x0000000000719509 in SPI_start_transaction () at spi.c:201
#6 0x00007f2da4a92315 in exec_stmt_commit (estate=0x7fffe04d3310,
stmt=0x2b69c68) at pl_exec.c:4723
#7 0x00007f2da4a8c893 in exec_stmt (estate=0x7fffe04d3310,
stmt=0x2b69c68) at pl_exec.c:2008
#8 0x00007f2da4a8c4f2 in exec_stmts (estate=0x7fffe04d3310,
stmts=0x2b69cb0) at pl_exec.c:1875
#9 0x00007f2da4a8c3a0 in exec_stmt_block (estate=0x7fffe04d3310,
block=0x2b69b08) at pl_exec.c:1816
#10 0x00007f2da4a89f61 in plpgsql_exec_function (func=0x2ac8d20,
fcinfo=0x7fffe04d3740, simple_eval_estate=0x0, atomic=false) at p
l_exec.c:586
#11 0x00007f2da4a847b0 in plpgsql_call_handler (fcinfo=0x7fffe04d3740)
at pl_handler.c:263
#12 0x0000000000a287c8 in fmgr_security_definer
(fcinfo=0x7fffe04d3740) at fmgr.c:748
#13 0x0000000000657fc1 in ExecuteCallStmt (stmt=0x2a981e8, params=0x0,
atomic=false, dest=0x2a98540) at functioncmds.c:2294
#14 0x00000000008b71b2 in standard_ProcessUtility (pstmt=0x2a982b8,
queryString=0x2a97698 "CALL transaction_test1();", context=PRO
CESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x2a98540,
completionTag=0x7fffe04d3f70 "") at utility.c:649
(More stack frames follow...)
Regards,
Amul Sul
On 6/29/18 13:07, amul sul wrote:
This happens because of in fmgr_security_definer() function we are
changing global variable SecurityRestrictionContext and in the
StartTransaction() insisting it should be zero, which is the problem.
Hmm, what is the reason for this insistation?
We could work around this for now by prohibiting transaction commands in
security definer procedures, similar to what we do in procedures with
GUC settings attached.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi,
On 2018-06-29 13:56:12 +0200, Peter Eisentraut wrote:
On 6/29/18 13:07, amul sul wrote:
This happens because of in fmgr_security_definer() function we are
changing global variable SecurityRestrictionContext and in the
StartTransaction() insisting it should be zero, which is the problem.Hmm, what is the reason for this insistation?
Because it's supposed to be reset by AbortTransaction(), after an error.
/*
* Reset user ID which might have been changed transiently. We need this
* to clean up in case control escaped out of a SECURITY DEFINER function
* or other local change of CurrentUserId; therefore, the prior value of
* SecurityRestrictionContext also needs to be restored.
*
* (Note: it is not necessary to restore session authorization or role
* settings here because those can only be changed via GUC, and GUC will
* take care of rolling them back if need be.)
*/
SetUserIdAndSecContext(s->prevUser, s->prevSecContext);
I think all the state managed by transactions should be reviewed for
interactions with procedures. Can't imagine this being the only issue.
Greetings,
Andres Freund
On Fri, Jun 29, 2018 at 5:26 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
On 6/29/18 13:07, amul sul wrote:
This happens because of in fmgr_security_definer() function we are
changing global variable SecurityRestrictionContext and in the
StartTransaction() insisting it should be zero, which is the problem.Hmm, what is the reason for this insistation?
We could work around this for now by prohibiting transaction commands in
security definer procedures, similar to what we do in procedures with
GUC settings attached.
I am not sure that I have understood this, apologies. Do you mean by
the following case:
postgres=# CREATE PROCEDURE transaction_test1() LANGUAGE plpgsql
SECURITY DEFINER SET work_mem to '16MB'
AS $$ BEGIN
COMMIT;
END $$;
CREATE PROCEDURE
postgres=# CALL transaction_test1();
ERROR: invalid transaction termination
CONTEXT: PL/pgSQL function transaction_test1() line 2 at COMMIT
Thanks.
Regards,
Amul
On 2018-06-29 10:19:17 -0700, Andres Freund wrote:
Hi,
On 2018-06-29 13:56:12 +0200, Peter Eisentraut wrote:
On 6/29/18 13:07, amul sul wrote:
This happens because of in fmgr_security_definer() function we are
changing global variable SecurityRestrictionContext and in the
StartTransaction() insisting it should be zero, which is the problem.Hmm, what is the reason for this insistation?
Because it's supposed to be reset by AbortTransaction(), after an error.
Does that make sense Peter?
I've added this thread to the open items list.
Greetings,
Andres Freund
On 03.07.18 19:20, Andres Freund wrote:
On 2018-06-29 10:19:17 -0700, Andres Freund wrote:
Hi,
On 2018-06-29 13:56:12 +0200, Peter Eisentraut wrote:
On 6/29/18 13:07, amul sul wrote:
This happens because of in fmgr_security_definer() function we are
changing global variable SecurityRestrictionContext and in the
StartTransaction() insisting it should be zero, which is the problem.Hmm, what is the reason for this insistation?
Because it's supposed to be reset by AbortTransaction(), after an error.
Does that make sense Peter?
I've added this thread to the open items list.
Proposed fix attached.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Prohibit-transaction-commands-in-security-definer-pr.patchtext/plain; charset=UTF-8; name=0001-Prohibit-transaction-commands-in-security-definer-pr.patch; x-mac-creator=0; x-mac-type=0Download
From d050e000ef2fa3856da274512ff3a111970cd180 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Wed, 4 Jul 2018 09:26:19 +0200
Subject: [PATCH] Prohibit transaction commands in security definer procedures
Starting and aborting transactions in security definer procedures
doesn't work. StartTransaction() insists that the security context
stack is empty, so this would currently cause a crash, and
AbortTransaction() resets it. This could be made to work by
reorganizing the code, but right now we just prohibit it.
Reported-by: amul sul <sulamul@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/CAAJ_b96Gupt_LFL7uNyy3c50-wbhA68NUjiK5%3DrF6_w%3Dpq_T%3DQ%40mail.gmail.com
---
doc/src/sgml/ref/create_procedure.sgml | 6 ++++++
src/backend/commands/functioncmds.c | 9 +++++++++
src/pl/plpgsql/src/expected/plpgsql_transaction.out | 12 ++++++++++++
src/pl/plpgsql/src/sql/plpgsql_transaction.sql | 13 +++++++++++++
4 files changed, 40 insertions(+)
diff --git a/doc/src/sgml/ref/create_procedure.sgml b/doc/src/sgml/ref/create_procedure.sgml
index f3c3bb006c..6c1de34b01 100644
--- a/doc/src/sgml/ref/create_procedure.sgml
+++ b/doc/src/sgml/ref/create_procedure.sgml
@@ -203,6 +203,12 @@ <title>Parameters</title>
conformance, but it is optional since, unlike in SQL, this feature
applies to all procedures not only external ones.
</para>
+
+ <para>
+ A <literal>SECURITY DEFINER</literal> procedure cannot execute
+ transaction control statements (for example, <command>COMMIT</command>
+ and <command>ROLLBACK</command>, depending on the language).
+ </para>
</listitem>
</varlistentry>
diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index 8864d9ae44..2cadc2e7b2 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -2244,6 +2244,15 @@ ExecuteCallStmt(CallStmt *stmt, ParamListInfo params, bool atomic, DestReceiver
if (!heap_attisnull(tp, Anum_pg_proc_proconfig, NULL))
callcontext->atomic = true;
+ /*
+ * In security definer procedures, we can't allow transaction commands.
+ * StartTransaction() insists that the security context stack is empty,
+ * and AbortTransaction() resets the security context. This could be
+ * reorganized, but right now it doesn't work.
+ */
+ if (((Form_pg_proc )GETSTRUCT(tp))->prosecdef)
+ callcontext->atomic = true;
+
/*
* Expand named arguments, defaults, etc.
*/
diff --git a/src/pl/plpgsql/src/expected/plpgsql_transaction.out b/src/pl/plpgsql/src/expected/plpgsql_transaction.out
index 7f008ac57e..1967b3fc53 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_transaction.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_transaction.out
@@ -130,6 +130,18 @@ $$;
CALL transaction_test5();
ERROR: invalid transaction termination
CONTEXT: PL/pgSQL function transaction_test5() line 3 at COMMIT
+-- SECURITY DEFINER currently disallow transaction statements
+CREATE PROCEDURE transaction_test5b()
+LANGUAGE plpgsql
+SECURITY DEFINER
+AS $$
+BEGIN
+ COMMIT;
+END;
+$$;
+CALL transaction_test5b();
+ERROR: invalid transaction termination
+CONTEXT: PL/pgSQL function transaction_test5b() line 3 at COMMIT
TRUNCATE test1;
-- nested procedure calls
CREATE PROCEDURE transaction_test6(c text)
diff --git a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql b/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
index eddc518bb6..2bdca8e165 100644
--- a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
@@ -116,6 +116,19 @@ CREATE PROCEDURE transaction_test5()
CALL transaction_test5();
+-- SECURITY DEFINER currently disallow transaction statements
+CREATE PROCEDURE transaction_test5b()
+LANGUAGE plpgsql
+SECURITY DEFINER
+AS $$
+BEGIN
+ COMMIT;
+END;
+$$;
+
+CALL transaction_test5b();
+
+
TRUNCATE test1;
-- nested procedure calls
base-commit: fc057b2b8fc3c3556d9f8cc0195c622aaad92c9e
--
2.18.0
On Jul 4, 2018, at 3:43 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
On 03.07.18 19:20, Andres Freund wrote:
On 2018-06-29 10:19:17 -0700, Andres Freund wrote:
Hi,
On 2018-06-29 13:56:12 +0200, Peter Eisentraut wrote:
On 6/29/18 13:07, amul sul wrote:
This happens because of in fmgr_security_definer() function we are
changing global variable SecurityRestrictionContext and in the
StartTransaction() insisting it should be zero, which is the problem.Hmm, what is the reason for this insistation?
Because it's supposed to be reset by AbortTransaction(), after an error.
Does that make sense Peter?
I've added this thread to the open items list.
Proposed fix attached.
First, reproduced the issue against HEAD and was able to successfully
do so.
Then, applied the patch and tested using original test case:
testing=# CREATE PROCEDURE transaction_test1() LANGUAGE plpgsql SECURITY DEFINER
testing-# AS $$ BEGIN
testing$# COMMIT;
testing$# END $$;
CREATE PROCEDURE
testing=# CALL transaction_test1();
2018-07-12 15:45:49.846 EDT [39928] ERROR: invalid transaction termination
2018-07-12 15:45:49.846 EDT [39928] CONTEXT: PL/pgSQL function transaction_test1() line 2 at COMMIT
2018-07-12 15:45:49.846 EDT [39928] STATEMENT: CALL transaction_test1();
ERROR: invalid transaction termination
CONTEXT: PL/pgSQL function transaction_test1() line 2 at COMMIT
So it handles it as expected.
Code and test cases look fine to me from what I can tell. My only suggestion
would be if we could add some guidance to the documentation on what languages
can support transaction control statements inside procedures with a SECURITY
DEFINER.
Jonathan
On 12.07.18 21:52, Jonathan S. Katz wrote:
So it handles it as expected.
Committed.
Code and test cases look fine to me from what I can tell. My only suggestion
would be if we could add some guidance to the documentation on what
languages
can support transaction control statements inside procedures with a SECURITY
DEFINER.
The documentation change is in the CREATE PROCEDURE ref page, since it's
independent of any language.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services