CREATEROLE and role ownership hierarchies

On 10/20/21, 11:46 AM, "Mark Dilger" <mark.dilger@enterprisedb.com> wrote:

The purpose of these patches is to fix the CREATEROLE escalation
attack vector misfeature. (Not everyone will see CREATEROLE that
way, but the perceived value of the patch set likely depends on how
much you see CREATEROLE in that light.)

Regarding the "attack vector misfeature" comment, I remember being
surprised when I first learned how much roles with CREATEROLE can do.
When I describe CREATEROLE to others, I am sure to emphasize the note
in the docs about such roles being "almost-superuser" roles.
CREATEROLE is a rather big hammer at the moment, so I certainly think
there is value in reducing its almost-superuser-ness.

I mentioned this in the other thread [0]/messages/by-id/53C7DF4C-8463-4647-9DFD-779B5E1861C4@amazon.com already, but the first thing
that comes to mind when I look at these patches is how upgrades might
work. Will we just make the bootstrap superuser the owner for all
roles when you first upgrade to v15? Also, are we just going to strip
the current CREATEROLE roles of much of their powers? Maybe it's
worth keeping a legacy CREATEROLE role attribute for upgraded clusters
that could eventually be removed down the road.

I'd also like to bring up my note about allowing users to transfer
role ownership. When I tested the patches earlier, REASSIGN OWNED BY
was failing with an "unexpected classid" ERROR. Besides REASSIGN
OWNED BY, perhaps there should be another mechanism for transferring
ownership on a role-by-role basis (i.e., ALTER ROLE OWNER TO). I
haven't looked at this new patch set too closely, so my apologies if
this has already been added.

Nathan

[0]: /messages/by-id/53C7DF4C-8463-4647-9DFD-779B5E1861C4@amazon.com

On Oct 21, 2021, at 4:04 PM, Bossart, Nathan <bossartn@amazon.com> wrote:

Regarding the "attack vector misfeature" comment, I remember being
surprised when I first learned how much roles with CREATEROLE can do.
When I describe CREATEROLE to others, I am sure to emphasize the note
in the docs about such roles being "almost-superuser" roles.
CREATEROLE is a rather big hammer at the moment, so I certainly think
there is value in reducing its almost-superuser-ness.

It is hard to know how many people are using CREATEROLE currently. There isn't much reason to give it out, since if you care enough about security to not give out superuser, you probably care too much about security to give away CREATEROLE.

I mentioned this in the other thread [0] already, but the first thing
that comes to mind when I look at these patches is how upgrades might
work. Will we just make the bootstrap superuser the owner for all
roles when you first upgrade to v15?

Yes, that's the idea. After upgrade, all roles will form a tree, with the bootstrap superuser at the root of the tree. The initial tree structure isn't very interesting, with all other roles directly owned by it, but from there the superuser can rearrange the tree, and after that non-superuser roles can manage whatever subtree of roles they are the root of.

Also, are we just going to strip
the current CREATEROLE roles of much of their powers? Maybe it's
worth keeping a legacy CREATEROLE role attribute for upgraded clusters
that could eventually be removed down the road.

The patch as written drastically reduces the power of the CREATEROLE attribute, in a non-backwards compatible way. I wondered if there would be complaints about that. If so, we could instead leave CREATEROLE alone, and create some other privileged role for the same thing, but it does start to look funny having a CREATEROLE privilege bit and also a privileged role named, perhaps, pg_can_create_roles.

I'd also like to bring up my note about allowing users to transfer
role ownership. When I tested the patches earlier, REASSIGN OWNED BY
was failing with an "unexpected classid" ERROR. Besides REASSIGN
OWNED BY, perhaps there should be another mechanism for transferring
ownership on a role-by-role basis (i.e., ALTER ROLE OWNER TO). I
haven't looked at this new patch set too closely, so my apologies if
this has already been added.

Yes, I completely agree with you on that. Both REASSIGN OWNED BY and ALTER ROLE OWNER TO should work. I'll take a look at the patches and repost with any adjustments that I find necessary to make those work.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2021-10-21 03:40, Mark Dilger wrote:

These patches have been split off the now deprecated monolithic
"Delegating superuser tasks to new security roles" thread at [1].

The purpose of these patches is to fix the CREATEROLE escalation
attack vector misfeature. (Not everyone will see CREATEROLE that way,
but the perceived value of the patch set likely depends on how much
you see CREATEROLE in that light.)

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.
2. Is it correct that REPLICATION/BYPASSRLS can be granted even if you
are not a super user, but have CREATEROLE and REPLICATION/BYPASSRLS?
3. I think it would be better to have an "DROP ROLE [ IF EXISTS ] name
[, ...] [CASCADE | RESTRICT]" like "DROP TABLE [ IF EXISTS ] name [,
...] [ CASCADE | RESTRICT ]". What do you think?

--
Regards,

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

On Oct 25, 2021, at 10:09 PM, Shinya Kato <Shinya11.Kato@oss.nttdata.com> wrote:

On 2021-10-21 03:40, Mark Dilger wrote:

These patches have been split off the now deprecated monolithic
"Delegating superuser tasks to new security roles" thread at [1].
The purpose of these patches is to fix the CREATEROLE escalation
attack vector misfeature. (Not everyone will see CREATEROLE that way,
but the perceived value of the patch set likely depends on how much
you see CREATEROLE in that light.)

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.

2. Is it correct that REPLICATION/BYPASSRLS can be granted even if you are not a super user, but have CREATEROLE and REPLICATION/BYPASSRLS?

It is intentional, yes. Whether it is correct is up for debate, but I think it is.

3. I think it would be better to have an "DROP ROLE [ IF EXISTS ] name [, ...] [CASCADE | RESTRICT]" like "DROP TABLE [ IF EXISTS ] name [, ...] [ CASCADE | RESTRICT ]". What do you think?

I agree it would be nice to have, but roles are cluster-global and there are technical difficulties in cascading into multiple databases to drop all objects owned by the role. There was also a debate [1]/messages/by-id/20211005025746.GN20998@tamriel.snowman.net about whether we would even want such behavior, leading to no real conclusion regarding how or if such a command should be implemented.

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.

[1]: /messages/by-id/20211005025746.GN20998@tamriel.snowman.net

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 10/21/21 19:21, Mark Dilger wrote:

Also, are we just going to strip
the current CREATEROLE roles of much of their powers? Maybe it's
worth keeping a legacy CREATEROLE role attribute for upgraded clusters
that could eventually be removed down the road.

The patch as written drastically reduces the power of the CREATEROLE attribute, in a non-backwards compatible way. I wondered if there would be complaints about that. If so, we could instead leave CREATEROLE alone, and create some other privileged role for the same thing, but it does start to look funny having a CREATEROLE privilege bit and also a privileged role named, perhaps, pg_can_create_roles.

Give that CREATEROLE currently just about amounts to being a superuser,
maybe there should be a pg_upgrade option to convert CREATEROLE to
SUPERUSER. I don't want to perpetuate the misfeature though, so let's
just bring it to an end.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

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.

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.

Thank you. It seems good to me.

By the way, I got the following execution result.
I was able to add the membership of a role with a different owner.
In brief, "a" was able to change the membership of owner "shinya".
Is this the correct behavior?
---
postgres=# CREATE ROLE a LOGIN;
CREATE ROLE
postgres=# GRANT pg_execute_server_program TO a WITH ADMIN OPTION;
GRANT ROLE
postgres=# CREATE ROLE b;
CREATE ROLE
postgres=# \du a
List of roles
Role name | Owner | Attributes | Member of
-----------+--------+------------+-----------------------------
a | shinya | | {pg_execute_server_program}

postgres=# \du b
List of roles
Role name | Owner | Attributes | Member of
-----------+--------+--------------+-----------
b | shinya | Cannot login | {}

postgres=# \c - a
You are now connected to database "postgres" as user "a".
postgres=> GRANT pg_execute_server_program TO b;
GRANT ROLE
postgres=> \du b
List of roles
Role name | Owner | Attributes | Member of
-----------+--------+--------------+-----------------------------
b | shinya | Cannot login | {pg_execute_server_program}
---

--
Regards,

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

On Oct 27, 2021, at 7:32 PM, Shinya Kato <Shinya11.Kato@oss.nttdata.com> wrote:

I was able to add the membership of a role with a different owner.
In brief, "a" was able to change the membership of owner "shinya".
Is this the correct behavior?

I believe it is required for backwards compatibility. In a green field, we might consider doing things differently.

The only intentional backward compatibility break in this patch set is the the behavior of CREATEROLE. The general hope is that such a compatibility break will help far more than it hurts, as CREATEROLE does not appear to be a well adopted feature. I would expect that breaking the behavior of the WITH ADMIN OPTION feature would cause a lot more pain.

Trying your example on both the unpatched and the patched sources, things appear to work as they should:

UNPATCHED
------------------
mark.dilger=# CREATE ROLE a LOGIN;
CREATE ROLE
mark.dilger=# GRANT pg_execute_server_program TO a WITH ADMIN OPTION;
GRANT ROLE
mark.dilger=# CREATE ROLE b;
CREATE ROLE
mark.dilger=# \du+ a
List of roles
Role name | Attributes | Member of | Description
-----------+------------+-----------------------------+-------------
a | | {pg_execute_server_program} |

mark.dilger=# \du+ b
List of roles
Role name | Attributes | Member of | Description
-----------+--------------+-----------+-------------
b | Cannot login | {} |

mark.dilger=# \c - a
You are now connected to database "mark.dilger" as user "a".
mark.dilger=> GRANT pg_execute_server_program TO b;
GRANT ROLE
mark.dilger=> \du+ b
List of roles
Role name | Attributes | Member of | Description
-----------+--------------+-----------------------------+-------------
b | Cannot login | {pg_execute_server_program} |

mark.dilger=> \du+ "mark.dilger"
List of roles
Role name | Attributes | Member of | Description
-------------+------------------------------------------------------------+-----------+-------------
mark.dilger | Superuser, Create role, Create DB, Replication, Bypass RLS | {} |

PATCHED:
---------------
mark.dilger=# CREATE ROLE a LOGIN;
CREATE ROLE
mark.dilger=# GRANT pg_execute_server_program TO a WITH ADMIN OPTION;
GRANT ROLE
mark.dilger=# CREATE ROLE b;
CREATE ROLE
mark.dilger=# \du+ a
List of roles
Role name | Owner | Attributes | Member of | Description
-----------+-------------+------------+-----------------------------+-------------
a | mark.dilger | | {pg_execute_server_program} |

mark.dilger=# \du+ b
List of roles
Role name | Owner | Attributes | Member of | Description
-----------+-------------+--------------+-----------+-------------
b | mark.dilger | Cannot login | {} |

mark.dilger=# \c - a
You are now connected to database "mark.dilger" as user "a".
mark.dilger=> GRANT pg_execute_server_program TO b;
GRANT ROLE
mark.dilger=> \du+ b
List of roles
Role name | Owner | Attributes | Member of | Description
-----------+-------------+--------------+-----------------------------+-------------
b | mark.dilger | Cannot login | {pg_execute_server_program} |

mark.dilger=> \du+ "mark.dilger"
List of roles
Role name | Owner | Attributes | Member of | Description
-------------+-------------+------------------------------------------------------------+-----------+-------------
mark.dilger | mark.dilger | Superuser, Create role, Create DB, Replication, Bypass RLS | {} |

You should notice that the owner of role "b" is the superuser "mark.dilger", and that owner's attributes are unchanged. But your point that role "a" can change the attributes of role "mark.dilger" is correct, as shown here:

mark.dilger=> GRANT pg_execute_server_program TO "mark.dilger";
GRANT ROLE
mark.dilger=> \du+ "mark.dilger"
List of roles
Role name | Owner | Attributes | Member of | Description
-------------+-------------+------------------------------------------------------------+-----------------------------+-------------
mark.dilger | mark.dilger | Superuser, Create role, Create DB, Replication, Bypass RLS | {pg_execute_server_program} |

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Mark Dilger <mark.dilger@enterprisedb.com> writes:

The only intentional backward compatibility break in this patch set is the the behavior of CREATEROLE. The general hope is that such a compatibility break will help far more than it hurts, as CREATEROLE does not appear to be a well adopted feature. I would expect that breaking the behavior of the WITH ADMIN OPTION feature would cause a lot more pain.

Even more to the point, WITH ADMIN OPTION is defined by the SQL standard.
The only way you get to mess with that is if you can convince people we
mis-implemented the standard.

regards, tom lane

On 2021-10-29 01:14, Tom Lane wrote:

Mark Dilger <mark.dilger@enterprisedb.com> writes:

The only intentional backward compatibility break in this patch set is
the the behavior of CREATEROLE. The general hope is that such a
compatibility break will help far more than it hurts, as CREATEROLE
does not appear to be a well adopted feature. I would expect that
breaking the behavior of the WITH ADMIN OPTION feature would cause a
lot more pain.

Even more to the point, WITH ADMIN OPTION is defined by the SQL
standard.
The only way you get to mess with that is if you can convince people we
mis-implemented the standard.

Thank you for the detailed explanation.
I now understand what you said.

--
Regards,

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

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?

--
Regards,

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

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

On Dec 21, 2021, at 5:11 PM, Shinya Kato <Shinya11.Kato@oss.nttdata.com> wrote:

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

Thank you.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Dec 21, 2021 at 8:26 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

On Dec 21, 2021, at 5:11 PM, Shinya Kato <Shinya11.Kato@oss.nttdata.com> wrote:

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

Thank you.

I reviewed and tested these and they LGTM. FYI the rebased v3 patches
upthread are raw diffs so git am won't apply them. I can add myself to
the CF as a reviewer if it is helpful.

On 12/23/21 16:06, Joshua Brindle wrote:

On Tue, Dec 21, 2021 at 8:26 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

On Dec 21, 2021, at 5:11 PM, Shinya Kato <Shinya11.Kato@oss.nttdata.com> wrote:

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

Thank you.

I reviewed and tested these and they LGTM. FYI the rebased v3 patches
upthread are raw diffs so git am won't apply them.

That's not at all unusual. I normally apply patches just using

   patch -p 1 < $patchfile

I can add myself to
the CF as a reviewer if it is helpful.

Please do.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Mon, Jan 3, 2022 at 5:08 PM Andrew Dunstan <andrew@dunslane.net> wrote:

On 12/23/21 16:06, Joshua Brindle wrote:

On Tue, Dec 21, 2021 at 8:26 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

On Dec 21, 2021, at 5:11 PM, Shinya Kato <Shinya11.Kato@oss.nttdata.com> wrote:

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

Thank you.

I reviewed and tested these and they LGTM. FYI the rebased v3 patches
upthread are raw diffs so git am won't apply them.

That's not at all unusual. I normally apply patches just using

patch -p 1 < $patchfile

I can add myself to
the CF as a reviewer if it is helpful.

Please do.

I just ran across this and I don't know if it is intended behavior or
not, can you tell me why this happens?

postgres=> \du+
List of roles
Role name | Owner | Attributes
| Member of | Description
-----------+----------+------------------------------------------------------------+-----------+-------------
brindle | brindle | Password valid until 2022-01-05 00:00:00-05
| {} |
joshua | postgres | Create role
| {} |
postgres | postgres | Superuser, Create role, Create DB,
Replication, Bypass RLS | {} |

postgres=> \password
Enter new password for user "brindle":
Enter it again:
ERROR: role "brindle" with OID 16384 owns itself

On Jan 4, 2022, at 6:35 AM, Joshua Brindle <joshua.brindle@crunchydata.com> wrote:

I just ran across this and I don't know if it is intended behavior or
not

<snip>

postgres=> \password
Enter new password for user "brindle":
Enter it again:
ERROR: role "brindle" with OID 16384 owns itself

No, that looks like a bug. Thanks for reviewing!

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Jan 4, 2022, at 9:07 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:

No, that looks like a bug.

I was able to reproduce that using REASSIGN OWNED BY to cause a user to own itself. Is that how you did it, or is there yet another way to get into that state?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jan 4, 2022 at 3:39 PM Mark Dilger <mark.dilger@enterprisedb.com> wrote:

On Jan 4, 2022, at 9:07 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:

No, that looks like a bug.

I was able to reproduce that using REASSIGN OWNED BY to cause a user to own itself. Is that how you did it, or is there yet another way to get into that state?

I did:
ALTER ROLE brindle OWNER TO brindle;