Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 35edcc0

Browse files
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 9164637 commit 35edcc0

File tree

10 files changed

+381
-48
lines changed

10 files changed

+381
-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
@@ -245,6 +245,9 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed,
245245
Relation indrel;
246246
Relation heaprel;
247247
LOCKMODE lockmode;
248+
Oid save_userid;
249+
int save_sec_context;
250+
int save_nestlevel;
248251

249252
if (parentcheck)
250253
lockmode = ShareLock;
@@ -261,9 +264,27 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed,
261264
*/
262265
heapid = IndexGetRelation(indrelid, true);
263266
if (OidIsValid(heapid))
267+
{
264268
heaprel = table_open(heapid, lockmode);
269+
270+
/*
271+
* Switch to the table owner's userid, so that any index functions are
272+
* run as that user. Also lock down security-restricted operations
273+
* and arrange to make GUC variable changes local to this command.
274+
*/
275+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
276+
SetUserIdAndSecContext(heaprel->rd_rel->relowner,
277+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
278+
save_nestlevel = NewGUCNestLevel();
279+
}
265280
else
281+
{
266282
heaprel = NULL;
283+
/* for "gcc -Og" https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78394 */
284+
save_userid = InvalidOid;
285+
save_sec_context = -1;
286+
save_nestlevel = -1;
287+
}
267288

268289
/*
269290
* Open the target index relations separately (like relation_openrv(), but
@@ -323,6 +344,12 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed,
323344
heapallindexed, rootdescend);
324345
}
325346

347+
/* Roll back any GUC changes executed by index functions */
348+
AtEOXact_GUC(false, save_nestlevel);
349+
350+
/* Restore userid and security context */
351+
SetUserIdAndSecContext(save_userid, save_sec_context);
352+
326353
/*
327354
* Release locks early. That's ok here because nothing in the called
328355
* 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
@@ -865,6 +865,9 @@ brin_summarize_range(PG_FUNCTION_ARGS)
865865
Oid heapoid;
866866
Relation indexRel;
867867
Relation heapRel;
868+
Oid save_userid;
869+
int save_sec_context;
870+
int save_nestlevel;
868871
double numSummarized = 0;
869872

870873
if (RecoveryInProgress())
@@ -891,7 +894,22 @@ brin_summarize_range(PG_FUNCTION_ARGS)
891894
*/
892895
heapoid = IndexGetRelation(indexoid, true);
893896
if (OidIsValid(heapoid))
897+
{
894898
heapRel = table_open(heapoid, ShareUpdateExclusiveLock);
899+
900+
/*
901+
* Autovacuum calls us. For its benefit, switch to the table owner's
902+
* userid, so that any index functions are run as that user. Also
903+
* lock down security-restricted operations and arrange to make GUC
904+
* variable changes local to this command. This is harmless, albeit
905+
* unnecessary, when called from SQL, because we fail shortly if the
906+
* user does not own the index.
907+
*/
908+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
909+
SetUserIdAndSecContext(heapRel->rd_rel->relowner,
910+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
911+
save_nestlevel = NewGUCNestLevel();
912+
}
895913
else
896914
heapRel = NULL;
897915

@@ -906,7 +924,7 @@ brin_summarize_range(PG_FUNCTION_ARGS)
906924
RelationGetRelationName(indexRel))));
907925

908926
/* User must own the index (comparable to privileges needed for VACUUM) */
909-
if (!pg_class_ownercheck(indexoid, GetUserId()))
927+
if (heapRel != NULL && !pg_class_ownercheck(indexoid, save_userid))
910928
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_INDEX,
911929
RelationGetRelationName(indexRel));
912930

@@ -924,6 +942,12 @@ brin_summarize_range(PG_FUNCTION_ARGS)
924942
/* OK, do it */
925943
brinsummarize(indexRel, heapRel, heapBlk, true, &numSummarized, NULL);
926944

945+
/* Roll back any GUC changes executed by index functions */
946+
AtEOXact_GUC(false, save_nestlevel);
947+
948+
/* Restore userid and security context */
949+
SetUserIdAndSecContext(save_userid, save_sec_context);
950+
927951
relation_close(indexRel, ShareUpdateExclusiveLock);
928952
relation_close(heapRel, ShareUpdateExclusiveLock);
929953

@@ -965,6 +989,9 @@ brin_desummarize_range(PG_FUNCTION_ARGS)
965989
* passed indexoid isn't an index then IndexGetRelation() will fail.
966990
* Rather than emitting a not-very-helpful error message, postpone
967991
* complaining, expecting that the is-it-an-index test below will fail.
992+
*
993+
* Unlike brin_summarize_range(), autovacuum never calls this. Hence, we
994+
* don't switch userid.
968995
*/
969996
heapoid = IndexGetRelation(indexoid, true);
970997
if (OidIsValid(heapoid))

src/backend/catalog/index.c

Lines changed: 51 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1421,6 +1421,9 @@ index_concurrently_build(Oid heapRelationId,
14211421
Oid indexRelationId)
14221422
{
14231423
Relation heapRel;
1424+
Oid save_userid;
1425+
int save_sec_context;
1426+
int save_nestlevel;
14241427
Relation indexRelation;
14251428
IndexInfo *indexInfo;
14261429

@@ -1430,7 +1433,16 @@ index_concurrently_build(Oid heapRelationId,
14301433
/* Open and lock the parent heap relation */
14311434
heapRel = table_open(heapRelationId, ShareUpdateExclusiveLock);
14321435

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

14361448
/*
@@ -1446,6 +1458,12 @@ index_concurrently_build(Oid heapRelationId,
14461458
/* Now build the index */
14471459
index_build(heapRel, indexRelation, indexInfo, false, true);
14481460

1461+
/* Roll back any GUC changes executed by index functions */
1462+
AtEOXact_GUC(false, save_nestlevel);
1463+
1464+
/* Restore userid and security context */
1465+
SetUserIdAndSecContext(save_userid, save_sec_context);
1466+
14491467
/* Close both the relations, but keep the locks */
14501468
table_close(heapRel, NoLock);
14511469
index_close(indexRelation, NoLock);
@@ -3283,7 +3301,17 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
32833301

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

32893317
/*
@@ -3296,16 +3324,6 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
32963324
/* mark build is concurrent just for consistency */
32973325
indexInfo->ii_Concurrent = true;
32983326

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

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

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

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

37073744
/*

0 commit comments

Comments
 (0)