Обсуждение: 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

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