On Thu, Jan 12, 2023 at 8:11 AM Robert Haas <robertmh...@gmail.com> wrote:
> On Wed, Jan 11, 2023 at 7:53 PM David G. Johnston > <david.g.johns...@gmail.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.
