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
From d050e000ef2fa3856da274512ff3a111970cd180 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <pete...@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 <sula...@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
