Re: pgsql: Add new GUC createrole_self_grant.

On Thu, Jan 12, 2023 at 8:11 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Wed, Jan 11, 2023 at 7:53 PM David G. Johnston
> <david(dot)g(dot)johnston(at)gmail(dot)com> wrote:
> > Justed wanted to chime in and say Robert has eloquently put into words
> much of what I have been thinking here, and that I concur that guiding the
> DBA to use care with the power they have been provided is a sane position
> to take.
> >
> > +1, and thank you.
>
> Thanks!
>
> Here's a patch. In it I make three changes, only one of which is
> directly relevant to the topic at hand:
>
> 1. Add a sentence to the documentation on writing SECURITY FUNCTIONS
> safely concerning createrole_self_grant.
> 2. Add a sentence to the documentation on SECURITY DEFINER referring
> to the section about writing such functions safely.
> 3. Remove a note discussing the fact that pre-8.3 versions did not
> have SET clauses for functions.
>
> I can separate this into multiple patches if desired. And of course
> you, Tom, or others may have suggestions on which of these changes
> should be included at all or how to word them better.
>
>
I'm still not really feeling how security definer is the problematic option
here. Security invoker scares me much more.

postgres=# create role unpriv login;
CREATE ROLE
postgres=# create role creator createrole login;
CREATE ROLE
postgres=# grant pg_read_all_data to creator with admin option;
postgres=#
create procedure makeunprivpowerful() LANGUAGE sql AS $$
grant pg_read_all_data to unpriv;
$$;
CREATE PROCEDURE
postgres=# alter procedure makeunprivpowerful() owner to unpriv;
ALTER PROCEDURE

postgres=# \c postgres unpriv
You are now connected to database "postgres" as user "unpriv".
postgres=> call makeunprivpowerful();
ERROR: must have admin option on role "pg_read_all_data"
CONTEXT: SQL function "makeunprivpowerful" statement 1

postgres=# \c postgres creator
You are now connected to database "postgres" as user "creator".
postgres=> call makeunprivpowerful();
CALL

Personally I think the best advice for a CREATEROLE granted user is to
never call routines they themselves don't own; or at least only those have
reviewed thoroughly and understand the consequences of. Regardless of
security definer/invoker.

In short, a low-privilege user can basically run anything without much fear
because they can't do harm anyway. Thus security invoker applies to them,
and having security definer gives them the ability to do some things
without needing to have permanent higher privileges. These are useful,
security related attributes with respect to them.

For a high-privilege I would argue that neither security invoker nor
definer are relevant considerations from a security point-of-view. If you
have high-privilege you must know what it is you are executing before you
execute it and anything bad it causes you to do using your privileges, or
higher if you run a security definer function blindly, is an administrative
failure, not a security hole.

I think it would be good to move the goalposts here a bit with respect to
encouraging safe behavior. But I also don't really think that it is fair
to make this a prerequisite for the feature.

If we cannot write a decent why sentence for the proposed paragraph I say
we don't commit it (the cross-ref should go in):

If the security definer function intends to create roles, and if it
is running as a non-superuser, <varname>createrole_self_grant</varname>
should also be set to a known value using the <literal>SET</literal>
clause.

This is a convenience feature that a CREATEROLE user can leverage if they
so choose. Anything bad coming of it is going to be strictly less worse
than whatever can happen just because the CREATEROLE user is being
careless. Whomever gets the admin privilege grant from the superuser when
the role is created may or may not have two other self-granted memberships
on the newly created role. Do the two optional grants really mean anything
important here compared to the newly created object and superuser-granted
admin privilege (which means that regardless of the GUC the same end state
can eventually be reached anyway)?

David J.