Re: CREATEROLE and role ownership hierarchies

On 2021-11-04 16:00, Shinya Kato wrote:
> On 2021-10-28 07:21, Mark Dilger wrote:
>>>> On Oct 25, 2021, at 10:09 PM, Shinya Kato 
>>>> <Shinya11.Kato@oss.nttdata.com> wrote:
>> 
>>>> Hi! Thank you for the patch.
>>>> I too think that CREATEROLE escalation attack is problem.
>>>> 
>>>> I have three comments.
>>>> 1. Is there a function to check the owner of a role, it would be 
>>>> nice to be able to check with \du or pg_roles view.
>>> 
>>> No, but that is a good idea.
>> 
>> These two ideas are implemented in v2.  Both \du and pg_roles show the
>> owner information.
>> 
>>> The current solution is to run REASSIGN OWNED in each database where 
>>> the role owns objects before running DROP ROLE. At that point, the 
>>> CASCADE option (not implemented) won't be needed.  Of course, I need 
>>> to post the next revision of this patch set addressing the 
>>> deficiencies that Nathan pointed out upthread to make that work.
>> 
>> REASSIGN OWNED and ALTER ROLE..OWNER TO now work in v2.
> 
> When ALTER ROLE with the privilege of REPLICATION, only the superuser
> is checked.
> Therefore, we have a strange situation where we can create a role but
> not change it.
> ---
> postgres=> SELECT current_user;
>  current_user
> --------------
>  test
> (1 row)
> 
> postgres=> \du test
>                        List of roles
>  Role name | Owner  |        Attributes        | Member of
> -----------+--------+--------------------------+-----------
>  test      | shinya | Create role, Replication | {}
> 
> postgres=> CREATE ROLE test2 REPLICATION;
> CREATE ROLE
> postgres=> ALTER ROLE test2 NOREPLICATION;
> 2021-11-04 14:24:02.687 JST [2615016] ERROR:  must be superuser to
> alter replication roles or change replication attribute
> 2021-11-04 14:24:02.687 JST [2615016] STATEMENT:  ALTER ROLE test2
> NOREPLICATION;
> ERROR:  must be superuser to alter replication roles or change
> replication attribute
> ---
> Wouldn't it be better to check if the role has CREATEROLE and 
> REPLICATION?
> The same is true for BYPASSRLS.
> 
> By the way, is this thread registered to CommitFest?

I fixed the patches because they cannot be applied to HEAD.

-- 
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION