Subject: CREATE FUNCTION ... SEARCH { DEFAULT | SYSTEM | SESSION }

From: Jeff Davis <[email-masked]>
Date: Fri, 11 Aug 2023 19:35:22 -0700

The attached patch implements a new SEARCH clause for CREATE FUNCTION.
The SEARCH clause controls the search_path used when executing
functions that were created without a SET clause.

Background:

Controlling search_path is critical for the correctness and security of
functions. Right now, the author of a function without a SET clause has
little ability to control the function's behavior, because even basic
operators like "+" involve search_path. This is a big problem for, e.g.
functions used in expression indexes which are called by any user with
write privileges on the table.

Motivation:

I'd like to (eventually) get to safe-by-default behavior. In other
words, the simplest function declaration should be safe for the most
common use cases.

To get there, we need some way to explicitly specify the less common
cases. Right now there's no way for the function author to indicate
that a function intends to use the session's search path. We also need
an easier way to specify that the user wants a safe search_path ("SET
search_path = pg_catalog, pg_temp" is arcane).

And when we know more about the user's actual intent, then it will be
easier to either form a transition plan to push users into the safer
behavior, or at least warn strongly when the user is doing something
dangerous (i.e. using a function that depends on the session's search
path as part of an expression index).

Today, the only information we have about the user's intent is the
presence or absence of a "SET search_path" clause, which is not a
strong signal.

Proposal:

Add SEARCH { DEFAULT | SYSTEM | SESSION } clause to CREATE/ALTER
function.

  * SEARCH DEFAULT is the same as no SEARCH clause at all, and ends up
stored in the catalog as prosearch='d'.
  * SEARCH SYSTEM means that we switch to the safe search path of
"pg_catalog, pg_temp"  when executing the function. Stored as
prosearch='y'.
  * SEARCH SESSION means that we don't switch the search_path when
executing the function, and it's inherited from the session. Stored as
prosearch='e'.

Regardless of the SEARCH clause, a "SET search_path" clause will
override it. The SEARCH clause only matters when "SET search_path" is
not there.

Additionally provide a GUC, defaulting to false for compatibility, that
can interpret prosearch='d' as if it were prosearch='y'. It could help
provide a transition path. I know there's a strong reluctance to adding
these kinds of GUCs; I can remove it and I think the patch will still
be worthwhile. Perhaps there are alternatives that could help with
migration at pg_dump time instead?

Benefits:

1. The user can be more explicit about their actual intent. Do they
want safety and consistency? Or the flexibility of using the session's
search_path?

2. We can more accurately serve the user's intent. For instance, the
safe search_path of "pg_catalog, pg_temp" is arcane and seems to be
there just because we don't have a way to specify that pg_temp be
excluded entirely. But perhaps in the future we *do* want to exclude
pg_temp entirely. Knowing that the user just wants "SEARCH SYSTEM"
allows us some freedom to do that.

3. Users can be forward-compatible by specifying the functions that
really do need to use the session's search path as SEARCH SESSION, so
that they will never be broken in the future. That gives us a cleaner
path toward making the default behavior safe.

-- 
Jeff Davis
PostgreSQL Contributor Team - AWS

From 0218b472bd1ae8152b8bc8d92de4b7cc13dbbbad Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email-masked]>
Date: Fri, 11 Aug 2023 14:33:36 -0700
Subject: [PATCH] CREATE FUNCTION ... SEARCH { DEFAULT | SYSTEM | SESSION }.

Control the search_path while executing functions created without an
explicit SET clause.

Allows users to be forward compatible by explicitly declaring that a
function depends on the session's search_path, or that it only depends
on the safe search_path of "pg_catalog, pg_temp".

Also adds a GUC that allows users to choose how to treat functions
that have not specified a SEARCH clause or a SET clause. The GUC is
false by default to preserve existing semantics, but eventually it
will default to true for greater safety.
---
 doc/src/sgml/config.sgml                      | 26 +++++++
 doc/src/sgml/ref/alter_function.sgml          | 15 ++++
 doc/src/sgml/ref/create_function.sgml         | 34 +++++++++
 src/backend/catalog/pg_aggregate.c            |  1 +
 src/backend/catalog/pg_proc.c                 | 12 ++++
 src/backend/commands/functioncmds.c           | 44 +++++++++++-
 src/backend/commands/typecmds.c               |  4 ++
 src/backend/optimizer/util/clauses.c          |  1 +
 src/backend/parser/gram.y                     | 15 ++++
 src/backend/utils/fmgr/fmgr.c                 | 24 +++++--
 src/backend/utils/misc/guc_tables.c           | 10 +++
 src/backend/utils/misc/postgresql.conf.sample |  2 +
 src/include/catalog/namespace.h               |  6 ++
 src/include/catalog/pg_proc.h                 | 15 ++++
 src/include/utils/guc.h                       |  1 +
 .../regress/expected/create_function_sql.out  | 69 +++++++++++++++++++
 src/test/regress/sql/create_function_sql.sql  | 35 ++++++++++
 17 files changed, 305 insertions(+), 9 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 11251fa05e..f366cc274c 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -10271,6 +10271,32 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
       </listitem>
      </varlistentry>
 
+     <varlistentry id="guc-safe-function-search-path" xreflabel="safe_function_search_path">
+      <term><varname>safe_function_search_path</varname> (<type>boolean</type>)
+      <indexterm>
+       <primary><varname>safe_function_search_path</varname> configuration parameter</primary>
+      </indexterm>
+      </term>
+      <listitem>
+       <para>
+        If set to <literal>true</literal>, functions declared with
+        <literal>SEARCH DEFAULT</literal> (or no <literal>SEARCH</literal>
+        clause) behave as if <literal>SEARCH SYSTEM</literal> was
+        specified. This is the safest option because functions will have
+        consistent behavior regardless of the session's current setting of
+        <xref linkend="guc-search-path"/>.
+       </para>
+       <para>
+        If set to <literal>false</literal> (the default), functions declared
+        with <literal>SEARCH DEFAULT</literal> (or no
+        <literal>SEARCH</literal> clause) behave as if <literal>SEARCH
+        SESSION</literal> was specified.
+       </para>
+       <para>
+        See <xref linkend="sql-createfunction"/> for more information.
+       </para>
+      </listitem>
+     </varlistentry>
      <varlistentry id="guc-backslash-quote" xreflabel="backslash_quote">
       <term><varname>backslash_quote</varname> (<type>enum</type>)
       <indexterm><primary>strings</primary><secondary>backslash quotes</secondary></indexterm>
diff --git a/doc/src/sgml/ref/alter_function.sgml b/doc/src/sgml/ref/alter_function.sgml
index 8193b17f25..61d601b68f 100644
--- a/doc/src/sgml/ref/alter_function.sgml
+++ b/doc/src/sgml/ref/alter_function.sgml
@@ -37,6 +37,7 @@ ALTER FUNCTION <replaceable>name</replaceable> [ ( [ [ <replaceable class="param
     CALLED ON NULL INPUT | RETURNS NULL ON NULL INPUT | STRICT
     IMMUTABLE | STABLE | VOLATILE
     [ NOT ] LEAKPROOF
+    SEARCH { DEFAULT | SYSTEM | SESSION }
     [ EXTERNAL ] SECURITY INVOKER | [ EXTERNAL ] SECURITY DEFINER
     PARALLEL { UNSAFE | RESTRICTED | SAFE }
     COST <replaceable class="parameter">execution_cost</replaceable>
@@ -198,6 +199,20 @@ ALTER FUNCTION <replaceable>name</replaceable> [ ( [ [ <replaceable class="param
     </listitem>
    </varlistentry>
 
+   <varlistentry>
+    <term><literal>SEARCH DEFAULT</literal></term>
+    <term><literal>SEARCH SYSTEM</literal></term>
+    <term><literal>SEARCH SESSION</literal></term>
+
+    <listitem>
+      <para>
+       Change the <literal>SEARCH</literal> behavior of the function to the
+       specified setting.  See <xref linkend="sql-createfunction"/> for
+       details.
+      </para>
+    </listitem>
+   </varlistentry>
+
    <varlistentry>
     <term><literal><optional> EXTERNAL </optional> SECURITY INVOKER</literal></term>
     <term><literal><optional> EXTERNAL </optional> SECURITY DEFINER</literal></term>
diff --git a/doc/src/sgml/ref/create_function.sgml b/doc/src/sgml/ref/create_function.sgml
index 863d99d1fc..82ca2955f4 100644
--- a/doc/src/sgml/ref/create_function.sgml
+++ b/doc/src/sgml/ref/create_function.sgml
@@ -31,6 +31,7 @@ CREATE [ OR REPLACE ] FUNCTION
     | { IMMUTABLE | STABLE | VOLATILE }
     | [ NOT ] LEAKPROOF
     | { CALLED ON NULL INPUT | RETURNS NULL ON NULL INPUT | STRICT }
+    | SEARCH { DEFAULT | SYSTEM | SESSION }
     | { [ EXTERNAL ] SECURITY INVOKER | [ EXTERNAL ] SECURITY DEFINER }
     | PARALLEL { UNSAFE | RESTRICTED | SAFE }
     | COST <replaceable class="parameter">execution_cost</replaceable>
@@ -402,6 +403,39 @@ CREATE [ OR REPLACE ] FUNCTION
      </listitem>
     </varlistentry>
 
+    <varlistentry>
+     <term><literal>SEARCH DEFAULT</literal></term>
+     <term><literal>SEARCH SYSTEM</literal></term>
+     <term><literal>SEARCH SESSION</literal></term>
+
+     <listitem>
+      <para>
+       These settings control the function's <xref linkend="guc-search-path"/>
+       while executing if not set explicitly with a <literal>SET</literal>
+       clause in the <command>CREATE FUNCTION</command> command.
+      </para>
+      <para>
+       <literal>SEARCH SYSTEM</literal> indicates that the function will use
+       <literal>pg_catalog, pg_temp</literal>. This is the safest option
+       because the function will have consistent behavior regardless of the
+       session's setting.
+      </para>
+      <para>
+       <literal>SEARCH SESSION</literal> indicates that the function will
+       inherit session's current <literal>search_path</literal>
+       setting. Behavior of the function may change when the session's
+       <literal>search_path</literal> changes.
+      </para>
+      <para>
+       <literal>SEARCH DEFAULT</literal> is the same as not specifying any
+       <literal>SEARCH</literal> clause at all. If <xref
+       linkend="guc-safe-function-search-path"/> is set to
+       <literal>true</literal>, then the function will use
+       <literal>pg_catalog, pg_temp</literal>; otherwise it will inherit the
+       session's current <literal>search_path</literal> setting.
+      </para>
+     </listitem>
+    </varlistentry>
    <varlistentry>
     <term><literal><optional>EXTERNAL</optional> SECURITY INVOKER</literal></term>
     <term><literal><optional>EXTERNAL</optional> SECURITY DEFINER</literal></term>
diff --git a/src/backend/catalog/pg_aggregate.c b/src/backend/catalog/pg_aggregate.c
index ebc4454743..e4ec68ec43 100644
--- a/src/backend/catalog/pg_aggregate.c
+++ b/src/backend/catalog/pg_aggregate.c
@@ -624,6 +624,7 @@ AggregateCreate(const char *aggName,
 							 NULL,	/* probin */
 							 NULL,	/* prosqlbody */
 							 PROKIND_AGGREGATE,
+							 PROSEARCH_DEFAULT, /* no SEARCH clause for aggregates */
 							 false, /* security invoker (currently not
 									 * definable for agg) */
 							 false, /* isLeakProof */
diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index b5fd364003..05917fd3df 100644
--- a/src/backend/catalog/pg_proc.c
+++ b/src/backend/catalog/pg_proc.c
@@ -81,6 +81,7 @@ ProcedureCreate(const char *procedureName,
 				const char *probin,
 				Node *prosqlbody,
 				char prokind,
+				char prosearch,
 				bool security_definer,
 				bool isLeakProof,
 				bool isStrict,
@@ -308,6 +309,7 @@ ProcedureCreate(const char *procedureName,
 	values[Anum_pg_proc_provariadic - 1] = ObjectIdGetDatum(variadicType);
 	values[Anum_pg_proc_prosupport - 1] = ObjectIdGetDatum(prosupport);
 	values[Anum_pg_proc_prokind - 1] = CharGetDatum(prokind);
+	values[Anum_pg_proc_prosearch - 1] = CharGetDatum(prosearch);
 	values[Anum_pg_proc_prosecdef - 1] = BoolGetDatum(security_definer);
 	values[Anum_pg_proc_proleakproof - 1] = BoolGetDatum(isLeakProof);
 	values[Anum_pg_proc_proisstrict - 1] = BoolGetDatum(isStrict);
@@ -988,6 +990,16 @@ sql_function_parse_error_callback(void *arg)
 	}
 }
 
+/*
+ * Check whether the search_path must be set while executing this function.
+ */
+bool
+prosearch_is_system(char prosearch)
+{
+	return (prosearch == PROSEARCH_SYSTEM ||
+			(prosearch == PROSEARCH_DEFAULT && safe_function_search_path));
+}
+
 /*
  * Adjust a syntax error occurring inside the function body of a CREATE
  * FUNCTION or DO command.  This can be used by any function validator or
diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index 7ba6a86ebe..7c2dfda01e 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -507,6 +507,7 @@ compute_common_attribute(ParseState *pstate,
 						 DefElem *defel,
 						 DefElem **volatility_item,
 						 DefElem **strict_item,
+						 DefElem **search_item,
 						 DefElem **security_item,
 						 DefElem **leakproof_item,
 						 List **set_items,
@@ -533,6 +534,13 @@ compute_common_attribute(ParseState *pstate,
 
 		*strict_item = defel;
 	}
+	else if (strcmp(defel->defname, "search") == 0)
+	{
+		if (*search_item)
+			errorConflictingDefElem(defel, pstate);
+
+		*search_item = defel;
+	}
 	else if (strcmp(defel->defname, "security") == 0)
 	{
 		if (*security_item)
@@ -603,6 +611,24 @@ procedure_error:
 	return false;
 }
 
+static char
+interpret_func_search(DefElem *defel)
+{
+	char	   *str = strVal(defel->arg);
+
+	if (strcmp(str, "default") == 0)
+		return PROSEARCH_DEFAULT;
+	else if (strcmp(str, "system") == 0)
+		return PROSEARCH_SYSTEM;
+	else if (strcmp(str, "session") == 0)
+		return PROSEARCH_SESSION;
+	else
+	{
+		elog(ERROR, "invalid volatility \"%s\"", str);
+		return 0;				/* keep compiler quiet */
+	}
+}
+
 static char
 interpret_func_volatility(DefElem *defel)
 {
@@ -725,6 +751,7 @@ compute_function_attributes(ParseState *pstate,
 							bool *windowfunc_p,
 							char *volatility_p,
 							bool *strict_p,
+							char *search_p,
 							bool *security_definer,
 							bool *leakproof_p,
 							ArrayType **proconfig,
@@ -740,6 +767,7 @@ compute_function_attributes(ParseState *pstate,
 	DefElem    *windowfunc_item = NULL;
 	DefElem    *volatility_item = NULL;
 	DefElem    *strict_item = NULL;
+	DefElem    *search_item = NULL;
 	DefElem    *security_item = NULL;
 	DefElem    *leakproof_item = NULL;
 	List	   *set_items = NIL;
@@ -786,6 +814,7 @@ compute_function_attributes(ParseState *pstate,
 										  defel,
 										  &volatility_item,
 										  &strict_item,
+										  &search_item,
 										  &security_item,
 										  &leakproof_item,
 										  &set_items,
@@ -814,6 +843,8 @@ compute_function_attributes(ParseState *pstate,
 		*volatility_p = interpret_func_volatility(volatility_item);
 	if (strict_item)
 		*strict_p = boolVal(strict_item->arg);
+	if (search_item)
+		*search_p = interpret_func_search(search_item);
 	if (security_item)
 		*security_definer = boolVal(security_item->arg);
 	if (leakproof_item)
@@ -1042,7 +1073,8 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
 				isStrict,
 				security,
 				isLeakProof;
-	char		volatility;
+	char		prosearch,
+				volatility;
 	ArrayType  *proconfig;
 	float4		procost;
 	float4		prorows;
@@ -1067,6 +1099,7 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
 	language = NULL;
 	isWindowFunc = false;
 	isStrict = false;
+	prosearch = PROSEARCH_DEFAULT;
 	security = false;
 	isLeakProof = false;
 	volatility = PROVOLATILE_VOLATILE;
@@ -1082,8 +1115,8 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
 								stmt->options,
 								&as_clause, &language, &transformDefElem,
 								&isWindowFunc, &volatility,
-								&isStrict, &security, &isLeakProof,
-								&proconfig, &procost, &prorows,
+								&isStrict, &prosearch, &security,
+								&isLeakProof, &proconfig, &procost, &prorows,
 								&prosupport, &parallel);
 
 	if (!language)
@@ -1271,6 +1304,7 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
 						   probin_str,	/* converted to text later */
 						   prosqlbody,
 						   stmt->is_procedure ? PROKIND_PROCEDURE : (isWindowFunc ? PROKIND_WINDOW : PROKIND_FUNCTION),
+						   prosearch,
 						   security,
 						   isLeakProof,
 						   isStrict,
@@ -1355,6 +1389,7 @@ AlterFunction(ParseState *pstate, AlterFunctionStmt *stmt)
 	ListCell   *l;
 	DefElem    *volatility_item = NULL;
 	DefElem    *strict_item = NULL;
+	DefElem    *search_item = NULL;
 	DefElem    *security_def_item = NULL;
 	DefElem    *leakproof_item = NULL;
 	List	   *set_items = NIL;
@@ -1399,6 +1434,7 @@ AlterFunction(ParseState *pstate, AlterFunctionStmt *stmt)
 									 defel,
 									 &volatility_item,
 									 &strict_item,
+									 &search_item,
 									 &security_def_item,
 									 &leakproof_item,
 									 &set_items,
@@ -1413,6 +1449,8 @@ AlterFunction(ParseState *pstate, AlterFunctionStmt *stmt)
 		procForm->provolatile = interpret_func_volatility(volatility_item);
 	if (strict_item)
 		procForm->proisstrict = boolVal(strict_item->arg);
+	if (search_item)
+		procForm->prosearch = interpret_func_search(search_item);
 	if (security_def_item)
 		procForm->prosecdef = boolVal(security_def_item->arg);
 	if (leakproof_item)
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 5e97606793..baa8dd706e 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -1763,6 +1763,7 @@ makeRangeConstructors(const char *name, Oid namespace,
 								 NULL,	/* probin */
 								 NULL,	/* prosqlbody */
 								 PROKIND_FUNCTION,
+								 PROSEARCH_DEFAULT,
 								 false, /* security_definer */
 								 false, /* leakproof */
 								 false, /* isStrict */
@@ -1828,6 +1829,7 @@ makeMultirangeConstructors(const char *name, Oid namespace,
 							 NULL,	/* probin */
 							 NULL,	/* prosqlbody */
 							 PROKIND_FUNCTION,
+							 PROSEARCH_DEFAULT,
 							 false, /* security_definer */
 							 false, /* leakproof */
 							 true,	/* isStrict */
@@ -1872,6 +1874,7 @@ makeMultirangeConstructors(const char *name, Oid namespace,
 							 NULL,	/* probin */
 							 NULL,	/* prosqlbody */
 							 PROKIND_FUNCTION,
+							 PROSEARCH_DEFAULT,
 							 false, /* security_definer */
 							 false, /* leakproof */
 							 true,	/* isStrict */
@@ -1910,6 +1913,7 @@ makeMultirangeConstructors(const char *name, Oid namespace,
 							 NULL,	/* probin */
 							 NULL,	/* prosqlbody */
 							 PROKIND_FUNCTION,
+							 PROSEARCH_DEFAULT,
 							 false, /* security_definer */
 							 false, /* leakproof */
 							 true,	/* isStrict */
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index da258968b8..ad93e0a28f 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -4474,6 +4474,7 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid,
 	 */
 	if (funcform->prolang != SQLlanguageId ||
 		funcform->prokind != PROKIND_FUNCTION ||
+		prosearch_is_system(funcform->prosearch) ||
 		funcform->prosecdef ||
 		funcform->proretset ||
 		funcform->prorettype == RECORDOID ||
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 15ece871a0..e264fa536f 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -8528,6 +8528,21 @@ common_func_opt_item:
 				{
 					$$ = makeDefElem("support", (Node *) $2, @1);
 				}
+			| SEARCH DEFAULT
+				{
+					/* we abuse the normal content of a DefElem here */
+					$$ = makeDefElem("search", (Node *) makeString("default"), @1);
+				}
+			| SEARCH SYSTEM_P
+				{
+					/* we abuse the normal content of a DefElem here */
+					$$ = makeDefElem("search", (Node *) makeString("system"), @1);
+				}
+			| SEARCH SESSION
+				{
+					/* we abuse the normal content of a DefElem here */
+					$$ = makeDefElem("search", (Node *) makeString("session"), @1);
+				}
 			| FunctionSetResetClause
 				{
 					/* we abuse the normal content of a DefElem here */
diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c
index 9dfdf890c5..2dc7224940 100644
--- a/src/backend/utils/fmgr/fmgr.c
+++ b/src/backend/utils/fmgr/fmgr.c
@@ -16,6 +16,7 @@
 #include "postgres.h"
 
 #include "access/detoast.h"
+#include "catalog/namespace.h"
 #include "catalog/pg_language.h"
 #include "catalog/pg_proc.h"
 #include "catalog/pg_type.h"
@@ -110,6 +111,7 @@ fmgr_lookupByName(const char *name)
 	return NULL;
 }
 
+
 /*
  * This routine fills a FmgrInfo struct, given the OID
  * of the function to be called.
@@ -203,6 +205,7 @@ fmgr_info_cxt_security(Oid functionId, FmgrInfo *finfo, MemoryContext mcxt,
 	 */
 	if (!ignore_security &&
 		(procedureStruct->prosecdef ||
+		 prosearch_is_system(procedureStruct->prosearch) ||
 		 !heap_attisnull(procedureTuple, Anum_pg_proc_proconfig, NULL) ||
 		 FmgrHookIsNeeded(functionId)))
 	{
@@ -612,6 +615,7 @@ struct fmgr_security_definer_cache
 {
 	FmgrInfo	flinfo;			/* lookup info for target function */
 	Oid			userid;			/* userid to set, or InvalidOid */
+	char	   *searchPath;		/* from SEARCH clause, if specified */
 	List	   *configNames;	/* GUC names to set, or NIL */
 	List	   *configValues;	/* GUC values to set, or NIL */
 	Datum		arg;			/* passthrough argument for plugin modules */
@@ -630,6 +634,9 @@ struct fmgr_security_definer_cache
 extern Datum
 fmgr_security_definer(PG_FUNCTION_ARGS)
 {
+	GucContext	context = superuser() ? PGC_SUSET : PGC_USERSET;
+	GucSource	source = PGC_S_SESSION;
+	GucAction	action = GUC_ACTION_SAVE;
 	Datum		result;
 	struct fmgr_security_definer_cache *volatile fcache;
 	FmgrInfo   *save_flinfo;
@@ -662,6 +669,9 @@ fmgr_security_definer(PG_FUNCTION_ARGS)
 				 fcinfo->flinfo->fn_oid);
 		procedureStruct = (Form_pg_proc) GETSTRUCT(tuple);
 
+		if (prosearch_is_system(procedureStruct->prosearch))
+			fcache->searchPath = NAMESPACE_SAFE_SEARCH_PATH;
+
 		if (procedureStruct->prosecdef)
 			fcache->userid = procedureStruct->proowner;
 
@@ -687,20 +697,22 @@ fmgr_security_definer(PG_FUNCTION_ARGS)
 
 	/* GetUserIdAndSecContext is cheap enough that no harm in a wasted call */
 	GetUserIdAndSecContext(&save_userid, &save_sec_context);
-	if (fcache->configNames != NIL) /* Need a new GUC nesting level */
+	if (fcache->searchPath != NULL || fcache->configNames != NIL) /* Need a new GUC nesting level */
 		save_nestlevel = NewGUCNestLevel();
 	else
-		save_nestlevel = 0;		/* keep compiler quiet */
+		save_nestlevel = 0;
 
 	if (OidIsValid(fcache->userid))
 		SetUserIdAndSecContext(fcache->userid,
 							   save_sec_context | SECURITY_LOCAL_USERID_CHANGE);
 
+	if (fcache->searchPath != NULL)
+		(void) set_config_option("search_path", fcache->searchPath,
+								 context, source,
+								 action, true, 0, false);
+
 	forboth(lc1, fcache->configNames, lc2, fcache->configValues)
 	{
-		GucContext	context = superuser() ? PGC_SUSET : PGC_USERSET;
-		GucSource	source = PGC_S_SESSION;
-		GucAction	action = GUC_ACTION_SAVE;
 		char	   *name = lfirst(lc1);
 		char	   *value = lfirst(lc2);
 
@@ -749,7 +761,7 @@ fmgr_security_definer(PG_FUNCTION_ARGS)
 
 	fcinfo->flinfo = save_flinfo;
 
-	if (fcache->configNames != NIL)
+	if (save_nestlevel > 0)
 		AtEOXact_GUC(true, save_nestlevel);
 	if (OidIsValid(fcache->userid))
 		SetUserIdAndSecContext(save_userid, save_sec_context);
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index f9dba43b8c..7b32235d52 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -504,6 +504,7 @@ bool		log_btree_build_stats = false;
 char	   *event_source;
 
 bool		row_security;
+bool		safe_function_search_path = false;
 bool		check_function_bodies = true;
 
 /*
@@ -1593,6 +1594,15 @@ struct config_bool ConfigureNamesBool[] =
 		true,
 		NULL, NULL, NULL
 	},
+	{
+		{"safe_function_search_path", PGC_SUSET, CLIENT_CONN_STATEMENT,
+			gettext_noop("Use safe search path for all functions, unless otherwise specified."),
+			NULL
+		},
+		&safe_function_search_path,
+		false,
+		NULL, NULL, NULL
+	},
 	{
 		{"check_function_bodies", PGC_USERSET, CLIENT_CONN_STATEMENT,
 			gettext_noop("Check routine bodies during CREATE FUNCTION and CREATE PROCEDURE."),
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index c768af9a73..accbfb5609 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -688,6 +688,8 @@
 #default_toast_compression = 'pglz'	# 'pglz' or 'lz4'
 #temp_tablespaces = ''			# a list of tablespace names, '' uses
 					# only default tablespace
+#safe_function_search_path = off	# change to safe search_path while
+					# executing functions
 #check_function_bodies = on
 #default_transaction_isolation = 'read committed'
 #default_transaction_read_only = off
diff --git a/src/include/catalog/namespace.h b/src/include/catalog/namespace.h
index 49ef619e4b..5567c1268f 100644
--- a/src/include/catalog/namespace.h
+++ b/src/include/catalog/namespace.h
@@ -76,6 +76,12 @@ typedef enum RVROption
 typedef void (*RangeVarGetRelidCallback) (const RangeVar *relation, Oid relId,
 										  Oid oldRelId, void *callback_arg);
 
+/*
+ * Safe search path for cases where the session search path can't be trusted
+ * (e.g. for SECURITY DEFINER functions).
+ */
+#define NAMESPACE_SAFE_SEARCH_PATH		"pg_catalog, pg_temp"
+
 #define RangeVarGetRelid(relation, lockmode, missing_ok) \
 	RangeVarGetRelidExtended(relation, lockmode, \
 							 (missing_ok) ? RVR_MISSING_OK : 0, NULL, NULL)
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index e7abe0b497..b9627ad493 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -58,6 +58,9 @@ CATALOG(pg_proc,1255,ProcedureRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(81,Proce
 	/* see PROKIND_ categories below */
 	char		prokind BKI_DEFAULT(f);
 
+	/* security definer */
+	char		prosearch BKI_DEFAULT(d);
+
 	/* security definer */
 	bool		prosecdef BKI_DEFAULT(f);
 
@@ -150,6 +153,15 @@ DECLARE_UNIQUE_INDEX(pg_proc_proname_args_nsp_index, 2691, ProcedureNameArgsNspI
 #define PROKIND_WINDOW 'w'
 #define PROKIND_PROCEDURE 'p'
 
+/*
+ * Symbolic values for prosearch column: these determine the search_path under
+ * which a function executes if not specified in a SET clause
+ * (proconfig). PROSEARCH_DEFAULT means that no SEARCH clause was specified.
+ */
+#define PROSEARCH_DEFAULT		'd'
+#define PROSEARCH_SYSTEM		'y'
+#define PROSEARCH_SESSION		'e'
+
 /*
  * Symbolic values for provolatile column: these indicate whether the result
  * of a function is dependent *only* on the values of its explicit arguments,
@@ -197,6 +209,7 @@ extern ObjectAddress ProcedureCreate(const char *procedureName,
 									 const char *probin,
 									 Node *prosqlbody,
 									 char prokind,
+									 char prosearch,
 									 bool security_definer,
 									 bool isLeakProof,
 									 bool isStrict,
@@ -213,6 +226,8 @@ extern ObjectAddress ProcedureCreate(const char *procedureName,
 									 float4 procost,
 									 float4 prorows);
 
+extern bool prosearch_is_system(char prosearch);
+
 extern bool function_parse_error_transpose(const char *prosrc);
 
 extern List *oid_array_to_list(Datum datum);
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index e89083ee0e..1241d095ba 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -249,6 +249,7 @@ extern PGDLLIMPORT bool log_executor_stats;
 extern PGDLLIMPORT bool log_statement_stats;
 extern PGDLLIMPORT bool log_btree_build_stats;
 
+extern PGDLLIMPORT bool safe_function_search_path;
 extern PGDLLIMPORT bool check_function_bodies;
 extern PGDLLIMPORT bool current_role_is_superuser;
 
diff --git a/src/test/regress/expected/create_function_sql.out b/src/test/regress/expected/create_function_sql.out
index 50aca5940f..72bbc02ee9 100644
--- a/src/test/regress/expected/create_function_sql.out
+++ b/src/test/regress/expected/create_function_sql.out
@@ -88,6 +88,75 @@ SELECT proname, provolatile FROM pg_proc
  functest_b_4 | v
 (4 rows)
 
+--
+-- SEARCH DEFAULT | SYSTEM | SESSION
+--
+CREATE FUNCTION show_search_path() RETURNS TEXT
+  LANGUAGE plpgsql AS
+$$
+  BEGIN
+    RETURN current_setting('search_path');
+  END;
+$$;
+SET safe_function_search_path = true;
+ALTER FUNCTION show_search_path() SEARCH DEFAULT;
+SELECT show_search_path();
+  show_search_path   
+---------------------
+ pg_catalog, pg_temp
+(1 row)
+
+ALTER FUNCTION show_search_path() SEARCH SYSTEM;
+SELECT show_search_path();
+  show_search_path   
+---------------------
+ pg_catalog, pg_temp
+(1 row)
+
+ALTER FUNCTION show_search_path() SEARCH SESSION;
+SELECT show_search_path();
+    show_search_path    
+------------------------
+ temp_func_test, public
+(1 row)
+
+RESET safe_function_search_path;
+ALTER FUNCTION show_search_path() SEARCH DEFAULT;
+SELECT show_search_path();
+    show_search_path    
+------------------------
+ temp_func_test, public
+(1 row)
+
+ALTER FUNCTION show_search_path() SEARCH SYSTEM;
+SELECT show_search_path();
+  show_search_path   
+---------------------
+ pg_catalog, pg_temp
+(1 row)
+
+ALTER FUNCTION show_search_path() SEARCH SESSION;
+SELECT show_search_path();
+    show_search_path    
+------------------------
+ temp_func_test, public
+(1 row)
+
+ALTER FUNCTION show_search_path() SEARCH SYSTEM SET search_path = test1;
+SELECT show_search_path();
+ show_search_path 
+------------------
+ test1
+(1 row)
+
+ALTER FUNCTION show_search_path() SEARCH SESSION SET search_path = test1;
+SELECT show_search_path();
+ show_search_path 
+------------------
+ test1
+(1 row)
+
+DROP FUNCTION show_search_path();
 --
 -- SECURITY DEFINER | INVOKER
 --
diff --git a/src/test/regress/sql/create_function_sql.sql b/src/test/regress/sql/create_function_sql.sql
index 89e9af3a49..b0756e1668 100644
--- a/src/test/regress/sql/create_function_sql.sql
+++ b/src/test/regress/sql/create_function_sql.sql
@@ -60,6 +60,41 @@ SELECT proname, provolatile FROM pg_proc
                      'functest_B_3'::regproc,
 		     'functest_B_4'::regproc) ORDER BY proname;
 
+--
+-- SEARCH DEFAULT | SYSTEM | SESSION
+--
+
+CREATE FUNCTION show_search_path() RETURNS TEXT
+  LANGUAGE plpgsql AS
+$$
+  BEGIN
+    RETURN current_setting('search_path');
+  END;
+$$;
+
+SET safe_function_search_path = true;
+ALTER FUNCTION show_search_path() SEARCH DEFAULT;
+SELECT show_search_path();
+ALTER FUNCTION show_search_path() SEARCH SYSTEM;
+SELECT show_search_path();
+ALTER FUNCTION show_search_path() SEARCH SESSION;
+SELECT show_search_path();
+
+RESET safe_function_search_path;
+ALTER FUNCTION show_search_path() SEARCH DEFAULT;
+SELECT show_search_path();
+ALTER FUNCTION show_search_path() SEARCH SYSTEM;
+SELECT show_search_path();
+ALTER FUNCTION show_search_path() SEARCH SESSION;
+SELECT show_search_path();
+
+ALTER FUNCTION show_search_path() SEARCH SYSTEM SET search_path = test1;
+SELECT show_search_path();
+ALTER FUNCTION show_search_path() SEARCH SESSION SET search_path = test1;
+SELECT show_search_path();
+
+DROP FUNCTION show_search_path();
+
 --
 -- SECURITY DEFINER | INVOKER
 --
-- 
2.34.1

---

From: Andres Freund <[email-masked]>
Date: Sat, 12 Aug 2023 11:25:59 -0700

Hi,

> words, the simplest function declaration should be safe for the most

I'm not sure that anything based, directly or indirectly, on search_path
really is a realistic way to get there.

> search_path = pg_catalog, pg_temp" is arcane).

No disagreement with that. Even if I don't yet agree that your proposal is a
convincing path to "easy security for PLs" - just making the search path stuff
less arcane is good.

> path as part of an expression index).

I think that'd be pretty painful from a UX perspective. Having to write
e.g. operators as operator(schema, op) just sucks as an experience. And with
extensions plenty of operators will live outside of pg_catalog, so there is
plenty things that will need qualifying.  And because of things like type
coercion search, which prefers "bettering fitting" coercions over search path
order, you can't just put "less important" things later in search path.

I wonder if we ought to work more on "fossilizing" the result of search path
resolutions at the time functions are created, rather than requiring the user
to do so explicitly.  Most of the problem here comes down to the fact that if
a user creates a function like 'a + b' we'll not resolve the operator, the
potential type coercions etc, when the function is created - we do so when the
function is executed.

We can't just store the oids at the time, because that'd end up very fragile -
tables/functions/... might be dropped and recreated etc and thus change their
oid. But we could change the core PLs to rewrite all the queries (*) so that
they schema qualify absolutely everything, including operators and implicit
type casts.

That way objects referenced by functions can still be replaced, but search
path can't be used to "inject" objects in different schemas. Obviously it
could lead to errors on some schema changes - e.g. changing a column type
might mean that a relevant cast lives in a different place than with the old
type - but I think that'll be quite rare. Perhaps we could offer a ALTER
FUNCTION ... REFRESH REFERENCES; or such?

One obvious downside of such an approach is that it requires some work with
each PL. I'm not sure that's avoidable - and I suspect that most "security
sensitive" functions are written in just a few languages.

(*) Obviously the one thing that doesn't work for is use of EXECUTE in plpgsql
and similar constructs elsewhere. I'm not sure there's much that can be done
to make that safe, but it's worth thinking about more.

Greetings,

Andres Freund

---

From: Jeff Davis <[email-masked]>
Date: Mon, 14 Aug 2023 12:25:30 -0700

> really is a realistic way to get there.

Can you explain a little more? I see what you mean generally, that
search_path is an imprecise thing, and that it leaves room for
ambiguity and mistakes.

But I also think we can do a lot better than we're doing today and
still retain the basic concept of search_path, which is good because
it's deeply integrated into postgres, and it's not clear that we're
going to get away from it any time soon.

> e.g. operators as operator(schema, op) just sucks as an experience.

I'm not suggesting that the user fully-qualify everything; I'm
suggesting that the include a "SET search_path" clause if they depend
on anything other than pg_catalog.

> And with
> plenty things that will need qualifying.

In my proposal, that would still involve a "SET search_path TO
myextension, pg_catalog, pg_temp".

The main reason that's bad is that adding pg_temp at the end is painful
UX -- just something that the user needs to remember to do with little
obvious reason or observable impact; but it has important security
implications. Perhaps we should just not implicitly include pg_temp for
a function's search_path (at least for the case of CREATE FUNCTION ...
SEARCH SYSTEM)?

>   And because of things like type

I understand this introduces some ambiguity, but you just can't include
schemas in the search_path that you don't trust, for similar reasons as
$PATH. If you have a few objects you'd like to access in another user's
schema, fully-qualify them.

> oid.

Robert suggested something along those lines[1]. I won't rule it out,
but I agree that there are quite a few things left to figure out.

>  But we could change the core PLs to rewrite all the queries (*) so
> that

So not quite like "SET search_path FROM CURRENT": you resolve it to a
specific "schemaname.objectname", but stop just short of resolving to a
specific OID?

An interesting compromise, but I'm not sure what the benefit is vs. SET
search_path FROM CURRENT (or some defined search_path).

> FUNCTION ... REFRESH REFERENCES; or such?

Hmm. I feel like that's making things more complicated. I'd find it
more straightforward to use something like Robert's approach of fully
parsing something, and then have the REFRESH command reparse it when
something needs updating. Or perhaps just create all of the dependency
entries more like a view query and then auto-refresh.

> to make that safe, but it's worth thinking about more.

I think it would be really nice to have some better control over the
search_path regardless, because it still helps with cases like this. A
lot of C functions build queries, and I don't think it's reasonable to
constantly worry about the ambiguity of the schema for "=".

Regards,
	Jeff Davis

[1]
https://www.postgresql.org/message-id/CA%2BTgmobd%3DeFRGWHhfG4mG2cA%2BdsVuA4jpBvD8N1NS%3DVc9eHFQg%40mail.gmail.com