Horton
Menu

Thread

  1. [BUG] SECURITY DEFINER on call handler makes daemon crash

    Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-03-19T07:22:45Z

    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>

  2. Re: [BUG] SECURITY DEFINER on call handler makes daemon crash

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-03-19T12:18:27Z

    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?

			regards, tom lane

  3. Re: [BUG] SECURITY DEFINER on call handler makes daemon crash

    Robert Haas <robertmhaas@gmail.com> — 2010-03-19T23:06:06Z

    On Fri, Mar 19, 2010 at 8:18 AM, Tom Lane <tgl@sss.pgh.pa.us> 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

  4. Re: [BUG] SECURITY DEFINER on call handler makes daemon crash

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-03-20T00:11:13Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Mar 19, 2010 at 8:18 AM, Tom Lane <tgl@sss.pgh.pa.us> 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

  5. Re: [BUG] SECURITY DEFINER on call handler makes daemon crash

    Robert Haas <robertmhaas@gmail.com> — 2010-03-20T02:17:41Z

    On Fri, Mar 19, 2010 at 8:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Fri, Mar 19, 2010 at 8:18 AM, Tom Lane <tgl@sss.pgh.pa.us> 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

  6. Re: [BUG] SECURITY DEFINER on call handler makes daemon crash

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2010-03-20T02:29:19Z

    (2010/03/20 11:17), Robert Haas wrote:
> On Fri, Mar 19, 2010 at 8:11 PM, Tom Lane<tgl@sss.pgh.pa.us>  wrote:
>> Robert Haas<robertmhaas@gmail.com>  writes:
>>> On Fri, Mar 19, 2010 at 8:18 AM, Tom Lane<tgl@sss.pgh.pa.us>  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.

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>

  7. Re: [BUG] SECURITY DEFINER on call handler makes daemon crash

    Josh Berkus <josh@agliodbs.com> — 2010-03-20T03:27:15Z

    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

  8. Re: [BUG] SECURITY DEFINER on call handler makes daemon crash

    Robert Haas <robertmhaas@gmail.com> — 2010-03-20T03:56:09Z

    On Fri, Mar 19, 2010 at 10:29 PM, KaiGai Kohei <kaigai@kaigai.gr.jp> 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

  9. Re: [BUG] SECURITY DEFINER on call handler makes daemon crash

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-03-20T04:37:19Z

    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.

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

  10. Re: [BUG] SECURITY DEFINER on call handler makes daemon crash

    Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-03-23T01:40:53Z

    (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>