From 0a811de40f3b4bb7fe5d4e43ea2f583a5275c0f6 Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Thu, 25 Jun 2026 11:18:20 -0700 Subject: [PATCH v2 1/2] Prevent broken grant chains when indirect grant options are held recursive_revoke() is meant to protect against cases where a grantor loses their grant option after issuing dependent grants to other roles. In that case, the user must specify CASCADE to prune the dependent ACLs. However, if a grantor happens to hold grant options (or table ownership) indirectly, whether via INHERIT or SUPERUSER, recursive_revoke() short-circuits and allows the REVOKE to break the grant chain. Symptoms include general user confusion as well as problems during dump/restore, which cannot recreate the original state. To fix, switch from aclmask() to aclmask_direct(). Discussion: https://postgr.es/m/CAOYmi%2B%3DKTLd%2BXsEP%3DTDiZ48iVf-CEc7JrZd5uhWPYWKEfOgyyQ%40mail.gmail.com Backpatch-through: 14 --- src/backend/utils/adt/acl.c | 19 +++-- src/test/regress/expected/privileges.out | 88 ++++++++++++++++++++++++ src/test/regress/sql/privileges.sql | 44 ++++++++++++ 3 files changed, 147 insertions(+), 4 deletions(-) diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c index 913d715a8c8..8bcda719e4c 100644 --- a/src/backend/utils/adt/acl.c +++ b/src/backend/utils/adt/acl.c @@ -102,6 +102,8 @@ static void check_circularity(const Acl *old_acl, const AclItem *mod_aip, Oid ownerId); static Acl *recursive_revoke(Acl *acl, Oid grantee, AclMode revoke_privs, Oid ownerId, DropBehavior behavior); +static AclMode aclmask_direct(const Acl *acl, Oid roleid, Oid ownerId, + AclMode mask, AclMaskHow how); static AclMode convert_any_priv_string(text *priv_type_text, const priv_map *privileges); @@ -1349,10 +1351,19 @@ recursive_revoke(Acl *acl, if (grantee == ownerId) return acl; - /* The grantee might still have some grant options via another grantor */ - still_has = aclmask(acl, grantee, ownerId, - ACL_GRANT_OPTION_FOR(revoke_privs), - ACLMASK_ALL); + /* + * The grantee might still have some grant options via another grantor, + * keeping the chain intact for those privileges. And if _all_ of the + * remaining privileges can still be granted by any role on the chain, + * we're done. + * + * Indirectly held grant options cannot keep the chain alive, so consult + * aclmask_direct() rather than aclmask(). (Otherwise, for example, a + * chain could be broken wherever the grantee was a superuser). + */ + still_has = aclmask_direct(acl, grantee, ownerId, + ACL_GRANT_OPTION_FOR(revoke_privs), + ACLMASK_ALL); revoke_privs &= ~ACL_OPTION_TO_PRIVS(still_has); if (revoke_privs == ACL_NO_RIGHTS) return acl; diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out index 9b7153c9cd5..4de9d749574 100644 --- a/src/test/regress/expected/privileges.out +++ b/src/test/regress/expected/privileges.out @@ -1885,6 +1885,8 @@ CREATE TABLE atest4 (a int); GRANT SELECT ON atest4 TO regress_priv_user2 WITH GRANT OPTION; GRANT UPDATE ON atest4 TO regress_priv_user2; GRANT SELECT ON atest4 TO GROUP regress_priv_group1 WITH GRANT OPTION; +GRANT SELECT ON atest4 TO PUBLIC WITH GRANT OPTION; -- fail +ERROR: grant options can only be granted to roles SET SESSION AUTHORIZATION regress_priv_user2; GRANT SELECT ON atest4 TO regress_priv_user3; GRANT UPDATE ON atest4 TO regress_priv_user3; -- fail @@ -1919,6 +1921,91 @@ SELECT has_table_privilege('regress_priv_user1', 'atest4', 'SELECT WITH GRANT OP t (1 row) +-- Test GRANT OPTION chains where the intermediate grantor has the privileges of +-- the owner (regress_priv_group1 -> regress_priv_user4 -> regress_priv_user2) +-- or inherits the option (regress_priv_group2 -> regress_priv_user1 -> +-- regress_priv_user3). +-- pin the group membership that we rely on below +SELECT pg_has_role('regress_priv_user4', 'regress_priv_group1', 'MEMBER'), + pg_has_role('regress_priv_user1', 'regress_priv_group2', 'MEMBER'); + pg_has_role | pg_has_role +-------------+------------- + t | t +(1 row) + +SET SESSION AUTHORIZATION regress_priv_group1; +CREATE TABLE atest4_groupowned (a int); +GRANT SELECT ON atest4_groupowned TO regress_priv_user4 WITH GRANT OPTION; +GRANT UPDATE ON atest4_groupowned TO regress_priv_group2 WITH GRANT OPTION; +GRANT UPDATE ON atest4_groupowned TO regress_priv_user1 WITH GRANT OPTION; +SET SESSION AUTHORIZATION regress_priv_user4; +GRANT SELECT ON atest4_groupowned TO regress_priv_user2; +SET SESSION AUTHORIZATION regress_priv_user1; +GRANT UPDATE ON atest4_groupowned TO regress_priv_user3; +SET SESSION AUTHORIZATION regress_priv_group1; +REVOKE SELECT ON atest4_groupowned FROM regress_priv_user4; -- fail +ERROR: dependent privileges exist +HINT: Use CASCADE to revoke them too. +SELECT has_table_privilege('regress_priv_user2', 'atest4_groupowned', 'SELECT'); -- true + has_table_privilege +--------------------- + t +(1 row) + +SELECT has_table_privilege('regress_priv_user4', 'atest4_groupowned', 'SELECT'); -- true + has_table_privilege +--------------------- + t +(1 row) + +REVOKE SELECT ON atest4_groupowned FROM regress_priv_user4 CASCADE; -- ok +SELECT has_table_privilege('regress_priv_user2', 'atest4_groupowned', 'SELECT'); -- false + has_table_privilege +--------------------- + f +(1 row) + +SELECT has_table_privilege('regress_priv_user4', 'atest4_groupowned', 'SELECT'); -- still true (inherited) + has_table_privilege +--------------------- + t +(1 row) + +REVOKE UPDATE ON atest4_groupowned FROM regress_priv_user1; -- fail +ERROR: dependent privileges exist +HINT: Use CASCADE to revoke them too. +SELECT has_table_privilege('regress_priv_user3', 'atest4_groupowned', 'UPDATE'); -- true + has_table_privilege +--------------------- + t +(1 row) + +SELECT has_table_privilege('regress_priv_user1', 'atest4_groupowned', 'UPDATE'); -- true + has_table_privilege +--------------------- + t +(1 row) + +REVOKE UPDATE ON atest4_groupowned FROM regress_priv_user1 CASCADE; -- ok +SELECT has_table_privilege('regress_priv_user3', 'atest4_groupowned', 'UPDATE'); -- false + has_table_privilege +--------------------- + f +(1 row) + +SELECT has_table_privilege('regress_priv_user1', 'atest4_groupowned', 'UPDATE'); -- still true (inherited) + has_table_privilege +--------------------- + t +(1 row) + +REVOKE UPDATE ON atest4_groupowned FROM regress_priv_group2 RESTRICT; -- ok +SELECT has_table_privilege('regress_priv_user1', 'atest4_groupowned', 'UPDATE'); -- false + has_table_privilege +--------------------- + f +(1 row) + -- security-restricted operations \c - CREATE ROLE regress_sro_user; @@ -3228,6 +3315,7 @@ DROP TABLE atest1; DROP TABLE atest2; DROP TABLE atest3; DROP TABLE atest4; +DROP TABLE atest4_groupowned; DROP TABLE atest5; DROP TABLE atest6; DROP TABLE atestc; diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql index 0113dfa5dab..c34f5196fe5 100644 --- a/src/test/regress/sql/privileges.sql +++ b/src/test/regress/sql/privileges.sql @@ -1226,6 +1226,7 @@ CREATE TABLE atest4 (a int); GRANT SELECT ON atest4 TO regress_priv_user2 WITH GRANT OPTION; GRANT UPDATE ON atest4 TO regress_priv_user2; GRANT SELECT ON atest4 TO GROUP regress_priv_group1 WITH GRANT OPTION; +GRANT SELECT ON atest4 TO PUBLIC WITH GRANT OPTION; -- fail SET SESSION AUTHORIZATION regress_priv_user2; @@ -1243,6 +1244,48 @@ SELECT has_table_privilege('regress_priv_user3', 'atest4', 'SELECT'); -- false SELECT has_table_privilege('regress_priv_user1', 'atest4', 'SELECT WITH GRANT OPTION'); -- true +-- Test GRANT OPTION chains where the intermediate grantor has the privileges of +-- the owner (regress_priv_group1 -> regress_priv_user4 -> regress_priv_user2) +-- or inherits the option (regress_priv_group2 -> regress_priv_user1 -> +-- regress_priv_user3). + +-- pin the group membership that we rely on below +SELECT pg_has_role('regress_priv_user4', 'regress_priv_group1', 'MEMBER'), + pg_has_role('regress_priv_user1', 'regress_priv_group2', 'MEMBER'); + +SET SESSION AUTHORIZATION regress_priv_group1; + +CREATE TABLE atest4_groupowned (a int); + +GRANT SELECT ON atest4_groupowned TO regress_priv_user4 WITH GRANT OPTION; +GRANT UPDATE ON atest4_groupowned TO regress_priv_group2 WITH GRANT OPTION; +GRANT UPDATE ON atest4_groupowned TO regress_priv_user1 WITH GRANT OPTION; + +SET SESSION AUTHORIZATION regress_priv_user4; + +GRANT SELECT ON atest4_groupowned TO regress_priv_user2; + +SET SESSION AUTHORIZATION regress_priv_user1; + +GRANT UPDATE ON atest4_groupowned TO regress_priv_user3; + +SET SESSION AUTHORIZATION regress_priv_group1; + +REVOKE SELECT ON atest4_groupowned FROM regress_priv_user4; -- fail +SELECT has_table_privilege('regress_priv_user2', 'atest4_groupowned', 'SELECT'); -- true +SELECT has_table_privilege('regress_priv_user4', 'atest4_groupowned', 'SELECT'); -- true +REVOKE SELECT ON atest4_groupowned FROM regress_priv_user4 CASCADE; -- ok +SELECT has_table_privilege('regress_priv_user2', 'atest4_groupowned', 'SELECT'); -- false +SELECT has_table_privilege('regress_priv_user4', 'atest4_groupowned', 'SELECT'); -- still true (inherited) + +REVOKE UPDATE ON atest4_groupowned FROM regress_priv_user1; -- fail +SELECT has_table_privilege('regress_priv_user3', 'atest4_groupowned', 'UPDATE'); -- true +SELECT has_table_privilege('regress_priv_user1', 'atest4_groupowned', 'UPDATE'); -- true +REVOKE UPDATE ON atest4_groupowned FROM regress_priv_user1 CASCADE; -- ok +SELECT has_table_privilege('regress_priv_user3', 'atest4_groupowned', 'UPDATE'); -- false +SELECT has_table_privilege('regress_priv_user1', 'atest4_groupowned', 'UPDATE'); -- still true (inherited) +REVOKE UPDATE ON atest4_groupowned FROM regress_priv_group2 RESTRICT; -- ok +SELECT has_table_privilege('regress_priv_user1', 'atest4_groupowned', 'UPDATE'); -- false -- security-restricted operations \c - @@ -1918,6 +1961,7 @@ DROP TABLE atest1; DROP TABLE atest2; DROP TABLE atest3; DROP TABLE atest4; +DROP TABLE atest4_groupowned; DROP TABLE atest5; DROP TABLE atest6; DROP TABLE atestc; -- 2.50.1 (Apple Git-155)