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 files changed, 21 insertions, 26 deletions

diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 0dda5bc27cd..71281f0e2fd 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -1356,14 +1356,18 @@ transformFKConstraints(ParseState *pstate, 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
@@ -1371,25 +1375,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.
-	 */
-	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);
@@ -1431,7 +1427,7 @@ transformIndexStmt(IndexStmt *stmt, const char *queryString)
 
 	free_parsestate(pstate);
 
-	/* Close relation, but keep the lock */
+	/* Close relation */
 	heap_close(rel, NoLock);
 
 	return stmt;
@@ -1722,9 +1718,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;
@@ -1743,14 +1744,8 @@ transformAlterTableStmt(AlterTableStmt *stmt, const char *queryString)
 	 */
 	stmt = (AlterTableStmt *) copyObject(stmt);
 
-	/*
-	 * Acquire exclusive 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
-	 * exclusive lock now because execution will; taking a lower grade lock
-	 * now and trying to upgrade later risks deadlock.
-	 */
-	rel = relation_openrv(stmt->relation, AccessExclusiveLock);
+	/* Caller is responsible for locking the relation */
+	rel = relation_open(relid, NoLock);
 
 	/* Set up pstate */
 	pstate = make_parsestate(NULL);
@@ -1866,7 +1861,7 @@ transformAlterTableStmt(AlterTableStmt *stmt, const char *queryString)
 		Assert(IsA(idxstmt, IndexStmt));
 		newcmd = makeNode(AlterTableCmd);
 		newcmd->subtype = AT_AddIndex;
-		newcmd->def = (Node *) transformIndexStmt((IndexStmt *) idxstmt,
+		newcmd->def = (Node *) transformIndexStmt(relid, (IndexStmt *) idxstmt,
 												  queryString);
 		newcmds = lappend(newcmds, newcmd);
 	}
@@ -1888,7 +1883,7 @@ transformAlterTableStmt(AlterTableStmt *stmt, const char *queryString)
 		newcmds = lappend(newcmds, newcmd);
 	}
 
-	/* Close rel but keep lock */
+	/* Close rel */
 	relation_close(rel, NoLock);
 
 	/*