Skip to content

Commit 48ca290

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 d8ab73f commit 48ca290

File tree

10 files changed

+361
-30
lines changed

10 files changed

+361
-30
lines changed

contrib/amcheck/expected/check_btree.out

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

168168
(1 row)
169169

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

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

contrib/amcheck/verify_nbtree.c

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
#include "miscadmin.h"
3535
#include "storage/lmgr.h"
3636
#include "storage/smgr.h"
37+
#include "utils/guc.h"
3738
#include "utils/memutils.h"
3839
#include "utils/snapmgr.h"
3940

@@ -206,6 +207,9 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed)
206207
Relation indrel;
207208
Relation heaprel;
208209
LOCKMODE lockmode;
210+
Oid save_userid;
211+
int save_sec_context;
212+
int save_nestlevel;
209213

210214
if (parentcheck)
211215
lockmode = ShareLock;
@@ -222,9 +226,27 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed)
222226
*/
223227
heapid = IndexGetRelation(indrelid, true);
224228
if (OidIsValid(heapid))
229+
{
225230
heaprel = heap_open(heapid, lockmode);
231+
232+
/*
233+
* Switch to the table owner's userid, so that any index functions are
234+
* run as that user. Also lock down security-restricted operations
235+
* and arrange to make GUC variable changes local to this command.
236+
*/
237+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
238+
SetUserIdAndSecContext(heaprel->rd_rel->relowner,
239+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
240+
save_nestlevel = NewGUCNestLevel();
241+
}
226242
else
243+
{
227244
heaprel = NULL;
245+
/* for "gcc -Og" https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78394 */
246+
save_userid = InvalidOid;
247+
save_sec_context = -1;
248+
save_nestlevel = -1;
249+
}
228250

229251
/*
230252
* Open the target index relations separately (like relation_openrv(), but
@@ -267,6 +289,12 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed)
267289
bt_check_every_level(indrel, heaprel, parentcheck, heapallindexed);
268290
}
269291

292+
/* Roll back any GUC changes executed by index functions */
293+
AtEOXact_GUC(false, save_nestlevel);
294+
295+
/* Restore userid and security context */
296+
SetUserIdAndSecContext(save_userid, save_sec_context);
297+
270298
/*
271299
* Release locks early. That's ok here because nothing in the called
272300
* routines will trigger shared cache invalidations to be sent, so we can

src/backend/access/brin/brin.c

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
#include "storage/bufmgr.h"
3131
#include "storage/freespace.h"
3232
#include "utils/builtins.h"
33+
#include "utils/guc.h"
3334
#include "utils/index_selfuncs.h"
3435
#include "utils/memutils.h"
3536
#include "utils/rel.h"
@@ -869,6 +870,9 @@ brin_summarize_range(PG_FUNCTION_ARGS)
869870
Oid heapoid;
870871
Relation indexRel;
871872
Relation heapRel;
873+
Oid save_userid;
874+
int save_sec_context;
875+
int save_nestlevel;
872876
double numSummarized = 0;
873877

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

@@ -910,7 +929,7 @@ brin_summarize_range(PG_FUNCTION_ARGS)
910929
RelationGetRelationName(indexRel))));
911930

912931
/* User must own the index (comparable to privileges needed for VACUUM) */
913-
if (!pg_class_ownercheck(indexoid, GetUserId()))
932+
if (heapRel != NULL && !pg_class_ownercheck(indexoid, save_userid))
914933
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_INDEX,
915934
RelationGetRelationName(indexRel));
916935

@@ -928,6 +947,12 @@ brin_summarize_range(PG_FUNCTION_ARGS)
928947
/* OK, do it */
929948
brinsummarize(indexRel, heapRel, heapBlk, true, &numSummarized, NULL);
930949

950+
/* Roll back any GUC changes executed by index functions */
951+
AtEOXact_GUC(false, save_nestlevel);
952+
953+
/* Restore userid and security context */
954+
SetUserIdAndSecContext(save_userid, save_sec_context);
955+
931956
relation_close(indexRel, ShareUpdateExclusiveLock);
932957
relation_close(heapRel, ShareUpdateExclusiveLock);
933958

@@ -969,6 +994,9 @@ brin_desummarize_range(PG_FUNCTION_ARGS)
969994
* passed indexoid isn't an index then IndexGetRelation() will fail.
970995
* Rather than emitting a not-very-helpful error message, postpone
971996
* complaining, expecting that the is-it-an-index test below will fail.
997+
*
998+
* Unlike brin_summarize_range(), autovacuum never calls this. Hence, we
999+
* don't switch userid.
9721000
*/
9731001
heapoid = IndexGetRelation(indexoid, true);
9741002
if (OidIsValid(heapoid))

src/backend/catalog/index.c

Lines changed: 30 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3271,7 +3271,17 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
32713271

32723272
/* Open and lock the parent heap relation */
32733273
heapRelation = heap_open(heapId, ShareUpdateExclusiveLock);
3274-
/* And the target index relation */
3274+
3275+
/*
3276+
* Switch to the table owner's userid, so that any index functions are run
3277+
* as that user. Also lock down security-restricted operations and
3278+
* arrange to make GUC variable changes local to this command.
3279+
*/
3280+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
3281+
SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
3282+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
3283+
save_nestlevel = NewGUCNestLevel();
3284+
32753285
indexRelation = index_open(indexId, RowExclusiveLock);
32763286

32773287
/*
@@ -3284,16 +3294,6 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
32843294
/* mark build is concurrent just for consistency */
32853295
indexInfo->ii_Concurrent = true;
32863296

3287-
/*
3288-
* Switch to the table owner's userid, so that any index functions are run
3289-
* as that user. Also lock down security-restricted operations and
3290-
* arrange to make GUC variable changes local to this command.
3291-
*/
3292-
GetUserIdAndSecContext(&save_userid, &save_sec_context);
3293-
SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
3294-
save_sec_context | SECURITY_RESTRICTED_OPERATION);
3295-
save_nestlevel = NewGUCNestLevel();
3296-
32973297
/*
32983298
* Scan the index and gather up all the TIDs into a tuplesort object.
32993299
*/
@@ -3756,6 +3756,9 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
37563756
Relation iRel,
37573757
heapRelation;
37583758
Oid heapId;
3759+
Oid save_userid;
3760+
int save_sec_context;
3761+
int save_nestlevel;
37593762
IndexInfo *indexInfo;
37603763
volatile bool skipped_constraint = false;
37613764
PGRUsage ru0;
@@ -3769,6 +3772,16 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
37693772
heapId = IndexGetRelation(indexId, false);
37703773
heapRelation = heap_open(heapId, ShareLock);
37713774

3775+
/*
3776+
* Switch to the table owner's userid, so that any index functions are run
3777+
* as that user. Also lock down security-restricted operations and
3778+
* arrange to make GUC variable changes local to this command.
3779+
*/
3780+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
3781+
SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
3782+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
3783+
save_nestlevel = NewGUCNestLevel();
3784+
37723785
/*
37733786
* Open the target index relation and get an exclusive lock on it, to
37743787
* ensure that no one else is touching this particular index.
@@ -3918,6 +3931,12 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
39183931
errdetail_internal("%s",
39193932
pg_rusage_show(&ru0))));
39203933

3934+
/* Roll back any GUC changes executed by index functions */
3935+
AtEOXact_GUC(false, save_nestlevel);
3936+
3937+
/* Restore userid and security context */
3938+
SetUserIdAndSecContext(save_userid, save_sec_context);
3939+
39213940
/* Close rels, but keep locks */
39223941
index_close(iRel, NoLock);
39233942
heap_close(heapRelation, NoLock);

src/backend/commands/cluster.c

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
#include "storage/smgr.h"
4545
#include "utils/acl.h"
4646
#include "utils/fmgroids.h"
47+
#include "utils/guc.h"
4748
#include "utils/inval.h"
4849
#include "utils/lsyscache.h"
4950
#include "utils/memutils.h"
@@ -268,6 +269,9 @@ void
268269
cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose)
269270
{
270271
Relation OldHeap;
272+
Oid save_userid;
273+
int save_sec_context;
274+
int save_nestlevel;
271275

272276
/* Check for user-requested abort. */
273277
CHECK_FOR_INTERRUPTS();
@@ -284,6 +288,16 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose)
284288
if (!OldHeap)
285289
return;
286290

291+
/*
292+
* Switch to the table owner's userid, so that any index functions are run
293+
* as that user. Also lock down security-restricted operations and
294+
* arrange to make GUC variable changes local to this command.
295+
*/
296+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
297+
SetUserIdAndSecContext(OldHeap->rd_rel->relowner,
298+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
299+
save_nestlevel = NewGUCNestLevel();
300+
287301
/*
288302
* Since we may open a new transaction for each relation, we have to check
289303
* that the relation still is what we think it is.
@@ -298,10 +312,10 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose)
298312
Form_pg_index indexForm;
299313

300314
/* Check that the user still owns the relation */
301-
if (!pg_class_ownercheck(tableOid, GetUserId()))
315+
if (!pg_class_ownercheck(tableOid, save_userid))
302316
{
303317
relation_close(OldHeap, AccessExclusiveLock);
304-
return;
318+
goto out;
305319
}
306320

307321
/*
@@ -315,7 +329,7 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose)
315329
if (RELATION_IS_OTHER_TEMP(OldHeap))
316330
{
317331
relation_close(OldHeap, AccessExclusiveLock);
318-
return;
332+
goto out;
319333
}
320334

321335
if (OidIsValid(indexOid))
@@ -326,7 +340,7 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose)
326340
if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(indexOid)))
327341
{
328342
relation_close(OldHeap, AccessExclusiveLock);
329-
return;
343+
goto out;
330344
}
331345

332346
/*
@@ -336,14 +350,14 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose)
336350
if (!HeapTupleIsValid(tuple)) /* probably can't happen */
337351
{
338352
relation_close(OldHeap, AccessExclusiveLock);
339-
return;
353+
goto out;
340354
}
341355
indexForm = (Form_pg_index) GETSTRUCT(tuple);
342356
if (!indexForm->indisclustered)
343357
{
344358
ReleaseSysCache(tuple);
345359
relation_close(OldHeap, AccessExclusiveLock);
346-
return;
360+
goto out;
347361
}
348362
ReleaseSysCache(tuple);
349363
}
@@ -397,7 +411,7 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose)
397411
!RelationIsPopulated(OldHeap))
398412
{
399413
relation_close(OldHeap, AccessExclusiveLock);
400-
return;
414+
goto out;
401415
}
402416

403417
/*
@@ -412,6 +426,13 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose)
412426
rebuild_relation(OldHeap, indexOid, verbose);
413427

414428
/* NB: rebuild_relation does heap_close() on OldHeap */
429+
430+
out:
431+
/* Roll back any GUC changes executed by index functions */
432+
AtEOXact_GUC(false, save_nestlevel);
433+
434+
/* Restore userid and security context */
435+
SetUserIdAndSecContext(save_userid, save_sec_context);
415436
}
416437

417438
/*

0 commit comments

Comments
 (0)