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, 41 insertions, 11 deletions

diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index f45d92df73d..beaa08358a2 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -54,11 +54,13 @@
 #include "rewrite/rewriteDefine.h"
 #include "rewrite/rewriteRemove.h"
 #include "storage/fd.h"
+#include "storage/lmgr.h"
 #include "tcop/pquery.h"
 #include "tcop/utility.h"
 #include "utils/acl.h"
 #include "utils/guc.h"
 #include "utils/syscache.h"
+#include "utils/lsyscache.h"
 
 
 /* Hook for plugins to get control in ProcessUtility() */
@@ -72,19 +74,17 @@ ProcessUtility_hook_type ProcessUtility_hook = NULL;
  * except when allowSystemTableMods is true.
  */
 void
-CheckRelationOwnership(RangeVar *rel, bool noCatalogs)
+CheckRelationOwnership(Oid relOid, bool noCatalogs)
 {
-	Oid			relOid;
 	HeapTuple	tuple;
 
-	relOid = RangeVarGetRelid(rel, 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);
+					   get_rel_name(relOid));
 
 	if (noCatalogs)
 	{
@@ -93,7 +93,7 @@ CheckRelationOwnership(RangeVar *rel, bool noCatalogs)
 			ereport(ERROR,
 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 					 errmsg("permission denied: \"%s\" is a system catalog",
-							rel->relname)));
+							get_rel_name(relOid))));
 	}
 
 	ReleaseSysCache(tuple);
@@ -751,9 +751,23 @@ standard_ProcessUtility(Node *parsetree,
 			{
 				List	   *stmts;
 				ListCell   *l;
+				AlterTableStmt *atstmt = (AlterTableStmt *) parsetree;
+				Oid			relid;
+				LOCKMODE	lockmode;
+
+				/*
+				 * 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.
+				 */
+				lockmode = AlterTableGetLockLevel(atstmt->cmds);
+				relid = RangeVarGetRelid(atstmt->relation, false);
+				LockRelationOid(relid, lockmode);
 
 				/* Run parse analysis ... */
-				stmts = transformAlterTableStmt((AlterTableStmt *) parsetree,
+				stmts = transformAlterTableStmt(relid,
+												atstmt,
 												queryString);
 
 				/* ... and do it */
@@ -764,7 +778,7 @@ standard_ProcessUtility(Node *parsetree,
 					if (IsA(stmt, AlterTableStmt))
 					{
 						/* Do the table alteration proper */
-						AlterTable((AlterTableStmt *) stmt);
+						AlterTable(relid, (AlterTableStmt *) stmt);
 					}
 					else
 					{
@@ -927,18 +941,33 @@ standard_ProcessUtility(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 = RangeVarGetRelid(stmt->relation, false);
+				LockRelationOid(relid, lockmode);
+				CheckRelationOwnership(relid, true);
 
 				/* Run parse analysis ... */
-				stmt = transformIndexStmt(stmt, queryString);
+				stmt = transformIndexStmt(relid, stmt, queryString);
 
 				/* ... and do it */
-				DefineIndex(stmt->relation,		/* relation */
+				DefineIndex(relid,		/* relation */
 							stmt->idxname,		/* index name */
 							InvalidOid, /* no predefined OID */
 							stmt->accessMethod, /* am name */
@@ -1105,7 +1134,8 @@ standard_ProcessUtility(Node *parsetree,
 
 		case T_CreateTrigStmt:
 			(void) CreateTrigger((CreateTrigStmt *) parsetree, queryString,
-								 InvalidOid, InvalidOid, false);
+								 InvalidOid, InvalidOid, InvalidOid,
+								 InvalidOid, false);
 			break;
 
 		case T_DropPropertyStmt: