From ef792f7856dea2576dcd9cab92b2b05fe955696b Mon Sep 17 00:00:00 2001 From: Noah Misch Date: Mon, 9 May 2022 08:35:08 -0700 Subject: [PATCH] Make relation-enumerating operations be security-restricted operations. When a feature enumerates relations and runs functions associated with all found relations, the feature's user shall not need to trust every user having permission to create objects. BRIN-specific functionality in autovacuum neglected to account for this, as did pg_amcheck and CLUSTER. An attacker having permission to create non-temp objects in at least one schema could execute arbitrary SQL functions under the identity of the bootstrap superuser. CREATE INDEX (not a relation-enumerating operation) and REINDEX protected themselves too late. This change extends to the non-enumerating amcheck interface. Back-patch to v10 (all supported versions). Sergey Shinderuk, reviewed (in earlier versions) by Alexander Lakhin. Reported by Alexander Lakhin. Security: CVE-2022-1552 --- src/backend/catalog/index.c | 41 +++++++++++++++++++-------- src/backend/commands/cluster.c | 35 ++++++++++++++++++----- src/backend/commands/indexcmds.c | 47 +++++++++++++++++++++++++++++-- src/backend/utils/init/miscinit.c | 24 +++++++++------ src/test/regress/expected/privileges.out | 35 +++++++++++++++++++++++ src/test/regress/sql/privileges.sql | 34 ++++++++++++++++++++++ 6 files changed, 187 insertions(+), 29 deletions(-) --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -2743,7 +2743,17 @@ /* Open and lock the parent heap relation */ heapRelation = heap_open(heapId, ShareUpdateExclusiveLock); - /* And the target index relation */ + + /* + * Switch to the table owner's userid, so that any index functions are run + * as that user. Also lock down security-restricted operations and + * arrange to make GUC variable changes local to this command. + */ + GetUserIdAndSecContext(&save_userid, &save_sec_context); + SetUserIdAndSecContext(heapRelation->rd_rel->relowner, + save_sec_context | SECURITY_RESTRICTED_OPERATION); + save_nestlevel = NewGUCNestLevel(); + indexRelation = index_open(indexId, RowExclusiveLock); /* @@ -2757,16 +2767,6 @@ indexInfo->ii_Concurrent = true; /* - * Switch to the table owner's userid, so that any index functions are run - * as that user. Also lock down security-restricted operations and - * arrange to make GUC variable changes local to this command. - */ - GetUserIdAndSecContext(&save_userid, &save_sec_context); - SetUserIdAndSecContext(heapRelation->rd_rel->relowner, - save_sec_context | SECURITY_RESTRICTED_OPERATION); - save_nestlevel = NewGUCNestLevel(); - - /* * Scan the index and gather up all the TIDs into a tuplesort object. */ ivinfo.index = indexRelation; @@ -3178,6 +3178,9 @@ Relation iRel, heapRelation; Oid heapId; + Oid save_userid; + int save_sec_context; + int save_nestlevel; IndexInfo *indexInfo; volatile bool skipped_constraint = false; @@ -3189,6 +3192,16 @@ heapRelation = heap_open(heapId, ShareLock); /* + * Switch to the table owner's userid, so that any index functions are run + * as that user. Also lock down security-restricted operations and + * arrange to make GUC variable changes local to this command. + */ + GetUserIdAndSecContext(&save_userid, &save_sec_context); + SetUserIdAndSecContext(heapRelation->rd_rel->relowner, + save_sec_context | SECURITY_RESTRICTED_OPERATION); + save_nestlevel = NewGUCNestLevel(); + + /* * Open the target index relation and get an exclusive lock on it, to * ensure that no one else is touching this particular index. */ @@ -3324,6 +3337,12 @@ heap_close(pg_index, RowExclusiveLock); } + /* Roll back any GUC changes executed by index functions */ + AtEOXact_GUC(false, save_nestlevel); + + /* Restore userid and security context */ + SetUserIdAndSecContext(save_userid, save_sec_context); + /* Close rels, but keep locks */ index_close(iRel, NoLock); heap_close(heapRelation, NoLock); --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -41,6 +41,7 @@ #include "storage/smgr.h" #include "utils/acl.h" #include "utils/fmgroids.h" +#include "utils/guc.h" #include "utils/inval.h" #include "utils/lsyscache.h" #include "utils/memutils.h" @@ -259,6 +260,9 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose) { Relation OldHeap; + Oid save_userid; + int save_sec_context; + int save_nestlevel; /* Check for user-requested abort. */ CHECK_FOR_INTERRUPTS(); @@ -276,6 +280,16 @@ return; /* + * Switch to the table owner's userid, so that any index functions are run + * as that user. Also lock down security-restricted operations and + * arrange to make GUC variable changes local to this command. + */ + GetUserIdAndSecContext(&save_userid, &save_sec_context); + SetUserIdAndSecContext(OldHeap->rd_rel->relowner, + save_sec_context | SECURITY_RESTRICTED_OPERATION); + save_nestlevel = NewGUCNestLevel(); + + /* * Since we may open a new transaction for each relation, we have to check * that the relation still is what we think it is. * @@ -289,10 +303,10 @@ Form_pg_index indexForm; /* Check that the user still owns the relation */ - if (!pg_class_ownercheck(tableOid, GetUserId())) + if (!pg_class_ownercheck(tableOid, save_userid)) { relation_close(OldHeap, AccessExclusiveLock); - return; + goto out; } /* @@ -306,7 +320,7 @@ if (RELATION_IS_OTHER_TEMP(OldHeap)) { relation_close(OldHeap, AccessExclusiveLock); - return; + goto out; } if (OidIsValid(indexOid)) @@ -317,7 +331,7 @@ if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(indexOid))) { relation_close(OldHeap, AccessExclusiveLock); - return; + goto out; } /* @@ -327,14 +341,14 @@ if (!HeapTupleIsValid(tuple)) /* probably can't happen */ { relation_close(OldHeap, AccessExclusiveLock); - return; + goto out; } indexForm = (Form_pg_index) GETSTRUCT(tuple); if (!indexForm->indisclustered) { ReleaseSysCache(tuple); relation_close(OldHeap, AccessExclusiveLock); - return; + goto out; } ReleaseSysCache(tuple); } @@ -388,7 +402,7 @@ !RelationIsPopulated(OldHeap)) { relation_close(OldHeap, AccessExclusiveLock); - return; + goto out; } /* @@ -403,6 +417,13 @@ rebuild_relation(OldHeap, indexOid, verbose); /* NB: rebuild_relation does heap_close() on OldHeap */ + +out: + /* Roll back any GUC changes executed by index functions */ + AtEOXact_GUC(false, save_nestlevel); + + /* Restore userid and security context */ + SetUserIdAndSecContext(save_userid, save_sec_context); } /* --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -44,6 +44,7 @@ #include "utils/acl.h" #include "utils/builtins.h" #include "utils/fmgroids.h" +#include "utils/guc.h" #include "utils/inval.h" #include "utils/lsyscache.h" #include "utils/memutils.h" @@ -329,8 +330,13 @@ LOCKTAG heaplocktag; LOCKMODE lockmode; Snapshot snapshot; + Oid root_save_userid; + int root_save_sec_context; + int root_save_nestlevel; int i; + root_save_nestlevel = NewGUCNestLevel(); + /* * Force non-concurrent build on temporary relations, even if CONCURRENTLY * was requested. Other backends can't access a temporary relation, so @@ -371,6 +377,15 @@ lockmode = concurrent ? ShareUpdateExclusiveLock : ShareLock; rel = heap_open(relationId, lockmode); + /* + * Switch to the table owner's userid, so that any index functions are run + * as that user. Also lock down security-restricted operations. We + * already arranged to make GUC variable changes local to this command. + */ + GetUserIdAndSecContext(&root_save_userid, &root_save_sec_context); + SetUserIdAndSecContext(rel->rd_rel->relowner, + root_save_sec_context | SECURITY_RESTRICTED_OPERATION); + relationId = RelationGetRelid(rel); namespaceId = RelationGetNamespace(rel); @@ -412,7 +427,7 @@ { AclResult aclresult; - aclresult = pg_namespace_aclcheck(namespaceId, GetUserId(), + aclresult = pg_namespace_aclcheck(namespaceId, root_save_userid, ACL_CREATE); if (aclresult != ACLCHECK_OK) aclcheck_error(aclresult, ACL_KIND_NAMESPACE, @@ -439,7 +454,7 @@ { AclResult aclresult; - aclresult = pg_tablespace_aclcheck(tablespaceId, GetUserId(), + aclresult = pg_tablespace_aclcheck(tablespaceId, root_save_userid, ACL_CREATE); if (aclresult != ACLCHECK_OK) aclcheck_error(aclresult, ACL_KIND_TABLESPACE, @@ -622,11 +637,23 @@ skip_build || concurrent, concurrent, !check_rights); + /* + * Roll back any GUC changes executed by index functions, and keep + * subsequent changes local to this command. It's barely possible that + * some index function changed a behavior-affecting GUC, e.g. xmloption, + * that affects subsequent steps. + */ + AtEOXact_GUC(false, root_save_nestlevel); + root_save_nestlevel = NewGUCNestLevel(); + /* Add any requested comment */ if (stmt->idxcomment != NULL) CreateComments(indexRelationId, RelationRelationId, 0, stmt->idxcomment); + AtEOXact_GUC(false, root_save_nestlevel); + SetUserIdAndSecContext(root_save_userid, root_save_sec_context); + if (!concurrent) { /* Close the heap and we're done, in the non-concurrent case */ @@ -705,6 +732,16 @@ /* Open and lock the parent heap relation */ rel = heap_openrv(stmt->relation, ShareUpdateExclusiveLock); + /* + * Switch to the table owner's userid, so that any index functions are run + * as that user. Also lock down security-restricted operations and + * arrange to make GUC variable changes local to this command. + */ + GetUserIdAndSecContext(&root_save_userid, &root_save_sec_context); + SetUserIdAndSecContext(rel->rd_rel->relowner, + root_save_sec_context | SECURITY_RESTRICTED_OPERATION); + root_save_nestlevel = NewGUCNestLevel(); + /* And the target index relation */ indexRelation = index_open(indexRelationId, RowExclusiveLock); @@ -720,6 +757,12 @@ /* Now build the index */ index_build(rel, indexRelation, indexInfo, stmt->primary, false); + /* Roll back any GUC changes executed by index functions */ + AtEOXact_GUC(false, root_save_nestlevel); + + /* Restore userid and security context */ + SetUserIdAndSecContext(root_save_userid, root_save_sec_context); + /* Close both the relations, but keep the locks */ heap_close(rel, NoLock); index_close(indexRelation, NoLock); --- a/src/backend/utils/init/miscinit.c +++ b/src/backend/utils/init/miscinit.c @@ -235,15 +235,21 @@ * with guc.c's internal state, so SET ROLE has to be disallowed. * * SECURITY_RESTRICTED_OPERATION indicates that we are inside an operation - * that does not wish to trust called user-defined functions at all. This - * bit prevents not only SET ROLE, but various other changes of session state - * that normally is unprotected but might possibly be used to subvert the - * calling session later. An example is replacing an existing prepared - * statement with new code, which will then be executed with the outer - * session's permissions when the prepared statement is next used. Since - * these restrictions are fairly draconian, we apply them only in contexts - * where the called functions are really supposed to be side-effect-free - * anyway, such as VACUUM/ANALYZE/REINDEX. + * that does not wish to trust called user-defined functions at all. The + * policy is to use this before operations, e.g. autovacuum and REINDEX, that + * enumerate relations of a database or schema and run functions associated + * with each found relation. The relation owner is the new user ID. Set this + * as soon as possible after locking the relation. Restore the old user ID as + * late as possible before closing the relation; restoring it shortly after + * close is also tolerable. If a command has both relation-enumerating and + * non-enumerating modes, e.g. ANALYZE, both modes set this bit. This bit + * prevents not only SET ROLE, but various other changes of session state that + * normally is unprotected but might possibly be used to subvert the calling + * session later. An example is replacing an existing prepared statement with + * new code, which will then be executed with the outer session's permissions + * when the prepared statement is next used. These restrictions are fairly + * draconian, but the functions called in relation-enumerating operations are + * really supposed to be side-effect-free anyway. * * Unlike GetUserId, GetUserIdAndSecContext does *not* Assert that the current * value of CurrentUserId is valid; nor does SetUserIdAndSecContext require --- a/src/test/regress/expected/privileges.out +++ b/src/test/regress/expected/privileges.out @@ -1194,6 +1194,41 @@ -- security-restricted operations \c - CREATE ROLE regress_sro_user; +-- Check that index expressions and predicates are run as the table's owner +-- A dummy index function checking current_user +CREATE FUNCTION sro_ifun(int) RETURNS int AS $$ +BEGIN + -- Below we set the table's owner to regress_sro_user + IF current_user <> 'regress_sro_user' THEN + RAISE 'called by %', current_user; + END IF; + RETURN $1; +END; +$$ LANGUAGE plpgsql IMMUTABLE; +-- Create a table owned by regress_sro_user +CREATE TABLE sro_tab (a int); +ALTER TABLE sro_tab OWNER TO regress_sro_user; +INSERT INTO sro_tab VALUES (1), (2), (3); +-- Create an expression index with a predicate +CREATE INDEX sro_idx ON sro_tab ((sro_ifun(a) + sro_ifun(0))) + WHERE sro_ifun(a + 10) > sro_ifun(10); +DROP INDEX sro_idx; +-- Do the same concurrently +CREATE INDEX CONCURRENTLY sro_idx ON sro_tab ((sro_ifun(a) + sro_ifun(0))) + WHERE sro_ifun(a + 10) > sro_ifun(10); +-- REINDEX +REINDEX TABLE sro_tab; +REINDEX INDEX sro_idx; +REINDEX TABLE CONCURRENTLY sro_tab; -- v12+ feature +ERROR: syntax error at or near "CONCURRENTLY" +LINE 1: REINDEX TABLE CONCURRENTLY sro_tab; + ^ +DROP INDEX sro_idx; +-- CLUSTER +CREATE INDEX sro_cluster_idx ON sro_tab ((sro_ifun(a) + sro_ifun(0))); +CLUSTER sro_tab USING sro_cluster_idx; +DROP INDEX sro_cluster_idx; +DROP TABLE sro_tab; SET SESSION AUTHORIZATION regress_sro_user; CREATE FUNCTION unwanted_grant() RETURNS void LANGUAGE sql AS 'GRANT regressgroup2 TO regress_sro_user'; --- a/src/test/regress/sql/privileges.sql +++ b/src/test/regress/sql/privileges.sql @@ -724,6 +724,40 @@ \c - CREATE ROLE regress_sro_user; +-- Check that index expressions and predicates are run as the table's owner + +-- A dummy index function checking current_user +CREATE FUNCTION sro_ifun(int) RETURNS int AS $$ +BEGIN + -- Below we set the table's owner to regress_sro_user + IF current_user <> 'regress_sro_user' THEN + RAISE 'called by %', current_user; + END IF; + RETURN $1; +END; +$$ LANGUAGE plpgsql IMMUTABLE; +-- Create a table owned by regress_sro_user +CREATE TABLE sro_tab (a int); +ALTER TABLE sro_tab OWNER TO regress_sro_user; +INSERT INTO sro_tab VALUES (1), (2), (3); +-- Create an expression index with a predicate +CREATE INDEX sro_idx ON sro_tab ((sro_ifun(a) + sro_ifun(0))) + WHERE sro_ifun(a + 10) > sro_ifun(10); +DROP INDEX sro_idx; +-- Do the same concurrently +CREATE INDEX CONCURRENTLY sro_idx ON sro_tab ((sro_ifun(a) + sro_ifun(0))) + WHERE sro_ifun(a + 10) > sro_ifun(10); +-- REINDEX +REINDEX TABLE sro_tab; +REINDEX INDEX sro_idx; +REINDEX TABLE CONCURRENTLY sro_tab; -- v12+ feature +DROP INDEX sro_idx; +-- CLUSTER +CREATE INDEX sro_cluster_idx ON sro_tab ((sro_ifun(a) + sro_ifun(0))); +CLUSTER sro_tab USING sro_cluster_idx; +DROP INDEX sro_cluster_idx; +DROP TABLE sro_tab; + SET SESSION AUTHORIZATION regress_sro_user; CREATE FUNCTION unwanted_grant() RETURNS void LANGUAGE sql AS 'GRANT regressgroup2 TO regress_sro_user';