From 3e65299a205b00c81e6c34958b425be53564001f Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot Date: Sat, 30 May 2026 14:35:40 +0000 Subject: [PATCH v26] Recheck permissions after lock acquisition in dependency recording After dependencyLockAndCheckObject() acquires a lock on a referenced object (which may have blocked on a concurrent DROP), re-verify any permission check that was done earlier in the statement. This closes the TOCTOU window where a REVOKE could land between the original permission check and the dependency recording. The approach: record SharedInvalidMessageCounter at the time of the original aclcheck. After acquiring the lock and verifying the object still exists, compare the saved counter with the current value. If it changed (meaning catalog invalidations arrived since the original check), re-verify the permission. Since the OID is fixed (no name re-resolution), a failure is definitive and no retry loop is needed. The tracking array lives in a dedicated AclCheckTrackContext memory context (child of TopMemoryContext). The context is reset whenever tracking is initialized (at the outermost ProcessUtility call that starts tracking), which frees all prior allocations and provides clean lifetime management. Recording is gated by aclcheck_tracking_active, which is set to true at the first ProcessUtility call and cleared on its return or in AbortTransaction. This ensures DML and queries pay no cost. aclcheck_tracked_count is saved on entry and restored on return of each ProcessUtility call, so that each nesting level's entries are kept separate. This prevents nested calls from polluting the outer level's tracked entries and ensures the outer DDL's recheck still finds its own tracked entries after a nested call returns. Alternatives considered: - To avoid allocating memory for each statement, keep the array in TopMemoryContext and never free it (only resetting the count). But that left the high-water mark allocated for the lifetime of the backend. - Passing privilege info (roleId, mode) as extra arguments through the dependency recording APIs (recordDependencyOn, recordMultipleDependencies, etc.) was discarded because expression-based dependencies (recordDependencyOnExpr, find_expr_references_walker) discover objects by walking expression trees: the caller never sees individual objects and cannot attach privilege info to them. Author: Bertrand Drouvot Reviewed-by: Jeff Davis Discussion: https://postgr.es/m/ZiYjn0eVc7pxVY45@ip-10-97-1-34.eu-west-3.compute.internal --- src/backend/access/transam/xact.c | 4 + src/backend/catalog/aclchk.c | 52 +++++++ src/backend/catalog/pg_depend.c | 113 +++++++++++--- src/backend/tcop/utility.c | 28 ++++ src/include/catalog/aclcheck_track.h | 69 +++++++++ .../expected/ddl-dependency-locking.out | 64 +++++++- .../specs/ddl-dependency-locking.spec | 140 ++++++++++++++++++ src/tools/pgindent/typedefs.list | 1 + 8 files changed, 450 insertions(+), 21 deletions(-) create mode 100644 src/include/catalog/aclcheck_track.h diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 5586fbe5b07..1a6088bf2f7 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -32,6 +32,7 @@ #include "access/xlogrecovery.h" #include "access/xlogutils.h" #include "access/xlogwait.h" +#include "catalog/aclcheck_track.h" #include "catalog/index.h" #include "catalog/namespace.h" #include "catalog/pg_enum.h" @@ -2961,6 +2962,9 @@ AbortTransaction(void) s->parallelModeLevel = 0; s->parallelChildXact = false; /* should be false already */ + /* Reset aclcheck tracking state */ + aclcheck_tracking_active = false; + /* * do abort processing */ diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 007ede997c5..2a3f1ebac5f 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -44,6 +44,7 @@ #include "access/sysattr.h" #include "access/tableam.h" #include "access/xact.h" +#include "catalog/aclcheck_track.h" #include "catalog/binary_upgrade.h" #include "catalog/catalog.h" #include "catalog/dependency.h" @@ -81,9 +82,19 @@ #include "utils/fmgroids.h" #include "utils/guc.h" #include "utils/lsyscache.h" +#include "utils/memutils.h" #include "utils/rel.h" #include "utils/syscache.h" +#define ACLCHECK_TRACK_INITIAL_SIZE 64 + +static MemoryContext AclCheckTrackContext = NULL; + +AclCheckEntry *aclcheck_tracked = NULL; +int aclcheck_tracked_count = 0; +int aclcheck_tracked_max = 0; +bool aclcheck_tracking_active = false; + /* * Internal format used by ALTER DEFAULT PRIVILEGES. */ @@ -3891,6 +3902,7 @@ object_aclcheck_ext(Oid classid, Oid objectid, Oid roleid, AclMode mode, bool *is_missing) { + aclcheck_track_record(classid, objectid, roleid, mode); if (object_aclmask_ext(classid, objectid, roleid, mode, ACLMASK_ANY, is_missing) != 0) return ACLCHECK_OK; @@ -4093,6 +4105,7 @@ AclResult pg_class_aclcheck_ext(Oid table_oid, Oid roleid, AclMode mode, bool *is_missing) { + aclcheck_track_record(RelationRelationId, table_oid, roleid, mode); if (pg_class_aclmask_ext(table_oid, roleid, mode, ACLMASK_ANY, is_missing) != 0) return ACLCHECK_OK; @@ -5036,3 +5049,42 @@ RemoveRoleFromInitPriv(Oid roleid, Oid classid, Oid objid, int32 objsubid) table_close(rel, RowExclusiveLock); } + +/* + * Reset the tracking array. Called at each top-level ProcessUtility() + * to avoid carrying stale entries from a previous statement. + * Resets the memory context, freeing all prior allocations. + */ +void +aclcheck_track_reset(void) +{ + if (AclCheckTrackContext == NULL) + { + AclCheckTrackContext = AllocSetContextCreate(TopMemoryContext, + "AclCheckTrackContext", + ALLOCSET_DEFAULT_SIZES); + } + else + { + MemoryContextReset(AclCheckTrackContext); + } + + aclcheck_tracked = (AclCheckEntry *) + MemoryContextAlloc(AclCheckTrackContext, + ACLCHECK_TRACK_INITIAL_SIZE * sizeof(AclCheckEntry)); + aclcheck_tracked_max = ACLCHECK_TRACK_INITIAL_SIZE; + aclcheck_tracked_count = 0; + aclcheck_tracking_active = true; +} + +/* + * Grow the tracking array when full. Doubles the size each time. + */ +void +aclcheck_track_grow(void) +{ + aclcheck_tracked_max *= 2; + aclcheck_tracked = (AclCheckEntry *) + repalloc(aclcheck_tracked, + aclcheck_tracked_max * sizeof(AclCheckEntry)); +} diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c index 9a7a401aced..8a4c2c9bb02 100644 --- a/src/backend/catalog/pg_depend.c +++ b/src/backend/catalog/pg_depend.c @@ -17,6 +17,7 @@ #include "access/genam.h" #include "access/htup_details.h" #include "access/table.h" +#include "catalog/aclcheck_track.h" #include "catalog/catalog.h" #include "catalog/dependency.h" #include "catalog/indexing.h" @@ -29,6 +30,8 @@ #include "miscadmin.h" #include "storage/lmgr.h" #include "storage/lock.h" +#include "storage/sinval.h" +#include "utils/acl.h" #include "utils/fmgroids.h" #include "utils/lsyscache.h" #include "utils/rel.h" @@ -38,6 +41,7 @@ static bool isObjectPinned(const ObjectAddress *object); static void dependencyLockAndCheckObject(Oid classId, Oid objectId); +static void recheckAcl(Oid classId, Oid objectId); /* @@ -732,21 +736,74 @@ isObjectPinned(const ObjectAddress *object) } +/* + * recheckAcl() + * + * Re-verify all tracked permission checks on the given object. + * + * Called after acquiring the lock and verifying the object still exists. + * If permission checks were tracked for this object and catalog + * invalidations arrived since the original checks, re-verify them all. + * This closes the TOCTOU window where a REVOKE could land between the + * original permission check and the dependency recording. + * + * Multiple checks may have been done on the same object with different + * modes or roles, so we iterate through all matching entries. + * + * Since we don't re-resolve names (the OID is fixed), there is no need for + * a retry loop here. If a recheck fails, it's a definitive failure. + * + * The linear scan over the tracking array is O(n) in the number of tracked + * entries, but that's fine since a typical DDL statement tracks only a few + * objects. + */ +static void +recheckAcl(Oid classId, Oid objectId) +{ + for (int i = aclcheck_tracked_count - 1; i >= 0; i--) + { + if (aclcheck_tracked[i].classId == classId && + aclcheck_tracked[i].objectId == objectId && + aclcheck_tracked[i].inval_count != SharedInvalidMessageCounter) + { + AclResult aclresult; + + if (classId == RelationRelationId) + aclresult = pg_class_aclcheck(objectId, + aclcheck_tracked[i].roleId, + aclcheck_tracked[i].mode); + else + aclresult = object_aclcheck(classId, objectId, + aclcheck_tracked[i].roleId, + aclcheck_tracked[i].mode); + + if (aclresult != ACLCHECK_OK) + { + ObjectAddress object; + + object.classId = classId; + object.objectId = objectId; + object.objectSubId = 0; + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied for %s", + getObjectDescription(&object, false)))); + } + } + } +} + + /* * dependencyLockAndCheckObject * - * Lock the object that we are about to record a dependency on. After it's - * locked, verify that it hasn't been dropped while we weren't looking. If it - * has been dropped, throw an an error. + * Lock the object that we are about to record a dependency on. After it's + * locked, verify that it hasn't been dropped while we weren't looking. If it + * has been dropped, throw an error. Then re-verify any tracked permission + * check to close the TOCTOU window. * * If the caller already holds a lock that conflicts with DROP - * (AccessShareLock or stronger), this does nothing. Callers should acquire - * locks already when they look up the referenced objects, but many callers - * currently do not. This is a backstop to make sure that we don't record a - * bogus reference permanently in the catalogs in that case. In the future, - * after we have tightened up all the callers to acquire locks earlier, this - * could just verify that the object is already locked and throw an error if - * not. + * (AccessShareLock or stronger), this skips lock acquisition entirely. */ static void dependencyLockAndCheckObject(Oid classId, Oid objectId) @@ -773,20 +830,25 @@ dependencyLockAndCheckObject(Oid classId, Oid objectId) 0); if (LockHeldByMe(&tag, AccessShareLock, true)) + { + /* Recheck ACL */ + recheckAcl(classId, objectId); return; + } - /* Assume we should lock the whole object not a sub-object */ + /* Acquire lock */ LockDatabaseObject(classId, objectId, 0, AccessShareLock); - /* - * Check that the object still exists. If the catalog has a suitable - * syscache, check that first. - */ + /* Check that the object still exists */ cache = get_object_catcache_oid(classId); if (cache != SYSCACHEID_INVALID) { if (SearchSysCacheExists1(cache, ObjectIdGetDatum(objectId))) + { + /* Recheck ACL */ + recheckAcl(classId, objectId); return; + } } /* @@ -814,6 +876,9 @@ dependencyLockAndCheckObject(Oid classId, Oid objectId) systable_endscan(scan); table_close(rel, AccessShareLock); + + /* Recheck ACL */ + recheckAcl(classId, objectId); } else { @@ -834,14 +899,22 @@ dependencyLockAndCheckObject(Oid classId, Oid objectId) Assert(!IsSharedRelation(objectId)); if (CheckRelationOidLockedByMe(objectId, AccessShareLock, true)) + { + recheckAcl(classId, objectId); return; + } + + /* Acquire lock */ LockRelationOid(objectId, AccessShareLock); - if (SearchSysCacheExists1(RELOID, ObjectIdGetDatum(objectId))) - return; - ereport(ERROR, - (errcode(ERRCODE_UNDEFINED_OBJECT), - errmsg("referenced relation was concurrently dropped"))); + /* Check that the object still exists */ + if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(objectId))) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("referenced relation was concurrently dropped"))); + + /* Recheck ACL */ + recheckAcl(classId, objectId); } } diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index 73a56f1df1d..0d0f48715b5 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -20,6 +20,7 @@ #include "access/twophase.h" #include "access/xact.h" #include "access/xlog.h" +#include "catalog/aclcheck_track.h" #include "catalog/namespace.h" #include "catalog/pg_authid.h" #include "catalog/pg_inherits.h" @@ -510,11 +511,29 @@ ProcessUtility(PlannedStmt *pstmt, DestReceiver *dest, QueryCompletion *qc) { + int save_aclcheck_count; + bool was_tracking = aclcheck_tracking_active; + Assert(IsA(pstmt, PlannedStmt)); Assert(pstmt->commandType == CMD_UTILITY); Assert(queryString != NULL); /* required as of 8.4 */ Assert(qc == NULL || qc->commandTag == CMDTAG_UNKNOWN); + /* + * Set up aclcheck tracking. For the top-level call, we initialize. For + * nested calls, we save the current position so we can discard entries + * from this level on return preserving the outer level's entries. + */ + if (!was_tracking) + { + aclcheck_track_reset(); + save_aclcheck_count = 0; + } + else + { + save_aclcheck_count = aclcheck_tracked_count; + } + /* * We provide a function hook variable that lets loadable plugins get * control when ProcessUtility is called. Such a plugin would normally @@ -528,6 +547,15 @@ ProcessUtility(PlannedStmt *pstmt, standard_ProcessUtility(pstmt, queryString, readOnlyTree, context, params, queryEnv, dest, qc); + + /* + * Restore the tracked entry count, discarding entries added at this + * nesting level while preserving outer levels' entries. + */ + aclcheck_tracked_count = save_aclcheck_count; + + if (!was_tracking) + aclcheck_tracking_active = false; } /* diff --git a/src/include/catalog/aclcheck_track.h b/src/include/catalog/aclcheck_track.h new file mode 100644 index 00000000000..0df90ccf42d --- /dev/null +++ b/src/include/catalog/aclcheck_track.h @@ -0,0 +1,69 @@ +/*------------------------------------------------------------------------- + * + * aclcheck_track.h + * Track permission checks for revalidation after lock acquisition + * in dependencyLockAndCheckObject(). + * + * DDL may perform ACL checks on referenced objects without first holding a lock + * on them. In that case, the lock is acquired much later, when recording + * dependencies. + * Track the ACL checks, so that we can re-check them after acquiring the + * lock while recording dependencies. + * + * XXX: consider refactoring so that we perform the name lookup, acquire the + * lock, and check ACLs all in unison, like RangeVarGetRelidExtended(). + * + * Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * src/include/catalog/aclcheck_track.h + * + *------------------------------------------------------------------------- + */ +#ifndef ACLCHECK_TRACK_H +#define ACLCHECK_TRACK_H + +#include "storage/sinval.h" +#include "utils/acl.h" + +typedef struct AclCheckEntry +{ + Oid classId; + Oid objectId; + Oid roleId; + AclMode mode; + uint64 inval_count; +} AclCheckEntry; + +extern AclCheckEntry *aclcheck_tracked; +extern int aclcheck_tracked_count; +extern int aclcheck_tracked_max; +extern bool aclcheck_tracking_active; + +extern void aclcheck_track_reset(void); +extern void aclcheck_track_grow(void); + +/* + * Record an aclcheck for later revalidation. + * + * Called from object_aclcheck_ext() and pg_class_aclcheck_ext(). + * Only records when inside an utility statement. + */ +static inline void +aclcheck_track_record(Oid classId, Oid objectId, Oid roleId, AclMode mode) +{ + if (!aclcheck_tracking_active) + return; + + if (aclcheck_tracked_count >= aclcheck_tracked_max) + aclcheck_track_grow(); + + aclcheck_tracked[aclcheck_tracked_count].classId = classId; + aclcheck_tracked[aclcheck_tracked_count].objectId = objectId; + aclcheck_tracked[aclcheck_tracked_count].roleId = roleId; + aclcheck_tracked[aclcheck_tracked_count].mode = mode; + aclcheck_tracked[aclcheck_tracked_count].inval_count = SharedInvalidMessageCounter; + aclcheck_tracked_count++; +} + +#endif /* ACLCHECK_TRACK_H */ diff --git a/src/test/isolation/expected/ddl-dependency-locking.out b/src/test/isolation/expected/ddl-dependency-locking.out index 636de281022..77cc7489170 100644 --- a/src/test/isolation/expected/ddl-dependency-locking.out +++ b/src/test/isolation/expected/ddl-dependency-locking.out @@ -1,4 +1,4 @@ -Parsed test spec with 2 sessions +Parsed test spec with 3 sessions starting permutation: s1_begin s1_create_function_in_schema s2_drop_schema s1_commit step s1_begin: BEGIN; @@ -135,3 +135,65 @@ step s2_drop_role: DROP ROLE regress_dependency; step s1_commit: COMMIT; step s2_drop_role: <... completed> ERROR: role "regress_dependency" cannot be dropped because some objects depend on it + +starting permutation: s2_begin s2_drop_fdw_wrapper s1_create_server_as_role_user s3_revoke_role s2_rollback +step s2_begin: BEGIN; +step s2_drop_fdw_wrapper: DROP FOREIGN DATA WRAPPER fdw_wrapper RESTRICT; +step s1_create_server_as_role_user: SET ROLE role_user; CREATE SERVER srv_role_revoked FOREIGN DATA WRAPPER fdw_wrapper; RESET ROLE; +step s3_revoke_role: REVOKE role_fdw FROM role_user; +step s2_rollback: ROLLBACK; +step s1_create_server_as_role_user: <... completed> +ERROR: permission denied for foreign-data wrapper fdw_wrapper + +starting permutation: s2_begin s2_drop_fdw_spi_func s1_create_server_spi_func s3_revoke_role s2_rollback +step s2_begin: BEGIN; +step s2_drop_fdw_spi_func: DROP FOREIGN DATA WRAPPER fdw_spi_func RESTRICT; +step s1_create_server_spi_func: SET ROLE role_user; CREATE SERVER srv_spi_func FOREIGN DATA WRAPPER fdw_spi_func; RESET ROLE; +step s3_revoke_role: REVOKE role_fdw FROM role_user; +step s2_rollback: ROLLBACK; +step s1_create_server_spi_func: <... completed> +ERROR: permission denied for foreign-data wrapper fdw_spi_func + +starting permutation: s2_begin s2_drop_fdw_spi_rollback s1_create_server_spi_rollback s3_revoke_role s2_rollback +step s2_begin: BEGIN; +step s2_drop_fdw_spi_rollback: DROP FOREIGN DATA WRAPPER fdw_spi_rollback RESTRICT; +step s1_create_server_spi_rollback: SET ROLE role_user; CREATE SERVER srv_spi_rollback FOREIGN DATA WRAPPER fdw_spi_rollback; RESET ROLE; +step s3_revoke_role: REVOKE role_fdw FROM role_user; +step s2_rollback: ROLLBACK; +step s1_create_server_spi_rollback: <... completed> +ERROR: permission denied for foreign-data wrapper fdw_spi_rollback + +starting permutation: s2_begin s2_drop_fdw_trigger s1_insert_trigger_ddl s3_revoke_role s2_rollback +step s2_begin: BEGIN; +step s2_drop_fdw_trigger: DROP FOREIGN DATA WRAPPER fdw_trigger RESTRICT; +step s1_insert_trigger_ddl: SET ROLE role_user; INSERT INTO trigger_tbl VALUES (1); RESET ROLE; +step s3_revoke_role: REVOKE role_fdw FROM role_user; +step s2_rollback: ROLLBACK; +step s1_insert_trigger_ddl: <... completed> +ERROR: permission denied for foreign-data wrapper fdw_trigger + +starting permutation: s2_begin s2_drop_fdw_trigger_spi s1_insert_trigger_spi_ddl s3_revoke_role s2_rollback +step s2_begin: BEGIN; +step s2_drop_fdw_trigger_spi: DROP FOREIGN DATA WRAPPER fdw_trigger_spi_func RESTRICT; +step s1_insert_trigger_spi_ddl: SET ROLE role_user; INSERT INTO trigger_spi_tbl VALUES (1); RESET ROLE; +step s3_revoke_role: REVOKE role_fdw FROM role_user; +step s2_rollback: ROLLBACK; +step s1_insert_trigger_spi_ddl: <... completed> +ERROR: permission denied for foreign-data wrapper fdw_trigger_spi_func + +starting permutation: s2_begin s2_drop_fdw_inner s1_create_server_nested_toctou s3_revoke_both_roles s2_rollback +step s2_begin: BEGIN; +step s2_drop_fdw_inner: DROP FOREIGN DATA WRAPPER fdw_inner RESTRICT; +step s1_create_server_nested_toctou: SET ROLE role_user; CREATE SERVER srv_outer FOREIGN DATA WRAPPER fdw_outer; RESET ROLE; +step s3_revoke_both_roles: REVOKE role_fdw, role_fdw_inner FROM role_user; +step s2_rollback: ROLLBACK; +step s1_create_server_nested_toctou: <... completed> +ERROR: permission denied for foreign-data wrapper fdw_inner + +starting permutation: s2_begin s2_drop_fdw_inner s1_create_server_nested_rollback s3_revoke_role_inner s2_rollback +step s2_begin: BEGIN; +step s2_drop_fdw_inner: DROP FOREIGN DATA WRAPPER fdw_inner RESTRICT; +step s1_create_server_nested_rollback: SET ROLE role_user; CREATE SERVER srv_outer_rollback FOREIGN DATA WRAPPER fdw_outer_rollback; RESET ROLE; +step s3_revoke_role_inner: REVOKE role_fdw_inner FROM role_user; +step s2_rollback: ROLLBACK; +step s1_create_server_nested_rollback: <... completed> diff --git a/src/test/isolation/specs/ddl-dependency-locking.spec b/src/test/isolation/specs/ddl-dependency-locking.spec index de5bd88d35e..72aa97c6dfd 100644 --- a/src/test/isolation/specs/ddl-dependency-locking.spec +++ b/src/test/isolation/specs/ddl-dependency-locking.spec @@ -11,7 +11,71 @@ setup CREATE FUNCTION f() RETURNS int LANGUAGE SQL RETURN 1; CREATE FUNCTION public.falter() RETURNS int LANGUAGE SQL RETURN 1; CREATE FOREIGN DATA WRAPPER fdw_wrapper; + CREATE ROLE role_fdw; + CREATE ROLE role_user LOGIN; + GRANT USAGE ON FOREIGN DATA WRAPPER fdw_wrapper TO role_fdw; + GRANT role_fdw TO role_user; CREATE ROLE regress_dependency; + CREATE FUNCTION spi_func(text[], oid) RETURNS void + LANGUAGE plpgsql AS $$ BEGIN EXECUTE 'CREATE TEMP TABLE IF NOT EXISTS validator_side_effect(x int)'; END; $$; + CREATE FUNCTION spi_func_rollback(text[], oid) RETURNS void + LANGUAGE plpgsql AS $$ + BEGIN + BEGIN + EXECUTE 'CREATE TEMP TABLE validator_will_rollback(x int)'; + RAISE EXCEPTION 'force rollback'; + EXCEPTION WHEN OTHERS THEN + NULL; + END; + END; $$; + CREATE FOREIGN DATA WRAPPER fdw_spi_func VALIDATOR spi_func; + CREATE FOREIGN DATA WRAPPER fdw_spi_rollback VALIDATOR spi_func_rollback; + GRANT USAGE ON FOREIGN DATA WRAPPER fdw_spi_func TO role_fdw; + GRANT USAGE ON FOREIGN DATA WRAPPER fdw_spi_rollback TO role_fdw; + CREATE FOREIGN DATA WRAPPER fdw_trigger; + GRANT USAGE ON FOREIGN DATA WRAPPER fdw_trigger TO role_fdw; + CREATE FUNCTION trg_create_server() RETURNS trigger + LANGUAGE plpgsql AS $$ BEGIN EXECUTE 'CREATE SERVER srv_from_trigger FOREIGN DATA WRAPPER fdw_trigger'; RETURN NEW; END; $$; + CREATE TABLE trigger_tbl(a int); + GRANT INSERT ON trigger_tbl TO role_user; + CREATE TRIGGER trg BEFORE INSERT ON trigger_tbl + FOR EACH ROW EXECUTE FUNCTION trg_create_server(); + CREATE FOREIGN DATA WRAPPER fdw_trigger_spi_func VALIDATOR spi_func; + GRANT USAGE ON FOREIGN DATA WRAPPER fdw_trigger_spi_func TO role_fdw; + CREATE FUNCTION trg_create_server_spi_func() RETURNS trigger + LANGUAGE plpgsql AS $$ BEGIN EXECUTE 'CREATE SERVER srv_from_trigger_spi FOREIGN DATA WRAPPER fdw_trigger_spi_func'; RETURN NEW; END; $$; + CREATE TABLE trigger_spi_tbl(a int); + GRANT INSERT ON trigger_spi_tbl TO role_user; + CREATE TRIGGER trg BEFORE INSERT ON trigger_spi_tbl + FOR EACH ROW EXECUTE FUNCTION trg_create_server_spi_func(); + CREATE FOREIGN DATA WRAPPER fdw_inner; + GRANT USAGE ON FOREIGN DATA WRAPPER fdw_inner TO role_fdw; + CREATE FUNCTION spi_func_create_server(text[], oid) RETURNS void + LANGUAGE plpgsql AS $$ + BEGIN + IF $2 = 'pg_catalog.pg_foreign_server'::regclass::oid THEN + EXECUTE 'CREATE SERVER srv_nested_inner FOREIGN DATA WRAPPER fdw_inner'; + END IF; + END; $$; + CREATE FOREIGN DATA WRAPPER fdw_outer VALIDATOR spi_func_create_server; + GRANT USAGE ON FOREIGN DATA WRAPPER fdw_outer TO role_fdw; + CREATE ROLE role_fdw_inner; + GRANT USAGE ON FOREIGN DATA WRAPPER fdw_inner TO role_fdw_inner; + GRANT role_fdw_inner TO role_user; + CREATE FUNCTION spi_func_create_server_rollback(text[], oid) RETURNS void + LANGUAGE plpgsql AS $$ + BEGIN + IF $2 = 'pg_catalog.pg_foreign_server'::regclass::oid THEN + BEGIN + EXECUTE 'CREATE SERVER srv_nested_rollback FOREIGN DATA WRAPPER fdw_inner'; + RAISE EXCEPTION 'force rollback'; + EXCEPTION WHEN OTHERS THEN + NULL; + END; + END IF; + END; $$; + CREATE FOREIGN DATA WRAPPER fdw_outer_rollback VALIDATOR spi_func_create_server_rollback; + GRANT USAGE ON FOREIGN DATA WRAPPER fdw_outer_rollback TO role_fdw; } teardown @@ -24,7 +88,19 @@ teardown DROP FUNCTION IF EXISTS alterschema.falter(); DROP DOMAIN IF EXISTS idid; DROP SERVER IF EXISTS srv_fdw_wrapper; + DROP SERVER IF EXISTS srv_role_revoked; + DROP SERVER IF EXISTS srv_spi_func; + DROP SERVER IF EXISTS srv_spi_rollback; + DROP SERVER IF EXISTS srv_from_trigger; + DROP SERVER IF EXISTS srv_from_trigger_spi; + DROP SERVER IF EXISTS srv_from_spi; + DROP SERVER IF EXISTS srv_nested_inner; + DROP SERVER IF EXISTS srv_nested_rollback; + DROP SERVER IF EXISTS srv_outer; + DROP SERVER IF EXISTS srv_outer_rollback; DROP TABLE IF EXISTS tabtype; + DROP TABLE IF EXISTS trigger_tbl; + DROP TABLE IF EXISTS trigger_spi_tbl; DROP SCHEMA IF EXISTS testschema; DROP SCHEMA IF EXISTS alterschema; DROP TYPE IF EXISTS public.foo; @@ -32,6 +108,22 @@ teardown DROP DOMAIN IF EXISTS id; DROP FUNCTION IF EXISTS f(); DROP FOREIGN DATA WRAPPER IF EXISTS fdw_wrapper; + DROP FOREIGN DATA WRAPPER IF EXISTS fdw_spi_func; + DROP FOREIGN DATA WRAPPER IF EXISTS fdw_spi_rollback; + DROP FOREIGN DATA WRAPPER IF EXISTS fdw_trigger; + DROP FOREIGN DATA WRAPPER IF EXISTS fdw_trigger_spi_func; + DROP FOREIGN DATA WRAPPER IF EXISTS fdw_inner; + DROP FOREIGN DATA WRAPPER IF EXISTS fdw_outer; + DROP FOREIGN DATA WRAPPER IF EXISTS fdw_outer_rollback; + DROP FUNCTION IF EXISTS spi_func(text[], oid); + DROP FUNCTION IF EXISTS spi_func_rollback(text[], oid); + DROP FUNCTION IF EXISTS spi_func_create_server(text[], oid); + DROP FUNCTION IF EXISTS spi_func_create_server_rollback(text[], oid); + DROP FUNCTION IF EXISTS trg_create_server(); + DROP FUNCTION IF EXISTS trg_create_server_spi_func(); + DROP ROLE IF EXISTS role_user; + DROP ROLE IF EXISTS role_fdw; + DROP ROLE IF EXISTS role_fdw_inner; DROP ROLE regress_dependency; } @@ -47,6 +139,13 @@ step "s1_alter_function_schema" { ALTER FUNCTION public.falter() SET SCHEMA alte step "s1_create_domain_with_domain" { CREATE DOMAIN idid as id; } step "s1_create_table_with_type" { CREATE TABLE tabtype(a footab); } step "s1_create_server_with_fdw_wrapper" { CREATE SERVER srv_fdw_wrapper FOREIGN DATA WRAPPER fdw_wrapper; } +step "s1_create_server_as_role_user" { SET ROLE role_user; CREATE SERVER srv_role_revoked FOREIGN DATA WRAPPER fdw_wrapper; RESET ROLE; } +step "s1_create_server_spi_func" { SET ROLE role_user; CREATE SERVER srv_spi_func FOREIGN DATA WRAPPER fdw_spi_func; RESET ROLE; } +step "s1_create_server_spi_rollback" { SET ROLE role_user; CREATE SERVER srv_spi_rollback FOREIGN DATA WRAPPER fdw_spi_rollback; RESET ROLE; } +step "s1_insert_trigger_ddl" { SET ROLE role_user; INSERT INTO trigger_tbl VALUES (1); RESET ROLE; } +step "s1_insert_trigger_spi_ddl" { SET ROLE role_user; INSERT INTO trigger_spi_tbl VALUES (1); RESET ROLE; } +step "s1_create_server_nested_toctou" { SET ROLE role_user; CREATE SERVER srv_outer FOREIGN DATA WRAPPER fdw_outer; RESET ROLE; } +step "s1_create_server_nested_rollback" { SET ROLE role_user; CREATE SERVER srv_outer_rollback FOREIGN DATA WRAPPER fdw_outer_rollback; RESET ROLE; } step "s1_commit" { COMMIT; } session "s2" @@ -60,8 +159,20 @@ step "s2_drop_footab_type" { DROP TYPE public.footab; } step "s2_drop_function_f" { DROP FUNCTION f(); } step "s2_drop_domain_id" { DROP DOMAIN id; } step "s2_drop_fdw_wrapper" { DROP FOREIGN DATA WRAPPER fdw_wrapper RESTRICT; } +step "s2_drop_fdw_spi_func" { DROP FOREIGN DATA WRAPPER fdw_spi_func RESTRICT; } +step "s2_drop_fdw_spi_rollback" { DROP FOREIGN DATA WRAPPER fdw_spi_rollback RESTRICT; } +step "s2_drop_fdw_trigger" { DROP FOREIGN DATA WRAPPER fdw_trigger RESTRICT; } +step "s2_drop_fdw_trigger_spi" { DROP FOREIGN DATA WRAPPER fdw_trigger_spi_func RESTRICT; } +step "s2_drop_fdw_inner" { DROP FOREIGN DATA WRAPPER fdw_inner RESTRICT; } step "s2_drop_role" { DROP ROLE regress_dependency; } step "s2_commit" { COMMIT; } +step "s2_rollback" { ROLLBACK; } + +session "s3" + +step "s3_revoke_role" { REVOKE role_fdw FROM role_user; } +step "s3_revoke_role_inner" { REVOKE role_fdw_inner FROM role_user; } +step "s3_revoke_both_roles" { REVOKE role_fdw, role_fdw_inner FROM role_user; } # create function - drop schema permutation "s1_begin" "s1_create_function_in_schema" "s2_drop_schema" "s1_commit" @@ -102,3 +213,32 @@ permutation "s1_begin" "s1_alter_function_owner" "s2_drop_role" "s1_commit" # was concurrently dropped", contains an OID that is not stable. # # permutation "s2_begin" "s2_drop_role" "s1_alter_function_owner" "s2_commit" + +# Role membership TOCTOU: permission via role revoked during lock wait. +permutation "s2_begin" "s2_drop_fdw_wrapper" "s1_create_server_as_role_user" "s3_revoke_role" "s2_rollback" + +# Role membership TOCTOU with SPI DDL in FDW validator. +permutation "s2_begin" "s2_drop_fdw_spi_func" "s1_create_server_spi_func" "s3_revoke_role" "s2_rollback" + +# Same as above but the validator's SPI DDL fails and rolls back its +# subtransaction. +permutation "s2_begin" "s2_drop_fdw_spi_rollback" "s1_create_server_spi_rollback" "s3_revoke_role" "s2_rollback" + +# Role membership TOCTOU with DDL triggered from DML: a trigger function does +# DDL via SPI during INSERT. +permutation "s2_begin" "s2_drop_fdw_trigger" "s1_insert_trigger_ddl" "s3_revoke_role" "s2_rollback" + +# Same as above but the DDL inside the trigger uses an FDW with a SPI validator, +# combining trigger-initiated DDL with nested SPI. +permutation "s2_begin" "s2_drop_fdw_trigger_spi" "s1_insert_trigger_spi_ddl" "s3_revoke_role" "s2_rollback" + +# TOCTOU on the nested SPI DDL itself: the validator creates a server using +# fdw_inner, session 2 blocks that by dropping fdw_inner, and the REVOKE +# removes access to fdw_inner. +permutation "s2_begin" "s2_drop_fdw_inner" "s1_create_server_nested_toctou" "s3_revoke_both_roles" "s2_rollback" + +# Rolled-back nested DDL should not cause the outer to fail: the validator +# tries to create a server using fdw_inner (via role_fdw_inner) but rolls back. +# The REVOKE removes role_fdw_inner, but the outer CREATE SERVER only needs +# role_fdw (for fdw_outer_rollback), so it should succeed. +permutation "s2_begin" "s2_drop_fdw_inner" "s1_create_server_nested_rollback" "s3_revoke_role_inner" "s2_rollback" diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 8cf40c87043..cb0cc25bda8 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -19,6 +19,7 @@ AbsoluteTime AccessMethodInfo AccessPriv Acl +AclCheckEntry AclItem AclMaskHow AclMode -- 2.47.3