Commit 5f173040 authored by Robert Haas's avatar Robert Haas

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
parent 540b4e5b
......@@ -27,6 +27,7 @@
#include "bootstrap/bootstrap.h"
#include "catalog/catalog.h"
#include "catalog/heap.h"
#include "catalog/namespace.h"
#include "catalog/pg_am.h"
#include "catalog/pg_attribute.h"
#include "catalog/pg_authid.h"
......@@ -282,6 +283,7 @@ Boot_DeclareIndexStmt:
XDECLARE INDEX boot_ident oidspec ON boot_ident USING boot_ident LPAREN boot_index_params RPAREN
{
IndexStmt *stmt = makeNode(IndexStmt);
Oid relationId;
do_start();
......@@ -303,7 +305,12 @@ Boot_DeclareIndexStmt:
stmt->initdeferred = false;
stmt->concurrent = false;
DefineIndex(stmt,
/* locks and races need not concern us in bootstrap mode */
relationId = RangeVarGetRelid(stmt->relation, NoLock,
false);
DefineIndex(relationId,
stmt,
$4,
false,
false,
......@@ -317,6 +324,7 @@ Boot_DeclareUniqueIndexStmt:
XDECLARE UNIQUE INDEX boot_ident oidspec ON boot_ident USING boot_ident LPAREN boot_index_params RPAREN
{
IndexStmt *stmt = makeNode(IndexStmt);
Oid relationId;
do_start();
......@@ -338,7 +346,12 @@ Boot_DeclareUniqueIndexStmt:
stmt->initdeferred = false;
stmt->concurrent = false;
DefineIndex(stmt,
/* locks and races need not concern us in bootstrap mode */
relationId = RangeVarGetRelid(stmt->relation, NoLock,
false);
DefineIndex(relationId,
stmt,
$5,
false,
false,
......
......@@ -1215,18 +1215,13 @@ index_constraint_create(Relation heapRelation,
*/
if (deferrable)
{
RangeVar *heapRel;
CreateTrigStmt *trigger;
heapRel = makeRangeVar(get_namespace_name(namespaceId),
pstrdup(RelationGetRelationName(heapRelation)),
-1);
trigger = makeNode(CreateTrigStmt);
trigger->trigname = (constraintType == CONSTRAINT_PRIMARY) ?
"PK_ConstraintTrigger" :
"Unique_ConstraintTrigger";
trigger->relation = heapRel;
trigger->relation = NULL;
trigger->funcname = SystemFuncName("unique_key_recheck");
trigger->args = NIL;
trigger->row = true;
......@@ -1239,7 +1234,8 @@ index_constraint_create(Relation heapRelation,
trigger->initdeferred = initdeferred;
trigger->constrrel = NULL;
(void) CreateTrigger(trigger, NULL, conOid, indexRelationId, true);
(void) CreateTrigger(trigger, NULL, RelationGetRelid(heapRelation),
InvalidOid, conOid, indexRelationId, true);
}
/*
......
......@@ -751,6 +751,25 @@ AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
heap_close(conRel, RowExclusiveLock);
}
/*
* get_constraint_relation_oids
* Find the IDs of the relations to which a constraint refers.
*/
void
get_constraint_relation_oids(Oid constraint_oid, Oid *conrelid, Oid *confrelid)
{
HeapTuple tup;
Form_pg_constraint con;
tup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(constraint_oid));
if (!HeapTupleIsValid(tup)) /* should not happen */
elog(ERROR, "cache lookup failed for constraint %u", constraint_oid);
con = (Form_pg_constraint) GETSTRUCT(tup);
*conrelid = con->conrelid;
*confrelid = con->confrelid;
ReleaseSysCache(tup);
}
/*
* get_relation_constraint_oid
* Find a constraint on the specified relation with the specified name.
......
......@@ -112,7 +112,6 @@ static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
*/
bool
CheckIndexCompatible(Oid oldId,
RangeVar *heapRelation,
char *accessMethodName,
List *attributeList,
List *exclusionOpNames)
......@@ -140,7 +139,7 @@ CheckIndexCompatible(Oid oldId,
Datum d;
/* Caller should already have the relation locked in some way. */
relationId = RangeVarGetRelid(heapRelation, NoLock, false);
relationId = IndexGetRelation(oldId, false);
/*
* We can pretend isconstraint = false unconditionally. It only serves to
......@@ -280,6 +279,8 @@ CheckIndexCompatible(Oid oldId,
* DefineIndex
* Creates a new index.
*
* 'relationId': the OID of the heap relation on which the index is to be
* created
* 'stmt': IndexStmt describing the properties of the new index.
* 'indexRelationId': normally InvalidOid, but during bootstrap can be
* nonzero to specify a preselected OID for the index.
......@@ -293,7 +294,8 @@ CheckIndexCompatible(Oid oldId,
* Returns the OID of the created index.
*/
Oid
DefineIndex(IndexStmt *stmt,
DefineIndex(Oid relationId,
IndexStmt *stmt,
Oid indexRelationId,
bool is_alter_table,
bool check_rights,
......@@ -306,7 +308,6 @@ DefineIndex(IndexStmt *stmt,
Oid *collationObjectId;
Oid *classObjectId;
Oid accessMethodId;
Oid relationId;
Oid namespaceId;
Oid tablespaceId;
List *indexColNames;
......@@ -325,6 +326,7 @@ DefineIndex(IndexStmt *stmt,
int n_old_snapshots;
LockRelId heaprelid;
LOCKTAG heaplocktag;
LOCKMODE lockmode;
Snapshot snapshot;
int i;
......@@ -343,14 +345,18 @@ DefineIndex(IndexStmt *stmt,
INDEX_MAX_KEYS)));
/*
* Open heap relation, acquire a suitable lock on it, remember its OID
*
* Only SELECT ... FOR UPDATE/SHARE are allowed while doing a standard
* index build; but for concurrent builds we allow INSERT/UPDATE/DELETE
* (but not VACUUM).
*
* NB: Caller is responsible for making sure that relationId refers
* to the relation on which the index should be built; except in bootstrap
* mode, this will typically require the caller to have already locked
* the relation. To avoid lock upgrade hazards, that lock should be at
* least as strong as the one we take here.
*/
rel = heap_openrv(stmt->relation,
(stmt->concurrent ? ShareUpdateExclusiveLock : ShareLock));
lockmode = stmt->concurrent ? ShareUpdateExclusiveLock : ShareLock;
rel = heap_open(relationId, lockmode);
relationId = RelationGetRelid(rel);
namespaceId = RelationGetNamespace(rel);
......
This diff is collapsed.
......@@ -43,6 +43,7 @@
#include "pgstat.h"
#include "rewrite/rewriteManip.h"
#include "storage/bufmgr.h"
#include "storage/lmgr.h"
#include "tcop/utility.h"
#include "utils/acl.h"
#include "utils/builtins.h"
......@@ -96,6 +97,13 @@ static void AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo,
* queryString is the source text of the CREATE TRIGGER command.
* This must be supplied if a whenClause is specified, else it can be NULL.
*
* relOid, if nonzero, is the relation on which the trigger should be
* created. If zero, the name provided in the statement will be looked up.
*
* refRelOid, if nonzero, is the relation to which the constraint trigger
* refers. If zero, the constraint relation name provided in the statement
* will be looked up as needed.
*
* constraintOid, if nonzero, says that this trigger is being created
* internally to implement that constraint. A suitable pg_depend entry will
* be made to link the trigger to that constraint. constraintOid is zero when
......@@ -118,7 +126,7 @@ static void AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo,
*/
Oid
CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
Oid constraintOid, Oid indexOid,
Oid relOid, Oid refRelOid, Oid constraintOid, Oid indexOid,
bool isInternal)
{
int16 tgtype;
......@@ -147,7 +155,10 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
ObjectAddress myself,
referenced;
rel = heap_openrv(stmt->relation, AccessExclusiveLock);
if (OidIsValid(relOid))
rel = heap_open(relOid, AccessExclusiveLock);
else
rel = heap_openrv(stmt->relation, AccessExclusiveLock);
/*
* Triggers must be on tables or views, and there are additional
......@@ -196,7 +207,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
errmsg("permission denied: \"%s\" is a system catalog",
RelationGetRelationName(rel))));
if (stmt->isconstraint && stmt->constrrel != NULL)
if (stmt->isconstraint)
{
/*
* We must take a lock on the target relation to protect against
......@@ -205,7 +216,14 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
* might end up creating a pg_constraint entry referencing a
* nonexistent table.
*/
constrrelid = RangeVarGetRelid(stmt->constrrel, AccessShareLock, false);
if (OidIsValid(refRelOid))
{
LockRelationOid(refRelOid, AccessShareLock);
constrrelid = refRelOid;
}
else if (stmt->constrrel != NULL)
constrrelid = RangeVarGetRelid(stmt->constrrel, AccessShareLock,
false);
}
/* permission checks */
......@@ -501,7 +519,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
ereport(ERROR,
(errcode(ERRCODE_DUPLICATE_OBJECT),
errmsg("trigger \"%s\" for relation \"%s\" already exists",
trigname, stmt->relation->relname)));
trigname, RelationGetRelationName(rel))));
}
systable_endscan(tgscan);
}
......
......@@ -2411,6 +2411,7 @@ _copyConstraint(const Constraint *from)
COPY_SCALAR_FIELD(fk_upd_action);
COPY_SCALAR_FIELD(fk_del_action);
COPY_NODE_FIELD(old_conpfeqop);
COPY_SCALAR_FIELD(old_pktable_oid);
COPY_SCALAR_FIELD(skip_validation);
COPY_SCALAR_FIELD(initially_valid);
......
......@@ -2239,6 +2239,7 @@ _equalConstraint(const Constraint *a, const Constraint *b)
COMPARE_SCALAR_FIELD(fk_upd_action);
COMPARE_SCALAR_FIELD(fk_del_action);
COMPARE_NODE_FIELD(old_conpfeqop);
COMPARE_SCALAR_FIELD(old_pktable_oid);
COMPARE_SCALAR_FIELD(skip_validation);
COMPARE_SCALAR_FIELD(initially_valid);
......
......@@ -2709,6 +2709,7 @@ _outConstraint(StringInfo str, const Constraint *node)
WRITE_CHAR_FIELD(fk_upd_action);
WRITE_CHAR_FIELD(fk_del_action);
WRITE_NODE_FIELD(old_conpfeqop);
WRITE_OID_FIELD(old_pktable_oid);
WRITE_BOOL_FIELD(skip_validation);
WRITE_BOOL_FIELD(initially_valid);
break;
......
......@@ -1903,14 +1903,18 @@ transformFKConstraints(CreateStmtContext *cxt,
* a predicate expression. There are several code paths that create indexes
* without bothering to call this, because they know they don't have any
* such expressions to deal with.
*
* To avoid race conditions, it's important that this function rely only on
* the passed-in relid (and not on stmt->relation) to determine the target
* relation.
*/
IndexStmt *
transformIndexStmt(IndexStmt *stmt, const char *queryString)
transformIndexStmt(Oid relid, IndexStmt *stmt, const char *queryString)
{
Relation rel;
ParseState *pstate;
RangeTblEntry *rte;
ListCell *l;
Relation rel;
/*
* We must not scribble on the passed-in IndexStmt, so copy it. (This is
......@@ -1918,26 +1922,17 @@ transformIndexStmt(IndexStmt *stmt, const char *queryString)
*/
stmt = (IndexStmt *) copyObject(stmt);
/*
* Open the parent table with appropriate locking. We must do this
* because addRangeTableEntry() would acquire only AccessShareLock,
* leaving DefineIndex() needing to do a lock upgrade with consequent risk
* of deadlock. Make sure this stays in sync with the type of lock
* DefineIndex() wants. If we are being called by ALTER TABLE, we will
* already hold a higher lock.
*/
rel = heap_openrv(stmt->relation,
(stmt->concurrent ? ShareUpdateExclusiveLock : ShareLock));
/* Set up pstate */
pstate = make_parsestate(NULL);
pstate->p_sourcetext = queryString;
/*
* Put the parent table into the rtable so that the expressions can refer
* to its fields without qualification.
* to its fields without qualification. Caller is responsible for locking
* relation, but we still need to open it.
*/
rte = addRangeTableEntry(pstate, stmt->relation, NULL, false, true);
rel = relation_open(relid, NoLock);
rte = addRangeTableEntryForRelation(pstate, rel, NULL, false, true);
/* no to join list, yes to namespaces */
addRTEtoQuery(pstate, rte, false, true, true);
......@@ -1998,7 +1993,7 @@ transformIndexStmt(IndexStmt *stmt, const char *queryString)
free_parsestate(pstate);
/* Close relation, but keep the lock */
/* Close relation */
heap_close(rel, NoLock);
return stmt;
......@@ -2317,9 +2312,14 @@ transformRuleStmt(RuleStmt *stmt, const char *queryString,
* Returns a List of utility commands to be done in sequence. One of these
* will be the transformed AlterTableStmt, but there may be additional actions
* to be done before and after the actual AlterTable() call.
*
* To avoid race conditions, it's important that this function rely only on
* the passed-in relid (and not on stmt->relation) to determine the target
* relation.
*/
List *
transformAlterTableStmt(AlterTableStmt *stmt, const char *queryString)
transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
const char *queryString)
{
Relation rel;
ParseState *pstate;
......@@ -2331,7 +2331,6 @@ transformAlterTableStmt(AlterTableStmt *stmt, const char *queryString)
List *newcmds = NIL;
bool skipValidation = true;
AlterTableCmd *newcmd;
LOCKMODE lockmode;
/*
* We must not scribble on the passed-in AlterTableStmt, so copy it. (This
......@@ -2339,29 +2338,8 @@ transformAlterTableStmt(AlterTableStmt *stmt, const char *queryString)
*/
stmt = (AlterTableStmt *) copyObject(stmt);
/*
* Determine the appropriate lock level for this list of subcommands.
*/
lockmode = AlterTableGetLockLevel(stmt->cmds);
/*
* Acquire appropriate lock on the target relation, which will be held
* until end of transaction. This ensures any decisions we make here
* based on the state of the relation will still be good at execution. We
* must get lock now because execution will later require it; taking a
* lower grade lock now and trying to upgrade later risks deadlock. Any
* new commands we add after this must not upgrade the lock level
* requested here.
*/
rel = relation_openrv_extended(stmt->relation, lockmode, stmt->missing_ok);
if (rel == NULL)
{
/* this message is consistent with relation_openrv */
ereport(NOTICE,
(errmsg("relation \"%s\" does not exist, skipping",
stmt->relation->relname)));
return NIL;
}
/* Caller is responsible for locking the relation */
rel = relation_open(relid, NoLock);
/* Set up pstate and CreateStmtContext */
pstate = make_parsestate(NULL);
......@@ -2483,7 +2461,7 @@ transformAlterTableStmt(AlterTableStmt *stmt, const char *queryString)
IndexStmt *idxstmt = (IndexStmt *) lfirst(l);
Assert(IsA(idxstmt, IndexStmt));
idxstmt = transformIndexStmt(idxstmt, queryString);
idxstmt = transformIndexStmt(relid, idxstmt, queryString);
newcmd = makeNode(AlterTableCmd);
newcmd->subtype = OidIsValid(idxstmt->indexOid) ? AT_AddIndexConstraint : AT_AddIndex;
newcmd->def = (Node *) idxstmt;
......@@ -2507,7 +2485,7 @@ transformAlterTableStmt(AlterTableStmt *stmt, const char *queryString)
newcmds = lappend(newcmds, newcmd);
}
/* Close rel but keep lock */
/* Close rel */
relation_close(rel, NoLock);
/*
......
......@@ -78,49 +78,6 @@ static void ProcessUtilitySlow(Node *parsetree,
static void ExecDropStmt(DropStmt *stmt, bool isTopLevel);
/*
* Verify user has ownership of specified relation, else ereport.
*
* If noCatalogs is true then we also deny access to system catalogs,
* except when allowSystemTableMods is true.
*/
void
CheckRelationOwnership(RangeVar *rel, bool noCatalogs)
{
Oid relOid;
HeapTuple tuple;
/*
* XXX: This is unsafe in the presence of concurrent DDL, since it is
* called before acquiring any lock on the target relation. However,
* locking the target relation (especially using something like
* AccessExclusiveLock) before verifying that the user has permissions is
* not appealing either.
*/
relOid = RangeVarGetRelid(rel, NoLock, false);
tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relOid));
if (!HeapTupleIsValid(tuple)) /* should not happen */
elog(ERROR, "cache lookup failed for relation %u", relOid);
if (!pg_class_ownercheck(relOid, GetUserId()))
aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
rel->relname);
if (noCatalogs)
{
if (!allowSystemTableMods &&
IsSystemClass(relOid, (Form_pg_class) GETSTRUCT(tuple)))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied: \"%s\" is a system catalog",
rel->relname)));
}
ReleaseSysCache(tuple);
}
/*
* CommandIsReadOnly: is an executable query read-only?
*
......@@ -1019,7 +976,8 @@ ProcessUtilitySlow(Node *parsetree,
if (OidIsValid(relid))
{
/* Run parse analysis ... */
stmts = transformAlterTableStmt(atstmt, queryString);
stmts = transformAlterTableStmt(relid, atstmt,
queryString);
/* ... and do it */
foreach(l, stmts)
......@@ -1160,18 +1118,36 @@ ProcessUtilitySlow(Node *parsetree,
case T_IndexStmt: /* CREATE INDEX */
{
IndexStmt *stmt = (IndexStmt *) parsetree;
Oid relid;
LOCKMODE lockmode;
if (stmt->concurrent)
PreventTransactionChain(isTopLevel,
"CREATE INDEX CONCURRENTLY");
CheckRelationOwnership(stmt->relation, true);
/*
* Look up the relation OID just once, right here at the
* beginning, so that we don't end up repeating the name
* lookup later and latching onto a different relation
* partway through. To avoid lock upgrade hazards, it's
* important that we take the strongest lock that will
* eventually be needed here, so the lockmode calculation
* needs to match what DefineIndex() does.
*/
lockmode = stmt->concurrent ? ShareUpdateExclusiveLock
: ShareLock;
relid =
RangeVarGetRelidExtended(stmt->relation, lockmode,
false, false,
RangeVarCallbackOwnsRelation,
NULL);
/* Run parse analysis ... */
stmt = transformIndexStmt(stmt, queryString);
stmt = transformIndexStmt(relid, stmt, queryString);
/* ... and do it */
DefineIndex(stmt,
DefineIndex(relid, /* OID of heap relation */
stmt,
InvalidOid, /* no predefined OID */
false, /* is_alter_table */
true, /* check_rights */
......@@ -1276,7 +1252,8 @@ ProcessUtilitySlow(Node *parsetree,
case T_CreateTrigStmt:
(void) CreateTrigger((CreateTrigStmt *) parsetree, queryString,
InvalidOid, InvalidOid, false);
InvalidOid, InvalidOid, InvalidOid,
InvalidOid, false);
break;
case T_CreatePLangStmt:
......
......@@ -247,6 +247,7 @@ extern char *ChooseConstraintName(const char *name1, const char *name2,
extern void AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
Oid newNspId, bool isType, ObjectAddresses *objsMoved);
extern void get_constraint_relation_oids(Oid constraint_oid, Oid *conrelid, Oid *confrelid);
extern Oid get_relation_constraint_oid(Oid relid, const char *conname, bool missing_ok);
extern Oid get_domain_constraint_oid(Oid typid, const char *conname, bool missing_ok);
......
......@@ -21,7 +21,8 @@
extern void RemoveObjects(DropStmt *stmt);
/* commands/indexcmds.c */
extern Oid DefineIndex(IndexStmt *stmt,
extern Oid DefineIndex(Oid relationId,
IndexStmt *stmt,
Oid indexRelationId,
bool is_alter_table,
bool check_rights,
......@@ -36,7 +37,6 @@ extern char *makeObjectName(const char *name1, const char *name2,
extern char *ChooseRelationName(const char *name1, const char *name2,
const char *label, Oid namespaceid);
extern bool CheckIndexCompatible(Oid oldId,
RangeVar *heapRelation,
char *accessMethodName,
List *attributeList,
List *exclusionOpNames);
......
......@@ -78,4 +78,6 @@ extern void AtEOSubXact_on_commit_actions(bool isCommit,
extern void RangeVarCallbackOwnsTable(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg);
extern void RangeVarCallbackOwnsRelation(const RangeVar *relation,
Oid relId, Oid oldRelId, void *noCatalogs);
#endif /* TABLECMDS_H */
......@@ -109,7 +109,7 @@ extern PGDLLIMPORT int SessionReplicationRole;
#define TRIGGER_DISABLED 'D'
extern Oid CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
Oid constraintOid, Oid indexOid,
Oid relOid, Oid refRelOid, Oid constraintOid, Oid indexOid,
bool isInternal);
extern void RemoveTriggerById(Oid trigOid);
......
......@@ -1652,6 +1652,7 @@ typedef struct Constraint
char fk_upd_action; /* ON UPDATE action */
char fk_del_action; /* ON DELETE action */
List *old_conpfeqop; /* pg_constraint.conpfeqop of my former self */
Oid old_pktable_oid; /* pg_constraint.confrelid of my former self */
/* Fields used for constraints that allow a NOT VALID specification */
bool skip_validation; /* skip validation of existing rows? */
......
......@@ -18,9 +18,10 @@
extern List *transformCreateStmt(CreateStmt *stmt, const char *queryString);
extern List *transformAlterTableStmt(AlterTableStmt *stmt,
extern List *transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
const char *queryString);
extern IndexStmt *transformIndexStmt(IndexStmt *stmt, const char *queryString);
extern IndexStmt *transformIndexStmt(Oid relid, IndexStmt *stmt,
const char *queryString);
extern void transformRuleStmt(RuleStmt *stmt, const char *queryString,
List **actions, Node **whereClause);
extern List *transformCreateSchemaStmt(CreateSchemaStmt *stmt);
......
......@@ -49,6 +49,4 @@ extern LogStmtLevel GetCommandLogLevel(Node *parsetree);
extern bool CommandIsReadOnly(Node *parsetree);
extern void CheckRelationOwnership(RangeVar *rel, bool noCatalogs);
#endif /* UTILITY_H */
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment