public inbox for [email protected]
help / color / mirror / Atom feedFrom: Jacob Champion <[email protected]>
To: PostgreSQL Hackers <[email protected]>
Subject: Re: REVOKE's CASCADE protection doesn't work with INHERITed table owners
Date: Thu, 25 Jun 2026 17:12:42 -0700
Message-ID: <CAOYmi+n_hw=SC5V1i3BmqfZPfPBRaUSJc+BeOXEKDwRue+WYrg@mail.gmail.com> (raw)
In-Reply-To: <CAOYmi+=KTLd+XsEP=TDiZ48iVf-CEc7JrZd5uhWPYWKEfOgyyQ@mail.gmail.com>
References: <CAOYmi+=KTLd+XsEP=TDiZ48iVf-CEc7JrZd5uhWPYWKEfOgyyQ@mail.gmail.com>
[moving to -hackers]
On Wed, Jun 24, 2026 at 2:57 PM Jacob Champion
<[email protected]> wrote:
> TL;DR: The protection in recursive_revoke() against broken GRANT
> OPTION chains doesn't seem to work properly when the grantee also
> holds the privileges of the grantor.
More accurately: "when an intermediate grantor in the chain only
indirectly holds the ability to grant."
> I think the issue is in recursive_revoke()'s usage of aclmask(), which
> in turn uses has_privs_of_role(). It doesn't seem like that's what was
> wanted in this particular case... thoughts?
I propose changing that to aclmask_direct(), as in the attached, and
backpatching all the way down.
To try to prove to myself that this works, I added tests to pin each
of the three cases that are treated differently by aclmask_direct():
1. the grantor has indirect ownership privileges
2. the grantor has indirect grant options via INHERIT
3. the grantor has indirect grant options via PUBLIC (which is already
disallowed in practice)
I also tried to expand the existing comment, both to point out the
pitfall and to explain why the short-circuit works. But I've rewritten
it at least a dozen times, so if anyone can tell me whether I've made
sense and/or used the terminology appropriately, I'd appreciate it.
> I'm pretty sure the following is unintended behavior. It looks
> potentially related to [1] as well.
(To fix [1] I suspect we need to make a similar tweak to
check_circularity(), but I haven't looked into that yet.)
Thanks!
--Jacob
[1] https://postgr.es/m/CAM6Zo8wD7RtQNhbQHODc9DobiW+GpT=tnqOSMz4+mnzA9m0zMg@mail.gmail.com
Attachments:
[application/octet-stream] v1-0001-Prevent-broken-grant-chains-when-indirect-grant-o.patch (10.0K, ../CAOYmi+n_hw=SC5V1i3BmqfZPfPBRaUSJc+BeOXEKDwRue+WYrg@mail.gmail.com/2-v1-0001-Prevent-broken-grant-chains-when-indirect-grant-o.patch)
download | inline diff:
From 1a0c40e4257d6b1fd19d850761dbbcc21800aef7 Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Thu, 25 Jun 2026 11:18:20 -0700
Subject: [PATCH v1] 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 01caa12eca7..21a874392d9 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);
@@ -1344,10 +1346,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 f6cc1a1029c..6840ff950c6 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -1862,6 +1862,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
@@ -1896,6 +1898,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;
@@ -3296,6 +3383,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 6cd9bb840ff..c9f80ba898b 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -1205,6 +1205,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;
@@ -1222,6 +1223,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 -
@@ -1955,6 +1998,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.34.1
view thread (5+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: REVOKE's CASCADE protection doesn't work with INHERITed table owners
In-Reply-To: <CAOYmi+n_hw=SC5V1i3BmqfZPfPBRaUSJc+BeOXEKDwRue+WYrg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox