Horton

Thread

Commits

  1. Prohibit transaction commands in security definer procedures

  1. Failed assertion due to procedure created with SECURITY DEFINER option

    Amul Sul <sulamul@gmail.com> — 2018-06-29T11:07:46Z

    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

  2. Re: Failed assertion due to procedure created with SECURITY DEFINER option

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-06-29T11:56:12Z

    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

  3. Re: Failed assertion due to procedure created with SECURITY DEFINER option

    Andres Freund <andres@anarazel.de> — 2018-06-29T17:19:17Z

    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

  4. Re: Failed assertion due to procedure created with SECURITY DEFINER option

    Amul Sul <sulamul@gmail.com> — 2018-07-03T10:58:30Z

    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

  5. Re: Failed assertion due to procedure created with SECURITY DEFINER option

    Andres Freund <andres@anarazel.de> — 2018-07-03T17:20:11Z

    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

  6. Re: Failed assertion due to procedure created with SECURITY DEFINER option

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-07-04T07:43:06Z

    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

  7. Re: Failed assertion due to procedure created with SECURITY DEFINER option

    Jonathan Katz <jonathan.katz@excoventures.com> — 2018-07-12T19:52:03Z

    > 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

  8. Re: Failed assertion due to procedure created with SECURITY DEFINER option

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-07-13T08:46:27Z

    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