Hello pgsql-hackers, Is there anyone willing to review the patches that I prepared? I'd have substatntially more confidence in the patches with a review from an upstream developer who is familiar with the code.
Regards, -Roberto On Mon, Jul 04, 2022 at 06:06:58PM -0400, Roberto C. Sánchez wrote: > On Wed, Jun 08, 2022 at 05:31:22PM -0400, Roberto C. Sánchez wrote: > > On Wed, Jun 08, 2022 at 04:15:47PM -0400, Tom Lane wrote: > > > Roberto =?iso-8859-1?Q?C=2E_S=E1nchez?= <robe...@debian.org> writes: > > > > I am investigating backporting the fixes for CVE-2022-1552 to 9.6 and > > > > 9.4 as part of Debian LTS and Extended LTS. I am aware that these > > > > releases are no longer supported upstream, but I have made an attempt at > > > > adapting commits ef792f7856dea2576dcd9cab92b2b05fe955696b and > > > > f26d5702857a9c027f84850af48b0eea0f3aa15c from the REL_10_STABLE branch. > > > > I would appreciate a review of the attached patches and any comments on > > > > things that may have been missed and/or adapted improperly. > > > > > > FWIW, I would not recommend being in a huge hurry to back-port those > > > changes, pending the outcome of this discussion: > > > > > > https://www.postgresql.org/message-id/flat/f8a4105f076544c180a87ef0c4822352%40stmuk.bayern.de > > > > > Thanks for the pointer. > > > > > We're going to have to tweak that code somehow, and it's not yet > > > entirely clear how. > > > > > I will monitor the discussion to see what comes of it. > > > Based on the discussion in the other thread, I have made an attempt to > backport commit 88b39e61486a8925a3861d50c306a51eaa1af8d6 to 9.6 and 9.4. > The only significant change that I had to make was to add the full > function signatures for the REVOKE/GRANT in the citext test. > > One question that I had about the change as committed is whether a > REVOKE is needed on s.citext_ne, like so: > > REVOKE ALL ON FUNCTION s.citext_ne FROM PUBLIC; > > Or (for pre-10): > > REVOKE ALL ON FUNCTION s.citext_ne(s.citext, s.citext) FROM PUBLIC; > > I ask because the comment immediately preceding the sequence of REVOKEs > includes the comment "Revoke all conceivably-relevant ACLs within the > extension." I am not especially knowledgable about deep internals, but > that function seems like it would belong in the same group with the > others. > > In any event, would someone be willing to review the attached patches > for correctness? I would like to shortly publish updates to 9.6 and 9.4 > in Debian and a review would be most appreciated. > > Regards, > > -Roberto > > -- > Roberto C. Sánchez > From ef792f7856dea2576dcd9cab92b2b05fe955696b Mon Sep 17 00:00:00 2001 > From: Noah Misch <n...@leadboat.com> > 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'; > From f26d5702857a9c027f84850af48b0eea0f3aa15c Mon Sep 17 00:00:00 2001 > From: Noah Misch <n...@leadboat.com> > Date: Mon, 9 May 2022 08:35:08 -0700 > Subject: [PATCH] In REFRESH MATERIALIZED VIEW, set user ID before running user > code. > > It intended to, but did not, achieve this. Adopt the new standard of > setting user ID just after locking the relation. Back-patch to v10 (all > supported versions). > > Reviewed by Simon Riggs. Reported by Alvaro Herrera. > > Security: CVE-2022-1552 > --- > src/backend/commands/matview.c | 30 > +++++++++++------------------- > src/test/regress/expected/privileges.out | 15 +++++++++++++++ > src/test/regress/sql/privileges.sql | 16 ++++++++++++++++ > 3 files changed, 42 insertions(+), 19 deletions(-) > > --- a/src/backend/commands/matview.c > +++ b/src/backend/commands/matview.c > @@ -164,6 +164,17 @@ > > lockmode, false, false, > > RangeVarCallbackOwnsTable, NULL); > matviewRel = heap_open(matviewOid, NoLock); > + relowner = matviewRel->rd_rel->relowner; > + > + /* > + * Switch to the owner's userid, so that any 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(relowner, > + save_sec_context | > SECURITY_RESTRICTED_OPERATION); > + save_nestlevel = NewGUCNestLevel(); > > /* Make sure it is a materialized view. */ > if (matviewRel->rd_rel->relkind != RELKIND_MATVIEW) > @@ -269,19 +280,6 @@ > */ > SetMatViewPopulatedState(matviewRel, !stmt->skipData); > > - relowner = matviewRel->rd_rel->relowner; > - > - /* > - * Switch to the owner's userid, so that any functions are run as that > - * user. Also arrange to make GUC variable changes local to this > command. > - * Don't lock it down too tight to create a temporary table just yet. > We > - * will switch modes when we are about to execute user code. > - */ > - GetUserIdAndSecContext(&save_userid, &save_sec_context); > - SetUserIdAndSecContext(relowner, > - save_sec_context | > SECURITY_LOCAL_USERID_CHANGE); > - save_nestlevel = NewGUCNestLevel(); > - > /* Concurrent refresh builds new data in temp tablespace, and does > diff. */ > if (concurrent) > { > @@ -304,12 +302,6 @@ > LockRelationOid(OIDNewHeap, AccessExclusiveLock); > dest = CreateTransientRelDestReceiver(OIDNewHeap); > > - /* > - * Now lock down security-restricted operations. > - */ > - SetUserIdAndSecContext(relowner, > - save_sec_context | > SECURITY_RESTRICTED_OPERATION); > - > /* Generate the data, if wanted. */ > if (!stmt->skipData) > refresh_matview_datafill(dest, dataQuery, queryString); > --- a/src/test/regress/expected/privileges.out > +++ b/src/test/regress/expected/privileges.out > @@ -1323,6 +1323,21 @@ > SQL statement "SELECT unwanted_grant()" > PL/pgSQL function sro_trojan() line 1 at PERFORM > SQL function "mv_action" statement 1 > +-- REFRESH MATERIALIZED VIEW CONCURRENTLY use of eval_const_expressions() > +SET SESSION AUTHORIZATION regress_sro_user; > +CREATE FUNCTION unwanted_grant_nofail(int) RETURNS int > + IMMUTABLE LANGUAGE plpgsql AS $$ > +BEGIN > + PERFORM unwanted_grant(); > + RAISE WARNING 'owned'; > + RETURN 1; > +EXCEPTION WHEN OTHERS THEN > + RETURN 2; > +END$$; > +CREATE MATERIALIZED VIEW sro_index_mv AS SELECT 1 AS c; > +CREATE UNIQUE INDEX ON sro_index_mv (c) WHERE unwanted_grant_nofail(1) > 0; > +\c - > +REFRESH MATERIALIZED VIEW sro_index_mv; > DROP OWNED BY regress_sro_user; > DROP ROLE regress_sro_user; > -- Admin options > --- a/src/test/regress/sql/privileges.sql > +++ b/src/test/regress/sql/privileges.sql > @@ -824,6 +824,22 @@ > REFRESH MATERIALIZED VIEW sro_mv; > BEGIN; SET CONSTRAINTS ALL IMMEDIATE; REFRESH MATERIALIZED VIEW sro_mv; > COMMIT; > > +-- REFRESH MATERIALIZED VIEW CONCURRENTLY use of eval_const_expressions() > +SET SESSION AUTHORIZATION regress_sro_user; > +CREATE FUNCTION unwanted_grant_nofail(int) RETURNS int > + IMMUTABLE LANGUAGE plpgsql AS $$ > +BEGIN > + PERFORM unwanted_grant(); > + RAISE WARNING 'owned'; > + RETURN 1; > +EXCEPTION WHEN OTHERS THEN > + RETURN 2; > +END$$; > +CREATE MATERIALIZED VIEW sro_index_mv AS SELECT 1 AS c; > +CREATE UNIQUE INDEX ON sro_index_mv (c) WHERE unwanted_grant_nofail(1) > 0; > +\c - > +REFRESH MATERIALIZED VIEW sro_index_mv; > + > DROP OWNED BY regress_sro_user; > DROP ROLE regress_sro_user; > > From 88b39e61486a8925a3861d50c306a51eaa1af8d6 Mon Sep 17 00:00:00 2001 > From: Noah Misch <n...@leadboat.com> > Date: Sat, 25 Jun 2022 09:07:41 -0700 > Subject: [PATCH] CREATE INDEX: use the original userid for more ACL checks. > > Commit a117cebd638dd02e5c2e791c25e43745f233111b used the original userid > for ACL checks located directly in DefineIndex(), but it still adopted > the table owner userid for more ACL checks than intended. That broke > dump/reload of indexes that refer to an operator class, collation, or > exclusion operator in a schema other than "public" or "pg_catalog". > Back-patch to v10 (all supported versions), like the earlier commit. > > Nathan Bossart and Noah Misch > > Discussion: > https://postgr.es/m/f8a4105f076544c180a87ef0c4822...@stmuk.bayern.de > --- > contrib/citext/Makefile | 2 > contrib/citext/expected/create_index_acl.out | 81 +++++++++++++++++++++++ > contrib/citext/sql/create_index_acl.sql | 82 ++++++++++++++++++++++++ > src/backend/commands/indexcmds.c | 92 > +++++++++++++++++++++++---- > 4 files changed, 244 insertions(+), 13 deletions(-) > create mode 100644 contrib/citext/expected/create_index_acl.out > create mode 100644 contrib/citext/sql/create_index_acl.sql > > --- a/contrib/citext/Makefile > +++ b/contrib/citext/Makefile > @@ -7,7 +7,7 @@ > citext--1.0--1.1.sql citext--unpackaged--1.0.sql > PGFILEDESC = "citext - case-insensitive character string data type" > > -REGRESS = citext > +REGRESS = create_index_acl citext > > ifdef USE_PGXS > PG_CONFIG = pg_config > --- /dev/null > +++ b/contrib/citext/expected/create_index_acl.out > @@ -0,0 +1,81 @@ > +-- Each DefineIndex() ACL check uses either the original userid or the table > +-- owner userid; see its header comment. Here, confirm that DefineIndex() > +-- uses its original userid where necessary. The test works by creating > +-- indexes that refer to as many sorts of objects as possible, with the table > +-- owner having as few applicable privileges as possible. (The > privileges.sql > +-- regress_sro_user tests look for the opposite defect; they confirm that > +-- DefineIndex() uses the table owner userid where necessary.) > +-- Don't override tablespaces; this version lacks allow_in_place_tablespaces. > +BEGIN; > +CREATE ROLE regress_minimal; > +CREATE SCHEMA s; > +CREATE EXTENSION citext SCHEMA s; > +-- Revoke all conceivably-relevant ACLs within the extension. The system > +-- doesn't check all these ACLs, but this will provide some coverage if that > +-- ever changes. > +REVOKE ALL ON TYPE s.citext FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_lt(s.citext, s.citext) FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_le(s.citext, s.citext) FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_eq(s.citext, s.citext) FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_ge(s.citext, s.citext) FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_gt(s.citext, s.citext) FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_cmp(s.citext, s.citext) FROM PUBLIC; > +-- Functions sufficient for making an index column that has the side effect > of > +-- changing search_path at expression planning time. > +CREATE FUNCTION public.setter() RETURNS bool VOLATILE > + LANGUAGE SQL AS $$SET search_path = s; SELECT true$$; > +CREATE FUNCTION s.const() RETURNS bool IMMUTABLE > + LANGUAGE SQL AS $$SELECT public.setter()$$; > +CREATE FUNCTION s.index_this_expr(s.citext, bool) RETURNS s.citext IMMUTABLE > + LANGUAGE SQL AS $$SELECT $1$$; > +REVOKE ALL ON FUNCTION public.setter() FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.const() FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.index_this_expr(s.citext, bool) FROM PUBLIC; > +-- Even for an empty table, expression planning calls s.const & > public.setter. > +GRANT EXECUTE ON FUNCTION public.setter() TO regress_minimal; > +GRANT EXECUTE ON FUNCTION s.const() TO regress_minimal; > +-- Function for index predicate. > +CREATE FUNCTION s.index_row_if(s.citext) RETURNS bool IMMUTABLE > + LANGUAGE SQL AS $$SELECT $1 IS NOT NULL$$; > +REVOKE ALL ON FUNCTION s.index_row_if(s.citext) FROM PUBLIC; > +-- Even for an empty table, CREATE INDEX checks ii_Predicate permissions. > +GRANT EXECUTE ON FUNCTION s.index_row_if(s.citext) TO regress_minimal; > +-- Non-extension, non-function objects. > +CREATE COLLATION s.coll (LOCALE="C"); > +CREATE TABLE s.x (y s.citext); > +ALTER TABLE s.x OWNER TO regress_minimal; > +-- Empty-table DefineIndex() > +CREATE UNIQUE INDEX u0rows ON s.x USING btree > + ((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_ops) > + WHERE s.index_row_if(y); > +ALTER TABLE s.x ADD CONSTRAINT e0rows EXCLUDE USING btree > + ((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=) > + WHERE (s.index_row_if(y)); > +-- Make the table nonempty. > +INSERT INTO s.x VALUES ('foo'), ('bar'); > +-- If the INSERT runs the planner on index expressions, a search_path change > +-- survives. As of 2022-06, the INSERT reuses a cached plan. It does so > even > +-- under debug_discard_caches, since each index is new-in-transaction. If > +-- future work changes a cache lifecycle, this RESET may become necessary. > +RESET search_path; > +-- For a nonempty table, owner needs permissions throughout ii_Expressions. > +GRANT EXECUTE ON FUNCTION s.index_this_expr(s.citext, bool) TO > regress_minimal; > +CREATE UNIQUE INDEX u2rows ON s.x USING btree > + ((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_ops) > + WHERE s.index_row_if(y); > +ALTER TABLE s.x ADD CONSTRAINT e2rows EXCLUDE USING btree > + ((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=) > + WHERE (s.index_row_if(y)); > +-- Shall not find s.coll via search_path, despite the s.const->public.setter > +-- call having set search_path=s during expression planning. Suppress the > +-- message itself, which depends on the database encoding. > +\set VERBOSITY terse > +DO $$ > +BEGIN > +ALTER TABLE s.x ADD CONSTRAINT underqualified EXCLUDE USING btree > + ((s.index_this_expr(y, s.const())) COLLATE coll WITH s.=) > + WHERE (s.index_row_if(y)); > +EXCEPTION WHEN OTHERS THEN RAISE EXCEPTION '%', sqlstate; END$$; > +ERROR: 42704 > +\set VERBOSITY default > +ROLLBACK; > --- /dev/null > +++ b/contrib/citext/sql/create_index_acl.sql > @@ -0,0 +1,82 @@ > +-- Each DefineIndex() ACL check uses either the original userid or the table > +-- owner userid; see its header comment. Here, confirm that DefineIndex() > +-- uses its original userid where necessary. The test works by creating > +-- indexes that refer to as many sorts of objects as possible, with the table > +-- owner having as few applicable privileges as possible. (The > privileges.sql > +-- regress_sro_user tests look for the opposite defect; they confirm that > +-- DefineIndex() uses the table owner userid where necessary.) > + > +-- Don't override tablespaces; this version lacks allow_in_place_tablespaces. > + > +BEGIN; > +CREATE ROLE regress_minimal; > +CREATE SCHEMA s; > +CREATE EXTENSION citext SCHEMA s; > +-- Revoke all conceivably-relevant ACLs within the extension. The system > +-- doesn't check all these ACLs, but this will provide some coverage if that > +-- ever changes. > +REVOKE ALL ON TYPE s.citext FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_lt(s.citext, s.citext) FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_le(s.citext, s.citext) FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_eq(s.citext, s.citext) FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_ge(s.citext, s.citext) FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_gt(s.citext, s.citext) FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_cmp(s.citext, s.citext) FROM PUBLIC; > +-- Functions sufficient for making an index column that has the side effect > of > +-- changing search_path at expression planning time. > +CREATE FUNCTION public.setter() RETURNS bool VOLATILE > + LANGUAGE SQL AS $$SET search_path = s; SELECT true$$; > +CREATE FUNCTION s.const() RETURNS bool IMMUTABLE > + LANGUAGE SQL AS $$SELECT public.setter()$$; > +CREATE FUNCTION s.index_this_expr(s.citext, bool) RETURNS s.citext IMMUTABLE > + LANGUAGE SQL AS $$SELECT $1$$; > +REVOKE ALL ON FUNCTION public.setter() FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.const() FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.index_this_expr(s.citext, bool) FROM PUBLIC; > +-- Even for an empty table, expression planning calls s.const & > public.setter. > +GRANT EXECUTE ON FUNCTION public.setter() TO regress_minimal; > +GRANT EXECUTE ON FUNCTION s.const() TO regress_minimal; > +-- Function for index predicate. > +CREATE FUNCTION s.index_row_if(s.citext) RETURNS bool IMMUTABLE > + LANGUAGE SQL AS $$SELECT $1 IS NOT NULL$$; > +REVOKE ALL ON FUNCTION s.index_row_if(s.citext) FROM PUBLIC; > +-- Even for an empty table, CREATE INDEX checks ii_Predicate permissions. > +GRANT EXECUTE ON FUNCTION s.index_row_if(s.citext) TO regress_minimal; > +-- Non-extension, non-function objects. > +CREATE COLLATION s.coll (LOCALE="C"); > +CREATE TABLE s.x (y s.citext); > +ALTER TABLE s.x OWNER TO regress_minimal; > +-- Empty-table DefineIndex() > +CREATE UNIQUE INDEX u0rows ON s.x USING btree > + ((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_ops) > + WHERE s.index_row_if(y); > +ALTER TABLE s.x ADD CONSTRAINT e0rows EXCLUDE USING btree > + ((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=) > + WHERE (s.index_row_if(y)); > +-- Make the table nonempty. > +INSERT INTO s.x VALUES ('foo'), ('bar'); > +-- If the INSERT runs the planner on index expressions, a search_path change > +-- survives. As of 2022-06, the INSERT reuses a cached plan. It does so > even > +-- under debug_discard_caches, since each index is new-in-transaction. If > +-- future work changes a cache lifecycle, this RESET may become necessary. > +RESET search_path; > +-- For a nonempty table, owner needs permissions throughout ii_Expressions. > +GRANT EXECUTE ON FUNCTION s.index_this_expr(s.citext, bool) TO > regress_minimal; > +CREATE UNIQUE INDEX u2rows ON s.x USING btree > + ((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_ops) > + WHERE s.index_row_if(y); > +ALTER TABLE s.x ADD CONSTRAINT e2rows EXCLUDE USING btree > + ((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=) > + WHERE (s.index_row_if(y)); > +-- Shall not find s.coll via search_path, despite the s.const->public.setter > +-- call having set search_path=s during expression planning. Suppress the > +-- message itself, which depends on the database encoding. > +\set VERBOSITY terse > +DO $$ > +BEGIN > +ALTER TABLE s.x ADD CONSTRAINT underqualified EXCLUDE USING btree > + ((s.index_this_expr(y, s.const())) COLLATE coll WITH s.=) > + WHERE (s.index_row_if(y)); > +EXCEPTION WHEN OTHERS THEN RAISE EXCEPTION '%', sqlstate; END$$; > +\set VERBOSITY default > +ROLLBACK; > --- a/src/backend/commands/indexcmds.c > +++ b/src/backend/commands/indexcmds.c > @@ -70,7 +70,10 @@ > Oid relId, > char *accessMethodName, Oid accessMethodId, > bool amcanorder, > - bool isconstraint); > + bool isconstraint, > + Oid ddl_userid, > + int ddl_sec_context, > + int *ddl_save_nestlevel); > static Oid GetIndexOpClass(List *opclass, Oid attrType, > char *accessMethodName, Oid accessMethodId); > static char *ChooseIndexName(const char *tabname, Oid namespaceId, > @@ -176,8 +179,7 @@ > * Compute the operator classes, collations, and exclusion operators for > * the new index, so we can test whether it's compatible with the > existing > * one. Note that ComputeIndexAttrs might fail here, but that's OK: > - * DefineIndex would have called this function with the same arguments > - * later on, and it would have failed then anyway. > + * DefineIndex would have failed later. > */ > indexInfo = makeNode(IndexInfo); > indexInfo->ii_Expressions = NIL; > @@ -195,7 +197,7 @@ > coloptions, attributeList, > exclusionOpNames, relationId, > accessMethodName, accessMethodId, > - amcanorder, isconstraint); > + amcanorder, isconstraint, InvalidOid, > 0, NULL); > > > /* Get the soon-obsolete pg_index tuple. */ > @@ -288,6 +290,19 @@ > * DefineIndex > * Creates a new index. > * > + * This function manages the current userid according to the needs of > pg_dump. > + * Recreating old-database catalog entries in new-database is fine, > regardless > + * of which users would have permission to recreate those entries now. > That's > + * just preservation of state. Running opaque expressions, like calling a > + * function named in a catalog entry or evaluating a pg_node_tree in a > catalog > + * entry, as anyone other than the object owner, is not fine. To adhere to > + * those principles and to remain fail-safe, use the table owner userid for > + * most ACL checks. Use the original userid for ACL checks reached without > + * traversing opaque expressions. (pg_dump can predict such ACL checks from > + * catalogs.) Overall, this is a mess. Future DDL development should > + * consider offering one DDL command for catalog setup and a separate DDL > + * command for steps that run opaque expressions. > + * > * 'relationId': the OID of the heap relation on which the index is to be > * created > * 'stmt': IndexStmt describing the properties of the new index. > @@ -598,7 +613,8 @@ > coloptions, stmt->indexParams, > stmt->excludeOpNames, relationId, > accessMethodName, accessMethodId, > - amcanorder, stmt->isconstraint); > + amcanorder, stmt->isconstraint, > root_save_userid, > + root_save_sec_context, > &root_save_nestlevel); > > /* > * Extra checks when creating a PRIMARY KEY index. > @@ -706,9 +722,8 @@ > > /* > * 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. > + * subsequent changes local to this command. This is essential if some > + * index function changed a behavior-affecting GUC, e.g. search_path. > */ > AtEOXact_GUC(false, root_save_nestlevel); > root_save_nestlevel = NewGUCNestLevel(); > @@ -1063,6 +1078,10 @@ > /* > * Compute per-index-column information, including indexed column numbers > * or index expressions, opclasses, and indoptions. > + * > + * If the caller switched to the table owner, ddl_userid is the role for ACL > + * checks reached without traversing opaque expressions. Otherwise, it's > + * InvalidOid, and other ddl_* arguments are undefined. > */ > static void > ComputeIndexAttrs(IndexInfo *indexInfo, > @@ -1076,11 +1095,16 @@ > char *accessMethodName, > Oid accessMethodId, > bool amcanorder, > - bool isconstraint) > + bool isconstraint, > + Oid ddl_userid, > + int ddl_sec_context, > + int *ddl_save_nestlevel) > { > ListCell *nextExclOp; > ListCell *lc; > int attn; > + Oid save_userid; > + int save_sec_context; > > /* Allocate space for exclusion operator info, if needed */ > if (exclusionOpNames) > @@ -1096,6 +1120,9 @@ > else > nextExclOp = NULL; > > + if (OidIsValid(ddl_userid)) > + GetUserIdAndSecContext(&save_userid, &save_sec_context); > + > /* > * process attributeList > */ > @@ -1190,10 +1217,24 @@ > typeOidP[attn] = atttype; > > /* > - * Apply collation override if any > + * Apply collation override if any. Use of ddl_userid is > necessary > + * due to ACL checks therein, and it's safe because collations > don't > + * contain opaque expressions (or non-opaque expressions). > */ > if (attribute->collation) > + { > + if (OidIsValid(ddl_userid)) > + { > + AtEOXact_GUC(false, *ddl_save_nestlevel); > + SetUserIdAndSecContext(ddl_userid, > ddl_sec_context); > + } > attcollation = get_collation_oid(attribute->collation, > false); > + if (OidIsValid(ddl_userid)) > + { > + SetUserIdAndSecContext(save_userid, > save_sec_context); > + *ddl_save_nestlevel = NewGUCNestLevel(); > + } > + } > > /* > * Check we have a collation iff it's a collatable type. The > only > @@ -1221,12 +1262,25 @@ > collationOidP[attn] = attcollation; > > /* > - * Identify the opclass to use. > + * Identify the opclass to use. Use of ddl_userid is necessary > due to > + * ACL checks therein. This is safe despite opclasses > containing > + * opaque expressions (specifically, functions), because only > + * superusers can define opclasses. > */ > + if (OidIsValid(ddl_userid)) > + { > + AtEOXact_GUC(false, *ddl_save_nestlevel); > + SetUserIdAndSecContext(ddl_userid, ddl_sec_context); > + } > classOidP[attn] = GetIndexOpClass(attribute->opclass, > > atttype, > > accessMethodName, > > accessMethodId); > + if (OidIsValid(ddl_userid)) > + { > + SetUserIdAndSecContext(save_userid, save_sec_context); > + *ddl_save_nestlevel = NewGUCNestLevel(); > + } > > /* > * Identify the exclusion operator, if any. > @@ -1240,9 +1294,23 @@ > > /* > * Find the operator --- it must accept the column > datatype > - * without runtime coercion (but binary compatibility > is OK) > + * without runtime coercion (but binary compatibility > is OK). > + * Operators contain opaque expressions (specifically, > functions). > + * compatible_oper_opid() boils down to oper() and > + * IsBinaryCoercible(). PostgreSQL would have security > problems > + * elsewhere if oper() started calling opaque > expressions. > */ > + if (OidIsValid(ddl_userid)) > + { > + AtEOXact_GUC(false, *ddl_save_nestlevel); > + SetUserIdAndSecContext(ddl_userid, > ddl_sec_context); > + } > opid = compatible_oper_opid(opname, atttype, atttype, > false); > + if (OidIsValid(ddl_userid)) > + { > + SetUserIdAndSecContext(save_userid, > save_sec_context); > + *ddl_save_nestlevel = NewGUCNestLevel(); > + } > > /* > * Only allow commutative operators to be used in > exclusion > From ef792f7856dea2576dcd9cab92b2b05fe955696b Mon Sep 17 00:00:00 2001 > From: Noah Misch <n...@leadboat.com> > 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'; > From f26d5702857a9c027f84850af48b0eea0f3aa15c Mon Sep 17 00:00:00 2001 > From: Noah Misch <n...@leadboat.com> > Date: Mon, 9 May 2022 08:35:08 -0700 > Subject: [PATCH] In REFRESH MATERIALIZED VIEW, set user ID before running user > code. > > It intended to, but did not, achieve this. Adopt the new standard of > setting user ID just after locking the relation. Back-patch to v10 (all > supported versions). > > Reviewed by Simon Riggs. Reported by Alvaro Herrera. > > Security: CVE-2022-1552 > --- > src/backend/commands/matview.c | 30 > +++++++++++------------------- > src/test/regress/expected/privileges.out | 15 +++++++++++++++ > src/test/regress/sql/privileges.sql | 16 ++++++++++++++++ > 3 files changed, 42 insertions(+), 19 deletions(-) > > --- a/src/backend/commands/matview.c > +++ b/src/backend/commands/matview.c > @@ -161,6 +161,17 @@ > > lockmode, false, false, > > RangeVarCallbackOwnsTable, NULL); > matviewRel = heap_open(matviewOid, NoLock); > + relowner = matviewRel->rd_rel->relowner; > + > + /* > + * Switch to the owner's userid, so that any 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(relowner, > + save_sec_context | > SECURITY_RESTRICTED_OPERATION); > + save_nestlevel = NewGUCNestLevel(); > > /* Make sure it is a materialized view. */ > if (matviewRel->rd_rel->relkind != RELKIND_MATVIEW) > @@ -233,19 +244,6 @@ > */ > SetMatViewPopulatedState(matviewRel, !stmt->skipData); > > - relowner = matviewRel->rd_rel->relowner; > - > - /* > - * Switch to the owner's userid, so that any functions are run as that > - * user. Also arrange to make GUC variable changes local to this > command. > - * Don't lock it down too tight to create a temporary table just yet. > We > - * will switch modes when we are about to execute user code. > - */ > - GetUserIdAndSecContext(&save_userid, &save_sec_context); > - SetUserIdAndSecContext(relowner, > - save_sec_context | > SECURITY_LOCAL_USERID_CHANGE); > - save_nestlevel = NewGUCNestLevel(); > - > /* Concurrent refresh builds new data in temp tablespace, and does > diff. */ > if (concurrent) > tableSpace = GetDefaultTablespace(RELPERSISTENCE_TEMP); > @@ -262,12 +260,6 @@ > LockRelationOid(OIDNewHeap, AccessExclusiveLock); > dest = CreateTransientRelDestReceiver(OIDNewHeap); > > - /* > - * Now lock down security-restricted operations. > - */ > - SetUserIdAndSecContext(relowner, > - save_sec_context | > SECURITY_RESTRICTED_OPERATION); > - > /* Generate the data, if wanted. */ > if (!stmt->skipData) > refresh_matview_datafill(dest, dataQuery, queryString); > --- a/src/test/regress/expected/privileges.out > +++ b/src/test/regress/expected/privileges.out > @@ -1266,6 +1266,21 @@ > SQL statement "SELECT unwanted_grant()" > PL/pgSQL function sro_trojan() line 1 at PERFORM > SQL function "mv_action" statement 1 > +-- REFRESH MATERIALIZED VIEW CONCURRENTLY use of eval_const_expressions() > +SET SESSION AUTHORIZATION regress_sro_user; > +CREATE FUNCTION unwanted_grant_nofail(int) RETURNS int > + IMMUTABLE LANGUAGE plpgsql AS $$ > +BEGIN > + PERFORM unwanted_grant(); > + RAISE WARNING 'owned'; > + RETURN 1; > +EXCEPTION WHEN OTHERS THEN > + RETURN 2; > +END$$; > +CREATE MATERIALIZED VIEW sro_index_mv AS SELECT 1 AS c; > +CREATE UNIQUE INDEX ON sro_index_mv (c) WHERE unwanted_grant_nofail(1) > 0; > +\c - > +REFRESH MATERIALIZED VIEW sro_index_mv; > DROP OWNED BY regress_sro_user; > DROP ROLE regress_sro_user; > -- Admin options > --- a/src/test/regress/sql/privileges.sql > +++ b/src/test/regress/sql/privileges.sql > @@ -784,6 +784,22 @@ > REFRESH MATERIALIZED VIEW sro_mv; > BEGIN; SET CONSTRAINTS ALL IMMEDIATE; REFRESH MATERIALIZED VIEW sro_mv; > COMMIT; > > +-- REFRESH MATERIALIZED VIEW CONCURRENTLY use of eval_const_expressions() > +SET SESSION AUTHORIZATION regress_sro_user; > +CREATE FUNCTION unwanted_grant_nofail(int) RETURNS int > + IMMUTABLE LANGUAGE plpgsql AS $$ > +BEGIN > + PERFORM unwanted_grant(); > + RAISE WARNING 'owned'; > + RETURN 1; > +EXCEPTION WHEN OTHERS THEN > + RETURN 2; > +END$$; > +CREATE MATERIALIZED VIEW sro_index_mv AS SELECT 1 AS c; > +CREATE UNIQUE INDEX ON sro_index_mv (c) WHERE unwanted_grant_nofail(1) > 0; > +\c - > +REFRESH MATERIALIZED VIEW sro_index_mv; > + > DROP OWNED BY regress_sro_user; > DROP ROLE regress_sro_user; > > From 88b39e61486a8925a3861d50c306a51eaa1af8d6 Mon Sep 17 00:00:00 2001 > From: Noah Misch <n...@leadboat.com> > Date: Sat, 25 Jun 2022 09:07:41 -0700 > Subject: [PATCH] CREATE INDEX: use the original userid for more ACL checks. > > Commit a117cebd638dd02e5c2e791c25e43745f233111b used the original userid > for ACL checks located directly in DefineIndex(), but it still adopted > the table owner userid for more ACL checks than intended. That broke > dump/reload of indexes that refer to an operator class, collation, or > exclusion operator in a schema other than "public" or "pg_catalog". > Back-patch to v10 (all supported versions), like the earlier commit. > > Nathan Bossart and Noah Misch > > Discussion: > https://postgr.es/m/f8a4105f076544c180a87ef0c4822...@stmuk.bayern.de > --- > contrib/citext/Makefile | 2 > contrib/citext/expected/create_index_acl.out | 81 +++++++++++++++++++++++ > contrib/citext/sql/create_index_acl.sql | 82 ++++++++++++++++++++++++ > src/backend/commands/indexcmds.c | 92 > +++++++++++++++++++++++---- > 4 files changed, 244 insertions(+), 13 deletions(-) > create mode 100644 contrib/citext/expected/create_index_acl.out > create mode 100644 contrib/citext/sql/create_index_acl.sql > > --- a/contrib/citext/Makefile > +++ b/contrib/citext/Makefile > @@ -7,7 +7,7 @@ > citext--1.1--1.0.sql citext--unpackaged--1.0.sql > PGFILEDESC = "citext - case-insensitive character string data type" > > -REGRESS = citext > +REGRESS = create_index_acl citext > > ifdef USE_PGXS > PG_CONFIG = pg_config > --- /dev/null > +++ b/contrib/citext/expected/create_index_acl.out > @@ -0,0 +1,81 @@ > +-- Each DefineIndex() ACL check uses either the original userid or the table > +-- owner userid; see its header comment. Here, confirm that DefineIndex() > +-- uses its original userid where necessary. The test works by creating > +-- indexes that refer to as many sorts of objects as possible, with the table > +-- owner having as few applicable privileges as possible. (The > privileges.sql > +-- regress_sro_user tests look for the opposite defect; they confirm that > +-- DefineIndex() uses the table owner userid where necessary.) > +-- Don't override tablespaces; this version lacks allow_in_place_tablespaces. > +BEGIN; > +CREATE ROLE regress_minimal; > +CREATE SCHEMA s; > +CREATE EXTENSION citext SCHEMA s; > +-- Revoke all conceivably-relevant ACLs within the extension. The system > +-- doesn't check all these ACLs, but this will provide some coverage if that > +-- ever changes. > +REVOKE ALL ON TYPE s.citext FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_lt(s.citext, s.citext) FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_le(s.citext, s.citext) FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_eq(s.citext, s.citext) FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_ge(s.citext, s.citext) FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_gt(s.citext, s.citext) FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_cmp(s.citext, s.citext) FROM PUBLIC; > +-- Functions sufficient for making an index column that has the side effect > of > +-- changing search_path at expression planning time. > +CREATE FUNCTION public.setter() RETURNS bool VOLATILE > + LANGUAGE SQL AS $$SET search_path = s; SELECT true$$; > +CREATE FUNCTION s.const() RETURNS bool IMMUTABLE > + LANGUAGE SQL AS $$SELECT public.setter()$$; > +CREATE FUNCTION s.index_this_expr(s.citext, bool) RETURNS s.citext IMMUTABLE > + LANGUAGE SQL AS $$SELECT $1$$; > +REVOKE ALL ON FUNCTION public.setter() FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.const() FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.index_this_expr(s.citext, bool) FROM PUBLIC; > +-- Even for an empty table, expression planning calls s.const & > public.setter. > +GRANT EXECUTE ON FUNCTION public.setter() TO regress_minimal; > +GRANT EXECUTE ON FUNCTION s.const() TO regress_minimal; > +-- Function for index predicate. > +CREATE FUNCTION s.index_row_if(s.citext) RETURNS bool IMMUTABLE > + LANGUAGE SQL AS $$SELECT $1 IS NOT NULL$$; > +REVOKE ALL ON FUNCTION s.index_row_if(s.citext) FROM PUBLIC; > +-- Even for an empty table, CREATE INDEX checks ii_Predicate permissions. > +GRANT EXECUTE ON FUNCTION s.index_row_if(s.citext) TO regress_minimal; > +-- Non-extension, non-function objects. > +CREATE COLLATION s.coll (LOCALE="C"); > +CREATE TABLE s.x (y s.citext); > +ALTER TABLE s.x OWNER TO regress_minimal; > +-- Empty-table DefineIndex() > +CREATE UNIQUE INDEX u0rows ON s.x USING btree > + ((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_ops) > + WHERE s.index_row_if(y); > +ALTER TABLE s.x ADD CONSTRAINT e0rows EXCLUDE USING btree > + ((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=) > + WHERE (s.index_row_if(y)); > +-- Make the table nonempty. > +INSERT INTO s.x VALUES ('foo'), ('bar'); > +-- If the INSERT runs the planner on index expressions, a search_path change > +-- survives. As of 2022-06, the INSERT reuses a cached plan. It does so > even > +-- under debug_discard_caches, since each index is new-in-transaction. If > +-- future work changes a cache lifecycle, this RESET may become necessary. > +RESET search_path; > +-- For a nonempty table, owner needs permissions throughout ii_Expressions. > +GRANT EXECUTE ON FUNCTION s.index_this_expr(s.citext, bool) TO > regress_minimal; > +CREATE UNIQUE INDEX u2rows ON s.x USING btree > + ((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_ops) > + WHERE s.index_row_if(y); > +ALTER TABLE s.x ADD CONSTRAINT e2rows EXCLUDE USING btree > + ((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=) > + WHERE (s.index_row_if(y)); > +-- Shall not find s.coll via search_path, despite the s.const->public.setter > +-- call having set search_path=s during expression planning. Suppress the > +-- message itself, which depends on the database encoding. > +\set VERBOSITY terse > +DO $$ > +BEGIN > +ALTER TABLE s.x ADD CONSTRAINT underqualified EXCLUDE USING btree > + ((s.index_this_expr(y, s.const())) COLLATE coll WITH s.=) > + WHERE (s.index_row_if(y)); > +EXCEPTION WHEN OTHERS THEN RAISE EXCEPTION '%', sqlstate; END$$; > +ERROR: 42704 > +\set VERBOSITY default > +ROLLBACK; > --- /dev/null > +++ b/contrib/citext/sql/create_index_acl.sql > @@ -0,0 +1,82 @@ > +-- Each DefineIndex() ACL check uses either the original userid or the table > +-- owner userid; see its header comment. Here, confirm that DefineIndex() > +-- uses its original userid where necessary. The test works by creating > +-- indexes that refer to as many sorts of objects as possible, with the table > +-- owner having as few applicable privileges as possible. (The > privileges.sql > +-- regress_sro_user tests look for the opposite defect; they confirm that > +-- DefineIndex() uses the table owner userid where necessary.) > + > +-- Don't override tablespaces; this version lacks allow_in_place_tablespaces. > + > +BEGIN; > +CREATE ROLE regress_minimal; > +CREATE SCHEMA s; > +CREATE EXTENSION citext SCHEMA s; > +-- Revoke all conceivably-relevant ACLs within the extension. The system > +-- doesn't check all these ACLs, but this will provide some coverage if that > +-- ever changes. > +REVOKE ALL ON TYPE s.citext FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_lt(s.citext, s.citext) FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_le(s.citext, s.citext) FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_eq(s.citext, s.citext) FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_ge(s.citext, s.citext) FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_gt(s.citext, s.citext) FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.citext_cmp(s.citext, s.citext) FROM PUBLIC; > +-- Functions sufficient for making an index column that has the side effect > of > +-- changing search_path at expression planning time. > +CREATE FUNCTION public.setter() RETURNS bool VOLATILE > + LANGUAGE SQL AS $$SET search_path = s; SELECT true$$; > +CREATE FUNCTION s.const() RETURNS bool IMMUTABLE > + LANGUAGE SQL AS $$SELECT public.setter()$$; > +CREATE FUNCTION s.index_this_expr(s.citext, bool) RETURNS s.citext IMMUTABLE > + LANGUAGE SQL AS $$SELECT $1$$; > +REVOKE ALL ON FUNCTION public.setter() FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.const() FROM PUBLIC; > +REVOKE ALL ON FUNCTION s.index_this_expr(s.citext, bool) FROM PUBLIC; > +-- Even for an empty table, expression planning calls s.const & > public.setter. > +GRANT EXECUTE ON FUNCTION public.setter() TO regress_minimal; > +GRANT EXECUTE ON FUNCTION s.const() TO regress_minimal; > +-- Function for index predicate. > +CREATE FUNCTION s.index_row_if(s.citext) RETURNS bool IMMUTABLE > + LANGUAGE SQL AS $$SELECT $1 IS NOT NULL$$; > +REVOKE ALL ON FUNCTION s.index_row_if(s.citext) FROM PUBLIC; > +-- Even for an empty table, CREATE INDEX checks ii_Predicate permissions. > +GRANT EXECUTE ON FUNCTION s.index_row_if(s.citext) TO regress_minimal; > +-- Non-extension, non-function objects. > +CREATE COLLATION s.coll (LOCALE="C"); > +CREATE TABLE s.x (y s.citext); > +ALTER TABLE s.x OWNER TO regress_minimal; > +-- Empty-table DefineIndex() > +CREATE UNIQUE INDEX u0rows ON s.x USING btree > + ((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_ops) > + WHERE s.index_row_if(y); > +ALTER TABLE s.x ADD CONSTRAINT e0rows EXCLUDE USING btree > + ((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=) > + WHERE (s.index_row_if(y)); > +-- Make the table nonempty. > +INSERT INTO s.x VALUES ('foo'), ('bar'); > +-- If the INSERT runs the planner on index expressions, a search_path change > +-- survives. As of 2022-06, the INSERT reuses a cached plan. It does so > even > +-- under debug_discard_caches, since each index is new-in-transaction. If > +-- future work changes a cache lifecycle, this RESET may become necessary. > +RESET search_path; > +-- For a nonempty table, owner needs permissions throughout ii_Expressions. > +GRANT EXECUTE ON FUNCTION s.index_this_expr(s.citext, bool) TO > regress_minimal; > +CREATE UNIQUE INDEX u2rows ON s.x USING btree > + ((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_ops) > + WHERE s.index_row_if(y); > +ALTER TABLE s.x ADD CONSTRAINT e2rows EXCLUDE USING btree > + ((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=) > + WHERE (s.index_row_if(y)); > +-- Shall not find s.coll via search_path, despite the s.const->public.setter > +-- call having set search_path=s during expression planning. Suppress the > +-- message itself, which depends on the database encoding. > +\set VERBOSITY terse > +DO $$ > +BEGIN > +ALTER TABLE s.x ADD CONSTRAINT underqualified EXCLUDE USING btree > + ((s.index_this_expr(y, s.const())) COLLATE coll WITH s.=) > + WHERE (s.index_row_if(y)); > +EXCEPTION WHEN OTHERS THEN RAISE EXCEPTION '%', sqlstate; END$$; > +\set VERBOSITY default > +ROLLBACK; > --- a/src/backend/commands/indexcmds.c > +++ b/src/backend/commands/indexcmds.c > @@ -65,7 +65,10 @@ > Oid relId, > char *accessMethodName, Oid accessMethodId, > bool amcanorder, > - bool isconstraint); > + bool isconstraint, > + Oid ddl_userid, > + int ddl_sec_context, > + int *ddl_save_nestlevel); > static Oid GetIndexOpClass(List *opclass, Oid attrType, > char *accessMethodName, Oid accessMethodId); > static char *ChooseIndexName(const char *tabname, Oid namespaceId, > @@ -168,8 +171,7 @@ > * Compute the operator classes, collations, and exclusion operators for > * the new index, so we can test whether it's compatible with the > existing > * one. Note that ComputeIndexAttrs might fail here, but that's OK: > - * DefineIndex would have called this function with the same arguments > - * later on, and it would have failed then anyway. > + * DefineIndex would have failed later. > */ > indexInfo = makeNode(IndexInfo); > indexInfo->ii_Expressions = NIL; > @@ -187,7 +189,7 @@ > coloptions, attributeList, > exclusionOpNames, relationId, > accessMethodName, accessMethodId, > - amcanorder, isconstraint); > + amcanorder, isconstraint, InvalidOid, > 0, NULL); > > > /* Get the soon-obsolete pg_index tuple. */ > @@ -280,6 +282,19 @@ > * DefineIndex > * Creates a new index. > * > + * This function manages the current userid according to the needs of > pg_dump. > + * Recreating old-database catalog entries in new-database is fine, > regardless > + * of which users would have permission to recreate those entries now. > That's > + * just preservation of state. Running opaque expressions, like calling a > + * function named in a catalog entry or evaluating a pg_node_tree in a > catalog > + * entry, as anyone other than the object owner, is not fine. To adhere to > + * those principles and to remain fail-safe, use the table owner userid for > + * most ACL checks. Use the original userid for ACL checks reached without > + * traversing opaque expressions. (pg_dump can predict such ACL checks from > + * catalogs.) Overall, this is a mess. Future DDL development should > + * consider offering one DDL command for catalog setup and a separate DDL > + * command for steps that run opaque expressions. > + * > * 'relationId': the OID of the heap relation on which the index is to be > * created > * 'stmt': IndexStmt describing the properties of the new index. > @@ -581,7 +596,8 @@ > coloptions, stmt->indexParams, > stmt->excludeOpNames, relationId, > accessMethodName, accessMethodId, > - amcanorder, stmt->isconstraint); > + amcanorder, stmt->isconstraint, > root_save_userid, > + root_save_sec_context, > &root_save_nestlevel); > > /* > * Extra checks when creating a PRIMARY KEY index. > @@ -639,9 +655,8 @@ > > /* > * 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. > + * subsequent changes local to this command. This is essential if some > + * index function changed a behavior-affecting GUC, e.g. search_path. > */ > AtEOXact_GUC(false, root_save_nestlevel); > root_save_nestlevel = NewGUCNestLevel(); > @@ -996,6 +1011,10 @@ > /* > * Compute per-index-column information, including indexed column numbers > * or index expressions, opclasses, and indoptions. > + * > + * If the caller switched to the table owner, ddl_userid is the role for ACL > + * checks reached without traversing opaque expressions. Otherwise, it's > + * InvalidOid, and other ddl_* arguments are undefined. > */ > static void > ComputeIndexAttrs(IndexInfo *indexInfo, > @@ -1009,11 +1028,16 @@ > char *accessMethodName, > Oid accessMethodId, > bool amcanorder, > - bool isconstraint) > + bool isconstraint, > + Oid ddl_userid, > + int ddl_sec_context, > + int *ddl_save_nestlevel) > { > ListCell *nextExclOp; > ListCell *lc; > int attn; > + Oid save_userid; > + int save_sec_context; > > /* Allocate space for exclusion operator info, if needed */ > if (exclusionOpNames) > @@ -1029,6 +1053,9 @@ > else > nextExclOp = NULL; > > + if (OidIsValid(ddl_userid)) > + GetUserIdAndSecContext(&save_userid, &save_sec_context); > + > /* > * process attributeList > */ > @@ -1123,10 +1150,24 @@ > typeOidP[attn] = atttype; > > /* > - * Apply collation override if any > + * Apply collation override if any. Use of ddl_userid is > necessary > + * due to ACL checks therein, and it's safe because collations > don't > + * contain opaque expressions (or non-opaque expressions). > */ > if (attribute->collation) > + { > + if (OidIsValid(ddl_userid)) > + { > + AtEOXact_GUC(false, *ddl_save_nestlevel); > + SetUserIdAndSecContext(ddl_userid, > ddl_sec_context); > + } > attcollation = get_collation_oid(attribute->collation, > false); > + if (OidIsValid(ddl_userid)) > + { > + SetUserIdAndSecContext(save_userid, > save_sec_context); > + *ddl_save_nestlevel = NewGUCNestLevel(); > + } > + } > > /* > * Check we have a collation iff it's a collatable type. The > only > @@ -1154,12 +1195,25 @@ > collationOidP[attn] = attcollation; > > /* > - * Identify the opclass to use. > + * Identify the opclass to use. Use of ddl_userid is necessary > due to > + * ACL checks therein. This is safe despite opclasses > containing > + * opaque expressions (specifically, functions), because only > + * superusers can define opclasses. > */ > + if (OidIsValid(ddl_userid)) > + { > + AtEOXact_GUC(false, *ddl_save_nestlevel); > + SetUserIdAndSecContext(ddl_userid, ddl_sec_context); > + } > classOidP[attn] = GetIndexOpClass(attribute->opclass, > > atttype, > > accessMethodName, > > accessMethodId); > + if (OidIsValid(ddl_userid)) > + { > + SetUserIdAndSecContext(save_userid, save_sec_context); > + *ddl_save_nestlevel = NewGUCNestLevel(); > + } > > /* > * Identify the exclusion operator, if any. > @@ -1173,9 +1227,23 @@ > > /* > * Find the operator --- it must accept the column > datatype > - * without runtime coercion (but binary compatibility > is OK) > + * without runtime coercion (but binary compatibility > is OK). > + * Operators contain opaque expressions (specifically, > functions). > + * compatible_oper_opid() boils down to oper() and > + * IsBinaryCoercible(). PostgreSQL would have security > problems > + * elsewhere if oper() started calling opaque > expressions. > */ > + if (OidIsValid(ddl_userid)) > + { > + AtEOXact_GUC(false, *ddl_save_nestlevel); > + SetUserIdAndSecContext(ddl_userid, > ddl_sec_context); > + } > opid = compatible_oper_opid(opname, atttype, atttype, > false); > + if (OidIsValid(ddl_userid)) > + { > + SetUserIdAndSecContext(save_userid, > save_sec_context); > + *ddl_save_nestlevel = NewGUCNestLevel(); > + } > > /* > * Only allow commutative operators to be used in > exclusion -- Roberto C. Sánchez http://people.connexer.com/~roberto http://www.connexer.com
