Skip to content

Commit 820ab11

Browse files
robertmhaasrobertmhaas
committed
Avoid repeated name lookups during table and index DDL.
If the name lookups come to different conclusions due to concurrent activity, we might perform some parts of the DDL on a different table than other parts. At least in the case of CREATE INDEX, this can be used to cause the permissions checks to be performed against a different table than the index creation, allowing for a privilege escalation attack. This changes the calling convention for DefineIndex, CreateTrigger, transformIndexStmt, transformAlterTableStmt, CheckIndexCompatible (in 9.2 and newer), and AlterTable (in 9.1 and older). In addition, CheckRelationOwnership is removed in 9.2 and newer and the calling convention is changed in older branches. A field has also been added to the Constraint node (FkConstraint in 8.4). Third-party code calling these functions or using the Constraint node will require updating. Report by Andres Freund. Patch by Robert Haas and Andres Freund, reviewed by Tom Lane. Security: CVE-2014-0062
1 parent c38c308 commit 820ab11

File tree

18 files changed

+234
-157
lines changed

18 files changed

+234
-157
lines changed

‎src/backend/bootstrap/bootparse.y

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
#include "bootstrap/bootstrap.h"
2828
#include "catalog/catalog.h"
2929
#include "catalog/heap.h"
30+
#include "catalog/namespace.h"
3031
#include "catalog/pg_am.h"
3132
#include "catalog/pg_attribute.h"
3233
#include "catalog/pg_authid.h"
@@ -281,6 +282,7 @@ Boot_DeclareIndexStmt:
281282
XDECLARE INDEX boot_ident oidspec ON boot_ident USING boot_ident LPAREN boot_index_params RPAREN
282283
{
283284
IndexStmt *stmt = makeNode(IndexStmt);
285+
Oid relationId;
284286

285287
do_start();
286288

@@ -302,7 +304,12 @@ Boot_DeclareIndexStmt:
302304
stmt->initdeferred = false;
303305
stmt->concurrent = false;
304306

305-
DefineIndex(stmt,
307+
/* locks and races need not concern us in bootstrap mode */
308+
relationId = RangeVarGetRelid(stmt->relation, NoLock,
309+
false);
310+
311+
DefineIndex(relationId,
312+
stmt,
306313
$4,
307314
false,
308315
false,
@@ -316,6 +323,7 @@ Boot_DeclareUniqueIndexStmt:
316323
XDECLARE UNIQUE INDEX boot_ident oidspec ON boot_ident USING boot_ident LPAREN boot_index_params RPAREN
317324
{
318325
IndexStmt *stmt = makeNode(IndexStmt);
326+
Oid relationId;
319327

320328
do_start();
321329

@@ -337,7 +345,12 @@ Boot_DeclareUniqueIndexStmt:
337345
stmt->initdeferred = false;
338346
stmt->concurrent = false;
339347

340-
DefineIndex(stmt,
348+
/* locks and races need not concern us in bootstrap mode */
349+
relationId = RangeVarGetRelid(stmt->relation, NoLock,
350+
false);
351+
352+
DefineIndex(relationId,
353+
stmt,
341354
$5,
342355
false,
343356
false,

‎src/backend/catalog/index.c

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1202,18 +1202,13 @@ index_constraint_create(Relation heapRelation,
12021202
*/
12031203
if (deferrable)
12041204
{
1205-
RangeVar *heapRel;
12061205
CreateTrigStmt *trigger;
12071206

1208-
heapRel = makeRangeVar(get_namespace_name(namespaceId),
1209-
pstrdup(RelationGetRelationName(heapRelation)),
1210-
-1);
1211-
12121207
trigger = makeNode(CreateTrigStmt);
12131208
trigger->trigname = (constraintType == CONSTRAINT_PRIMARY) ?
12141209
"PK_ConstraintTrigger" :
12151210
"Unique_ConstraintTrigger";
1216-
trigger->relation = heapRel;
1211+
trigger->relation = NULL;
12171212
trigger->funcname = SystemFuncName("unique_key_recheck");
12181213
trigger->args = NIL;
12191214
trigger->row = true;
@@ -1226,7 +1221,8 @@ index_constraint_create(Relation heapRelation,
12261221
trigger->initdeferred = initdeferred;
12271222
trigger->constrrel = NULL;
12281223

1229-
(void) CreateTrigger(trigger, NULL, conOid, indexRelationId, true);
1224+
(void) CreateTrigger(trigger, NULL, RelationGetRelid(heapRelation),
1225+
InvalidOid, conOid, indexRelationId, true);
12301226
}
12311227

12321228
/*

‎src/backend/catalog/pg_constraint.c

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -745,6 +745,25 @@ AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
745745
heap_close(conRel, RowExclusiveLock);
746746
}
747747

748+
/*
749+
* get_constraint_relation_oids
750+
* Find the IDs of the relations to which a constraint refers.
751+
*/
752+
void
753+
get_constraint_relation_oids(Oid constraint_oid, Oid *conrelid, Oid *confrelid)
754+
{
755+
HeapTuple tup;
756+
Form_pg_constraint con;
757+
758+
tup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(constraint_oid));
759+
if (!HeapTupleIsValid(tup)) /* should not happen */
760+
elog(ERROR, "cache lookup failed for constraint %u", constraint_oid);
761+
con = (Form_pg_constraint) GETSTRUCT(tup);
762+
*conrelid = con->conrelid;
763+
*confrelid = con->confrelid;
764+
ReleaseSysCache(tup);
765+
}
766+
748767
/*
749768
* get_relation_constraint_oid
750769
* Find a constraint on the specified relation with the specified name.

‎src/backend/commands/indexcmds.c

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,6 @@ static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
111111
*/
112112
bool
113113
CheckIndexCompatible(Oid oldId,
114-
RangeVar *heapRelation,
115114
char *accessMethodName,
116115
List *attributeList,
117116
List *exclusionOpNames)
@@ -139,7 +138,7 @@ CheckIndexCompatible(Oid oldId,
139138
Datum d;
140139

141140
/* Caller should already have the relation locked in some way. */
142-
relationId = RangeVarGetRelid(heapRelation, NoLock, false);
141+
relationId = IndexGetRelation(oldId, false);
143142

144143
/*
145144
* We can pretend isconstraint = false unconditionally. It only serves to
@@ -279,6 +278,8 @@ CheckIndexCompatible(Oid oldId,
279278
* DefineIndex
280279
* Creates a new index.
281280
*
281+
* 'relationId': the OID of the heap relation on which the index is to be
282+
* created
282283
* 'stmt': IndexStmt describing the properties of the new index.
283284
* 'indexRelationId': normally InvalidOid, but during bootstrap can be
284285
* nonzero to specify a preselected OID for the index.
@@ -292,7 +293,8 @@ CheckIndexCompatible(Oid oldId,
292293
* Returns the OID of the created index.
293294
*/
294295
Oid
295-
DefineIndex(IndexStmt *stmt,
296+
DefineIndex(Oid relationId,
297+
IndexStmt *stmt,
296298
Oid indexRelationId,
297299
bool is_alter_table,
298300
bool check_rights,
@@ -305,7 +307,6 @@ DefineIndex(IndexStmt *stmt,
305307
Oid *collationObjectId;
306308
Oid *classObjectId;
307309
Oid accessMethodId;
308-
Oid relationId;
309310
Oid namespaceId;
310311
Oid tablespaceId;
311312
List *indexColNames;
@@ -325,6 +326,7 @@ DefineIndex(IndexStmt *stmt,
325326
int n_old_snapshots;
326327
LockRelId heaprelid;
327328
LOCKTAG heaplocktag;
329+
LOCKMODE lockmode;
328330
Snapshot snapshot;
329331
int i;
330332

@@ -343,14 +345,18 @@ DefineIndex(IndexStmt *stmt,
343345
INDEX_MAX_KEYS)));
344346

345347
/*
346-
* Open heap relation, acquire a suitable lock on it, remember its OID
347-
*
348348
* Only SELECT ... FOR UPDATE/SHARE are allowed while doing a standard
349349
* index build; but for concurrent builds we allow INSERT/UPDATE/DELETE
350350
* (but not VACUUM).
351+
*
352+
* NB: Caller is responsible for making sure that relationId refers
353+
* to the relation on which the index should be built; except in bootstrap
354+
* mode, this will typically require the caller to have already locked
355+
* the relation. To avoid lock upgrade hazards, that lock should be at
356+
* least as strong as the one we take here.
351357
*/
352-
rel = heap_openrv(stmt->relation,
353-
(stmt->concurrent ? ShareUpdateExclusiveLock : ShareLock));
358+
lockmode = stmt->concurrent ? ShareUpdateExclusiveLock : ShareLock;
359+
rel = heap_open(relationId, lockmode);
354360

355361
relationId = RelationGetRelid(rel);
356362
namespaceId = RelationGetNamespace(rel);

0 commit comments

Comments
 (0)