It is a bug report in a corner case.

When we assign "SECURITY DEFINER" attribute on plpgsql_call_handler(),
it makes server process crashed.

postgres=# ALTER FUNCTION plpgsql_call_handler() security definer;
ALTER FUNCTION

postgres=# CREATE FUNCTION foo(text) RETURNS text AS $$
BEGIN
RETURN $1 || '_aaa';
END
$$ LANGUAGE plpgsql;
CREATE FUNCTION
postgres=# select foo('aaa');
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!> \q

[scenario]
1. PostgreSQL calls fmgr_info_cxt_security() to initialize FmgrInfo
of the foo() invocation.

2. This invocation eventually calls fmgr_info_other_lang(), because
foo() is implemented with plpgsql. It also calls fmgr_info() to
fetch the function pointer of the plpgsql_call_handler().

3. However, this invocation returns fmgr_security_definer, because
it is marked as security definer function. Then, it is copied to
FmgrInfo->fn_addr of the foo().

4. Then, at the first call of fmgr_security_definer(), it also calls
fmgr_info_cxt_security() with ignore_security = true, to initialize
secondary FmgrInfo.
However, its fn_addr is initialized to fmgr_security_definer()
because of step (1) - (3).

5. Finally, fmgr_security_definer() calls itself infinity, as long as
stack is available.

I wonder whether the fmgr_info() is an appropriate choice at
fmgr_info_other_lang(), because user intended to call the foo(), not
plpgsql_call_handler(), although it actuall calls language call-handler
in fact.

I think fmgr_info_cxt_security() with ignore_security = true should
be used here, instead.

Thanks,
--
KaiGai Kohei <kaigai@ak.jp.nec.com>

Search Discussions

  • Tom Lane at Mar 19, 2010 at 12:18 pm

    KaiGai Kohei writes:
    When we assign "SECURITY DEFINER" attribute on plpgsql_call_handler(),
    it makes server process crashed.
    So don't do that. Whatever possessed you to think that's a sensible
    idea anyway?

    regards, tom lane
  • Robert Haas at Mar 19, 2010 at 11:06 pm

    On Fri, Mar 19, 2010 at 8:18 AM, Tom Lane wrote:
    KaiGai Kohei <kaigai@ak.jp.nec.com> writes:
    When we assign "SECURITY DEFINER" attribute on plpgsql_call_handler(),
    it makes server process crashed.
    So don't do that.  Whatever possessed you to think that's a sensible
    idea anyway?
    It might not be sensible, but the whole server going down as a result
    doesn't seem very sensible either.

    ...Robert
  • Tom Lane at Mar 20, 2010 at 12:11 am

    Robert Haas writes:
    On Fri, Mar 19, 2010 at 8:18 AM, Tom Lane wrote:
    KaiGai Kohei <kaigai@ak.jp.nec.com> writes:
    When we assign "SECURITY DEFINER" attribute on plpgsql_call_handler(),
    it makes server process crashed.
    So don't do that.  Whatever possessed you to think that's a sensible
    idea anyway?
    It might not be sensible, but the whole server going down as a result
    doesn't seem very sensible either.
    [ shrug... ] If you would like to start enumerating the ways in which
    you can crash the server with erroneous pg_proc entries for C functions,
    go for it. It'll keep you out of trouble for a very long time.

    regards, tom lane
  • Robert Haas at Mar 20, 2010 at 2:17 am

    On Fri, Mar 19, 2010 at 8:11 PM, Tom Lane wrote:
    Robert Haas <robertmhaas@gmail.com> writes:
    On Fri, Mar 19, 2010 at 8:18 AM, Tom Lane wrote:
    KaiGai Kohei <kaigai@ak.jp.nec.com> writes:
    When we assign "SECURITY DEFINER" attribute on plpgsql_call_handler(),
    it makes server process crashed.
    So don't do that.  Whatever possessed you to think that's a sensible
    idea anyway?
    It might not be sensible, but the whole server going down as a result
    doesn't seem very sensible either.
    [ shrug... ]  If you would like to start enumerating the ways in which
    you can crash the server with erroneous pg_proc entries for C functions,
    go for it.  It'll keep you out of trouble for a very long time.
    It's obviously not possible to make this bulletproof in general, but
    that doesn't mean we should crash just for fun.

    ...Robert
  • KaiGai Kohei at Mar 20, 2010 at 3:06 am

    (2010/03/20 11:17), Robert Haas wrote:
    On Fri, Mar 19, 2010 at 8:11 PM, Tom Lanewrote:
    Robert Haas<robertmhaas@gmail.com> writes:
    On Fri, Mar 19, 2010 at 8:18 AM, Tom Lanewrote:
    KaiGai Kohei<kaigai@ak.jp.nec.com> writes:
    When we assign "SECURITY DEFINER" attribute on plpgsql_call_handler(),
    it makes server process crashed.
    So don't do that. Whatever possessed you to think that's a sensible
    idea anyway?
    It might not be sensible, but the whole server going down as a result
    doesn't seem very sensible either.
    [ shrug... ] If you would like to start enumerating the ways in which
    you can crash the server with erroneous pg_proc entries for C functions,
    go for it. It'll keep you out of trouble for a very long time.
    It's obviously not possible to make this bulletproof in general, but
    that doesn't mean we should crash just for fun.
    I'd like to put the question in anotherexpression.

    Is it an expected behavior that PostgreSQL tries to execute foo() with
    privileges of the owner of language call handler because of its security
    definer property? This server crash is just a result.

    Thanks,
    --
    KaiGai Kohei <kaigai@kaigai.gr.jp>
  • Robert Haas at Mar 20, 2010 at 3:56 am

    On Fri, Mar 19, 2010 at 10:29 PM, KaiGai Kohei wrote:
    Is it an expected behavior that PostgreSQL tries to execute foo() with
    privileges of the owner of language call handler because of its security
    definer property? This server crash is just a result.
    I'm inclined to feel (and Tom's response only reinforces this) that
    the actual behavior isn't critical. I'd be happy with (1) executing
    foo() with the privileges of the language owner or (2) ignoring the
    SECURITY DEFINER attribute in this context and executing foo() without
    changing privileges or (3) throwing an error. We should just do
    whatever complicates the code the least. Your proposed patch seems
    good from that point of view, though I'm not clear on whether it's
    otherwise reasonable or which of the above behaviors it actually
    implements.

    ...Robert
  • Tom Lane at Mar 20, 2010 at 4:37 am

    KaiGai Kohei writes:
    Is it an expected behavior that PostgreSQL tries to execute foo() with
    privileges of the owner of language call handler because of its security
    definer property? This server crash is just a result.
    A language call handler has no function properties of its own --- which
    is why attaching SECURITY DEFINER to it is both useless and meaningless.
    The appropriate function properties for any call are those of the user
    function being called, which the handler is merely a support for.

    You could argue that we should put call handlers into their own table
    instead of pg_proc, since they aren't really user-callable functions;
    that would prevent people from thinking that something like this is
    sane. However, they share just enough infrastructure with real
    functions that it didn't seem worth doing it that way.

    I see no value whatsoever in making the world safe for people to attach
    SECURITY DEFINER to handlers. It's an incorrect declaration, and
    superusers need to know better than to declare C functions with
    incorrect properties.

    regards, tom lane
  • KaiGai Kohei at Mar 23, 2010 at 1:42 am

    (2010/03/20 13:37), Tom Lane wrote:
    KaiGai Kohei<kaigai@kaigai.gr.jp> writes:
    Is it an expected behavior that PostgreSQL tries to execute foo() with
    privileges of the owner of language call handler because of its security
    definer property? This server crash is just a result.
    A language call handler has no function properties of its own --- which
    is why attaching SECURITY DEFINER to it is both useless and meaningless.
    The appropriate function properties for any call are those of the user
    function being called, which the handler is merely a support for.
    OK, I also think the call handlers should work transparently like air.
    You could argue that we should put call handlers into their own table
    instead of pg_proc, since they aren't really user-callable functions;
    that would prevent people from thinking that something like this is
    sane. However, they share just enough infrastructure with real
    functions that it didn't seem worth doing it that way.

    I see no value whatsoever in making the world safe for people to attach
    SECURITY DEFINER to handlers. It's an incorrect declaration, and
    superusers need to know better than to declare C functions with
    incorrect properties.
    I basically agree that this matter does not happen unless user intend to
    assign SECURITY DEFINER attribute on the language call handlers, although
    it is nonsense.
    The reason why I reported it is that it was not clear for me whether the
    development team knows this matter, and I doubted the design we can assign
    this nonsense attribute from the perspective of fool-proof.

    Fool-proof is a term from human-interface. It tells us manufactured-product
    should work safely, even if user tries to abuse it. A microwave-oven does not
    work without closing its door, for example.
    In my sense, we should care about known matters at least.

    Thanks,
    --
    KaiGai Kohei <kaigai@ak.jp.nec.com>
  • Josh Berkus at Mar 20, 2010 at 3:35 am

    On 3/19/10 5:18 AM, Tom Lane wrote:
    When we assign "SECURITY DEFINER" attribute on plpgsql_call_handler(),
    it makes server process crashed.
    So don't do that. Whatever possessed you to think that's a sensible
    idea anyway?
    PATIENT: Doctor, it hurts when I do this!

    DOCTOR: So stop doing that.

    ;-)


    --
    -- Josh Berkus
    PostgreSQL Experts Inc.
    http://www.pgexperts.com

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedMar 19, '10 at 7:23a
activeMar 23, '10 at 1:42a
posts10
users5
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2021 Grokbase