Thread: [BUG] SECURITY DEFINER on call handler makes daemon crash
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>
Attachment
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
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
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
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
(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>
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
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
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
(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>