From: Noah Misch <noah@leadboat.com>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-bugs@lists.postgresql.org, Färber, Franz-Josef (StMUK) <Franz-Josef.Faerber@stmuk.bayern.de>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: Extension pg_trgm, permissions and pg_dump order
Date: Wed, 15 Jun 2022 22:42:18 -0700
Message-ID: <20220616054218.GA3980475@rfd.leadboat.com> (raw)
In-Reply-To: <20220613034551.GB3728437@rfd.leadboat.com>
References: <20220526055047.GA3153526@rfd.leadboat.com>
<20220528045122.GA3228881@rfd.leadboat.com>
<20220529033033.GA1973706@nathanxps13>
<20220529043403.GB3265235@rfd.leadboat.com>
<YpWZ02YEN9pipA6M@paquier.xyz>
<20220531152855.GA2236210@nathanxps13>
<20220603181724.GA2672350@nathanxps13>
<20220606063056.GA3527347@rfd.leadboat.com>
<20220609051708.GA3615673@rfd.leadboat.com>
<20220613034551.GB3728437@rfd.leadboat.com>
On Sun, Jun 12, 2022 at 08:45:51PM -0700, Noah Misch wrote:
> On Wed, Jun 08, 2022 at 10:17:08PM -0700, Noah Misch wrote:
> > On Sun, Jun 05, 2022 at 11:30:56PM -0700, Noah Misch wrote:
> > > On Fri, Jun 03, 2022 at 11:17:24AM -0700, Nathan Bossart wrote:
> > > > On Tue, May 31, 2022 at 08:28:55AM -0700, Nathan Bossart wrote:
> > > > > I've spent some time looking at all the code impacted by a117ceb and
> > > > > 0abc1a0, and I've yet to identify any additional problems besides the one
> > > > > with ResolveOpClass(). However, I'm far from confident in this analysis,
> > > > > and I still need to try out the ereport() approach that Noah suggested. I
> > > > > would welcome any assistance identifying other problem areas.
> > > >
> > > > Ah, I think I am missing something. For example, the code that identifies
> > > > the exclusion operator (immediately following the call to ResolveOpClass())
> > > > is likely subject to a similar problem.
> > >
> > > That would not surprise me. Do you have what you need to continue?
> >
> > I'll use that window on Sunday to review what you have. If you have work in
> > progress, please get it to the mailing list by then.
>
> I have the patch mostly done. I will send something late on Wednesday.
I also found post-202205 CREATE INDEX reaching an improper "permission denied"
for the namespace containing a collation. The attached version fixes and
tests all three improper denials. Also, I added discards of GUC changes
before switching to the original userid. Without those, an index expression
changing search_path affects the collation lookup. Unfortunately, this is
making ComputeIndexAttrs() look like an unplanned mess. I considered two
alternatives:
1. Move all "List *names -> oid" translation activities from
ComputeIndexAttrs() to transformIndexStmt(). I ruled this out for a
back-patched fix, because such a change would tend to break compatibility
with the PGXN modules that call transformIndexStmt() or DefineIndex(). It
might be a good long-term direction, though the note in the header of
transformIndexStmt() gives another obstacle.
2. Move all "List *names -> oid" translation activities from
ComputeIndexAttrs() to DefineIndex(), before table_open(). There's nothing
ruling out this option, but I ran out of time to try it. It would be weird
to have per-attribute logic in both DefineIndex() and ComputeIndexAttrs(),
so I'm skeptical about this helping.
For partitioned tables, it would add good defense-in-depth to perform the
"List *names -> oid" translations once per SQL statement, not once per
partition as has been the case. (1) would move in that direction.
Thanks,
nm
Attachments:create-index-non-opaque-acl-v4nm.patch (text/plain, 17.5K) download
Author: Noah Misch <noah@leadboat.com>
Commit: Noah Misch <noah@leadboat.com>
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, reviewed by FIXME.
Discussion: https://postgr.es/m/f8a4105f076544c180a87ef0c4822352@stmuk.bayern.de
diff --git a/contrib/citext/Makefile b/contrib/citext/Makefileindex 789932f..35db6ea 100644--- a/contrib/citext/Makefile+++ b/contrib/citext/Makefile@@ -11,7 +11,7 @@ DATA = citext--1.4.sql \
citext--1.0--1.1.sql
PGFILEDESC = "citext - case-insensitive character string data type"
-REGRESS = citext citext_utf8+REGRESS = create_index_acl citext citext_utf8
ifdef USE_PGXS
PG_CONFIG = pg_config
diff --git a/contrib/citext/expected/create_index_acl.out b/contrib/citext/expected/create_index_acl.outnew file mode 100644index 0000000..ec7a742--- /dev/null+++ b/contrib/citext/expected/create_index_acl.out@@ -0,0 +1,80 @@+-- 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.)+SET allow_in_place_tablespaces = true;+CREATE TABLESPACE regress_create_idx_tblspace LOCATION '';+RESET 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_pattern_lt FROM PUBLIC;+REVOKE ALL ON FUNCTION s.citext_pattern_le FROM PUBLIC;+REVOKE ALL ON FUNCTION s.citext_eq FROM PUBLIC;+REVOKE ALL ON FUNCTION s.citext_pattern_ge FROM PUBLIC;+REVOKE ALL ON FUNCTION s.citext_pattern_gt FROM PUBLIC;+REVOKE ALL ON FUNCTION s.citext_pattern_cmp 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 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 FROM PUBLIC;+-- Even for an empty table, CREATE INDEX checks ii_Predicate permissions.+GRANT EXECUTE ON FUNCTION s.index_row_if 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_pattern_ops)+ TABLESPACE regress_create_idx_tblspace+ 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.=)+ USING INDEX TABLESPACE regress_create_idx_tblspace+ WHERE (s.index_row_if(y));+-- For a nonempty table, owner needs permissions throughout ii_Expressions.+INSERT INTO s.x VALUES ('foo'), ('bar');+GRANT EXECUTE ON FUNCTION s.index_this_expr TO regress_minimal;+CREATE UNIQUE INDEX u2rows ON s.x USING btree+ ((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_pattern_ops)+ TABLESPACE regress_create_idx_tblspace+ 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.=)+ USING INDEX TABLESPACE regress_create_idx_tblspace+ 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 sqlstate+ALTER TABLE s.x ADD CONSTRAINT underqualified EXCLUDE USING btree+ ((s.index_this_expr(y, s.const())) COLLATE coll WITH s.=)+ USING INDEX TABLESPACE regress_create_idx_tblspace+ WHERE (s.index_row_if(y));+ERROR: 42704+\set VERBOSITY default+ROLLBACK;+DROP TABLESPACE regress_create_idx_tblspace;diff --git a/contrib/citext/sql/create_index_acl.sql b/contrib/citext/sql/create_index_acl.sqlnew file mode 100644index 0000000..f739522--- /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.)++SET allow_in_place_tablespaces = true;+CREATE TABLESPACE regress_create_idx_tblspace LOCATION '';+RESET 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_pattern_lt FROM PUBLIC;+REVOKE ALL ON FUNCTION s.citext_pattern_le FROM PUBLIC;+REVOKE ALL ON FUNCTION s.citext_eq FROM PUBLIC;+REVOKE ALL ON FUNCTION s.citext_pattern_ge FROM PUBLIC;+REVOKE ALL ON FUNCTION s.citext_pattern_gt FROM PUBLIC;+REVOKE ALL ON FUNCTION s.citext_pattern_cmp 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 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 FROM PUBLIC;+-- Even for an empty table, CREATE INDEX checks ii_Predicate permissions.+GRANT EXECUTE ON FUNCTION s.index_row_if 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_pattern_ops)+ TABLESPACE regress_create_idx_tblspace+ 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.=)+ USING INDEX TABLESPACE regress_create_idx_tblspace+ WHERE (s.index_row_if(y));+-- For a nonempty table, owner needs permissions throughout ii_Expressions.+INSERT INTO s.x VALUES ('foo'), ('bar');+GRANT EXECUTE ON FUNCTION s.index_this_expr TO regress_minimal;+CREATE UNIQUE INDEX u2rows ON s.x USING btree+ ((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_pattern_ops)+ TABLESPACE regress_create_idx_tblspace+ 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.=)+ USING INDEX TABLESPACE regress_create_idx_tblspace+ 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 sqlstate+ALTER TABLE s.x ADD CONSTRAINT underqualified EXCLUDE USING btree+ ((s.index_this_expr(y, s.const())) COLLATE coll WITH s.=)+ USING INDEX TABLESPACE regress_create_idx_tblspace+ WHERE (s.index_row_if(y));+\set VERBOSITY default+ROLLBACK;++DROP TABLESPACE regress_create_idx_tblspace;diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.cindex eac13ac..99f5ab8 100644--- a/src/backend/commands/indexcmds.c+++ b/src/backend/commands/indexcmds.c@@ -80,7 +80,10 @@ static void ComputeIndexAttrs(IndexInfo *indexInfo,
Oid relId,
const char *accessMethodName, Oid accessMethodId,
bool amcanorder,
- bool isconstraint);+ bool isconstraint,+ Oid ddl_userid,+ int ddl_sec_context,+ int *ddl_save_nestlevel);
static char *ChooseIndexName(const char *tabname, Oid namespaceId,
List *colnames, List *exclusionOpNames,
bool primary, bool isconstraint);
@@ -220,9 +223,8 @@ CheckIndexCompatible(Oid oldId,
* 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. Our attributeList- * contains only key attributes, thus we're filling ii_NumIndexAttrs and+ * DefineIndex would have failed later. Our attributeList contains only+ * key attributes, thus we're filling ii_NumIndexAttrs and
* ii_NumIndexKeyAttrs with same value.
*/
indexInfo = makeIndexInfo(numberOfAttributes, numberOfAttributes,
@@ -236,7 +238,7 @@ CheckIndexCompatible(Oid oldId,
coloptions, attributeList,
exclusionOpNames, relationId,
accessMethodName, accessMethodId,
- amcanorder, isconstraint);+ amcanorder, isconstraint, InvalidOid, 0, NULL);
/* Get the soon-obsolete pg_index tuple. */
@@ -482,6 +484,19 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool progress)
* 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.
@@ -890,7 +905,8 @@ DefineIndex(Oid relationId,
coloptions, allIndexParams,
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.
@@ -1170,11 +1186,8 @@ DefineIndex(Oid relationId,
/*
* 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. This improves bug-compatibility with- * older PostgreSQL versions. They did the AtEOXact_GUC() here for the- * purpose of clearing the above default_tablespace change.+ * 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();
@@ -1730,6 +1743,10 @@ CheckPredicate(Expr *predicate)
* Compute per-index-column information, including indexed column numbers
* or index expressions, opclasses and their options. Note, all output vectors
* should be allocated for all columns, including "including" ones.
+ *+ * 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,
@@ -1743,12 +1760,17 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
const 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;
int nkeycols = indexInfo->ii_NumIndexKeyAttrs;
+ Oid save_userid;+ int save_sec_context;
/* Allocate space for exclusion operator info, if needed */
if (exclusionOpNames)
@@ -1762,6 +1784,9 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
else
nextExclOp = NULL;
+ if (OidIsValid(ddl_userid))+ GetUserIdAndSecContext(&save_userid, &save_sec_context);+
/*
* process attributeList
*/
@@ -1892,10 +1917,24 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
}
/*
- * 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
@@ -1923,12 +1962,25 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
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] = ResolveOpClass(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.
@@ -1942,9 +1994,23 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
/*
* 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: noah@leadboat.com, nathandbossart@gmail.com, michael@paquier.xyz, Franz-Josef.Faerber@stmuk.bayern.de, tgl@sss.pgh.pa.us
Subject: Re: Extension pg_trgm, permissions and pg_dump order
In-Reply-To: <20220616054218.GA3980475@rfd.leadboat.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox