v1-0003-Restrict-the-privileges-of-CREATEROLE-users.patch

From f6b3e35013ed76ba92a1bdedd5658895cce0c440 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Mon, 21 Nov 2022 15:29:00 -0500
Subject: [PATCH v1 3/4] Restrict the privileges of CREATEROLE users.

Previously, CREATEROLE users were permitted to make nearly arbitrary
changes to roles that they didn't create, with certain exceptions,
such as superuser roles.  Instead, allow CREATEROLE users to make such
changes only to roles for which they possess ADMIN OPTION, and to
grant membership only in roles for which they possess ADMIN OPTION.

When a CREATEROLE user who is not a superuser creates a role, grant
ADMIN OPTION on the newly-created role to the creator, so that they
can administer roles they create or for which they have been given
privileges.

With these changes, CREATEROLE users still have very significant
powers that unprivileged users do not receive: they can alter, rename,
drop, comment on, change the password for, and change security labels
on roles.  However, they can now do these things only for roles for
which they possess some privileges, rather than all non-superuser
roles; moreover, they cannot grant a role such as
pg_execute_server_program unless they themselves possess it.

FIXME: Add more regression tests.
FIXME: Add documentation.
---
 src/backend/catalog/objectaddress.c       |   9 +-
 src/backend/commands/user.c               | 100 ++++++++++++++++------
 src/test/regress/expected/create_role.out |  37 ++++----
 src/test/regress/sql/create_role.sql      |  23 ++---
 4 files changed, 106 insertions(+), 63 deletions(-)

diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index fe97fbf79d..c574e43c2a 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -2543,7 +2543,9 @@ check_object_ownership(Oid roleid, ObjectType objtype, ObjectAddress address,
 
 			/*
 			 * We treat roles as being "owned" by those with CREATEROLE priv,
-			 * except that superusers are only owned by superusers.
+			 * provided that they also have admin option on the role.
+			 *
+			 * However, superusers are only owned by superusers.
 			 */
 			if (superuser_arg(address.objectId))
 			{
@@ -2558,6 +2560,11 @@ check_object_ownership(Oid roleid, ObjectType objtype, ObjectAddress address,
 					ereport(ERROR,
 							(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 							 errmsg("must have CREATEROLE privilege")));
+				if (!is_admin_of_role(address.objectId, roleid))
+					ereport(ERROR,
+							(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+							 errmsg("must have admin option on role \"%s\"",
+									GetUserNameFromId(roleid, true))));
 			}
 			break;
 		case OBJECT_TSPARSER:
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 37fc4f9627..1bfc42a157 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -519,6 +519,42 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
 		}
 	}
 
+	/*
+	 * If the current user isn't a superuser, make them an admin of the new
+	 * role so that they can administer the new object they just created.
+	 * Superusers will be able to do that anyway.
+	 *
+	 * The grantor of record for this implicit grant is the bootstrap
+	 * superuser, which means that the CREATEROLE user cannot revoke the
+	 * grant. They can however grant the created role back to themselves
+	 * with different options, since they enjoy ADMIN OPTION on it.
+	 */
+	if (!superuser())
+	{
+		RoleSpec   *current_role = makeNode(RoleSpec);
+		GrantRoleOptions	poptself;
+
+		current_role->roletype = ROLESPEC_CURRENT_ROLE;
+		current_role->location = -1;
+
+		poptself.specified = GRANT_ROLE_SPECIFIED_ADMIN
+			| GRANT_ROLE_SPECIFIED_INHERIT
+			| GRANT_ROLE_SPECIFIED_SET;
+		poptself.admin = true;
+		poptself.inherit = false;
+		poptself.set = false;
+
+		AddRoleMems(BOOTSTRAP_SUPERUSERID, stmt->role, roleid,
+					list_make1(current_role), list_make1_oid(GetUserId()),
+					BOOTSTRAP_SUPERUSERID, &poptself);
+
+		/*
+		 * We must make the implicit grant visible to the code below, else
+		 * the additional grants will fail.
+		 */
+		CommandCounterIncrement();
+	}
+
 	/*
 	 * Add the specified members to this new role. adminmembers get the admin
 	 * option, rolemembers don't.
@@ -694,9 +730,7 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
 	/*
 	 * To mess with a superuser or replication role in any way you gotta be
 	 * superuser.  We also insist on superuser to change the BYPASSRLS
-	 * property.  Otherwise, if you don't have createrole, you're only allowed
-	 * to (1) change your own password or (2) add members to a role for which
-	 * you have ADMIN OPTION.
+	 * property.
 	 */
 	if (authform->rolsuper || dissuper)
 	{
@@ -719,29 +753,35 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 					 errmsg("must be superuser to change bypassrls attribute")));
 	}
-	else if (!have_createrole_privilege())
+
+	/*
+	 * Most changes to a role require that you both have CREATEROLE privileges
+	 * and also ADMIN OPTION on the role.
+	 */
+	if (!have_createrole_privilege() ||
+		!is_admin_of_role(GetUserId(), roleid))
 	{
-		/* things you certainly can't do without CREATEROLE */
+		/* things an unprivileged user certainly can't do */
 		if (dinherit || dcreaterole || dcreatedb || dcanlogin || dconnlimit ||
 			dvalidUntil)
 			ereport(ERROR,
 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 					 errmsg("permission denied")));
 
-		/* without CREATEROLE, can only change your own password */
+		/* an unprivileged user can change their own password */
 		if (dpassword && roleid != currentUserId)
 			ereport(ERROR,
 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 					 errmsg("must have CREATEROLE privilege to change another user's password")));
-
-		/* without CREATEROLE, can only add members to roles you admin */
-		if (drolemembers && !is_admin_of_role(currentUserId, roleid))
-			ereport(ERROR,
-					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-					 errmsg("must have admin option on role \"%s\" to add members",
-							rolename)));
 	}
 
+	/* To add members to a role, you need ADMIN OPTION. */
+	if (drolemembers && !is_admin_of_role(currentUserId, roleid))
+		ereport(ERROR,
+				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+				 errmsg("must have admin option on role \"%s\" to add members",
+						rolename)));
+
 	/* Convert validuntil to internal form */
 	if (dvalidUntil)
 	{
@@ -935,8 +975,9 @@ AlterRoleSet(AlterRoleSetStmt *stmt)
 		shdepLockAndCheckObject(AuthIdRelationId, roleid);
 
 		/*
-		 * To mess with a superuser you gotta be superuser; else you need
-		 * createrole, or just want to change your own settings
+		 * To mess with a superuser you gotta be superuser; otherwise you
+		 * need CREATEROLE plus admin option on the target role; unless you're
+		 * just trying to change your own settings
 		 */
 		if (roleform->rolsuper)
 		{
@@ -947,7 +988,9 @@ AlterRoleSet(AlterRoleSetStmt *stmt)
 		}
 		else
 		{
-			if (!have_createrole_privilege() && roleid != GetUserId())
+			if ((!have_createrole_privilege() ||
+				!is_admin_of_role(GetUserId(), roleid))
+				&& roleid != GetUserId())
 				ereport(ERROR,
 						(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 						 errmsg("permission denied")));
@@ -1067,13 +1110,18 @@ DropRole(DropRoleStmt *stmt)
 
 		/*
 		 * For safety's sake, we allow createrole holders to drop ordinary
-		 * roles but not superuser roles.  This is mainly to avoid the
-		 * scenario where you accidentally drop the last superuser.
+		 * roles but not superuser roles, and only if they also have ADMIN
+		 * OPTION.
 		 */
 		if (roleform->rolsuper && !superuser())
 			ereport(ERROR,
 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 					 errmsg("must be superuser to drop superusers")));
+		if (!is_admin_of_role(GetUserId(), roleid))
+			ereport(ERROR,
+					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+					 errmsg("must have admin option on role \"%s\"",
+							role)));
 
 		/* DROP hook for the role being removed */
 		InvokeObjectDropHook(AuthIdRelationId, roleid, 0);
@@ -1312,7 +1360,8 @@ RenameRole(const char *oldname, const char *newname)
 				 errmsg("role \"%s\" already exists", newname)));
 
 	/*
-	 * createrole is enough privilege unless you want to mess with a superuser
+	 * Only superusers can mess with superusers. Otherwise, a user with
+	 * CREATEROLE can rename a role for which they have ADMIN OPTION.
 	 */
 	if (((Form_pg_authid) GETSTRUCT(oldtuple))->rolsuper)
 	{
@@ -1323,7 +1372,8 @@ RenameRole(const char *oldname, const char *newname)
 	}
 	else
 	{
-		if (!have_createrole_privilege())
+		if (!have_createrole_privilege() ||
+			!is_admin_of_role(GetUserId(), roleid))
 			ereport(ERROR,
 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 					 errmsg("permission denied to rename role")));
@@ -2022,11 +2072,9 @@ check_role_membership_authorization(Oid currentUserId, Oid roleid,
 	else
 	{
 		/*
-		 * Otherwise, must have createrole or admin option on the role to be
-		 * changed.
+		 * Otherwise, must have admin option on the role to be changed.
 		 */
-		if (!has_createrole_privilege(currentUserId) &&
-			!is_admin_of_role(currentUserId, roleid))
+		if (!is_admin_of_role(currentUserId, roleid))
 			ereport(ERROR,
 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 					 errmsg("must have admin option on role \"%s\"",
@@ -2048,7 +2096,7 @@ check_role_membership_authorization(Oid currentUserId, Oid roleid,
  * be passed as InvalidOid, and this function will infer the user to be
  * recorded as the grantor. In many cases, this will be the current user, but
  * things get more complicated when the current user doesn't possess ADMIN
- * OPTION on the role but rather relies on having CREATEROLE privileges, or
+ * OPTION on the role but rather relies on having SUPERUSER privileges, or
  * on inheriting the privileges of a role which does have ADMIN OPTION. See
  * below for details.
  *
@@ -2074,7 +2122,7 @@ check_role_grantor(Oid currentUserId, Oid roleid, Oid grantorId, bool is_grant)
 		 * not depend on any other existing grants, so always default to this
 		 * interpretation when possible.
 		 */
-		if (has_createrole_privilege(currentUserId))
+		if (superuser_arg(currentUserId))
 			return BOOTSTRAP_SUPERUSERID;
 
 		/*
diff --git a/src/test/regress/expected/create_role.out b/src/test/regress/expected/create_role.out
index 4e67d72760..6e0cce4579 100644
--- a/src/test/regress/expected/create_role.out
+++ b/src/test/regress/expected/create_role.out
@@ -13,7 +13,7 @@ CREATE ROLE regress_nosuch_bypassrls BYPASSRLS;
 ERROR:  must be superuser to create bypassrls users
 -- ok, having CREATEROLE is enough to create users with these privileges
 CREATE ROLE regress_createdb CREATEDB;
-CREATE ROLE regress_createrole CREATEROLE;
+CREATE ROLE regress_createrole CREATEROLE NOINHERIT;
 CREATE ROLE regress_login LOGIN;
 CREATE ROLE regress_inherit INHERIT;
 CREATE ROLE regress_connection_limit CONNECTION LIMIT 5;
@@ -70,20 +70,35 @@ ALTER VIEW tenant_view OWNER TO regress_role_admin;
 ERROR:  must be owner of view tenant_view
 DROP VIEW tenant_view;
 ERROR:  must be owner of view tenant_view
--- fail, cannot take ownership of these objects from regress_tenant
+-- fail, we don't inherit permissions from regress_tenant
 REASSIGN OWNED BY regress_tenant TO regress_createrole;
 ERROR:  permission denied to reassign objects
--- ok, having CREATEROLE is enough to create roles in privileged roles
+-- fail, CREATEROLE is not enough to create roles in privileged roles
 CREATE ROLE regress_read_all_data IN ROLE pg_read_all_data;
+ERROR:  must have admin option on role "pg_read_all_data"
 CREATE ROLE regress_write_all_data IN ROLE pg_write_all_data;
+ERROR:  must have admin option on role "pg_write_all_data"
 CREATE ROLE regress_monitor IN ROLE pg_monitor;
+ERROR:  must have admin option on role "pg_monitor"
 CREATE ROLE regress_read_all_settings IN ROLE pg_read_all_settings;
+ERROR:  must have admin option on role "pg_read_all_settings"
 CREATE ROLE regress_read_all_stats IN ROLE pg_read_all_stats;
+ERROR:  must have admin option on role "pg_read_all_stats"
 CREATE ROLE regress_stat_scan_tables IN ROLE pg_stat_scan_tables;
+ERROR:  must have admin option on role "pg_stat_scan_tables"
 CREATE ROLE regress_read_server_files IN ROLE pg_read_server_files;
+ERROR:  must have admin option on role "pg_read_server_files"
 CREATE ROLE regress_write_server_files IN ROLE pg_write_server_files;
+ERROR:  must have admin option on role "pg_write_server_files"
 CREATE ROLE regress_execute_server_program IN ROLE pg_execute_server_program;
+ERROR:  must have admin option on role "pg_execute_server_program"
 CREATE ROLE regress_signal_backend IN ROLE pg_signal_backend;
+ERROR:  must have admin option on role "pg_signal_backend"
+-- fail, role still owns database objects
+DROP ROLE regress_tenant;
+ERROR:  role "regress_tenant" cannot be dropped because some objects depend on it
+DETAIL:  owner of table tenant_table
+owner of view tenant_view
 -- fail, creation of these roles failed above so they do not now exist
 SET SESSION AUTHORIZATION regress_role_admin;
 DROP ROLE regress_nosuch_superuser;
@@ -114,22 +129,6 @@ DROP ROLE regress_password_null;
 DROP ROLE regress_noiseword;
 DROP ROLE regress_inroles;
 DROP ROLE regress_adminroles;
-DROP ROLE regress_rolecreator;
-DROP ROLE regress_read_all_data;
-DROP ROLE regress_write_all_data;
-DROP ROLE regress_monitor;
-DROP ROLE regress_read_all_settings;
-DROP ROLE regress_read_all_stats;
-DROP ROLE regress_stat_scan_tables;
-DROP ROLE regress_read_server_files;
-DROP ROLE regress_write_server_files;
-DROP ROLE regress_execute_server_program;
-DROP ROLE regress_signal_backend;
--- fail, role still owns database objects
-DROP ROLE regress_tenant;
-ERROR:  role "regress_tenant" cannot be dropped because some objects depend on it
-DETAIL:  owner of table tenant_table
-owner of view tenant_view
 -- fail, cannot drop ourself nor superusers
 DROP ROLE regress_role_super;
 ERROR:  must be superuser to drop superusers
diff --git a/src/test/regress/sql/create_role.sql b/src/test/regress/sql/create_role.sql
index 292dc08797..d491684db3 100644
--- a/src/test/regress/sql/create_role.sql
+++ b/src/test/regress/sql/create_role.sql
@@ -11,7 +11,7 @@ CREATE ROLE regress_nosuch_bypassrls BYPASSRLS;
 
 -- ok, having CREATEROLE is enough to create users with these privileges
 CREATE ROLE regress_createdb CREATEDB;
-CREATE ROLE regress_createrole CREATEROLE;
+CREATE ROLE regress_createrole CREATEROLE NOINHERIT;
 CREATE ROLE regress_login LOGIN;
 CREATE ROLE regress_inherit INHERIT;
 CREATE ROLE regress_connection_limit CONNECTION LIMIT 5;
@@ -71,10 +71,10 @@ DROP TABLE tenant_table;
 ALTER VIEW tenant_view OWNER TO regress_role_admin;
 DROP VIEW tenant_view;
 
--- fail, cannot take ownership of these objects from regress_tenant
+-- fail, we don't inherit permissions from regress_tenant
 REASSIGN OWNED BY regress_tenant TO regress_createrole;
 
--- ok, having CREATEROLE is enough to create roles in privileged roles
+-- fail, CREATEROLE is not enough to create roles in privileged roles
 CREATE ROLE regress_read_all_data IN ROLE pg_read_all_data;
 CREATE ROLE regress_write_all_data IN ROLE pg_write_all_data;
 CREATE ROLE regress_monitor IN ROLE pg_monitor;
@@ -86,6 +86,9 @@ CREATE ROLE regress_write_server_files IN ROLE pg_write_server_files;
 CREATE ROLE regress_execute_server_program IN ROLE pg_execute_server_program;
 CREATE ROLE regress_signal_backend IN ROLE pg_signal_backend;
 
+-- fail, role still owns database objects
+DROP ROLE regress_tenant;
+
 -- fail, creation of these roles failed above so they do not now exist
 SET SESSION AUTHORIZATION regress_role_admin;
 DROP ROLE regress_nosuch_superuser;
@@ -109,20 +112,6 @@ DROP ROLE regress_password_null;
 DROP ROLE regress_noiseword;
 DROP ROLE regress_inroles;
 DROP ROLE regress_adminroles;
-DROP ROLE regress_rolecreator;
-DROP ROLE regress_read_all_data;
-DROP ROLE regress_write_all_data;
-DROP ROLE regress_monitor;
-DROP ROLE regress_read_all_settings;
-DROP ROLE regress_read_all_stats;
-DROP ROLE regress_stat_scan_tables;
-DROP ROLE regress_read_server_files;
-DROP ROLE regress_write_server_files;
-DROP ROLE regress_execute_server_program;
-DROP ROLE regress_signal_backend;
-
--- fail, role still owns database objects
-DROP ROLE regress_tenant;
 
 -- fail, cannot drop ourself nor superusers
 DROP ROLE regress_role_super;
-- 
2.24.3 (Apple Git-128)