Skip to content

Commit 7f098f7

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 d387588 commit 7f098f7

File tree

10 files changed

+382
-50
lines changed

10 files changed

+382
-50
lines changed

‎contrib/amcheck/expected/check_btree.out

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,11 +168,34 @@ SELECT bt_index_check('toasty', true);
168168

169169
(1 row)
170170

171+
--
172+
-- Check that index expressions and predicates are run as the table's owner
173+
--
174+
TRUNCATE bttest_a;
175+
INSERT INTO bttest_a SELECT * FROM generate_series(1, 1000);
176+
ALTER TABLE bttest_a OWNER TO regress_bttest_role;
177+
-- A dummy index function checking current_user
178+
CREATE FUNCTION ifun(int8) RETURNS int8 AS $$
179+
BEGIN
180+
ASSERT current_user = 'regress_bttest_role',
181+
format('ifun(%s) called by %s', $1, current_user);
182+
RETURN $1;
183+
END;
184+
$$ LANGUAGE plpgsql IMMUTABLE;
185+
CREATE INDEX bttest_a_expr_idx ON bttest_a ((ifun(id) + ifun(0)))
186+
WHERE ifun(id + 10) > ifun(10);
187+
SELECT bt_index_check('bttest_a_expr_idx', true);
188+
bt_index_check
189+
----------------
190+
191+
(1 row)
192+
171193
-- cleanup
172194
DROP TABLE bttest_a;
173195
DROP TABLE bttest_b;
174196
DROP TABLE bttest_multi;
175197
DROP TABLE delete_test_table;
176198
DROP TABLE toast_bug;
199+
DROP FUNCTION ifun(int8);
177200
DROP OWNED BY regress_bttest_role; -- permissions
178201
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
@@ -110,11 +110,32 @@ INSERT INTO toast_bug SELECT repeat('a', 2200);
110110
-- Should not get false positive report of corruption:
111111
SELECT bt_index_check('toasty', true);
112112

113+
--
114+
-- Check that index expressions and predicates are run as the table's owner
115+
--
116+
TRUNCATE bttest_a;
117+
INSERT INTO bttest_a SELECT * FROM generate_series(1, 1000);
118+
ALTER TABLE bttest_a OWNER TO regress_bttest_role;
119+
-- A dummy index function checking current_user
120+
CREATE FUNCTION ifun(int8) RETURNS int8 AS $$
121+
BEGIN
122+
ASSERT current_user = 'regress_bttest_role',
123+
format('ifun(%s) called by %s', $1, current_user);
124+
RETURN $1;
125+
END;
126+
$$ LANGUAGE plpgsql IMMUTABLE;
127+
128+
CREATE INDEX bttest_a_expr_idx ON bttest_a ((ifun(id) + ifun(0)))
129+
WHERE ifun(id + 10) > ifun(10);
130+
131+
SELECT bt_index_check('bttest_a_expr_idx', true);
132+
113133
-- cleanup
114134
DROP TABLE bttest_a;
115135
DROP TABLE bttest_b;
116136
DROP TABLE bttest_multi;
117137
DROP TABLE delete_test_table;
118138
DROP TABLE toast_bug;
139+
DROP FUNCTION ifun(int8);
119140
DROP OWNED BY regress_bttest_role; -- permissions
120141
DROP ROLE regress_bttest_role;

‎contrib/amcheck/verify_nbtree.c

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,9 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed,
228228
Relation indrel;
229229
Relation heaprel;
230230
LOCKMODE lockmode;
231+
Oid save_userid;
232+
int save_sec_context;
233+
int save_nestlevel;
231234

232235
if (parentcheck)
233236
lockmode = ShareLock;
@@ -244,9 +247,27 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed,
244247
*/
245248
heapid = IndexGetRelation(indrelid, true);
246249
if (OidIsValid(heapid))
250+
{
247251
heaprel = table_open(heapid, lockmode);
252+
253+
/*
254+
* Switch to the table owner's userid, so that any index functions are
255+
* run as that user. Also lock down security-restricted operations
256+
* and arrange to make GUC variable changes local to this command.
257+
*/
258+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
259+
SetUserIdAndSecContext(heaprel->rd_rel->relowner,
260+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
261+
save_nestlevel = NewGUCNestLevel();
262+
}
248263
else
264+
{
249265
heaprel = NULL;
266+
/* for "gcc -Og" https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78394 */
267+
save_userid = InvalidOid;
268+
save_sec_context = -1;
269+
save_nestlevel = -1;
270+
}
250271

251272
/*
252273
* Open the target index relations separately (like relation_openrv(), but
@@ -293,6 +314,12 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed,
293314
heapallindexed, rootdescend);
294315
}
295316

317+
/* Roll back any GUC changes executed by index functions */
318+
AtEOXact_GUC(false, save_nestlevel);
319+
320+
/* Restore userid and security context */
321+
SetUserIdAndSecContext(save_userid, save_sec_context);
322+
296323
/*
297324
* Release locks early. That's ok here because nothing in the called
298325
* 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
@@ -873,6 +873,9 @@ brin_summarize_range(PG_FUNCTION_ARGS)
873873
Oid heapoid;
874874
Relation indexRel;
875875
Relation heapRel;
876+
Oid save_userid;
877+
int save_sec_context;
878+
int save_nestlevel;
876879
double numSummarized = 0;
877880

878881
if (RecoveryInProgress())
@@ -899,7 +902,22 @@ brin_summarize_range(PG_FUNCTION_ARGS)
899902
*/
900903
heapoid = IndexGetRelation(indexoid, true);
901904
if (OidIsValid(heapoid))
905+
{
902906
heapRel = table_open(heapoid, ShareUpdateExclusiveLock);
907+
908+
/*
909+
* Autovacuum calls us. For its benefit, switch to the table owner's
910+
* userid, so that any index functions are run as that user. Also
911+
* lock down security-restricted operations and arrange to make GUC
912+
* variable changes local to this command. This is harmless, albeit
913+
* unnecessary, when called from SQL, because we fail shortly if the
914+
* user does not own the index.
915+
*/
916+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
917+
SetUserIdAndSecContext(heapRel->rd_rel->relowner,
918+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
919+
save_nestlevel = NewGUCNestLevel();
920+
}
903921
else
904922
heapRel = NULL;
905923

@@ -914,7 +932,7 @@ brin_summarize_range(PG_FUNCTION_ARGS)
914932
RelationGetRelationName(indexRel))));
915933

916934
/* User must own the index (comparable to privileges needed for VACUUM) */
917-
if (!pg_class_ownercheck(indexoid, GetUserId()))
935+
if (heapRel != NULL && !pg_class_ownercheck(indexoid, save_userid))
918936
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_INDEX,
919937
RelationGetRelationName(indexRel));
920938

@@ -932,6 +950,12 @@ brin_summarize_range(PG_FUNCTION_ARGS)
932950
/* OK, do it */
933951
brinsummarize(indexRel, heapRel, heapBlk, true, &numSummarized, NULL);
934952

953+
/* Roll back any GUC changes executed by index functions */
954+
AtEOXact_GUC(false, save_nestlevel);
955+
956+
/* Restore userid and security context */
957+
SetUserIdAndSecContext(save_userid, save_sec_context);
958+
935959
relation_close(indexRel, ShareUpdateExclusiveLock);
936960
relation_close(heapRel, ShareUpdateExclusiveLock);
937961

@@ -973,6 +997,9 @@ brin_desummarize_range(PG_FUNCTION_ARGS)
973997
* passed indexoid isn't an index then IndexGetRelation() will fail.
974998
* Rather than emitting a not-very-helpful error message, postpone
975999
* complaining, expecting that the is-it-an-index test below will fail.
1000+
*
1001+
* Unlike brin_summarize_range(), autovacuum never calls this. Hence, we
1002+
* don't switch userid.
9761003
*/
9771004
heapoid = IndexGetRelation(indexoid, true);
9781005
if (OidIsValid(heapoid))

‎src/backend/catalog/index.c

Lines changed: 51 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1400,6 +1400,9 @@ index_concurrently_build(Oid heapRelationId,
14001400
Oid indexRelationId)
14011401
{
14021402
Relation heapRel;
1403+
Oid save_userid;
1404+
int save_sec_context;
1405+
int save_nestlevel;
14031406
Relation indexRelation;
14041407
IndexInfo *indexInfo;
14051408

@@ -1409,7 +1412,16 @@ index_concurrently_build(Oid heapRelationId,
14091412
/* Open and lock the parent heap relation */
14101413
heapRel = table_open(heapRelationId, ShareUpdateExclusiveLock);
14111414

1412-
/* And the target index relation */
1415+
/*
1416+
* Switch to the table owner's userid, so that any index functions are run
1417+
* as that user. Also lock down security-restricted operations and
1418+
* arrange to make GUC variable changes local to this command.
1419+
*/
1420+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
1421+
SetUserIdAndSecContext(heapRel->rd_rel->relowner,
1422+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
1423+
save_nestlevel = NewGUCNestLevel();
1424+
14131425
indexRelation = index_open(indexRelationId, RowExclusiveLock);
14141426

14151427
/*
@@ -1425,6 +1437,12 @@ index_concurrently_build(Oid heapRelationId,
14251437
/* Now build the index */
14261438
index_build(heapRel, indexRelation, indexInfo, false, true);
14271439

1440+
/* Roll back any GUC changes executed by index functions */
1441+
AtEOXact_GUC(false, save_nestlevel);
1442+
1443+
/* Restore userid and security context */
1444+
SetUserIdAndSecContext(save_userid, save_sec_context);
1445+
14281446
/* Close both the relations, but keep the locks */
14291447
table_close(heapRel, NoLock);
14301448
index_close(indexRelation, NoLock);
@@ -3282,7 +3300,17 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
32823300

32833301
/* Open and lock the parent heap relation */
32843302
heapRelation = table_open(heapId, ShareUpdateExclusiveLock);
3285-
/* And the target index relation */
3303+
3304+
/*
3305+
* Switch to the table owner's userid, so that any index functions are run
3306+
* as that user. Also lock down security-restricted operations and
3307+
* arrange to make GUC variable changes local to this command.
3308+
*/
3309+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
3310+
SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
3311+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
3312+
save_nestlevel = NewGUCNestLevel();
3313+
32863314
indexRelation = index_open(indexId, RowExclusiveLock);
32873315

32883316
/*
@@ -3295,16 +3323,6 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
32953323
/* mark build is concurrent just for consistency */
32963324
indexInfo->ii_Concurrent = true;
32973325

3298-
/*
3299-
* Switch to the table owner's userid, so that any index functions are run
3300-
* as that user. Also lock down security-restricted operations and
3301-
* arrange to make GUC variable changes local to this command.
3302-
*/
3303-
GetUserIdAndSecContext(&save_userid, &save_sec_context);
3304-
SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
3305-
save_sec_context | SECURITY_RESTRICTED_OPERATION);
3306-
save_nestlevel = NewGUCNestLevel();
3307-
33083326
/*
33093327
* Scan the index and gather up all the TIDs into a tuplesort object.
33103328
*/
@@ -3508,6 +3526,9 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
35083526
Relation iRel,
35093527
heapRelation;
35103528
Oid heapId;
3529+
Oid save_userid;
3530+
int save_sec_context;
3531+
int save_nestlevel;
35113532
IndexInfo *indexInfo;
35123533
volatile bool skipped_constraint = false;
35133534
PGRUsage ru0;
@@ -3522,6 +3543,16 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
35223543
heapId = IndexGetRelation(indexId, false);
35233544
heapRelation = table_open(heapId, ShareLock);
35243545

3546+
/*
3547+
* Switch to the table owner's userid, so that any index functions are run
3548+
* as that user. Also lock down security-restricted operations and
3549+
* arrange to make GUC variable changes local to this command.
3550+
*/
3551+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
3552+
SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
3553+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
3554+
save_nestlevel = NewGUCNestLevel();
3555+
35253556
if (progress)
35263557
{
35273558
pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
@@ -3695,12 +3726,18 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
36953726
errdetail_internal("%s",
36963727
pg_rusage_show(&ru0))));
36973728

3698-
if (progress)
3699-
pgstat_progress_end_command();
3729+
/* Roll back any GUC changes executed by index functions */
3730+
AtEOXact_GUC(false, save_nestlevel);
3731+
3732+
/* Restore userid and security context */
3733+
SetUserIdAndSecContext(save_userid, save_sec_context);
37003734

37013735
/* Close rels, but keep locks */
37023736
index_close(iRel, NoLock);
37033737
table_close(heapRelation, NoLock);
3738+
3739+
if (progress)
3740+
pgstat_progress_end_command();
37043741
}
37053742

37063743
/*

0 commit comments

Comments
 (0)