From b8f0dff492cfe689e8516b0353d004f483838640 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 20 Feb 2025 15:34:55 -0500 Subject: [PATCH v1 1/2] Avoid race condition between "GRANT role" and "DROP ROLE". Concurrently dropping either the granted role or the grantee does not stop GRANT from completing, instead resulting in a dangling role reference in pg_auth_members. That's relatively harmless in the short run, but it does result in garbage output from pg_dumpall and therefore problems for pg_upgrade. This patch deals with the problem by adding the granted and grantee roles as explicit shared dependencies of the pg_auth_members entry. That's a bit indirect, but it works because the pg_shdepend code applies the necessary locking and rechecking. Commit 6566133c5 previously established similar handling for the grantor column of pg_auth_members; it's not clear why it didn't cover the other two role OID columns. A side-effect of this approach is that DROP OWNED BY will now drop pg_auth_members entries that mention the target role as either the granted or grantee role. That's clearly appropriate for the grantee, and it doesn't seem too far out of line for the granted role. (One could argue that this makes DropRole's code to auto-drop pg_auth_members entries unnecessary, but I chose to leave it in place since perhaps some people's workflows expect that to work without a DROP OWNED BY.) Reported-by: Virender Singla Discussion: https://postgr.es/m/CAM6Zo8woa62ZFHtMKox6a4jb8qQ=w87R2L0K8347iE-juQL2EA@mail.gmail.com Backpatch-through: TBD --- doc/src/sgml/ref/drop_owned.sgml | 2 +- src/backend/commands/user.c | 22 +++++++++++++---- src/test/regress/expected/privileges.out | 30 ++++++++++++++++++++++++ src/test/regress/sql/privileges.sql | 15 ++++++++++++ 4 files changed, 63 insertions(+), 6 deletions(-) diff --git a/doc/src/sgml/ref/drop_owned.sgml b/doc/src/sgml/ref/drop_owned.sgml index efda01a39e..46e1c229ec 100644 --- a/doc/src/sgml/ref/drop_owned.sgml +++ b/doc/src/sgml/ref/drop_owned.sgml @@ -33,7 +33,7 @@ DROP OWNED BY { name | CURRENT_ROLE database that are owned by one of the specified roles. Any privileges granted to the given roles on objects in the current database or on shared objects (databases, tablespaces, configuration - parameters) will also be revoked. + parameters, or other roles) will also be revoked. diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c index 0db174e6f1..0c84886e82 100644 --- a/src/backend/commands/user.c +++ b/src/backend/commands/user.c @@ -30,6 +30,7 @@ #include "commands/defrem.h" #include "commands/seclabel.h" #include "commands/user.h" +#include "lib/qunique.h" #include "libpq/crypt.h" #include "miscadmin.h" #include "storage/lmgr.h" @@ -489,7 +490,7 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt) * Advance command counter so we can see new record; else tests in * AddRoleMems may fail. */ - if (addroleto || adminmembers || rolemembers) + if (addroleto || adminmembers || rolemembers || !superuser()) CommandCounterIncrement(); /* Default grant. */ @@ -1904,7 +1905,8 @@ AddRoleMems(Oid currentUserId, const char *rolename, Oid roleid, else { Oid objectId; - Oid *newmembers = palloc(sizeof(Oid)); + Oid *newmembers = (Oid *) palloc(3 * sizeof(Oid)); + int nnewmembers; /* * The values for these options can be taken directly from 'popt'. @@ -1946,12 +1948,22 @@ AddRoleMems(Oid currentUserId, const char *rolename, Oid roleid, new_record, new_record_nulls); CatalogTupleInsert(pg_authmem_rel, tuple); - /* updateAclDependencies wants to pfree array inputs */ - newmembers[0] = grantorId; + /* + * Record dependencies on the roleid, member, and grantor, as if a + * pg_auth_members entry were an object ACL. + * updateAclDependencies() requires an input array that is + * palloc'd (it will free it), sorted, and de-duped. + */ + newmembers[0] = roleid; + newmembers[1] = memberid; + newmembers[2] = grantorId; + qsort(newmembers, 3, sizeof(Oid), oid_cmp); + nnewmembers = qunique(newmembers, 3, sizeof(Oid), oid_cmp); + updateAclDependencies(AuthMemRelationId, objectId, 0, InvalidOid, 0, NULL, - 1, newmembers); + nnewmembers, newmembers); } /* CCI after each change, in case there are duplicates in list */ diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out index 6b01313101..a76256405f 100644 --- a/src/test/regress/expected/privileges.out +++ b/src/test/regress/expected/privileges.out @@ -113,6 +113,36 @@ CREATE USER regress_priv_user2; CREATE USER regress_priv_user3; CREATE USER regress_priv_user4; CREATE USER regress_priv_user5; +-- DROP OWNED should also act on granted and granted-to roles +GRANT regress_priv_user1 TO regress_priv_user2; +GRANT regress_priv_user2 TO regress_priv_user3; +SELECT roleid::regrole, member::regrole FROM pg_auth_members + WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole) + ORDER BY roleid::regrole::text; + roleid | member +--------------------+-------------------- + regress_priv_user1 | regress_priv_user2 + regress_priv_user2 | regress_priv_user3 +(2 rows) + +REASSIGN OWNED BY regress_priv_user2 TO regress_priv_user4; -- no effect +SELECT roleid::regrole, member::regrole FROM pg_auth_members + WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole) + ORDER BY roleid::regrole::text; + roleid | member +--------------------+-------------------- + regress_priv_user1 | regress_priv_user2 + regress_priv_user2 | regress_priv_user3 +(2 rows) + +DROP OWNED BY regress_priv_user2; -- removes both grants +SELECT roleid::regrole, member::regrole FROM pg_auth_members + WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole) + ORDER BY roleid::regrole::text; + roleid | member +--------+-------- +(0 rows) + GRANT pg_read_all_data TO regress_priv_user6; GRANT pg_write_all_data TO regress_priv_user7; GRANT pg_read_all_settings TO regress_priv_user8 WITH ADMIN OPTION; diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql index 60e7443bf5..d195aaf137 100644 --- a/src/test/regress/sql/privileges.sql +++ b/src/test/regress/sql/privileges.sql @@ -90,6 +90,21 @@ CREATE USER regress_priv_user3; CREATE USER regress_priv_user4; CREATE USER regress_priv_user5; +-- DROP OWNED should also act on granted and granted-to roles +GRANT regress_priv_user1 TO regress_priv_user2; +GRANT regress_priv_user2 TO regress_priv_user3; +SELECT roleid::regrole, member::regrole FROM pg_auth_members + WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole) + ORDER BY roleid::regrole::text; +REASSIGN OWNED BY regress_priv_user2 TO regress_priv_user4; -- no effect +SELECT roleid::regrole, member::regrole FROM pg_auth_members + WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole) + ORDER BY roleid::regrole::text; +DROP OWNED BY regress_priv_user2; -- removes both grants +SELECT roleid::regrole, member::regrole FROM pg_auth_members + WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole) + ORDER BY roleid::regrole::text; + GRANT pg_read_all_data TO regress_priv_user6; GRANT pg_write_all_data TO regress_priv_user7; GRANT pg_read_all_settings TO regress_priv_user8 WITH ADMIN OPTION; -- 2.43.5