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/access/brin/brin.c | 30 ++++++++++++++++- src/backend/catalog/index.c | 41 +++++++++++++++++------ src/backend/commands/cluster.c | 35 ++++++++++++++++---- src/backend/commands/indexcmds.c | 53 +++++++++++++++++++++++++++++-- src/backend/utils/init/miscinit.c | 24 ++++++++------ src/test/regress/expected/privileges.out | 42 ++++++++++++++++++++++++ src/test/regress/sql/privileges.sql | 36 +++++++++++++++++++++ 7 files changed, 231 insertions(+), 30 deletions(-) --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -28,6 +28,7 @@ #include "pgstat.h" #include "storage/bufmgr.h" #include "storage/freespace.h" +#include "utils/guc.h" #include "utils/index_selfuncs.h" #include "utils/memutils.h" #include "utils/rel.h" @@ -786,6 +787,9 @@ Oid heapoid; Relation indexRel; Relation heapRel; + Oid save_userid; + int save_sec_context; + int save_nestlevel; double numSummarized = 0; if (RecoveryInProgress()) @@ -799,10 +803,28 @@ * passed indexoid isn't an index then IndexGetRelation() will fail. * Rather than emitting a not-very-helpful error message, postpone * complaining, expecting that the is-it-an-index test below will fail. + * + * unlike brin_summarize_range(), autovacuum never calls this. hence, we + * don't switch userid. */ heapoid = IndexGetRelation(indexoid, true); if (OidIsValid(heapoid)) + { heapRel = heap_open(heapoid, ShareUpdateExclusiveLock); + + /* + * Autovacuum calls us. For its benefit, 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. This is harmless, albeit + * unnecessary, when called from SQL, because we fail shortly if the + * user does not own the index. + */ + GetUserIdAndSecContext(&save_userid, &save_sec_context); + SetUserIdAndSecContext(heapRel->rd_rel->relowner, + save_sec_context | SECURITY_RESTRICTED_OPERATION); + save_nestlevel = NewGUCNestLevel(); + } else heapRel = NULL; @@ -817,7 +839,7 @@ RelationGetRelationName(indexRel)))); /* User must own the index (comparable to privileges needed for VACUUM) */ - if (!pg_class_ownercheck(indexoid, GetUserId())) + if (heapRel != NULL && !pg_class_ownercheck(indexoid, save_userid)) aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS, RelationGetRelationName(indexRel)); @@ -835,6 +857,12 @@ /* OK, do it */ brinsummarize(indexRel, heapRel, &numSummarized, NULL); + /* 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); + relation_close(indexRel, ShareUpdateExclusiveLock); relation_close(heapRel, ShareUpdateExclusiveLock); --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -2908,7 +2908,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); /* @@ -2922,16 +2932,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; @@ -3395,6 +3395,9 @@ Relation iRel, heapRelation; Oid heapId; + Oid save_userid; + int save_sec_context; + int save_nestlevel; IndexInfo *indexInfo; volatile bool skipped_constraint = false; PGRUsage ru0; @@ -3409,6 +3412,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. */ @@ -3550,6 +3563,12 @@ errdetail_internal("%s", pg_rusage_show(&ru0)))); + /* 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 @@ -44,6 +44,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" @@ -260,6 +261,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(); @@ -277,6 +281,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. * @@ -290,10 +304,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; } /* @@ -307,7 +321,7 @@ if (RELATION_IS_OTHER_TEMP(OldHeap)) { relation_close(OldHeap, AccessExclusiveLock); - return; + goto out; } if (OidIsValid(indexOid)) @@ -318,7 +332,7 @@ if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(indexOid))) { relation_close(OldHeap, AccessExclusiveLock); - return; + goto out; } /* @@ -328,14 +342,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); } @@ -389,7 +403,7 @@ !RelationIsPopulated(OldHeap)) { relation_close(OldHeap, AccessExclusiveLock); - return; + goto out; } /* @@ -404,6 +418,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 @@ -49,6 +49,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" @@ -339,8 +340,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 @@ -381,6 +387,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); @@ -422,7 +437,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, @@ -449,7 +464,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, @@ -679,15 +694,33 @@ if (!OidIsValid(indexRelationId)) { + /* 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); + heap_close(rel, NoLock); return address; } + /* + * 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 */ @@ -766,6 +799,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); @@ -781,6 +824,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 @@ -365,15 +365,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. * * SECURITY_NOFORCE_RLS indicates that we are inside an operation which should * ignore the FORCE ROW LEVEL SECURITY per-table indication. This is used to --- a/src/test/regress/expected/privileges.out +++ b/src/test/regress/expected/privileges.out @@ -1244,6 +1244,48 @@ -- 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 + ASSERT current_user = 'regress_sro_user', + format('sro_ifun(%s) called by %s', $1, current_user); + 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; +-- BRIN index +CREATE INDEX sro_brin ON sro_tab USING brin ((sro_ifun(a) + sro_ifun(0))); +SELECT brin_summarize_new_values('sro_brin'); + brin_summarize_new_values +--------------------------- + 0 +(1 row) + +DROP TABLE sro_tab; SET SESSION AUTHORIZATION regress_sro_user; CREATE FUNCTION unwanted_grant() RETURNS void LANGUAGE sql AS 'GRANT regress_group2 TO regress_sro_user'; --- a/src/test/regress/sql/privileges.sql +++ b/src/test/regress/sql/privileges.sql @@ -762,6 +762,42 @@ \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 + ASSERT current_user = 'regress_sro_user', + format('sro_ifun(%s) called by %s', $1, current_user); + 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; +-- BRIN index +CREATE INDEX sro_brin ON sro_tab USING brin ((sro_ifun(a) + sro_ifun(0))); +SELECT brin_summarize_new_values('sro_brin'); +DROP TABLE sro_tab; + SET SESSION AUTHORIZATION regress_sro_user; CREATE FUNCTION unwanted_grant() RETURNS void LANGUAGE sql AS 'GRANT regress_group2 TO regress_sro_user';