Skip to content

Commit 00377b9

Browse files
nmischnmisch
committed
CREATE INDEX: use the original userid for more ACL checks.
Commit a117ceb 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
1 parent 2f2e24d commit 00377b9

File tree

4 files changed

+256
-16
lines changed

4 files changed

+256
-16
lines changed

‎contrib/citext/Makefile

+1-1
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ DATA = citext--1.4.sql \
1111
citext--1.0--1.1.sql
1212
PGFILEDESC = "citext - case-insensitive character string data type"
1313

14-
REGRESS = citext citext_utf8
14+
REGRESS = create_index_acl citext citext_utf8
1515

1616
ifdef USE_PGXS
1717
PG_CONFIG = pg_config

‎contrib/citext/expected/create_index_acl.out

+86
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
-- Each DefineIndex() ACL check uses either the original userid or the table
2+
-- owner userid; see its header comment. Here, confirm that DefineIndex()
3+
-- uses its original userid where necessary. The test works by creating
4+
-- indexes that refer to as many sorts of objects as possible, with the table
5+
-- owner having as few applicable privileges as possible. (The privileges.sql
6+
-- regress_sro_user tests look for the opposite defect; they confirm that
7+
-- DefineIndex() uses the table owner userid where necessary.)
8+
SET allow_in_place_tablespaces = true;
9+
CREATE TABLESPACE regress_create_idx_tblspace LOCATION '';
10+
RESET allow_in_place_tablespaces;
11+
BEGIN;
12+
CREATE ROLE regress_minimal;
13+
CREATE SCHEMA s;
14+
CREATE EXTENSION citext SCHEMA s;
15+
-- Revoke all conceivably-relevant ACLs within the extension. The system
16+
-- doesn't check all these ACLs, but this will provide some coverage if that
17+
-- ever changes.
18+
REVOKE ALL ON TYPE s.citext FROM PUBLIC;
19+
REVOKE ALL ON FUNCTION s.citext_pattern_lt FROM PUBLIC;
20+
REVOKE ALL ON FUNCTION s.citext_pattern_le FROM PUBLIC;
21+
REVOKE ALL ON FUNCTION s.citext_eq FROM PUBLIC;
22+
REVOKE ALL ON FUNCTION s.citext_pattern_ge FROM PUBLIC;
23+
REVOKE ALL ON FUNCTION s.citext_pattern_gt FROM PUBLIC;
24+
REVOKE ALL ON FUNCTION s.citext_pattern_cmp FROM PUBLIC;
25+
-- Functions sufficient for making an index column that has the side effect of
26+
-- changing search_path at expression planning time.
27+
CREATE FUNCTION public.setter() RETURNS bool VOLATILE
28+
LANGUAGE SQL AS $$SET search_path = s; SELECT true$$;
29+
CREATE FUNCTION s.const() RETURNS bool IMMUTABLE
30+
LANGUAGE SQL AS $$SELECT public.setter()$$;
31+
CREATE FUNCTION s.index_this_expr(s.citext, bool) RETURNS s.citext IMMUTABLE
32+
LANGUAGE SQL AS $$SELECT $1$$;
33+
REVOKE ALL ON FUNCTION public.setter FROM PUBLIC;
34+
REVOKE ALL ON FUNCTION s.const FROM PUBLIC;
35+
REVOKE ALL ON FUNCTION s.index_this_expr FROM PUBLIC;
36+
-- Even for an empty table, expression planning calls s.const & public.setter.
37+
GRANT EXECUTE ON FUNCTION public.setter TO regress_minimal;
38+
GRANT EXECUTE ON FUNCTION s.const TO regress_minimal;
39+
-- Function for index predicate.
40+
CREATE FUNCTION s.index_row_if(s.citext) RETURNS bool IMMUTABLE
41+
LANGUAGE SQL AS $$SELECT $1 IS NOT NULL$$;
42+
REVOKE ALL ON FUNCTION s.index_row_if FROM PUBLIC;
43+
-- Even for an empty table, CREATE INDEX checks ii_Predicate permissions.
44+
GRANT EXECUTE ON FUNCTION s.index_row_if TO regress_minimal;
45+
-- Non-extension, non-function objects.
46+
CREATE COLLATION s.coll (LOCALE="C");
47+
CREATE TABLE s.x (y s.citext);
48+
ALTER TABLE s.x OWNER TO regress_minimal;
49+
-- Empty-table DefineIndex()
50+
CREATE UNIQUE INDEX u0rows ON s.x USING btree
51+
((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_pattern_ops)
52+
TABLESPACE regress_create_idx_tblspace
53+
WHERE s.index_row_if(y);
54+
ALTER TABLE s.x ADD CONSTRAINT e0rows EXCLUDE USING btree
55+
((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=)
56+
USING INDEX TABLESPACE regress_create_idx_tblspace
57+
WHERE (s.index_row_if(y));
58+
-- Make the table nonempty.
59+
INSERT INTO s.x VALUES ('foo'), ('bar');
60+
-- If the INSERT runs the planner on index expressions, a search_path change
61+
-- survives. As of 2022-06, the INSERT reuses a cached plan. It does so even
62+
-- under debug_discard_caches, since each index is new-in-transaction. If
63+
-- future work changes a cache lifecycle, this RESET may become necessary.
64+
RESET search_path;
65+
-- For a nonempty table, owner needs permissions throughout ii_Expressions.
66+
GRANT EXECUTE ON FUNCTION s.index_this_expr TO regress_minimal;
67+
CREATE UNIQUE INDEX u2rows ON s.x USING btree
68+
((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_pattern_ops)
69+
TABLESPACE regress_create_idx_tblspace
70+
WHERE s.index_row_if(y);
71+
ALTER TABLE s.x ADD CONSTRAINT e2rows EXCLUDE USING btree
72+
((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=)
73+
USING INDEX TABLESPACE regress_create_idx_tblspace
74+
WHERE (s.index_row_if(y));
75+
-- Shall not find s.coll via search_path, despite the s.const->public.setter
76+
-- call having set search_path=s during expression planning. Suppress the
77+
-- message itself, which depends on the database encoding.
78+
\set VERBOSITY sqlstate
79+
ALTER TABLE s.x ADD CONSTRAINT underqualified EXCLUDE USING btree
80+
((s.index_this_expr(y, s.const())) COLLATE coll WITH s.=)
81+
USING INDEX TABLESPACE regress_create_idx_tblspace
82+
WHERE (s.index_row_if(y));
83+
ERROR: 42704
84+
\set VERBOSITY default
85+
ROLLBACK;
86+
DROP TABLESPACE regress_create_idx_tblspace;

‎contrib/citext/sql/create_index_acl.sql

+88
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
-- Each DefineIndex() ACL check uses either the original userid or the table
2+
-- owner userid; see its header comment. Here, confirm that DefineIndex()
3+
-- uses its original userid where necessary. The test works by creating
4+
-- indexes that refer to as many sorts of objects as possible, with the table
5+
-- owner having as few applicable privileges as possible. (The privileges.sql
6+
-- regress_sro_user tests look for the opposite defect; they confirm that
7+
-- DefineIndex() uses the table owner userid where necessary.)
8+
9+
SET allow_in_place_tablespaces = true;
10+
CREATE TABLESPACE regress_create_idx_tblspace LOCATION '';
11+
RESET allow_in_place_tablespaces;
12+
13+
BEGIN;
14+
CREATE ROLE regress_minimal;
15+
CREATE SCHEMA s;
16+
CREATE EXTENSION citext SCHEMA s;
17+
-- Revoke all conceivably-relevant ACLs within the extension. The system
18+
-- doesn't check all these ACLs, but this will provide some coverage if that
19+
-- ever changes.
20+
REVOKE ALL ON TYPE s.citext FROM PUBLIC;
21+
REVOKE ALL ON FUNCTION s.citext_pattern_lt FROM PUBLIC;
22+
REVOKE ALL ON FUNCTION s.citext_pattern_le FROM PUBLIC;
23+
REVOKE ALL ON FUNCTION s.citext_eq FROM PUBLIC;
24+
REVOKE ALL ON FUNCTION s.citext_pattern_ge FROM PUBLIC;
25+
REVOKE ALL ON FUNCTION s.citext_pattern_gt FROM PUBLIC;
26+
REVOKE ALL ON FUNCTION s.citext_pattern_cmp FROM PUBLIC;
27+
-- Functions sufficient for making an index column that has the side effect of
28+
-- changing search_path at expression planning time.
29+
CREATE FUNCTION public.setter() RETURNS bool VOLATILE
30+
LANGUAGE SQL AS $$SET search_path = s; SELECT true$$;
31+
CREATE FUNCTION s.const() RETURNS bool IMMUTABLE
32+
LANGUAGE SQL AS $$SELECT public.setter()$$;
33+
CREATE FUNCTION s.index_this_expr(s.citext, bool) RETURNS s.citext IMMUTABLE
34+
LANGUAGE SQL AS $$SELECT $1$$;
35+
REVOKE ALL ON FUNCTION public.setter FROM PUBLIC;
36+
REVOKE ALL ON FUNCTION s.const FROM PUBLIC;
37+
REVOKE ALL ON FUNCTION s.index_this_expr FROM PUBLIC;
38+
-- Even for an empty table, expression planning calls s.const & public.setter.
39+
GRANT EXECUTE ON FUNCTION public.setter TO regress_minimal;
40+
GRANT EXECUTE ON FUNCTION s.const TO regress_minimal;
41+
-- Function for index predicate.
42+
CREATE FUNCTION s.index_row_if(s.citext) RETURNS bool IMMUTABLE
43+
LANGUAGE SQL AS $$SELECT $1 IS NOT NULL$$;
44+
REVOKE ALL ON FUNCTION s.index_row_if FROM PUBLIC;
45+
-- Even for an empty table, CREATE INDEX checks ii_Predicate permissions.
46+
GRANT EXECUTE ON FUNCTION s.index_row_if TO regress_minimal;
47+
-- Non-extension, non-function objects.
48+
CREATE COLLATION s.coll (LOCALE="C");
49+
CREATE TABLE s.x (y s.citext);
50+
ALTER TABLE s.x OWNER TO regress_minimal;
51+
-- Empty-table DefineIndex()
52+
CREATE UNIQUE INDEX u0rows ON s.x USING btree
53+
((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_pattern_ops)
54+
TABLESPACE regress_create_idx_tblspace
55+
WHERE s.index_row_if(y);
56+
ALTER TABLE s.x ADD CONSTRAINT e0rows EXCLUDE USING btree
57+
((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=)
58+
USING INDEX TABLESPACE regress_create_idx_tblspace
59+
WHERE (s.index_row_if(y));
60+
-- Make the table nonempty.
61+
INSERT INTO s.x VALUES ('foo'), ('bar');
62+
-- If the INSERT runs the planner on index expressions, a search_path change
63+
-- survives. As of 2022-06, the INSERT reuses a cached plan. It does so even
64+
-- under debug_discard_caches, since each index is new-in-transaction. If
65+
-- future work changes a cache lifecycle, this RESET may become necessary.
66+
RESET search_path;
67+
-- For a nonempty table, owner needs permissions throughout ii_Expressions.
68+
GRANT EXECUTE ON FUNCTION s.index_this_expr TO regress_minimal;
69+
CREATE UNIQUE INDEX u2rows ON s.x USING btree
70+
((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_pattern_ops)
71+
TABLESPACE regress_create_idx_tblspace
72+
WHERE s.index_row_if(y);
73+
ALTER TABLE s.x ADD CONSTRAINT e2rows EXCLUDE USING btree
74+
((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=)
75+
USING INDEX TABLESPACE regress_create_idx_tblspace
76+
WHERE (s.index_row_if(y));
77+
-- Shall not find s.coll via search_path, despite the s.const->public.setter
78+
-- call having set search_path=s during expression planning. Suppress the
79+
-- message itself, which depends on the database encoding.
80+
\set VERBOSITY sqlstate
81+
ALTER TABLE s.x ADD CONSTRAINT underqualified EXCLUDE USING btree
82+
((s.index_this_expr(y, s.const())) COLLATE coll WITH s.=)
83+
USING INDEX TABLESPACE regress_create_idx_tblspace
84+
WHERE (s.index_row_if(y));
85+
\set VERBOSITY default
86+
ROLLBACK;
87+
88+
DROP TABLESPACE regress_create_idx_tblspace;

‎src/backend/commands/indexcmds.c

+81-15
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,10 @@ static void ComputeIndexAttrs(IndexInfo *indexInfo,
8080
Oid relId,
8181
const char *accessMethodName, Oid accessMethodId,
8282
bool amcanorder,
83-
bool isconstraint);
83+
bool isconstraint,
84+
Oid ddl_userid,
85+
int ddl_sec_context,
86+
int *ddl_save_nestlevel);
8487
static char *ChooseIndexName(const char *tabname, Oid namespaceId,
8588
List *colnames, List *exclusionOpNames,
8689
bool primary, bool isconstraint);
@@ -220,9 +223,8 @@ CheckIndexCompatible(Oid oldId,
220223
* Compute the operator classes, collations, and exclusion operators for
221224
* the new index, so we can test whether it's compatible with the existing
222225
* one. Note that ComputeIndexAttrs might fail here, but that's OK:
223-
* DefineIndex would have called this function with the same arguments
224-
* later on, and it would have failed then anyway. Our attributeList
225-
* contains only key attributes, thus we're filling ii_NumIndexAttrs and
226+
* DefineIndex would have failed later. Our attributeList contains only
227+
* key attributes, thus we're filling ii_NumIndexAttrs and
226228
* ii_NumIndexKeyAttrs with same value.
227229
*/
228230
indexInfo = makeIndexInfo(numberOfAttributes, numberOfAttributes,
@@ -236,7 +238,7 @@ CheckIndexCompatible(Oid oldId,
236238
coloptions, attributeList,
237239
exclusionOpNames, relationId,
238240
accessMethodName, accessMethodId,
239-
amcanorder, isconstraint);
241+
amcanorder, isconstraint, InvalidOid, 0, NULL);
240242

241243

242244
/* Get the soon-obsolete pg_index tuple. */
@@ -482,6 +484,19 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool progress)
482484
* DefineIndex
483485
* Creates a new index.
484486
*
487+
* This function manages the current userid according to the needs of pg_dump.
488+
* Recreating old-database catalog entries in new-database is fine, regardless
489+
* of which users would have permission to recreate those entries now. That's
490+
* just preservation of state. Running opaque expressions, like calling a
491+
* function named in a catalog entry or evaluating a pg_node_tree in a catalog
492+
* entry, as anyone other than the object owner, is not fine. To adhere to
493+
* those principles and to remain fail-safe, use the table owner userid for
494+
* most ACL checks. Use the original userid for ACL checks reached without
495+
* traversing opaque expressions. (pg_dump can predict such ACL checks from
496+
* catalogs.) Overall, this is a mess. Future DDL development should
497+
* consider offering one DDL command for catalog setup and a separate DDL
498+
* command for steps that run opaque expressions.
499+
*
485500
* 'relationId': the OID of the heap relation on which the index is to be
486501
* created
487502
* 'stmt': IndexStmt describing the properties of the new index.
@@ -890,7 +905,8 @@ DefineIndex(Oid relationId,
890905
coloptions, allIndexParams,
891906
stmt->excludeOpNames, relationId,
892907
accessMethodName, accessMethodId,
893-
amcanorder, stmt->isconstraint);
908+
amcanorder, stmt->isconstraint, root_save_userid,
909+
root_save_sec_context, &root_save_nestlevel);
894910

895911
/*
896912
* Extra checks when creating a PRIMARY KEY index.
@@ -1170,11 +1186,8 @@ DefineIndex(Oid relationId,
11701186

11711187
/*
11721188
* Roll back any GUC changes executed by index functions, and keep
1173-
* subsequent changes local to this command. It's barely possible that
1174-
* some index function changed a behavior-affecting GUC, e.g. xmloption,
1175-
* that affects subsequent steps. This improves bug-compatibility with
1176-
* older PostgreSQL versions. They did the AtEOXact_GUC() here for the
1177-
* purpose of clearing the above default_tablespace change.
1189+
* subsequent changes local to this command. This is essential if some
1190+
* index function changed a behavior-affecting GUC, e.g. search_path.
11781191
*/
11791192
AtEOXact_GUC(false, root_save_nestlevel);
11801193
root_save_nestlevel = NewGUCNestLevel();
@@ -1730,6 +1743,10 @@ CheckPredicate(Expr *predicate)
17301743
* Compute per-index-column information, including indexed column numbers
17311744
* or index expressions, opclasses and their options. Note, all output vectors
17321745
* should be allocated for all columns, including "including" ones.
1746+
*
1747+
* If the caller switched to the table owner, ddl_userid is the role for ACL
1748+
* checks reached without traversing opaque expressions. Otherwise, it's
1749+
* InvalidOid, and other ddl_* arguments are undefined.
17331750
*/
17341751
static void
17351752
ComputeIndexAttrs(IndexInfo *indexInfo,
@@ -1743,12 +1760,17 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
17431760
const char *accessMethodName,
17441761
Oid accessMethodId,
17451762
bool amcanorder,
1746-
bool isconstraint)
1763+
bool isconstraint,
1764+
Oid ddl_userid,
1765+
int ddl_sec_context,
1766+
int *ddl_save_nestlevel)
17471767
{
17481768
ListCell *nextExclOp;
17491769
ListCell *lc;
17501770
int attn;
17511771
int nkeycols = indexInfo->ii_NumIndexKeyAttrs;
1772+
Oid save_userid;
1773+
int save_sec_context;
17521774

17531775
/* Allocate space for exclusion operator info, if needed */
17541776
if (exclusionOpNames)
@@ -1762,6 +1784,9 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
17621784
else
17631785
nextExclOp = NULL;
17641786

1787+
if (OidIsValid(ddl_userid))
1788+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
1789+
17651790
/*
17661791
* process attributeList
17671792
*/
@@ -1892,10 +1917,24 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
18921917
}
18931918

18941919
/*
1895-
* Apply collation override if any
1920+
* Apply collation override if any. Use of ddl_userid is necessary
1921+
* due to ACL checks therein, and it's safe because collations don't
1922+
* contain opaque expressions (or non-opaque expressions).
18961923
*/
18971924
if (attribute->collation)
1925+
{
1926+
if (OidIsValid(ddl_userid))
1927+
{
1928+
AtEOXact_GUC(false, *ddl_save_nestlevel);
1929+
SetUserIdAndSecContext(ddl_userid, ddl_sec_context);
1930+
}
18981931
attcollation = get_collation_oid(attribute->collation, false);
1932+
if (OidIsValid(ddl_userid))
1933+
{
1934+
SetUserIdAndSecContext(save_userid, save_sec_context);
1935+
*ddl_save_nestlevel = NewGUCNestLevel();
1936+
}
1937+
}
18991938

19001939
/*
19011940
* Check we have a collation iff it's a collatable type. The only
@@ -1923,12 +1962,25 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
19231962
collationOidP[attn] = attcollation;
19241963

19251964
/*
1926-
* Identify the opclass to use.
1965+
* Identify the opclass to use. Use of ddl_userid is necessary due to
1966+
* ACL checks therein. This is safe despite opclasses containing
1967+
* opaque expressions (specifically, functions), because only
1968+
* superusers can define opclasses.
19271969
*/
1970+
if (OidIsValid(ddl_userid))
1971+
{
1972+
AtEOXact_GUC(false, *ddl_save_nestlevel);
1973+
SetUserIdAndSecContext(ddl_userid, ddl_sec_context);
1974+
}
19281975
classOidP[attn] = ResolveOpClass(attribute->opclass,
19291976
atttype,
19301977
accessMethodName,
19311978
accessMethodId);
1979+
if (OidIsValid(ddl_userid))
1980+
{
1981+
SetUserIdAndSecContext(save_userid, save_sec_context);
1982+
*ddl_save_nestlevel = NewGUCNestLevel();
1983+
}
19321984

19331985
/*
19341986
* Identify the exclusion operator, if any.
@@ -1942,9 +1994,23 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
19421994

19431995
/*
19441996
* Find the operator --- it must accept the column datatype
1945-
* without runtime coercion (but binary compatibility is OK)
1997+
* without runtime coercion (but binary compatibility is OK).
1998+
* Operators contain opaque expressions (specifically, functions).
1999+
* compatible_oper_opid() boils down to oper() and
2000+
* IsBinaryCoercible(). PostgreSQL would have security problems
2001+
* elsewhere if oper() started calling opaque expressions.
19462002
*/
2003+
if (OidIsValid(ddl_userid))
2004+
{
2005+
AtEOXact_GUC(false, *ddl_save_nestlevel);
2006+
SetUserIdAndSecContext(ddl_userid, ddl_sec_context);
2007+
}
19472008
opid = compatible_oper_opid(opname, atttype, atttype, false);
2009+
if (OidIsValid(ddl_userid))
2010+
{
2011+
SetUserIdAndSecContext(save_userid, save_sec_context);
2012+
*ddl_save_nestlevel = NewGUCNestLevel();
2013+
}
19482014

19492015
/*
19502016
* Only allow commutative operators to be used in exclusion

0 commit comments

Comments
 (0)