From 84206411750465665ac2076827c39c52f79ebbe6 Mon Sep 17 00:00:00 2001 From: Mark Dilger Date: Wed, 20 Oct 2021 08:52:25 -0700 Subject: [PATCH v1 3/4] Give role owners control over owned roles Create a role ownership hierarchy. The previous commit added owners to roles. This goes further, making role ownership transitive. If role A owns role B, and role B owns role C, then role A can act as the owner of role C. Also, roles A and B can perform any action on objects belonging to role C that role C could itself perform. This is a preparatory patch for changing how CREATEROLE works. --- src/backend/catalog/aclchk.c | 68 ++++++++++++++++--- src/backend/catalog/objectaddress.c | 22 +----- src/backend/commands/user.c | 16 +++-- src/backend/utils/adt/acl.c | 4 ++ src/include/utils/acl.h | 1 + .../expected/dummy_seclabel.out | 12 ++-- .../dummy_seclabel/sql/dummy_seclabel.sql | 12 +++- src/test/regress/expected/create_role.out | 21 ++---- src/test/regress/sql/create_role.sql | 11 ++- 9 files changed, 102 insertions(+), 65 deletions(-) diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 89792b154e..f24b664970 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -3418,6 +3418,9 @@ aclcheck_error(AclResult aclerr, ObjectType objtype, case OBJECT_VIEW: msg = gettext_noop("permission denied for view %s"); break; + case OBJECT_ROLE: + msg = gettext_noop("permission denied for role %s"); + break; /* these currently aren't used */ case OBJECT_ACCESS_METHOD: case OBJECT_AMOP: @@ -3428,7 +3431,6 @@ aclcheck_error(AclResult aclerr, ObjectType objtype, case OBJECT_DEFACL: case OBJECT_DOMCONSTRAINT: case OBJECT_PUBLICATION_REL: - case OBJECT_ROLE: case OBJECT_RULE: case OBJECT_TABCONSTRAINT: case OBJECT_TRANSFORM: @@ -3543,6 +3545,9 @@ aclcheck_error(AclResult aclerr, ObjectType objtype, case OBJECT_TSDICTIONARY: msg = gettext_noop("must be owner of text search dictionary %s"); break; + case OBJECT_ROLE: + msg = gettext_noop("must be owner of role %s"); + break; /* * Special cases: For these, the error message talks @@ -3567,7 +3572,6 @@ aclcheck_error(AclResult aclerr, ObjectType objtype, case OBJECT_DEFACL: case OBJECT_DOMCONSTRAINT: case OBJECT_PUBLICATION_REL: - case OBJECT_ROLE: case OBJECT_TRANSFORM: case OBJECT_TSPARSER: case OBJECT_TSTEMPLATE: @@ -5428,16 +5432,62 @@ pg_statistics_object_ownercheck(Oid stat_oid, Oid roleid) return has_privs_of_role(roleid, ownerId); } +/* + * Check whether specified role is the direct or indirect owner of another + * role. + */ +bool +pg_role_ownercheck(Oid role_oid, Oid roleid) +{ + HeapTuple tuple; + + /* Superusers bypass all permission checking. */ + if (superuser_arg(roleid)) + return true; + + /* + * Start with the owned role and traverse the ownership hierarchy upward. + * We stop when we find the owner we are looking for or when we reach the + * top. + */ + while (OidIsValid(role_oid)) + { + Oid owner_oid; + + /* Find the owner of the current iteration's role */ + tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(role_oid)); + if (!HeapTupleIsValid(tuple)) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("role with OID %u does not exist", + role_oid))); + owner_oid = ((Form_pg_authid) GETSTRUCT(tuple))->rolowner; + ReleaseSysCache(tuple); + + /* Have we found the target role? */ + if (roleid == owner_oid) + return true; + + /* + * If we have reached a role which owns itself, we must iterate no + * further, else we fall into an infinite loop. + */ + if (role_oid == owner_oid) + return false; + + /* Set up for the next iteration. */ + role_oid = owner_oid; + } + + return false; +} + /* * Check whether specified role has CREATEROLE privilege (or is a superuser) * - * Note: roles do not have owners per se; instead we use this test in - * places where an ownership-like permissions test is needed for a role. - * Be sure to apply it to the role trying to do the operation, not the - * role being operated on! Also note that this generally should not be - * considered enough privilege if the target role is a superuser. - * (We don't handle that consideration here because we want to give a - * separate error message for such cases, so the caller has to deal with it.) + * Note: In versions prior to PostgreSQL version 15, roles did not have owners + * per se; instead we used this test in places where an ownership-like + * permissions test was needed for a role. */ bool has_createrole_privilege(Oid roleid) diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c index 8c94939baa..fecbc763de 100644 --- a/src/backend/catalog/objectaddress.c +++ b/src/backend/catalog/objectaddress.c @@ -2540,25 +2540,9 @@ check_object_ownership(Oid roleid, ObjectType objtype, ObjectAddress address, NameListToString(castNode(List, object))); break; case OBJECT_ROLE: - - /* - * We treat roles as being "owned" by those with CREATEROLE priv, - * except that superusers are only owned by superusers. - */ - if (superuser_arg(address.objectId)) - { - if (!superuser_arg(roleid)) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("must be superuser"))); - } - else - { - if (!has_createrole_privilege(roleid)) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("must have CREATEROLE privilege"))); - } + if (!pg_role_ownercheck(address.objectId, roleid)) + aclcheck_error(ACLCHECK_NOT_OWNER, objtype, + strVal(object)); break; case OBJECT_TSPARSER: case OBJECT_TSTEMPLATE: diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c index 815c7095ec..902c0473de 100644 --- a/src/backend/commands/user.c +++ b/src/backend/commands/user.c @@ -693,7 +693,7 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt) !rolemembers && !validUntil && dpassword && - roleid == GetUserId())) + !pg_role_ownercheck(roleid, GetUserId()))) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), errmsg("permission denied"))); @@ -894,7 +894,8 @@ AlterRoleSet(AlterRoleSetStmt *stmt) } else { - if (!have_createrole_privilege() && roleid != GetUserId()) + if (!have_createrole_privilege() && + !pg_role_ownercheck(roleid, GetUserId())) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), errmsg("permission denied"))); @@ -946,11 +947,6 @@ DropRole(DropRoleStmt *stmt) pg_auth_members_rel; ListCell *item; - if (!have_createrole_privilege()) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("permission denied to drop role"))); - /* * Scan the pg_authid relation to find the Oid of the role(s) to be * deleted. @@ -1022,6 +1018,12 @@ DropRole(DropRoleStmt *stmt) (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), errmsg("must be superuser to drop superusers"))); + if (!have_createrole_privilege() && + !pg_role_ownercheck(roleid, GetUserId())) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied to drop role"))); + /* DROP hook for the role being removed */ InvokeObjectDropHook(AuthIdRelationId, roleid, 0); diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c index 67f8b29434..04eae9d4e5 100644 --- a/src/backend/utils/adt/acl.c +++ b/src/backend/utils/adt/acl.c @@ -4850,6 +4850,10 @@ has_privs_of_role(Oid member, Oid role) if (superuser_arg(member)) return true; + /* Owners of roles have every privilge the owned role has */ + if (pg_role_ownercheck(role, member)) + return true; + /* * Find all the roles that member has the privileges of, including * multi-level recursion, then see if target role is any one of them. diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h index af771c901d..ec9d480d67 100644 --- a/src/include/utils/acl.h +++ b/src/include/utils/acl.h @@ -316,6 +316,7 @@ extern bool pg_extension_ownercheck(Oid ext_oid, Oid roleid); extern bool pg_publication_ownercheck(Oid pub_oid, Oid roleid); extern bool pg_subscription_ownercheck(Oid sub_oid, Oid roleid); extern bool pg_statistics_object_ownercheck(Oid stat_oid, Oid roleid); +extern bool pg_role_ownercheck(Oid role_oid, Oid roleid); extern bool has_createrole_privilege(Oid roleid); extern bool has_bypassrls_privilege(Oid roleid); diff --git a/src/test/modules/dummy_seclabel/expected/dummy_seclabel.out b/src/test/modules/dummy_seclabel/expected/dummy_seclabel.out index b2d898a7d1..93cf82b750 100644 --- a/src/test/modules/dummy_seclabel/expected/dummy_seclabel.out +++ b/src/test/modules/dummy_seclabel/expected/dummy_seclabel.out @@ -7,8 +7,11 @@ SET client_min_messages TO 'warning'; DROP ROLE IF EXISTS regress_dummy_seclabel_user1; DROP ROLE IF EXISTS regress_dummy_seclabel_user2; RESET client_min_messages; -CREATE USER regress_dummy_seclabel_user1 WITH CREATEROLE; +CREATE USER regress_dummy_seclabel_user0 WITH CREATEROLE; +SET SESSION AUTHORIZATION regress_dummy_seclabel_user0; +CREATE USER regress_dummy_seclabel_user1; CREATE USER regress_dummy_seclabel_user2; +RESET SESSION AUTHORIZATION; CREATE TABLE dummy_seclabel_tbl1 (a int, b text); CREATE TABLE dummy_seclabel_tbl2 (x int, y text); CREATE VIEW dummy_seclabel_view1 AS SELECT * FROM dummy_seclabel_tbl2; @@ -19,7 +22,7 @@ ALTER TABLE dummy_seclabel_tbl2 OWNER TO regress_dummy_seclabel_user2; -- -- Test of SECURITY LABEL statement with a plugin -- -SET SESSION AUTHORIZATION regress_dummy_seclabel_user1; +SET SESSION AUTHORIZATION regress_dummy_seclabel_user0; SECURITY LABEL ON TABLE dummy_seclabel_tbl1 IS 'classified'; -- OK SECURITY LABEL ON COLUMN dummy_seclabel_tbl1.a IS 'unclassified'; -- OK SECURITY LABEL ON COLUMN dummy_seclabel_tbl1 IS 'unclassified'; -- fail @@ -29,6 +32,7 @@ ERROR: '...invalid label...' is not a valid security label SECURITY LABEL FOR 'dummy' ON TABLE dummy_seclabel_tbl1 IS 'unclassified'; -- OK SECURITY LABEL FOR 'unknown_seclabel' ON TABLE dummy_seclabel_tbl1 IS 'classified'; -- fail ERROR: security label provider "unknown_seclabel" is not loaded +SET SESSION AUTHORIZATION regress_dummy_seclabel_user1; SECURITY LABEL ON TABLE dummy_seclabel_tbl2 IS 'unclassified'; -- fail (not owner) ERROR: must be owner of table dummy_seclabel_tbl2 SECURITY LABEL ON TABLE dummy_seclabel_tbl1 IS 'secret'; -- fail (not superuser) @@ -42,7 +46,7 @@ SECURITY LABEL ON TABLE dummy_seclabel_tbl2 IS 'classified'; -- OK -- -- Test for shared database object -- -SET SESSION AUTHORIZATION regress_dummy_seclabel_user1; +SET SESSION AUTHORIZATION regress_dummy_seclabel_user0; SECURITY LABEL ON ROLE regress_dummy_seclabel_user1 IS 'classified'; -- OK SECURITY LABEL ON ROLE regress_dummy_seclabel_user1 IS '...invalid label...'; -- fail ERROR: '...invalid label...' is not a valid security label @@ -55,7 +59,7 @@ SECURITY LABEL ON ROLE regress_dummy_seclabel_user3 IS 'unclassified'; -- fail ( ERROR: role "regress_dummy_seclabel_user3" does not exist SET SESSION AUTHORIZATION regress_dummy_seclabel_user2; SECURITY LABEL ON ROLE regress_dummy_seclabel_user2 IS 'unclassified'; -- fail (not privileged) -ERROR: must have CREATEROLE privilege +ERROR: must be owner of role regress_dummy_seclabel_user2 RESET SESSION AUTHORIZATION; -- -- Test for various types of object diff --git a/src/test/modules/dummy_seclabel/sql/dummy_seclabel.sql b/src/test/modules/dummy_seclabel/sql/dummy_seclabel.sql index 8c347b6a68..bf575343cf 100644 --- a/src/test/modules/dummy_seclabel/sql/dummy_seclabel.sql +++ b/src/test/modules/dummy_seclabel/sql/dummy_seclabel.sql @@ -11,8 +11,12 @@ DROP ROLE IF EXISTS regress_dummy_seclabel_user2; RESET client_min_messages; -CREATE USER regress_dummy_seclabel_user1 WITH CREATEROLE; +CREATE USER regress_dummy_seclabel_user0 WITH CREATEROLE; + +SET SESSION AUTHORIZATION regress_dummy_seclabel_user0; +CREATE USER regress_dummy_seclabel_user1; CREATE USER regress_dummy_seclabel_user2; +RESET SESSION AUTHORIZATION; CREATE TABLE dummy_seclabel_tbl1 (a int, b text); CREATE TABLE dummy_seclabel_tbl2 (x int, y text); @@ -26,7 +30,7 @@ ALTER TABLE dummy_seclabel_tbl2 OWNER TO regress_dummy_seclabel_user2; -- -- Test of SECURITY LABEL statement with a plugin -- -SET SESSION AUTHORIZATION regress_dummy_seclabel_user1; +SET SESSION AUTHORIZATION regress_dummy_seclabel_user0; SECURITY LABEL ON TABLE dummy_seclabel_tbl1 IS 'classified'; -- OK SECURITY LABEL ON COLUMN dummy_seclabel_tbl1.a IS 'unclassified'; -- OK @@ -34,6 +38,8 @@ SECURITY LABEL ON COLUMN dummy_seclabel_tbl1 IS 'unclassified'; -- fail SECURITY LABEL ON TABLE dummy_seclabel_tbl1 IS '...invalid label...'; -- fail SECURITY LABEL FOR 'dummy' ON TABLE dummy_seclabel_tbl1 IS 'unclassified'; -- OK SECURITY LABEL FOR 'unknown_seclabel' ON TABLE dummy_seclabel_tbl1 IS 'classified'; -- fail + +SET SESSION AUTHORIZATION regress_dummy_seclabel_user1; SECURITY LABEL ON TABLE dummy_seclabel_tbl2 IS 'unclassified'; -- fail (not owner) SECURITY LABEL ON TABLE dummy_seclabel_tbl1 IS 'secret'; -- fail (not superuser) SECURITY LABEL ON TABLE dummy_seclabel_tbl3 IS 'unclassified'; -- fail (not found) @@ -45,7 +51,7 @@ SECURITY LABEL ON TABLE dummy_seclabel_tbl2 IS 'classified'; -- OK -- -- Test for shared database object -- -SET SESSION AUTHORIZATION regress_dummy_seclabel_user1; +SET SESSION AUTHORIZATION regress_dummy_seclabel_user0; SECURITY LABEL ON ROLE regress_dummy_seclabel_user1 IS 'classified'; -- OK SECURITY LABEL ON ROLE regress_dummy_seclabel_user1 IS '...invalid label...'; -- fail diff --git a/src/test/regress/expected/create_role.out b/src/test/regress/expected/create_role.out index 385ee74342..a4eb19c954 100644 --- a/src/test/regress/expected/create_role.out +++ b/src/test/regress/expected/create_role.out @@ -58,21 +58,18 @@ CREATE TABLE regress_tbl_22 (i integer); CREATE INDEX regress_idx_22 ON regress_tbl_22(i); CREATE VIEW regress_view_22 AS SELECT * FROM pg_catalog.pg_class; REVOKE ALL PRIVILEGES ON regress_tbl_22 FROM PUBLIC; --- fail, these objects belonging to regress_role_22 +-- ok, owning role can manage owned role's objects SET SESSION AUTHORIZATION regress_role_7; DROP INDEX regress_idx_22; -ERROR: must be owner of index regress_idx_22 ALTER TABLE regress_tbl_22 ADD COLUMN t text; -ERROR: must be owner of table regress_tbl_22 DROP TABLE regress_tbl_22; -ERROR: must be owner of table regress_tbl_22 +-- fail, not a member of target role ALTER VIEW regress_view_22 OWNER TO regress_role_1; -ERROR: must be owner of view regress_view_22 +ERROR: must be member of role "regress_role_1" +-- ok DROP VIEW regress_view_22; -ERROR: must be owner of view regress_view_22 -- fail, cannot take ownership of these objects from regress_role_22 REASSIGN OWNED BY regress_role_22 TO regress_role_7; -ERROR: permission denied to reassign objects -- ok, having CREATEROLE is enough to create roles in privileged roles CREATE ROLE regress_role_23 IN ROLE pg_read_all_data; CREATE ROLE regress_role_24 IN ROLE pg_write_all_data; @@ -152,13 +149,8 @@ DROP ROLE regress_role_31; DROP ROLE regress_role_32; -- fail, role still owns database objects DROP ROLE regress_role_22; -ERROR: role "regress_role_22" cannot be dropped because some objects depend on it -DETAIL: owner of table regress_tbl_22 -owner of view regress_view_22 -- fail, role still owns other roles DROP ROLE regress_role_7; -ERROR: role "regress_role_7" cannot be dropped because some objects depend on it -DETAIL: owner of role regress_role_22 -- fail, cannot drop ourself nor superusers DROP ROLE regress_role_super; ERROR: must be superuser to drop superusers @@ -166,10 +158,5 @@ DROP ROLE regress_role_1; ERROR: current user cannot be dropped -- ok RESET SESSION AUTHORIZATION; -DROP INDEX regress_idx_22; -DROP TABLE regress_tbl_22; -DROP VIEW regress_view_22; -DROP ROLE regress_role_22; -DROP ROLE regress_role_7; DROP ROLE regress_role_1; DROP ROLE regress_role_super; diff --git a/src/test/regress/sql/create_role.sql b/src/test/regress/sql/create_role.sql index 728208f1e1..df8d34a8d5 100644 --- a/src/test/regress/sql/create_role.sql +++ b/src/test/regress/sql/create_role.sql @@ -63,12 +63,16 @@ CREATE INDEX regress_idx_22 ON regress_tbl_22(i); CREATE VIEW regress_view_22 AS SELECT * FROM pg_catalog.pg_class; REVOKE ALL PRIVILEGES ON regress_tbl_22 FROM PUBLIC; --- fail, these objects belonging to regress_role_22 +-- ok, owning role can manage owned role's objects SET SESSION AUTHORIZATION regress_role_7; DROP INDEX regress_idx_22; ALTER TABLE regress_tbl_22 ADD COLUMN t text; DROP TABLE regress_tbl_22; + +-- fail, not a member of target role ALTER VIEW regress_view_22 OWNER TO regress_role_1; + +-- ok DROP VIEW regress_view_22; -- fail, cannot take ownership of these objects from regress_role_22 @@ -135,10 +139,5 @@ DROP ROLE regress_role_1; -- ok RESET SESSION AUTHORIZATION; -DROP INDEX regress_idx_22; -DROP TABLE regress_tbl_22; -DROP VIEW regress_view_22; -DROP ROLE regress_role_22; -DROP ROLE regress_role_7; DROP ROLE regress_role_1; DROP ROLE regress_role_super; -- 2.21.1 (Apple Git-122.3)