Skip to content

Commit a117ceb

Browse files
nmischnmisch
committed
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
1 parent f45f8b7 commit a117ceb

File tree

10 files changed

+378
-48
lines changed

10 files changed

+378
-48
lines changed

‎contrib/amcheck/expected/check_btree.out

+23
Original file line numberDiff line numberDiff line change
@@ -177,11 +177,34 @@ SELECT bt_index_check('toasty', true);
177177

178178
(1 row)
179179

180+
--
181+
-- Check that index expressions and predicates are run as the table's owner
182+
--
183+
TRUNCATE bttest_a;
184+
INSERT INTO bttest_a SELECT * FROM generate_series(1, 1000);
185+
ALTER TABLE bttest_a OWNER TO regress_bttest_role;
186+
-- A dummy index function checking current_user
187+
CREATE FUNCTION ifun(int8) RETURNS int8 AS $$
188+
BEGIN
189+
ASSERT current_user = 'regress_bttest_role',
190+
format('ifun(%s) called by %s', $1, current_user);
191+
RETURN $1;
192+
END;
193+
$$ LANGUAGE plpgsql IMMUTABLE;
194+
CREATE INDEX bttest_a_expr_idx ON bttest_a ((ifun(id) + ifun(0)))
195+
WHERE ifun(id + 10) > ifun(10);
196+
SELECT bt_index_check('bttest_a_expr_idx', true);
197+
bt_index_check
198+
----------------
199+
200+
(1 row)
201+
180202
-- cleanup
181203
DROP TABLE bttest_a;
182204
DROP TABLE bttest_b;
183205
DROP TABLE bttest_multi;
184206
DROP TABLE delete_test_table;
185207
DROP TABLE toast_bug;
208+
DROP FUNCTION ifun(int8);
186209
DROP OWNED BY regress_bttest_role; -- permissions
187210
DROP ROLE regress_bttest_role;

‎contrib/amcheck/sql/check_btree.sql

+21
Original file line numberDiff line numberDiff line change
@@ -115,11 +115,32 @@ INSERT INTO toast_bug SELECT repeat('a', 2200);
115115
-- Should not get false positive report of corruption:
116116
SELECT bt_index_check('toasty', true);
117117

118+
--
119+
-- Check that index expressions and predicates are run as the table's owner
120+
--
121+
TRUNCATE bttest_a;
122+
INSERT INTO bttest_a SELECT * FROM generate_series(1, 1000);
123+
ALTER TABLE bttest_a OWNER TO regress_bttest_role;
124+
-- A dummy index function checking current_user
125+
CREATE FUNCTION ifun(int8) RETURNS int8 AS $$
126+
BEGIN
127+
ASSERT current_user = 'regress_bttest_role',
128+
format('ifun(%s) called by %s', $1, current_user);
129+
RETURN $1;
130+
END;
131+
$$ LANGUAGE plpgsql IMMUTABLE;
132+
133+
CREATE INDEX bttest_a_expr_idx ON bttest_a ((ifun(id) + ifun(0)))
134+
WHERE ifun(id + 10) > ifun(10);
135+
136+
SELECT bt_index_check('bttest_a_expr_idx', true);
137+
118138
-- cleanup
119139
DROP TABLE bttest_a;
120140
DROP TABLE bttest_b;
121141
DROP TABLE bttest_multi;
122142
DROP TABLE delete_test_table;
123143
DROP TABLE toast_bug;
144+
DROP FUNCTION ifun(int8);
124145
DROP OWNED BY regress_bttest_role; -- permissions
125146
DROP ROLE regress_bttest_role;

‎contrib/amcheck/verify_nbtree.c

+27
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,9 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed,
249249
Relation indrel;
250250
Relation heaprel;
251251
LOCKMODE lockmode;
252+
Oid save_userid;
253+
int save_sec_context;
254+
int save_nestlevel;
252255

253256
if (parentcheck)
254257
lockmode = ShareLock;
@@ -265,9 +268,27 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed,
265268
*/
266269
heapid = IndexGetRelation(indrelid, true);
267270
if (OidIsValid(heapid))
271+
{
268272
heaprel = table_open(heapid, lockmode);
273+
274+
/*
275+
* Switch to the table owner's userid, so that any index functions are
276+
* run as that user. Also lock down security-restricted operations
277+
* and arrange to make GUC variable changes local to this command.
278+
*/
279+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
280+
SetUserIdAndSecContext(heaprel->rd_rel->relowner,
281+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
282+
save_nestlevel = NewGUCNestLevel();
283+
}
269284
else
285+
{
270286
heaprel = NULL;
287+
/* for "gcc -Og" https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78394 */
288+
save_userid = InvalidOid;
289+
save_sec_context = -1;
290+
save_nestlevel = -1;
291+
}
271292

272293
/*
273294
* Open the target index relations separately (like relation_openrv(), but
@@ -326,6 +347,12 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed,
326347
heapallindexed, rootdescend);
327348
}
328349

350+
/* Roll back any GUC changes executed by index functions */
351+
AtEOXact_GUC(false, save_nestlevel);
352+
353+
/* Restore userid and security context */
354+
SetUserIdAndSecContext(save_userid, save_sec_context);
355+
329356
/*
330357
* Release locks early. That's ok here because nothing in the called
331358
* routines will trigger shared cache invalidations to be sent, so we can

‎src/backend/access/brin/brin.c

+28-1
Original file line numberDiff line numberDiff line change
@@ -1008,6 +1008,9 @@ brin_summarize_range(PG_FUNCTION_ARGS)
10081008
Oid heapoid;
10091009
Relation indexRel;
10101010
Relation heapRel;
1011+
Oid save_userid;
1012+
int save_sec_context;
1013+
int save_nestlevel;
10111014
double numSummarized = 0;
10121015

10131016
if (RecoveryInProgress())
@@ -1031,7 +1034,22 @@ brin_summarize_range(PG_FUNCTION_ARGS)
10311034
*/
10321035
heapoid = IndexGetRelation(indexoid, true);
10331036
if (OidIsValid(heapoid))
1037+
{
10341038
heapRel = table_open(heapoid, ShareUpdateExclusiveLock);
1039+
1040+
/*
1041+
* Autovacuum calls us. For its benefit, switch to the table owner's
1042+
* userid, so that any index functions are run as that user. Also
1043+
* lock down security-restricted operations and arrange to make GUC
1044+
* variable changes local to this command. This is harmless, albeit
1045+
* unnecessary, when called from SQL, because we fail shortly if the
1046+
* user does not own the index.
1047+
*/
1048+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
1049+
SetUserIdAndSecContext(heapRel->rd_rel->relowner,
1050+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
1051+
save_nestlevel = NewGUCNestLevel();
1052+
}
10351053
else
10361054
heapRel = NULL;
10371055

@@ -1046,7 +1064,7 @@ brin_summarize_range(PG_FUNCTION_ARGS)
10461064
RelationGetRelationName(indexRel))));
10471065

10481066
/* User must own the index (comparable to privileges needed for VACUUM) */
1049-
if (!pg_class_ownercheck(indexoid, GetUserId()))
1067+
if (heapRel != NULL && !pg_class_ownercheck(indexoid, save_userid))
10501068
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_INDEX,
10511069
RelationGetRelationName(indexRel));
10521070

@@ -1064,6 +1082,12 @@ brin_summarize_range(PG_FUNCTION_ARGS)
10641082
/* OK, do it */
10651083
brinsummarize(indexRel, heapRel, heapBlk, true, &numSummarized, NULL);
10661084

1085+
/* Roll back any GUC changes executed by index functions */
1086+
AtEOXact_GUC(false, save_nestlevel);
1087+
1088+
/* Restore userid and security context */
1089+
SetUserIdAndSecContext(save_userid, save_sec_context);
1090+
10671091
relation_close(indexRel, ShareUpdateExclusiveLock);
10681092
relation_close(heapRel, ShareUpdateExclusiveLock);
10691093

@@ -1102,6 +1126,9 @@ brin_desummarize_range(PG_FUNCTION_ARGS)
11021126
* passed indexoid isn't an index then IndexGetRelation() will fail.
11031127
* Rather than emitting a not-very-helpful error message, postpone
11041128
* complaining, expecting that the is-it-an-index test below will fail.
1129+
*
1130+
* Unlike brin_summarize_range(), autovacuum never calls this. Hence, we
1131+
* don't switch userid.
11051132
*/
11061133
heapoid = IndexGetRelation(indexoid, true);
11071134
if (OidIsValid(heapoid))

‎src/backend/catalog/index.c

+51-14
Original file line numberDiff line numberDiff line change
@@ -1445,6 +1445,9 @@ index_concurrently_build(Oid heapRelationId,
14451445
Oid indexRelationId)
14461446
{
14471447
Relation heapRel;
1448+
Oid save_userid;
1449+
int save_sec_context;
1450+
int save_nestlevel;
14481451
Relation indexRelation;
14491452
IndexInfo *indexInfo;
14501453

@@ -1454,7 +1457,16 @@ index_concurrently_build(Oid heapRelationId,
14541457
/* Open and lock the parent heap relation */
14551458
heapRel = table_open(heapRelationId, ShareUpdateExclusiveLock);
14561459

1457-
/* And the target index relation */
1460+
/*
1461+
* Switch to the table owner's userid, so that any index functions are run
1462+
* as that user. Also lock down security-restricted operations and
1463+
* arrange to make GUC variable changes local to this command.
1464+
*/
1465+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
1466+
SetUserIdAndSecContext(heapRel->rd_rel->relowner,
1467+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
1468+
save_nestlevel = NewGUCNestLevel();
1469+
14581470
indexRelation = index_open(indexRelationId, RowExclusiveLock);
14591471

14601472
/*
@@ -1470,6 +1482,12 @@ index_concurrently_build(Oid heapRelationId,
14701482
/* Now build the index */
14711483
index_build(heapRel, indexRelation, indexInfo, false, true);
14721484

1485+
/* Roll back any GUC changes executed by index functions */
1486+
AtEOXact_GUC(false, save_nestlevel);
1487+
1488+
/* Restore userid and security context */
1489+
SetUserIdAndSecContext(save_userid, save_sec_context);
1490+
14731491
/* Close both the relations, but keep the locks */
14741492
table_close(heapRel, NoLock);
14751493
index_close(indexRelation, NoLock);
@@ -3299,7 +3317,17 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
32993317

33003318
/* Open and lock the parent heap relation */
33013319
heapRelation = table_open(heapId, ShareUpdateExclusiveLock);
3302-
/* And the target index relation */
3320+
3321+
/*
3322+
* Switch to the table owner's userid, so that any index functions are run
3323+
* as that user. Also lock down security-restricted operations and
3324+
* arrange to make GUC variable changes local to this command.
3325+
*/
3326+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
3327+
SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
3328+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
3329+
save_nestlevel = NewGUCNestLevel();
3330+
33033331
indexRelation = index_open(indexId, RowExclusiveLock);
33043332

33053333
/*
@@ -3312,16 +3340,6 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
33123340
/* mark build is concurrent just for consistency */
33133341
indexInfo->ii_Concurrent = true;
33143342

3315-
/*
3316-
* Switch to the table owner's userid, so that any index functions are run
3317-
* as that user. Also lock down security-restricted operations and
3318-
* arrange to make GUC variable changes local to this command.
3319-
*/
3320-
GetUserIdAndSecContext(&save_userid, &save_sec_context);
3321-
SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
3322-
save_sec_context | SECURITY_RESTRICTED_OPERATION);
3323-
save_nestlevel = NewGUCNestLevel();
3324-
33253343
/*
33263344
* Scan the index and gather up all the TIDs into a tuplesort object.
33273345
*/
@@ -3530,6 +3548,9 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
35303548
Relation iRel,
35313549
heapRelation;
35323550
Oid heapId;
3551+
Oid save_userid;
3552+
int save_sec_context;
3553+
int save_nestlevel;
35333554
IndexInfo *indexInfo;
35343555
volatile bool skipped_constraint = false;
35353556
PGRUsage ru0;
@@ -3557,6 +3578,16 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
35573578
if (!heapRelation)
35583579
return;
35593580

3581+
/*
3582+
* Switch to the table owner's userid, so that any index functions are run
3583+
* as that user. Also lock down security-restricted operations and
3584+
* arrange to make GUC variable changes local to this command.
3585+
*/
3586+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
3587+
SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
3588+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
3589+
save_nestlevel = NewGUCNestLevel();
3590+
35603591
if (progress)
35613592
{
35623593
const int progress_cols[] = {
@@ -3775,12 +3806,18 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
37753806
errdetail_internal("%s",
37763807
pg_rusage_show(&ru0))));
37773808

3778-
if (progress)
3779-
pgstat_progress_end_command();
3809+
/* Roll back any GUC changes executed by index functions */
3810+
AtEOXact_GUC(false, save_nestlevel);
3811+
3812+
/* Restore userid and security context */
3813+
SetUserIdAndSecContext(save_userid, save_sec_context);
37803814

37813815
/* Close rels, but keep locks */
37823816
index_close(iRel, NoLock);
37833817
table_close(heapRelation, NoLock);
3818+
3819+
if (progress)
3820+
pgstat_progress_end_command();
37843821
}
37853822

37863823
/*

0 commit comments

Comments
 (0)