Skip to content

Commit ef792f7

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 b05f4ae commit ef792f7

File tree

10 files changed

+335
-30
lines changed

10 files changed

+335
-30
lines changed

contrib/amcheck/expected/check_btree.out

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,8 +85,31 @@ WHERE relation = ANY(ARRAY['bttest_a', 'bttest_a_idx', 'bttest_b', 'bttest_b_idx
8585
(0 rows)
8686

8787
COMMIT;
88+
--
89+
-- Check that index expressions and predicates are run as the table's owner
90+
--
91+
TRUNCATE bttest_a;
92+
INSERT INTO bttest_a SELECT * FROM generate_series(1, 1000);
93+
ALTER TABLE bttest_a OWNER TO regress_bttest_role;
94+
-- A dummy index function checking current_user
95+
CREATE FUNCTION ifun(int8) RETURNS int8 AS $$
96+
BEGIN
97+
ASSERT current_user = 'regress_bttest_role',
98+
format('ifun(%s) called by %s', $1, current_user);
99+
RETURN $1;
100+
END;
101+
$$ LANGUAGE plpgsql IMMUTABLE;
102+
CREATE INDEX bttest_a_expr_idx ON bttest_a ((ifun(id) + ifun(0)))
103+
WHERE ifun(id + 10) > ifun(10);
104+
SELECT bt_index_check('bttest_a_expr_idx');
105+
bt_index_check
106+
----------------
107+
108+
(1 row)
109+
88110
-- cleanup
89111
DROP TABLE bttest_a;
90112
DROP TABLE bttest_b;
113+
DROP FUNCTION ifun(int8);
91114
DROP OWNED BY regress_bttest_role; -- permissions
92115
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
@@ -54,8 +54,29 @@ WHERE relation = ANY(ARRAY['bttest_a', 'bttest_a_idx', 'bttest_b', 'bttest_b_idx
5454
AND pid = pg_backend_pid();
5555
COMMIT;
5656

57+
--
58+
-- Check that index expressions and predicates are run as the table's owner
59+
--
60+
TRUNCATE bttest_a;
61+
INSERT INTO bttest_a SELECT * FROM generate_series(1, 1000);
62+
ALTER TABLE bttest_a OWNER TO regress_bttest_role;
63+
-- A dummy index function checking current_user
64+
CREATE FUNCTION ifun(int8) RETURNS int8 AS $$
65+
BEGIN
66+
ASSERT current_user = 'regress_bttest_role',
67+
format('ifun(%s) called by %s', $1, current_user);
68+
RETURN $1;
69+
END;
70+
$$ LANGUAGE plpgsql IMMUTABLE;
71+
72+
CREATE INDEX bttest_a_expr_idx ON bttest_a ((ifun(id) + ifun(0)))
73+
WHERE ifun(id + 10) > ifun(10);
74+
75+
SELECT bt_index_check('bttest_a_expr_idx');
76+
5777
-- cleanup
5878
DROP TABLE bttest_a;
5979
DROP TABLE bttest_b;
80+
DROP FUNCTION ifun(int8);
6081
DROP OWNED BY regress_bttest_role; -- permissions
6182
DROP ROLE regress_bttest_role;

contrib/amcheck/verify_nbtree.c

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
#include "miscadmin.h"
2727
#include "storage/lmgr.h"
2828
#include "storage/smgr.h"
29+
#include "utils/guc.h"
2930
#include "utils/memutils.h"
3031
#include "utils/snapmgr.h"
3132

@@ -163,6 +164,9 @@ bt_index_check_internal(Oid indrelid, bool parentcheck)
163164
Relation indrel;
164165
Relation heaprel;
165166
LOCKMODE lockmode;
167+
Oid save_userid;
168+
int save_sec_context;
169+
int save_nestlevel;
166170

167171
if (parentcheck)
168172
lockmode = ShareLock;
@@ -179,9 +183,27 @@ bt_index_check_internal(Oid indrelid, bool parentcheck)
179183
*/
180184
heapid = IndexGetRelation(indrelid, true);
181185
if (OidIsValid(heapid))
186+
{
182187
heaprel = heap_open(heapid, lockmode);
188+
189+
/*
190+
* Switch to the table owner's userid, so that any index functions are
191+
* run as that user. Also lock down security-restricted operations
192+
* and arrange to make GUC variable changes local to this command.
193+
*/
194+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
195+
SetUserIdAndSecContext(heaprel->rd_rel->relowner,
196+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
197+
save_nestlevel = NewGUCNestLevel();
198+
}
183199
else
200+
{
184201
heaprel = NULL;
202+
/* for "gcc -Og" https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78394 */
203+
save_userid = InvalidOid;
204+
save_sec_context = -1;
205+
save_nestlevel = -1;
206+
}
185207

186208
/*
187209
* Open the target index relations separately (like relation_openrv(), but
@@ -219,6 +241,12 @@ bt_index_check_internal(Oid indrelid, bool parentcheck)
219241
bt_check_every_level(indrel, parentcheck);
220242
}
221243

244+
/* Roll back any GUC changes executed by index functions */
245+
AtEOXact_GUC(false, save_nestlevel);
246+
247+
/* Restore userid and security context */
248+
SetUserIdAndSecContext(save_userid, save_sec_context);
249+
222250
/*
223251
* Release locks early. That's ok here because nothing in the called
224252
* 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"
@@ -866,6 +867,9 @@ brin_summarize_range(PG_FUNCTION_ARGS)
866867
Oid heapoid;
867868
Relation indexRel;
868869
Relation heapRel;
870+
Oid save_userid;
871+
int save_sec_context;
872+
int save_nestlevel;
869873
double numSummarized = 0;
870874

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

@@ -907,7 +926,7 @@ brin_summarize_range(PG_FUNCTION_ARGS)
907926
RelationGetRelationName(indexRel))));
908927

909928
/* User must own the index (comparable to privileges needed for VACUUM) */
910-
if (!pg_class_ownercheck(indexoid, GetUserId()))
929+
if (heapRel != NULL && !pg_class_ownercheck(indexoid, save_userid))
911930
aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
912931
RelationGetRelationName(indexRel));
913932

@@ -925,6 +944,12 @@ brin_summarize_range(PG_FUNCTION_ARGS)
925944
/* OK, do it */
926945
brinsummarize(indexRel, heapRel, heapBlk, true, &numSummarized, NULL);
927946

947+
/* Roll back any GUC changes executed by index functions */
948+
AtEOXact_GUC(false, save_nestlevel);
949+
950+
/* Restore userid and security context */
951+
SetUserIdAndSecContext(save_userid, save_sec_context);
952+
928953
relation_close(indexRel, ShareUpdateExclusiveLock);
929954
relation_close(heapRel, ShareUpdateExclusiveLock);
930955

@@ -966,6 +991,9 @@ brin_desummarize_range(PG_FUNCTION_ARGS)
966991
* passed indexoid isn't an index then IndexGetRelation() will fail.
967992
* Rather than emitting a not-very-helpful error message, postpone
968993
* complaining, expecting that the is-it-an-index test below will fail.
994+
*
995+
* Unlike brin_summarize_range(), autovacuum never calls this. Hence, we
996+
* don't switch userid.
969997
*/
970998
heapoid = IndexGetRelation(indexoid, true);
971999
if (OidIsValid(heapoid))

src/backend/catalog/index.c

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

29272927
/* Open and lock the parent heap relation */
29282928
heapRelation = heap_open(heapId, ShareUpdateExclusiveLock);
2929-
/* And the target index relation */
2929+
2930+
/*
2931+
* Switch to the table owner's userid, so that any index functions are run
2932+
* as that user. Also lock down security-restricted operations and
2933+
* arrange to make GUC variable changes local to this command.
2934+
*/
2935+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
2936+
SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
2937+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
2938+
save_nestlevel = NewGUCNestLevel();
2939+
29302940
indexRelation = index_open(indexId, RowExclusiveLock);
29312941

29322942
/*
@@ -2939,16 +2949,6 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
29392949
/* mark build is concurrent just for consistency */
29402950
indexInfo->ii_Concurrent = true;
29412951

2942-
/*
2943-
* Switch to the table owner's userid, so that any index functions are run
2944-
* as that user. Also lock down security-restricted operations and
2945-
* arrange to make GUC variable changes local to this command.
2946-
*/
2947-
GetUserIdAndSecContext(&save_userid, &save_sec_context);
2948-
SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
2949-
save_sec_context | SECURITY_RESTRICTED_OPERATION);
2950-
save_nestlevel = NewGUCNestLevel();
2951-
29522952
/*
29532953
* Scan the index and gather up all the TIDs into a tuplesort object.
29542954
*/
@@ -3411,6 +3411,9 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
34113411
Relation iRel,
34123412
heapRelation;
34133413
Oid heapId;
3414+
Oid save_userid;
3415+
int save_sec_context;
3416+
int save_nestlevel;
34143417
IndexInfo *indexInfo;
34153418
volatile bool skipped_constraint = false;
34163419
PGRUsage ru0;
@@ -3424,6 +3427,16 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
34243427
heapId = IndexGetRelation(indexId, false);
34253428
heapRelation = heap_open(heapId, ShareLock);
34263429

3430+
/*
3431+
* Switch to the table owner's userid, so that any index functions are run
3432+
* as that user. Also lock down security-restricted operations and
3433+
* arrange to make GUC variable changes local to this command.
3434+
*/
3435+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
3436+
SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
3437+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
3438+
save_nestlevel = NewGUCNestLevel();
3439+
34273440
/*
34283441
* Open the target index relation and get an exclusive lock on it, to
34293442
* ensure that no one else is touching this particular index.
@@ -3565,6 +3578,12 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
35653578
errdetail_internal("%s",
35663579
pg_rusage_show(&ru0))));
35673580

3581+
/* Roll back any GUC changes executed by index functions */
3582+
AtEOXact_GUC(false, save_nestlevel);
3583+
3584+
/* Restore userid and security context */
3585+
SetUserIdAndSecContext(save_userid, save_sec_context);
3586+
35683587
/* Close rels, but keep locks */
35693588
index_close(iRel, NoLock);
35703589
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"
@@ -260,6 +261,9 @@ void
260261
cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose)
261262
{
262263
Relation OldHeap;
264+
Oid save_userid;
265+
int save_sec_context;
266+
int save_nestlevel;
263267

264268
/* Check for user-requested abort. */
265269
CHECK_FOR_INTERRUPTS();
@@ -276,6 +280,16 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose)
276280
if (!OldHeap)
277281
return;
278282

283+
/*
284+
* Switch to the table owner's userid, so that any index functions are run
285+
* as that user. Also lock down security-restricted operations and
286+
* arrange to make GUC variable changes local to this command.
287+
*/
288+
GetUserIdAndSecContext(&save_userid, &save_sec_context);
289+
SetUserIdAndSecContext(OldHeap->rd_rel->relowner,
290+
save_sec_context | SECURITY_RESTRICTED_OPERATION);
291+
save_nestlevel = NewGUCNestLevel();
292+
279293
/*
280294
* Since we may open a new transaction for each relation, we have to check
281295
* that the relation still is what we think it is.
@@ -290,10 +304,10 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose)
290304
Form_pg_index indexForm;
291305

292306
/* Check that the user still owns the relation */
293-
if (!pg_class_ownercheck(tableOid, GetUserId()))
307+
if (!pg_class_ownercheck(tableOid, save_userid))
294308
{
295309
relation_close(OldHeap, AccessExclusiveLock);
296-
return;
310+
goto out;
297311
}
298312

299313
/*
@@ -307,7 +321,7 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose)
307321
if (RELATION_IS_OTHER_TEMP(OldHeap))
308322
{
309323
relation_close(OldHeap, AccessExclusiveLock);
310-
return;
324+
goto out;
311325
}
312326

313327
if (OidIsValid(indexOid))
@@ -318,7 +332,7 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose)
318332
if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(indexOid)))
319333
{
320334
relation_close(OldHeap, AccessExclusiveLock);
321-
return;
335+
goto out;
322336
}
323337

324338
/*
@@ -328,14 +342,14 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose)
328342
if (!HeapTupleIsValid(tuple)) /* probably can't happen */
329343
{
330344
relation_close(OldHeap, AccessExclusiveLock);
331-
return;
345+
goto out;
332346
}
333347
indexForm = (Form_pg_index) GETSTRUCT(tuple);
334348
if (!indexForm->indisclustered)
335349
{
336350
ReleaseSysCache(tuple);
337351
relation_close(OldHeap, AccessExclusiveLock);
338-
return;
352+
goto out;
339353
}
340354
ReleaseSysCache(tuple);
341355
}
@@ -389,7 +403,7 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose)
389403
!RelationIsPopulated(OldHeap))
390404
{
391405
relation_close(OldHeap, AccessExclusiveLock);
392-
return;
406+
goto out;
393407
}
394408

395409
/*
@@ -404,6 +418,13 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose)
404418
rebuild_relation(OldHeap, indexOid, verbose);
405419

406420
/* NB: rebuild_relation does heap_close() on OldHeap */
421+
422+
out:
423+
/* Roll back any GUC changes executed by index functions */
424+
AtEOXact_GUC(false, save_nestlevel);
425+
426+
/* Restore userid and security context */
427+
SetUserIdAndSecContext(save_userid, save_sec_context);
407428
}
408429

409430
/*

0 commit comments

Comments
 (0)