Re: Request for assistance to backport CVE-2022-1552 fixes to 9.6 and 9.4

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?= <roberto(at)debian(dot)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 <noah(at)leadboat(dot)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 <noah(at)leadboat(dot)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 <noah(at)leadboat(dot)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/f8a4105f076544c180a87ef0c4822352@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 <noah(at)leadboat(dot)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 <noah(at)leadboat(dot)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 <noah(at)leadboat(dot)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/f8a4105f076544c180a87ef0c4822352@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