8000 CREATE INDEX: use the original userid for more ACL checks. · postgres/postgres@6d49cc2 · GitHub
[go: up one dir, main page]
Skip to content
8000

Commit 6d49cc2

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 c41edb3 commit 6d49cc2

File tree

4 files changed

+245
-14
lines changed

4 files changed

+245
-14
lines changed

‎contrib/citext/Makefile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ DATA = citext--1.4.sql citext--1.4--1.5.sql \
99
citext--1.0--1.1.sql citext--unpackaged--1.0.sql
1010
PGFILEDESC = "citext - case-insensitive character string data type"
1111

12-
REGRESS = citext
12+
REGRESS = create_index_acl citext
1313

1414
ifdef USE_PGXS
1515
PG_CONFIG = pg_config

‎contrib/citext/expected/create_index_acl.out

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
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+
-- Don't override tablespaces; this version lacks allow_in_place_tablespaces.
9+
BEGIN;
10+
CREATE ROLE regress_minimal;
11+
CREATE SCHEMA s;
12+
CREATE EXTENSION citext SCHEMA s;
13+
-- Revoke all conceivably-relevant ACLs within the extension. The system
14+
-- doesn't check all these ACLs, but this will provide some coverage if that
15+
-- ever changes.
16+
REVOKE ALL ON TYPE s.citext FROM PUBLIC;
17+
REVOKE ALL ON FUNCTION s.citext_pattern_lt FROM PUBLIC;
18+
REVOKE ALL ON FUNCTION s.citext_pattern_le FROM PUBLIC;
19+
REVOKE ALL ON FUNCTION s.citext_eq FROM PUBLIC;
20+
REVOKE ALL ON FUNCTION s.citext_pattern_ge FROM PUBLIC;
21+
REVOKE ALL ON FUNCTION s.citext_pattern_gt FROM PUBLIC;
22+
REVOKE ALL ON FUNCTION s.citext_pattern_cmp FROM PUBLIC;
23+
-- Functions sufficient for making an index column that has the side effect of
24+
-- changing search_path at expression planning time.
25+
CREATE FUNCTION public.setter() RETURNS bool VOLATILE
26+
LANGUAGE SQL AS $$SET search_path = s; SELECT true$$;
27+
CREATE FUNCTION s.const() RETURNS bool IMMUTABLE
28+
LANGUAGE SQL AS $$SELECT public.setter()$$;
29+
CREATE FUNCTION s.index_this_expr(s.citext, bool) RETURNS s.citext IMMUTABLE
30+
LANGUAGE SQL AS $$SELECT $1$$;
31+
REVOKE ALL ON FUNCTION public.setter FROM PUBLIC;
32+
REVOKE ALL ON FUNCTION s.const FROM PUBLIC;
33+
REVOKE ALL ON FUNCTION s.index_this_expr FROM PUBLIC;
34+
-- Even for an empty table, expression planning calls s.const & public.setter.
35+
GRANT EXECUTE ON FUNCTION public.setter TO regress_minimal;
36+
GRANT EXECUTE ON FUNCTION s.const TO regress_minimal;
37+
-- Function for index predicate.
38+
CREATE FUNCTION s.index_row_if(s.citext) RETURNS bool IMMUTABLE
39+
LANGUAGE SQL AS $$SELECT $1 IS NOT NULL$$;
40+
REVOKE ALL ON FUNCTION s.index_row_if FROM PUBLIC;
41+
-- Even for an empty table, CREATE INDEX checks ii_Predicate permissions.
42+
GRANT EXECUTE ON FUNCTION s.index_row_if TO regress_minimal;
43+
-- Non-extension, non-function objects.
44+
CREATE COLLATION s.coll (LOCALE="C");
45+
CREATE TABLE s.x (y s.citext);
46+
ALTER TABLE s.x OWNER TO regress_minimal;
47+
-- Empty-table DefineIndex()
48+
CREATE UNIQUE INDEX u0rows ON s.x USING btree
49+
((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_pattern_ops)
50+
WHERE s.index_row_if(y);
51+
ALTER TABLE s.x ADD CONSTRAINT e0rows EXCLUDE USING btree
52+
((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=)
53+
WHERE (s.index_row_if(y));
54+
-- Make the table nonempty.
55+
INSERT INTO s.x VALUES ('foo'), ('bar');
56+
-- If the INSERT runs the planner on index expressions, a search_path change
57+
-- survives. As of 2022-06, the INSERT reuses a cached plan. It does so even
58+
-- under debug_discard_caches, since each index is new-in-transaction. If
59+
-- future work changes a cache lifecycle, this RESET may become necessary.
60+
RESET search_path;
61+
-- For a nonempty table, owner needs permissions throughout ii_Expressions.
62+
GRANT EXECUTE ON FUNCTION s.index_this_expr TO regress_minimal;
63+
CREATE UNIQUE INDEX u2rows ON s.x USING btree
64+
((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_pattern_ops)
65+
WHERE s.index_row_if(y);
66+
ALTER TABLE s.x ADD CONSTRAINT e2rows EXCLUDE USING btree
67+
((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=)
68+
WHERE (s.index_row_if(y));
69+
-- Shall not find s.coll via search_path, despite the s.const->public.setter
70+
-- call having set search_path=s during expression planning. Suppress the
71+
-- message itself, which depends on the database encoding.
72+
\set VERBOSITY terse
73+
DO $$
74+
BEGIN
75+
ALTER TABLE s.x ADD CONSTRAINT underqualified EXCLUDE USING btree
76+
((s.index_this_expr(y, s.const())) COLLATE coll WITH s.=)
77+
WHERE (s.index_row_if(y));
78+
EXCEPTION WHEN OTHERS THEN RAISE EXCEPTION '%', sqlstate; END$$;
79+
ERROR: 42704
80+
\set VERBOSITY default
81+
ROLLBACK;

‎contrib/citext/sql/create_index_acl.sql

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
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 F438 +
-- regress_sro_user tests look for the opposite defect; they confirm that
7+
-- DefineIndex() uses the table owner userid where necessary.)
8+
9+
-- Don't override tablespaces; this version lacks allow_in_place_tablespaces.
10+
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+
WHERE s.index_row_if(y);
53+
ALTER TABLE s.x ADD CONSTRAINT e0rows EXCLUDE USING btree
54+
((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=)
55+
WHERE (s.index_row_if(y));
56+
-- Make the table nonempty.
57+
INSERT INTO s.x VALUES ('foo'), ('bar');
58+
-- If the INSERT runs the planner on index expressions, a search_path change
59+
-- survives. As of 2022-06, the INSERT reuses a cached plan. It does so even
60+
-- under debug_discard_caches, since each index is new-in-transaction. If
61+
-- future work changes a cache lifecycle, this RESET may become necessary.
62+
RESET search_path;
63+
-- For a nonempty table, owner needs permissions throughout ii_Expressions.
64+
GRANT EXECUTE ON FUNCTION s.index_this_expr TO regress_minimal;
65+
CREATE UNIQUE INDEX u2rows ON s.x USING btree
66+
((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_pattern_ops)
67+
WHERE s.index_row_if(y);
68+
ALTER TABLE s.x ADD CONSTRAINT e2rows EXCLUDE USING btree
69+
((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=)
70+
WHERE (s.index_row_if(y));
71+
-- Shall not find s.coll via search_path, despite the s.const->public.setter
72+
-- call having set search_path=s during expression planning. Suppress the
73+
-- message itself, which depends on the database encoding.
74+
\set VERBOSITY terse
75+
DO $$
76+
BEGIN
77+
ALTER TABLE s.x ADD CONSTRAINT underqualified EXCLUDE USING btree
78+
((s.index_this_expr(y, s.const())) COLLATE coll WITH s.=)
79+
WHERE (s.index_row_if(y));
80+
EXCEPTION WHEN OTHERS THEN RAISE EXCEPTION '%', sqlstate; END$$;
81+
\set VERBOSITY default
82+
ROLLBACK;

‎src/backend/commands/indexcmds.c

Lines changed: 81 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,10 @@ static void ComputeIndexAttrs(IndexInfo *indexInfo,
7878
Oid relId,
7979
const char *accessMethodName, Oid accessMethodId,
8080
bool amcanorder,
81-
bool isconstraint);
81+
bool isconstraint,
82+
Oid ddl_userid,
83+
int ddl_sec_context,
84+
int *ddl_save_nestlevel);
8285
static char *ChooseIndexName(const char *tabname, Oid namespaceId,
8386
List *colnames, List *exclusionOpNames,
8487
bool primary, bool isconstraint);
@@ -186,9 +189,8 @@ CheckIndexCompatible(Oid oldId,
186189
* Compute the operator classes, collations, and exclusion operators for
187190
* the new index, so we can test whether it's compatible with the existing
188191
* one. Note that ComputeIndexAttrs might fail here, but that's OK:
189-
* DefineIndex would have called this function with the same arguments
190-
* later on, and it would have failed then anyway. Our attributeList
191-
* contains only key attributes, thus we're filling ii_NumIndexAttrs and
192+
* DefineIndex would have failed later. Our attributeList contains only
193+
* key attributes, thus we're filling ii_NumIndexAttrs and
192194
* ii_NumIndexKeyAttrs with same value.
193195
*/
194196
indexInfo = makeNode(IndexInfo);
@@ -212,7 +214,7 @@ CheckIndexCompatible(Oid oldId,
212214
coloptions, attributeList,
213215
exclusionOpNames, relationId,
214216
accessMethodName, accessMethodId,
215-
amcanorder, isconstraint);
217+
amcanorder, isconstraint, InvalidOid, 0, NULL);
216218

217219

218220
/* Get the soon-obsolete pg_index tuple. */
@@ -305,6 +307,19 @@ CheckIndexCompatible(Oid oldId,
305307
* DefineIndex
306308
* Creates a new index.
307309
*
310+
* This function manages the current userid according to the needs of pg_dump.
311+
* Recreating old-database catalog entries in new-database is fine, regardless
312+
* of which users would have permission to recreate those entries now. That's
313+
* just preservation of state. Running opaque expressions, like calling a
314+
* function named in a catalog entry or evaluating a pg_node_tree in a catalog
315+
* entry, as anyone other than the object owner, is not fine. To adhere to
316+
* those principles and to remain fail-safe, use the table owner userid for
317+
* most ACL checks. Use the original userid for ACL checks reached without
318+
* traversing opaque expressions. (pg_dump can predict such ACL checks from
319+
* catalogs.) Overall, this is a mess. Future DDL development should
320+
* consider offering one DDL command for catalog setup and a separate DDL
321+
* command for steps that run opaque expressions.
322+
*
308323
* 'relationId': the OID of the heap relation on which the index is to be
309324
* created
310325
* 'stmt': IndexStmt describing the properties of the new index.
@@ -697,7 +712,8 @@ DefineIndex(Oid relationId,
697712
coloptions, allIndexParams,
698713
stmt->excludeOpNames, relationId,
699714
accessMethodName, accessMethodId,
700-
amcanorder, stmt->isconstraint);
715+
amcanorder, stmt->isconstraint, root_save_userid,
716+
root_save_sec_context, &root_save_nestlevel);
701717

702718
/*
703719
* Extra checks when creating a PRIMARY KEY index.
@@ -966,9 +982,8 @@ DefineIndex(Oid relationId,
966982

967983
/*
968984
* Roll back any GUC changes executed by index functions, and keep
969-
* subsequent changes local to this command. It's barely possible that
970-
* some index function changed a behavior-affecting GUC, e.g. xmloption,
971-
* that affects subsequent steps.
985+
* subsequent changes local to this command. This is essential if some
986+
* index function changed a behavior-affecting GUC, e.g. search_path.
972987
*/
973988
AtEOXact_GUC(false, root_save_nestlevel);
974989
root_save_nestlevel = NewGUCNestLevel();
@@ -1582,6 +1597,10 @@ CheckPredicate(Expr *predicate)
15821597
* Compute per-index-column information, including indexed column numbers
15831598
* or index expressions, opclasses, and indoptions. Note, all output vectors
15841599
* should be allocated for all columns, including "including" ones.
1600+
*
1601+
* If the caller switched to the table owner, ddl_userid is the role for ACL
1602+
* checks reached without traversing opaque expressions. Otherwise, it's
1603+
* InvalidOid, and other ddl_* arguments are undefined.
15851604
*/
15861605
static void
15871606
ComputeIndexAttrs(IndexInfo *indexInfo,
@@ -1595,12 +1614,17 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
15951614
const char *accessMethodName,
15961615
Oid accessMethodId,
15971616
bool amcanorder,
1598-
bool isconstraint)
1617+
bool isconstraint,
1618+
Oid ddl_userid,
1619+
int ddl_sec_context,
1620+
int *ddl_save_nestlevel)
15991621
{
16001622
ListCell *nextExclOp;
16011623
ListCell *lc;
16021624
int attn;
16031625
int nkeycols = indexInfo->ii_NumIndexKeyAttrs;
1626+
Oid save_userid;
1627+
int save_sec_context;
16041628

16051629
/* Allocate space for exclusion operator info, if needed */
16061630
if (exclusionOpNames)
@@ -1614,6 +1638,9 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
16141638
else
16151639
nextExclOp = NULL;
16161640

1641+
if (OidIsValid(ddl_userid))
1642+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
1643+
16171644
/*
16181645
* process attributeList
16191646
*/
@@ -1744,10 +1771,24 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
17441771
}
17451772

17461773
/*
1747-
* Apply collation override if any
1774+
* Apply collation override if any. Use of ddl_userid is necessary
1775+
* due to ACL checks therein, and it's safe because collations don't
1776+
* contain opaque expressions (or non-opaque expressions).
17481777
*/
17491778
if (attribute->collation)
1779+
{
1780+
if (OidIsValid(ddl_userid))
1781+
{
1782+
AtEOXact_GUC(false, *ddl_save_nestlevel);
1783+
SetUserIdAndSecContext(ddl_userid, ddl_sec_context);
1784+
}
17501785
attcollation = get_collation_oid(attribute->collation, false);
1786+
if (OidIsValid(ddl_userid))
1787+
{
1788+
SetUserIdAndSecContext(save_userid, save_sec_context);
1789+
*ddl_save_nestlevel = NewGUCNestLevel();
1790+
}
1791+
}
17511792

17521793
/*
17531794
* Check we have a collation iff it's a collatable type. The only
@@ -1775,12 +1816,25 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
17751816
collationOidP[attn] = attcollation;
17761817

17771818
/*
1778-
* Identify the opclass to use.
1819+
* Identify the opclass to use. Use of ddl_userid is necessary due to
1820+
* ACL checks therein. This is safe despite opclasses containing
1821+
* opaque expressions (specifically, functions), because only
1822+
* superusers can define opclasses.
17791823
*/
1824+
if (OidIsValid(ddl_userid))
1825+
{
1826+
AtEOXact_GUC(false, *ddl_save_nestlevel);
1827+
SetUserIdAndSecContext(ddl_userid, ddl_sec_context);
1828+
}
17801829
classOidP[attn] = ResolveOpClass(attribute->opclass,
17811830
atttype,
17821831
accessMethodName,
17831832
accessMethodId);
1833+
if (OidIsValid(ddl_userid))
1834+
{
1835+
SetUserIdAndSecContext(save_userid, save_sec_context);
1836+
*ddl_save_nestlevel = NewGUCNestLevel();
1837+
}
17841838

17851839
/*
17861840
* Identify the exclusion operator, if any.
@@ -1794,9 +1848,23 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
17941848

17951849
/*
17961850
* Find the operator --- it must accept the column datatype
1797-
* without runtime coercion (but binary compatibility is OK)
1851+
* without runtime coercion (but binary compatibility is OK).
1852+
* Operators contain opaque expressions (specifically, functions).
1853+
* compatible_oper_opid() boils down to oper() and
1854+
* IsBinaryCoercible(). PostgreSQL would have security problems
1855+
* elsewhere if oper() started calling opaque expressions.
17981856
*/
1857+
if (OidIsValid(ddl_userid))
1858+
{
1859+
AtEOXact_GUC(false, *ddl_save_nestlevel);
1860+
SetUserIdAndSecContext(ddl_userid, ddl_sec_context);
1861+
}
17991862
opid = compatible_oper_opid(opname, atttype, atttype, false);
1863+
if (OidIsValid(ddl_userid))
1864+
{
1865+
SetUserIdAndSecContext(save_userid, save_sec_context);
1866+
*ddl_save_nestlevel = NewGUCNestLevel();
1867+
}
18001868

18011869
/*
18021870
* Only allow commutative operators to be used in exclusion

0 commit comments

Comments
 (0)
0