[CF 5018] v4 - Extension security improvement: Add support for extensions with an owned schema

This branch was automatically generated by a robot using patches from an
email thread registered at:

https://commitfest.postgresql.org/patch/5018

The branch will be overwritten each time a new patch version is posted to
the thread, and also periodically to check for bitrot caused by changes
on the master branch.

Patch(es): https://www.postgresql.org/message-id/CAGECzQS02M6YPDXemo36tShO-ZYObjqnyTJyVttua1PGyN4xRw@mail.gmail.com
Author(s): Jelte Fennema-Nio

Author: Commitfest Bot

doc/src/sgml/extend.sgml

@@ -814,6 +814,40 @@ RETURNS anycompatible AS ...
       </listitem>
      </varlistentry>
 
+     <varlistentry id="extend-extensions-files-owned-schema">
+      <term><varname>owned_schema</varname> (<type>boolean</type>)</term>
+      <listitem>
+       <para>
+        An extension should set <firstterm>owned_schema</firstterm> to
+        <literal>true</literal> in its control file if the extension wants a
+        dedicated schema for its objects. Such a schema should not exist yet at
+        the time of extension creation, and will be created automatically by
+        <literal>CREATE EXTENSION</literal>. The default is
+        <literal>false</literal>, i.e., the extension can be installed into an
+        existing schema.
+       </para>
+       <para>
+        Having a schema owned by the extension can make it much easier to
+        reason about possible <literal>search_path</literal> injection attacks.
+        For instance with an owned schema, it is generally safe to set the
+        <literal>search_path</literal> of a <literal>SECURITY DEFINER</literal>
+        function to the schema of the extension. While without an owned schema
+        it might not be safe to do so, because a malicious user could insert
+        objects in that schema and thus <link
+        linkend="sql-createfunction-security"> cause malicious to be executed
+        as superuser</link>. Similarly, having an owned schema can make it safe
+        by default to execute general-purpose SQL in the extension script,
+        because the search_path now only contains trusted schemas. Without an
+        owned schema it's <link linkend="extend-extensions-security-scripts">
+        recommended to manually change the search_path</link>.
+       </para>
+       <para>
+        Apart from the security considerations, having an owned schema can help
+        prevent naming conflicts between objects of different extensions.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry id="extend-extensions-files-schema">
       <term><varname>schema</varname> (<type>string</type>)</term>
       <listitem>

doc/src/sgml/ref/create_extension.sgml

@@ -104,7 +104,9 @@ CREATE EXTENSION [ IF NOT EXISTS ] <replaceable class="parameter">extension_name
        <para>
         The name of the schema in which to install the extension's
         objects, given that the extension allows its contents to be
-        relocated.  The named schema must already exist.
+        relocated.  The named schema must already exist, unless
+        <literal>owned_schema</literal> is set to <literal>true</literal> in
+        the control file, then the schema must not exist.
         If not specified, and the extension's control file does not specify a
         schema either, the current default object creation schema is used.
        </para>

src/backend/commands/extension.c

@@ -90,6 +90,8 @@ typedef struct ExtensionControlFile
 									 * MODULE_PATHNAME */
 	char	   *comment;		/* comment, if any */
 	char	   *schema;			/* target schema (allowed if !relocatable) */
+	bool		owned_schema;	/* if the schema should be owned by the
+								 * extension */
 	bool		relocatable;	/* is ALTER EXTENSION SET SCHEMA supported? */
 	bool		superuser;		/* must be superuser to install? */
 	bool		trusted;		/* allow becoming superuser on the fly? */
@@ -611,6 +613,14 @@ parse_extension_control_file(ExtensionControlFile *control,
 		{
 			control->schema = pstrdup(item->value);
 		}
+		else if (strcmp(item->name, "owned_schema") == 0)
+		{
+			if (!parse_bool(item->value, &control->owned_schema))
+				ereport(ERROR,
+						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+						 errmsg("parameter \"%s\" requires a Boolean value",
+								item->name)));
+		}
 		else if (strcmp(item->name, "relocatable") == 0)
 		{
 			if (!parse_bool(item->value, &control->relocatable))
@@ -1744,8 +1754,11 @@ CreateExtensionInternal(char *extensionName,
 	 */
 	if (schemaName)
 	{
-		/* If the user is giving us the schema name, it must exist already. */
-		schemaOid = get_namespace_oid(schemaName, false);
+		/*
+		 * If the user is giving us the schema name, it must exist already if
+		 * the extension does not want to own the schema
+		 */
+		schemaOid = get_namespace_oid(schemaName, control->owned_schema);
 	}
 
 	if (control->schema != NULL)
@@ -1767,7 +1780,10 @@ CreateExtensionInternal(char *extensionName,
 
 		/* Always use the schema from control file for current extension. */
 		schemaName = control->schema;
+	}
 
+	if (schemaName)
+	{
 		/* Find or create the schema in case it does not exist. */
 		schemaOid = get_namespace_oid(schemaName, true);
 
@@ -1788,8 +1804,22 @@ CreateExtensionInternal(char *extensionName,
 			 */
 			schemaOid = get_namespace_oid(schemaName, false);
 		}
+		else if (control->owned_schema)
+		{
+			ereport(ERROR,
+					(errcode(ERRCODE_DUPLICATE_SCHEMA),
+					 errmsg("schema \"%s\" already exists",
+							schemaName)));
+		}
+
 	}
-	else if (!OidIsValid(schemaOid))
+	else if (control->owned_schema)
+	{
+		ereport(ERROR,
+				(errcode(ERRCODE_UNDEFINED_SCHEMA),
+				 errmsg("no schema has been selected to create in")));
+	}
+	else
 	{
 		/*
 		 * Neither user nor author of the extension specified schema; use the
@@ -1856,6 +1886,7 @@ CreateExtensionInternal(char *extensionName,
 	 */
 	address = InsertExtensionTuple(control->name, extowner,
 								   schemaOid, control->relocatable,
+								   control->owned_schema,
 								   versionName,
 								   PointerGetDatum(NULL),
 								   PointerGetDatum(NULL),
@@ -2061,7 +2092,8 @@ CreateExtension(ParseState *pstate, CreateExtensionStmt *stmt)
  */
 ObjectAddress
 InsertExtensionTuple(const char *extName, Oid extOwner,
-					 Oid schemaOid, bool relocatable, const char *extVersion,
+					 Oid schemaOid, bool relocatable, bool ownedSchema,
+					 const char *extVersion,
 					 Datum extConfig, Datum extCondition,
 					 List *requiredExtensions)
 {
@@ -2091,6 +2123,7 @@ InsertExtensionTuple(const char *extName, Oid extOwner,
 	values[Anum_pg_extension_extowner - 1] = ObjectIdGetDatum(extOwner);
 	values[Anum_pg_extension_extnamespace - 1] = ObjectIdGetDatum(schemaOid);
 	values[Anum_pg_extension_extrelocatable - 1] = BoolGetDatum(relocatable);
+	values[Anum_pg_extension_extownedschema - 1] = BoolGetDatum(ownedSchema);
 	values[Anum_pg_extension_extversion - 1] = CStringGetTextDatum(extVersion);
 
 	if (extConfig == PointerGetDatum(NULL))
@@ -2135,6 +2168,17 @@ InsertExtensionTuple(const char *extName, Oid extOwner,
 	record_object_address_dependencies(&myself, refobjs, DEPENDENCY_NORMAL);
 	free_object_addresses(refobjs);
 
+	if (ownedSchema)
+	{
+		ObjectAddress schemaAddress = {
+			.classId = NamespaceRelationId,
+			.objectId = schemaOid,
+		};
+
+		recordDependencyOn(&schemaAddress, &myself, DEPENDENCY_EXTENSION);
+	}
+
+
 	/* Post creation hook for new extension */
 	InvokeObjectPostCreateHook(ExtensionRelationId, extensionOid, 0);
 
@@ -3053,11 +3097,10 @@ AlterExtensionNamespace(const char *extensionName, const char *newschema, Oid *o
 	HeapTuple	depTup;
 	ObjectAddresses *objsMoved;
 	ObjectAddress extAddr;
+	bool		ownedSchema;
 
 	extensionOid = get_extension_oid(extensionName, false);
 
-	nspOid = LookupCreationNamespace(newschema);
-
 	/*
 	 * Permission check: must own extension.  Note that we don't bother to
 	 * check ownership of the individual member objects ...
@@ -3066,22 +3109,6 @@ AlterExtensionNamespace(const char *extensionName, const char *newschema, Oid *o
 		aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_EXTENSION,
 					   extensionName);
 
-	/* Permission check: must have creation rights in target namespace */
-	aclresult = object_aclcheck(NamespaceRelationId, nspOid, GetUserId(), ACL_CREATE);
-	if (aclresult != ACLCHECK_OK)
-		aclcheck_error(aclresult, OBJECT_SCHEMA, newschema);
-
-	/*
-	 * If the schema is currently a member of the extension, disallow moving
-	 * the extension into the schema.  That would create a dependency loop.
-	 */
-	if (getExtensionOfObject(NamespaceRelationId, nspOid) == extensionOid)
-		ereport(ERROR,
-				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-				 errmsg("cannot move extension \"%s\" into schema \"%s\" "
-						"because the extension contains the schema",
-						extensionName, newschema)));
-
 	/* Locate the pg_extension tuple */
 	extRel = table_open(ExtensionRelationId, RowExclusiveLock);
 
@@ -3105,14 +3132,38 @@ AlterExtensionNamespace(const char *extensionName, const char *newschema, Oid *o
 
 	systable_endscan(extScan);
 
-	/*
-	 * If the extension is already in the target schema, just silently do
-	 * nothing.
-	 */
-	if (extForm->extnamespace == nspOid)
+	ownedSchema = extForm->extownedschema;
+
+	if (!ownedSchema)
 	{
-		table_close(extRel, RowExclusiveLock);
-		return InvalidObjectAddress;
+		nspOid = LookupCreationNamespace(newschema);
+
+		/* Permission check: must have creation rights in target namespace */
+		aclresult = object_aclcheck(NamespaceRelationId, nspOid, GetUserId(), ACL_CREATE);
+		if (aclresult != ACLCHECK_OK)
+			aclcheck_error(aclresult, OBJECT_SCHEMA, newschema);
+
+		/*
+		 * If the schema is currently a member of the extension, disallow
+		 * moving the extension into the schema.  That would create a
+		 * dependency loop.
+		 */
+		if (getExtensionOfObject(NamespaceRelationId, nspOid) == extensionOid)
+			ereport(ERROR,
+					(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+					 errmsg("cannot move extension \"%s\" into schema \"%s\" "
+							"because the extension contains the schema",
+							extensionName, newschema)));
+
+		/*
+		 * If the extension is already in the target schema, just silently do
+		 * nothing.
+		 */
+		if (extForm->extnamespace == nspOid)
+		{
+			table_close(extRel, RowExclusiveLock);
+			return InvalidObjectAddress;
+		}
 	}
 
 	/* Check extension is supposed to be relocatable */
@@ -3185,6 +3236,13 @@ AlterExtensionNamespace(const char *extensionName, const char *newschema, Oid *o
 			}
 		}
 
+		/*
+		 * We don't actually have to move any objects anything for owned
+		 * schemas, because we simply rename the schema.
+		 */
+		if (ownedSchema)
+			continue;
+
 		/*
 		 * Otherwise, ignore non-membership dependencies.  (Currently, the
 		 * only other case we could see here is a normal dependency from
@@ -3228,18 +3286,26 @@ AlterExtensionNamespace(const char *extensionName, const char *newschema, Oid *o
 
 	relation_close(depRel, AccessShareLock);
 
-	/* Now adjust pg_extension.extnamespace */
-	extForm->extnamespace = nspOid;
+	if (ownedSchema)
+	{
+		RenameSchema(get_namespace_name(oldNspOid), newschema);
+		table_close(extRel, RowExclusiveLock);
+	}
+	else
+	{
+		/* Now adjust pg_extension.extnamespace */
+		extForm->extnamespace = nspOid;
 
-	CatalogTupleUpdate(extRel, &extTup->t_self, extTup);
+		CatalogTupleUpdate(extRel, &extTup->t_self, extTup);
 
-	table_close(extRel, RowExclusiveLock);
+		table_close(extRel, RowExclusiveLock);
 
-	/* update dependency to point to the new schema */
-	if (changeDependencyFor(ExtensionRelationId, extensionOid,
-							NamespaceRelationId, oldNspOid, nspOid) != 1)
-		elog(ERROR, "could not change schema dependency for extension %s",
-			 NameStr(extForm->extname));
+		/* update dependency to point to the new schema */
+		if (changeDependencyFor(ExtensionRelationId, extensionOid,
+								NamespaceRelationId, oldNspOid, nspOid) != 1)
+			elog(ERROR, "could not change schema dependency for extension %s",
+				 NameStr(extForm->extname));
+	}
 
 	InvokeObjectPostAlterHook(ExtensionRelationId, extensionOid, 0);
 

src/backend/utils/adt/pg_upgrade_support.c

@@ -19,6 +19,7 @@
 #include "catalog/pg_subscription_rel.h"
 #include "catalog/pg_type.h"
 #include "commands/extension.h"
+#include "commands/schemacmds.h"
 #include "miscadmin.h"
 #include "replication/logical.h"
 #include "replication/origin.h"
@@ -184,41 +185,45 @@ Datum
 binary_upgrade_create_empty_extension(PG_FUNCTION_ARGS)
 {
 	text	   *extName;
-	text	   *schemaName;
+	char	   *schemaName;
 	bool		relocatable;
+	bool		ownedschema;
 	text	   *extVersion;
 	Datum		extConfig;
 	Datum		extCondition;
 	List	   *requiredExtensions;
+	Oid			schemaOid;
 
 	CHECK_IS_BINARY_UPGRADE;
 
 	/* We must check these things before dereferencing the arguments */
 	if (PG_ARGISNULL(0) ||
 		PG_ARGISNULL(1) ||
 		PG_ARGISNULL(2) ||
-		PG_ARGISNULL(3))
+		PG_ARGISNULL(3) ||
+		PG_ARGISNULL(4))
 		elog(ERROR, "null argument to binary_upgrade_create_empty_extension is not allowed");
 
 	extName = PG_GETARG_TEXT_PP(0);
-	schemaName = PG_GETARG_TEXT_PP(1);
+	schemaName = text_to_cstring(PG_GETARG_TEXT_PP(1));
 	relocatable = PG_GETARG_BOOL(2);
-	extVersion = PG_GETARG_TEXT_PP(3);
+	ownedschema = PG_GETARG_BOOL(3);
+	extVersion = PG_GETARG_TEXT_PP(4);
 
-	if (PG_ARGISNULL(4))
+	if (PG_ARGISNULL(5))
 		extConfig = PointerGetDatum(NULL);
 	else
-		extConfig = PG_GETARG_DATUM(4);
+		extConfig = PG_GETARG_DATUM(5);
 
-	if (PG_ARGISNULL(5))
+	if (PG_ARGISNULL(6))
 		extCondition = PointerGetDatum(NULL);
 	else
-		extCondition = PG_GETARG_DATUM(5);
+		extCondition = PG_GETARG_DATUM(6);
 
 	requiredExtensions = NIL;
-	if (!PG_ARGISNULL(6))
+	if (!PG_ARGISNULL(7))
 	{
-		ArrayType  *textArray = PG_GETARG_ARRAYTYPE_P(6);
+		ArrayType  *textArray = PG_GETARG_ARRAYTYPE_P(7);
 		Datum	   *textDatums;
 		int			ndatums;
 		int			i;
@@ -233,10 +238,28 @@ binary_upgrade_create_empty_extension(PG_FUNCTION_ARGS)
 		}
 	}
 
+	if (ownedschema)
+	{
+		CreateSchemaStmt *csstmt = makeNode(CreateSchemaStmt);
+
+		csstmt->schemaname = schemaName;
+		csstmt->authrole = NULL;	/* will be created by current user */
+		csstmt->schemaElts = NIL;
+		csstmt->if_not_exists = false;
+		schemaOid = CreateSchemaCommand(csstmt, "(generated CREATE SCHEMA command)",
+										-1, -1);
+
+	}
+	else
+	{
+		schemaOid = get_namespace_oid(schemaName, false);
+	}
+
 	InsertExtensionTuple(text_to_cstring(extName),
 						 GetUserId(),
-						 get_namespace_oid(text_to_cstring(schemaName), false),
+						 schemaOid,
 						 relocatable,
+						 ownedschema,
 						 text_to_cstring(extVersion),
 						 extConfig,
 						 extCondition,