In extensions, don't replace objects not belonging to the extension.

Previously, if an extension script did CREATE OR REPLACE and there was
an existing object not belonging to the extension, it would overwrite
the object and adopt it into the extension.  This is problematic, first
because the overwrite is probably unintentional, and second because we
didn't change the object's ownership.  Thus a hostile user could create
an object in advance of an expected CREATE EXTENSION command, and would
then have ownership rights on an extension object, which could be
modified for trojan-horse-type attacks.

Hence, forbid CREATE OR REPLACE of an existing object unless it already
belongs to the extension.  (Note that we've always forbidden replacing
an object that belongs to some other extension; only the behavior for
previously-free-standing objects changes here.)

For the same reason, also fail CREATE IF NOT EXISTS when there is
an existing object that doesn't belong to the extension.

Our thanks to Sven Klemm for reporting this problem.

Security: CVE-2022-2625

Author: tglsfdc

doc/src/sgml/extend.sgml

@@ -1288,17 +1288,6 @@ SELECT * FROM pg_extension_update_paths('<replaceable>extension_name</replaceabl
       trusted if it depends on another one, unless that other one is always
       installed in <literal>pg_catalog</literal>.
      </para>
-
-     <para>
-      Do <emphasis>not</emphasis> use <command>CREATE OR REPLACE
-      FUNCTION</command>, except in an update script that must change the
-      definition of a function that is known to be an extension member
-      already.  (Likewise for other <literal>OR REPLACE</literal> options.)
-      Using <literal>OR REPLACE</literal> unnecessarily not only has a risk
-      of accidentally overwriting someone else's function, but it creates a
-      security hazard since the overwritten function would still be owned by
-      its original owner, who could modify it.
-     </para>
     </sect3>
    </sect2>
 

src/backend/catalog/pg_collation.c

@@ -78,15 +78,25 @@ CollationCreate(const char *collname, Oid collnamespace,
 	 * friendlier error message.  The unique index provides a backstop against
 	 * race conditions.
 	 */
-	if (SearchSysCacheExists3(COLLNAMEENCNSP,
-							  PointerGetDatum(collname),
-							  Int32GetDatum(collencoding),
-							  ObjectIdGetDatum(collnamespace)))
+	oid = GetSysCacheOid3(COLLNAMEENCNSP,
+						  Anum_pg_collation_oid,
+						  PointerGetDatum(collname),
+						  Int32GetDatum(collencoding),
+						  ObjectIdGetDatum(collnamespace));
+	if (OidIsValid(oid))
 	{
 		if (quiet)
 			return InvalidOid;
 		else if (if_not_exists)
 		{
+			/*
+			 * If we are in an extension script, insist that the pre-existing
+			 * object be a member of the extension, to avoid security risks.
+			 */
+			ObjectAddressSet(myself, CollationRelationId, oid);
+			checkMembershipInCurrentExtension(&myself);
+
+			/* OK to skip */
 			ereport(NOTICE,
 					(errcode(ERRCODE_DUPLICATE_OBJECT),
 					 collencoding == -1
@@ -116,16 +126,19 @@ CollationCreate(const char *collname, Oid collnamespace,
 	 * so we take a ShareRowExclusiveLock earlier, to protect against
 	 * concurrent changes fooling this check.
 	 */
-	if ((collencoding == -1 &&
-		 SearchSysCacheExists3(COLLNAMEENCNSP,
-							   PointerGetDatum(collname),
-							   Int32GetDatum(GetDatabaseEncoding()),
-							   ObjectIdGetDatum(collnamespace))) ||
-		(collencoding != -1 &&
-		 SearchSysCacheExists3(COLLNAMEENCNSP,
-							   PointerGetDatum(collname),
-							   Int32GetDatum(-1),
-							   ObjectIdGetDatum(collnamespace))))
+	if (collencoding == -1)
+		oid = GetSysCacheOid3(COLLNAMEENCNSP,
+							  Anum_pg_collation_oid,
+							  PointerGetDatum(collname),
+							  Int32GetDatum(GetDatabaseEncoding()),
+							  ObjectIdGetDatum(collnamespace));
+	else
+		oid = GetSysCacheOid3(COLLNAMEENCNSP,
+							  Anum_pg_collation_oid,
+							  PointerGetDatum(collname),
+							  Int32GetDatum(-1),
+							  ObjectIdGetDatum(collnamespace));
+	if (OidIsValid(oid))
 	{
 		if (quiet)
 		{
@@ -134,6 +147,14 @@ CollationCreate(const char *collname, Oid collnamespace,
 		}
 		else if (if_not_exists)
 		{
+			/*
+			 * If we are in an extension script, insist that the pre-existing
+			 * object be a member of the extension, to avoid security risks.
+			 */
+			ObjectAddressSet(myself, CollationRelationId, oid);
+			checkMembershipInCurrentExtension(&myself);
+
+			/* OK to skip */
 			table_close(rel, NoLock);
 			ereport(NOTICE,
 					(errcode(ERRCODE_DUPLICATE_OBJECT),

src/backend/catalog/pg_depend.c

@@ -124,22 +124,23 @@ recordMultipleDependencies(const ObjectAddress *depender,
 
 /*
  * If we are executing a CREATE EXTENSION operation, mark the given object
- * as being a member of the extension.  Otherwise, do nothing.
+ * as being a member of the extension, or check that it already is one.
+ * Otherwise, do nothing.
  *
  * This must be called during creation of any user-definable object type
  * that could be a member of an extension.
  *
- * If isReplace is true, the object already existed (or might have already
- * existed), so we must check for a pre-existing extension membership entry.
- * Passing false is a guarantee that the object is newly created, and so
- * could not already be a member of any extension.
+ * isReplace must be true if the object already existed, and false if it is
+ * newly created.  In the former case we insist that it already be a member
+ * of the current extension.  In the latter case we can skip checking whether
+ * it is already a member of any extension.
  *
  * Note: isReplace = true is typically used when updating a object in
- * CREATE OR REPLACE and similar commands.  The net effect is that if an
- * extension script uses such a command on a pre-existing free-standing
- * object, the object will be absorbed into the extension.  If the object
- * is already a member of some other extension, the command will fail.
- * This behavior is desirable for cases such as replacing a shell type.
+ * CREATE OR REPLACE and similar commands.  We used to allow the target
+ * object to not already be an extension member, instead silently absorbing
+ * it into the current extension.  However, this was both error-prone
+ * (extensions might accidentally overwrite free-standing objects) and
+ * a security hazard (since the object would retain its previous ownership).
  */
 void
 recordDependencyOnCurrentExtension(const ObjectAddress *object,
@@ -157,6 +158,12 @@ recordDependencyOnCurrentExtension(const ObjectAddress *object,
 		{
 			Oid			oldext;
 
+			/*
+			 * Side note: these catalog lookups are safe only because the
+			 * object is a pre-existing one.  In the not-isReplace case, the
+			 * caller has most likely not yet done a CommandCounterIncrement
+			 * that would make the new object visible.
+			 */
 			oldext = getExtensionOfObject(object->classId, object->objectId);
 			if (OidIsValid(oldext))
 			{
@@ -170,6 +177,13 @@ recordDependencyOnCurrentExtension(const ObjectAddress *object,
 								getObjectDescription(object),
 								get_extension_name(oldext))));
 			}
+			/* It's a free-standing object, so reject */
+			ereport(ERROR,
+					(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+					 errmsg("%s is not a member of extension \"%s\"",
+							getObjectDescription(object),
+							get_extension_name(CurrentExtensionObject)),
+					 errdetail("An extension is not allowed to replace an object that it does not own.")));
 		}
 
 		/* OK, record it as a member of CurrentExtensionObject */
@@ -181,6 +195,49 @@ recordDependencyOnCurrentExtension(const ObjectAddress *object,
 	}
 }
 
+/*
+ * If we are executing a CREATE EXTENSION operation, check that the given
+ * object is a member of the extension, and throw an error if it isn't.
+ * Otherwise, do nothing.
+ *
+ * This must be called whenever a CREATE IF NOT EXISTS operation (for an
+ * object type that can be an extension member) has found that an object of
+ * the desired name already exists.  It is insecure for an extension to use
+ * IF NOT EXISTS except when the conflicting object is already an extension
+ * member; otherwise a hostile user could substitute an object with arbitrary
+ * properties.
+ */
+void
+checkMembershipInCurrentExtension(const ObjectAddress *object)
+{
+	/*
+	 * This is actually the same condition tested in
+	 * recordDependencyOnCurrentExtension; but we want to issue a
+	 * differently-worded error, and anyway it would be pretty confusing to
+	 * call recordDependencyOnCurrentExtension in these circumstances.
+	 */
+
+	/* Only whole objects can be extension members */
+	Assert(object->objectSubId == 0);
+
+	if (creating_extension)
+	{
+		Oid			oldext;
+
+		oldext = getExtensionOfObject(object->classId, object->objectId);
+		/* If already a member of this extension, OK */
+		if (oldext == CurrentExtensionObject)
+			return;
+		/* Else complain */
+		ereport(ERROR,
+				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+				 errmsg("%s is not a member of extension \"%s\"",
+						getObjectDescription(object),
+						get_extension_name(CurrentExtensionObject)),
+				 errdetail("An extension may only use CREATE ... IF NOT EXISTS to skip object creation if the conflicting object is one that it already owns.")));
+	}
+}
+
 /*
  * deleteDependencyRecordsFor -- delete all records with given depender
  * classId/objectId.  Returns the number of records deleted.

src/backend/catalog/pg_operator.c

@@ -874,7 +874,7 @@ makeOperatorDependencies(HeapTuple tuple,
 
 	/* Dependency on extension */
 	if (makeExtensionDep)
-		recordDependencyOnCurrentExtension(&myself, true);
+		recordDependencyOnCurrentExtension(&myself, isUpdate);
 
 	return myself;
 }

src/backend/catalog/pg_type.c

@@ -541,8 +541,11 @@ TypeCreate(Oid newTypeOid,
  * rebuild should be true if this is a pre-existing type.  We will remove
  * existing dependencies and rebuild them from scratch.  This is needed for
  * ALTER TYPE, and also when replacing a shell type.  We don't remove any
- * existing extension dependency, though (hence, if makeExtensionDep is also
- * true and the type belongs to some other extension, an error will occur).
+ * existing extension dependency, though; hence, if makeExtensionDep is also
+ * true and we're in an extension script, an error will occur unless the
+ * type already belongs to the current extension.  That's the behavior we
+ * want when replacing a shell type, which is the only case where both flags
+ * are true.
  */
 void
 GenerateTypeDependencies(HeapTuple typeTuple,

src/backend/commands/createas.c

@@ -242,15 +242,27 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
 	if (stmt->if_not_exists)
 	{
 		Oid			nspid;
+		Oid			oldrelid;
 
-		nspid = RangeVarGetCreationNamespace(stmt->into->rel);
+		nspid = RangeVarGetCreationNamespace(into->rel);
 
-		if (get_relname_relid(stmt->into->rel->relname, nspid))
+		oldrelid = get_relname_relid(into->rel->relname, nspid);
+		if (OidIsValid(oldrelid))
 		{
+			/*
+			 * The relation exists and IF NOT EXISTS has been specified.
+			 *
+			 * If we are in an extension script, insist that the pre-existing
+			 * object be a member of the extension, to avoid security risks.
+			 */
+			ObjectAddressSet(address, RelationRelationId, oldrelid);
+			checkMembershipInCurrentExtension(&address);
+
+			/* OK to skip */
 			ereport(NOTICE,
 					(errcode(ERRCODE_DUPLICATE_TABLE),
 					 errmsg("relation \"%s\" already exists, skipping",
-							stmt->into->rel->relname)));
+							into->rel->relname)));
 			return InvalidObjectAddress;
 		}
 	}

src/backend/commands/foreigncmds.c

@@ -883,13 +883,22 @@ CreateForeignServer(CreateForeignServerStmt *stmt)
 	ownerId = GetUserId();
 
 	/*
-	 * Check that there is no other foreign server by this name. Do nothing if
-	 * IF NOT EXISTS was enforced.
+	 * Check that there is no other foreign server by this name.  If there is
+	 * one, do nothing if IF NOT EXISTS was specified.
 	 */
-	if (GetForeignServerByName(stmt->servername, true) != NULL)
+	srvId = get_foreign_server_oid(stmt->servername, true);
+	if (OidIsValid(srvId))
 	{
 		if (stmt->if_not_exists)
 		{
+			/*
+			 * If we are in an extension script, insist that the pre-existing
+			 * object be a member of the extension, to avoid security risks.
+			 */
+			ObjectAddressSet(myself, ForeignServerRelationId, srvId);
+			checkMembershipInCurrentExtension(&myself);
+
+			/* OK to skip */
 			ereport(NOTICE,
 					(errcode(ERRCODE_DUPLICATE_OBJECT),
 					 errmsg("server \"%s\" already exists, skipping",
@@ -1178,6 +1187,10 @@ CreateUserMapping(CreateUserMappingStmt *stmt)
 	{
 		if (stmt->if_not_exists)
 		{
+			/*
+			 * Since user mappings aren't members of extensions (see comments
+			 * below), no need for checkMembershipInCurrentExtension here.
+			 */
 			ereport(NOTICE,
 					(errcode(ERRCODE_DUPLICATE_OBJECT),
 					 errmsg("user mapping for \"%s\" already exists for server \"%s\", skipping",

src/backend/commands/schemacmds.c

@@ -112,14 +112,25 @@ CreateSchemaCommand(CreateSchemaStmt *stmt, const char *queryString,
 	 * the permissions checks, but since CREATE TABLE IF NOT EXISTS makes its
 	 * creation-permission check first, we do likewise.
 	 */
-	if (stmt->if_not_exists &&
-		SearchSysCacheExists1(NAMESPACENAME, PointerGetDatum(schemaName)))
+	if (stmt->if_not_exists)
 	{
-		ereport(NOTICE,
-				(errcode(ERRCODE_DUPLICATE_SCHEMA),
-				 errmsg("schema \"%s\" already exists, skipping",
-						schemaName)));
-		return InvalidOid;
+		namespaceId = get_namespace_oid(schemaName, true);
+		if (OidIsValid(namespaceId))
+		{
+			/*
+			 * If we are in an extension script, insist that the pre-existing
+			 * object be a member of the extension, to avoid security risks.
+			 */
+			ObjectAddressSet(address, NamespaceRelationId, namespaceId);
+			checkMembershipInCurrentExtension(&address);
+
+			/* OK to skip */
+			ereport(NOTICE,
+					(errcode(ERRCODE_DUPLICATE_SCHEMA),
+					 errmsg("schema \"%s\" already exists, skipping",
+							schemaName)));
+			return InvalidOid;
+		}
 	}
 
 	/*

src/backend/commands/sequence.c

@@ -149,6 +149,14 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq)
 		RangeVarGetAndCheckCreationNamespace(seq->sequence, NoLock, &seqoid);
 		if (OidIsValid(seqoid))
 		{
+			/*
+			 * If we are in an extension script, insist that the pre-existing
+			 * object be a member of the extension, to avoid security risks.
+			 */
+			ObjectAddressSet(address, RelationRelationId, seqoid);
+			checkMembershipInCurrentExtension(&address);
+
+			/* OK to skip */
 			ereport(NOTICE,
 					(errcode(ERRCODE_DUPLICATE_TABLE),
 					 errmsg("relation \"%s\" already exists, skipping",

src/backend/commands/statscmds.c

@@ -176,6 +176,10 @@ CreateStatistics(CreateStatsStmt *stmt)
 	{
 		if (stmt->if_not_exists)
 		{
+			/*
+			 * Since stats objects aren't members of extensions (see comments
+			 * below), no need for checkMembershipInCurrentExtension here.
+			 */
 			ereport(NOTICE,
 					(errcode(ERRCODE_DUPLICATE_OBJECT),
 					 errmsg("statistics object \"%s\" already exists, skipping",

src/backend/commands/view.c

@@ -190,7 +190,7 @@ DefineVirtualRelation(RangeVar *relation, List *tlist, bool replace,
 		CommandCounterIncrement();
 
 		/*
-		 * Finally update the view options.
+		 * Update the view's options.
 		 *
 		 * The new options list replaces the existing options list, even if
 		 * it's empty.
@@ -203,8 +203,22 @@ DefineVirtualRelation(RangeVar *relation, List *tlist, bool replace,
 		/* EventTriggerAlterTableStart called by ProcessUtilitySlow */
 		AlterTableInternal(viewOid, atcmds, true);
 
+		/*
+		 * There is very little to do here to update the view's dependencies.
+		 * Most view-level dependency relationships, such as those on the
+		 * owner, schema, and associated composite type, aren't changing.
+		 * Because we don't allow changing type or collation of an existing
+		 * view column, those dependencies of the existing columns don't
+		 * change either, while the AT_AddColumnToView machinery took care of
+		 * adding such dependencies for new view columns.  The dependencies of
+		 * the view's query could have changed arbitrarily, but that was dealt
+		 * with inside StoreViewQuery.  What remains is only to check that
+		 * view replacement is allowed when we're creating an extension.
+		 */
 		ObjectAddressSet(address, RelationRelationId, viewOid);
 
+		recordDependencyOnCurrentExtension(&address, true);
+
 		/*
 		 * Seems okay, so return the OID of the pre-existing view.
 		 */