Skip to content

Commit ab49ce7

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 e5b5a21 commit ab49ce7

File tree

10 files changed

+378
-48
lines changed

10 files changed

+378
-48
lines changed

‎contrib/amcheck/expected/check_btree.out

Lines changed: 23 additions & 0 deletions
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

Lines changed: 21 additions & 0 deletions
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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,9 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed,
248248
Relation indrel;
249249
Relation heaprel;
250250
LOCKMODE lockmode;
251+
Oid save_userid;
252+
int save_sec_context;
253+
int save_nestlevel;
251254

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

271292
/*
272293
* 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

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1007,6 +1007,9 @@ brin_summarize_range(PG_FUNCTION_ARGS)
10071007
Oid heapoid;
10081008
Relation indexRel;
10091009
Relation heapRel;
1010+
Oid save_userid;
1011+
int save_sec_context;
1012+
int save_nestlevel;
10101013
double numSummarized = 0;
10111014

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

@@ -1048,7 +1066,7 @@ brin_summarize_range(PG_FUNCTION_ARGS)
10481066
RelationGetRelationName(indexRel))));
10491067

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

@@ -1066,6 +1084,12 @@ brin_summarize_range(PG_FUNCTION_ARGS)
10661084
/* OK, do it */
10671085
brinsummarize(indexRel, heapRel, heapBlk, true, &numSummarized, NULL);
10681086

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

@@ -1107,6 +1131,9 @@ brin_desummarize_range(PG_FUNCTION_ARGS)
11071131
* passed indexoid isn't an index then IndexGetRelation() will fail.
11081132
* Rather than emitting a not-very-helpful error message, postpone
11091133
* complaining, expecting that the is-it-an-index test below will fail.
1134+
*
1135+
* Unlike brin_summarize_range(), autovacuum never calls this. Hence, we
1136+
* don't switch userid.
11101137
*/
11111138
heapoid = IndexGetRelation(indexoid, true);
11121139
if (OidIsValid(heapoid))

‎src/backend/catalog/index.c

Lines changed: 51 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1424,6 +1424,9 @@ index_concurrently_build(Oid heapRelationId,
14241424
Oid indexRelationId)
14251425
{
14261426
Relation heapRel;
1427+
Oid save_userid;
1428+
int save_sec_context;
1429+
int save_nestlevel;
14271430
Relation indexRelation;
14281431
IndexInfo *indexInfo;
14291432

@@ -1433,7 +1436,16 @@ index_concurrently_build(Oid heapRelationId,
14331436
/* Open and lock the parent heap relation */
14341437
heapRel = table_open(heapRelationId, ShareUpdateExclusiveLock);
14351438

1436-
/* And the target index relation */
1439+
/*
1440+
* Switch to the table owner's userid, so that any index functions are run
1441+
* as that user. Also lock down security-restricted operations and
1442+
* arrange to make GUC variable changes local to this command.
1443+
*/
1444+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
1445+
SetUserIdAndSecContext(heapRel->rd_rel->relowner,
1446+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
1447+
save_nestlevel = NewGUCNestLevel();
1448+
14371449
indexRelation = index_open(indexRelationId, RowExclusiveLock);
14381450

14391451
/*
@@ -1449,6 +1461,12 @@ index_concurrently_build(Oid heapRelationId,
14491461
/* Now build the index */
14501462
index_build(heapRel, indexRelation, indexInfo, false, true);
14511463

1464+
/* Roll back any GUC changes executed by index functions */
1465+
AtEOXact_GUC(false, save_nestlevel);
1466+
1467+
/* Restore userid and security context */
1468+
SetUserIdAndSecContext(save_userid, save_sec_context);
1469+
14521470
/* Close both the relations, but keep the locks */
14531471
table_close(heapRel, NoLock);
14541472
index_close(indexRelation, NoLock);
@@ -3294,7 +3312,17 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
32943312

32953313
/* Open and lock the parent heap relation */
32963314
heapRelation = table_open(heapId, ShareUpdateExclusiveLock);
3297-
/* And the target index relation */
3315+
3316+
/*
3317+
* Switch to the table owner's userid, so that any index functions are run
3318+
* as that user. Also lock down security-restricted operations and
3319+
* arrange to make GUC variable changes local to this command.
3320+
*/
3321+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
3322+
SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
3323+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
3324+
save_nestlevel = NewGUCNestLevel();
3325+
32983326
indexRelation = index_open(indexId, RowExclusiveLock);
32993327

33003328
/*
@@ -3307,16 +3335,6 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
33073335
/* mark build is concurrent just for consistency */
33083336
indexInfo->ii_Concurrent = true;
33093337

3310-
/*
3311-
* Switch to the table owner's userid, so that any index functions are run
3312-
* as that user. Also lock down security-restricted operations and
3313-
* arrange to make GUC variable changes local to this command.
3314-
*/
3315-
GetUserIdAndSecContext(&save_userid, &save_sec_context);
3316-
SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
3317-
save_sec_context | SECURITY_RESTRICTED_OPERATION);
3318-
save_nestlevel = NewGUCNestLevel();
3319-
33203338
/*
33213339
* Scan the index and gather up all the TIDs into a tuplesort object.
33223340
*/
@@ -3525,6 +3543,9 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
35253543
Relation iRel,
35263544
heapRelation;
35273545
Oid heapId;
3546+
Oid save_userid;
3547+
int save_sec_context;
3548+
int save_nestlevel;
35283549
IndexInfo *indexInfo;
35293550
volatile bool skipped_constraint = false;
35303551
PGRUsage ru0;
@@ -3552,6 +3573,16 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
35523573
if (!heapRelation)
35533574
return;
35543575

3576+
/*
3577+
* Switch to the table owner's userid, so that any index functions are run
3578+
* as that user. Also lock down security-restricted operations and
3579+
* arrange to make GUC variable changes local to this command.
3580+
*/
3581+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
3582+
SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
3583+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
3584+
save_nestlevel = NewGUCNestLevel();
3585+
35553586
if (progress)
35563587
{
35573588
const int progress_cols[] = {
@@ -3770,12 +3801,18 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
37703801
errdetail_internal("%s",
37713802
pg_rusage_show(&ru0))));
37723803

3773-
if (progress)
3774-
pgstat_progress_end_command();
3804+
/* Roll back any GUC changes executed by index functions */
3805+
AtEOXact_GUC(false, save_nestlevel);
3806+
3807+
/* Restore userid and security context */
3808+
SetUserIdAndSecContext(save_userid, save_sec_context);
37753809

37763810
/* Close rels, but keep locks */
37773811
index_close(iRel, NoLock);
37783812
table_close(heapRelation, NoLock);
3813+
3814+
if (progress)
3815+
pgstat_progress_end_command();
37793816
}
37803817

37813818
/*

0 commit comments

Comments
 (0)