From 94fcda77f88b55a4c6e06a2e4367c1bd322c07b4 Mon Sep 17 00:00:00 2001
From: Greg Stark <stark@mit.edu>
Date: Mon, 13 Jun 2022 16:47:49 -0400
Subject: [PATCH 1/2] Add checks on search_path for IMMUTABLE and SECURITY
 DEFINER functions

---
 src/backend/commands/functioncmds.c | 76 +++++++++++++++++++++++++++++
 src/backend/utils/misc/guc.c        | 57 +++++++++++++++++++++-
 src/include/utils/guc.h             |  1 +
 3 files changed, 133 insertions(+), 1 deletion(-)

diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index 00a6d282cf..9347ed70e2 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -675,6 +675,78 @@ update_proconfig_value(ArrayType *a, List *set_items)
 	return a;
 }
 
+/* We only enforce constraints on IMMUTABLE functions (because they can be
+ * used in indexes and search_path hacking can corrupt indexes for everyone)
+ * or SECURITY DEFINER functions (for obvious reasons). For these functions we
+ * enforce that the proconfig GUC settings should include search_path and that
+ * that search path should follow the recommendations listed at
+ * https://www.postgresql.org/docs/current/sql-createfunction.html#SQL-CREATEFUNCTION-SECURITY
+ */ 
+
+/* XXX This logic should perhaps be moved to namespace.c XXX */
+#include "utils/varlena.h"
+
+static void
+verify_proconfig_value(ArrayType *a, char provolatile, bool prosecdef) {
+	char *proconfig_search_path = NULL;
+	List	   *namelist;
+	ListCell   *l;
+	bool has_dollar_user = false;
+	bool pg_catalog_first = true;
+	bool pg_temp_last = false;
+	bool is_first = true;
+	
+	if (provolatile != PROVOLATILE_IMMUTABLE && !prosecdef) {
+		return;
+	}
+
+	if (a != NULL) {
+		proconfig_search_path = GUCArrayLookup(a, "search_path");
+	}
+
+	if (!proconfig_search_path)
+	{
+		elog(WARNING, "IMMUTABLE and SECURITY DEFINER functions must have search_path set");
+		return;
+	}
+
+	/* Parse string into list of identifiers
+	 * GUCArrayLookup returns a pstrdup'd string so this is safe
+	 */
+	if (!SplitIdentifierString(proconfig_search_path, ',', &namelist))
+	{
+		/* syntax error in name list */
+		elog(ERROR, "invalid list syntax in search_path setting on function");
+	}
+
+	foreach(l, namelist)
+	{
+		char	   *curname = (char *) lfirst(l);
+
+		/* It's not last unless it's last... If it's missing pg_temp is
+		 * implicitly added as first */
+		pg_temp_last = false;
+
+		if (strcmp(curname, "$user") == 0)
+			has_dollar_user = true;
+		if (strcmp(curname, "pg_catalog") == 0 && !is_first)
+			pg_catalog_first = false;
+		if (strcmp(curname,"pg_temp") == 0)
+			pg_temp_last = true;
+		
+		is_first = false;
+	}
+
+	if (has_dollar_user)
+		elog(WARNING, "IMMUTABLE or SECURITY DEFINER functions should not have a search path including $user");
+
+	if (!pg_catalog_first)
+		elog(WARNING, "IMMUTABLE or SECURITY DEFINER functions should have pg_catalog first in search_path (or omit it implicitly placing it first)");
+
+	if (!pg_temp_last)
+		elog(WARNING, "IMMUTABLE or SECURITY DEFINER functions should explicitly place pg_temp last or else it's implicitly first");
+}
+
 static Oid
 interpret_func_support(DefElem *defel)
 {
@@ -1161,6 +1233,8 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
 		}
 	}
 
+	verify_proconfig_value(proconfig, volatility, security);
+
 	/*
 	 * Convert remaining parameters of CREATE to form wanted by
 	 * ProcedureCreate.
@@ -1490,6 +1564,8 @@ AlterFunction(ParseState *pstate, AlterFunctionStmt *stmt)
 		/* update according to each SET or RESET item, left to right */
 		a = update_proconfig_value(a, set_items);
 
+		verify_proconfig_value(a, procForm->provolatile, procForm->prosecdef);
+
 		/* update the tuple */
 		memset(repl_repl, false, sizeof(repl_repl));
 		repl_repl[Anum_pg_proc_proconfig - 1] = true;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index a7cc49898b..55eed02851 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -11435,7 +11435,6 @@ ProcessGUCArray(ArrayType *array,
 	}
 }
 
-
 /*
  * Add an entry to an option array.  The array parameter may be NULL
  * to indicate the current table entry is NULL.
@@ -11514,6 +11513,62 @@ GUCArrayAdd(ArrayType *array, const char *name, const char *value)
 	return a;
 }
 
+/*
+ * Return palloced string with the value for a GUC in a GUCArray (i.e. array
+ * of foo=bar text datums) with name search_name. Returns NULL if the value of
+ * the GUC can not be parsed using ParseLongOption or if it's absent from the
+ * array.
+ */
+char *
+GUCArrayLookup(ArrayType *array, const char *search_name)
+{
+	int			i;
+
+	Assert(array != NULL);
+	Assert(ARR_ELEMTYPE(array) == TEXTOID);
+	Assert(ARR_NDIM(array) == 1);
+	Assert(ARR_LBOUND(array)[0] == 1);
+
+	for (i = 1; i <= ARR_DIMS(array)[0]; i++)
+	{
+		Datum		d;
+		bool		isnull;
+		char	   *s;
+		char	   *name;
+		char	   *value;
+		char	   *valuecopy = NULL;
+		bool        done = false;
+
+		d = array_ref(array, 1, &i,
+					  -1 /* varlenarray */ ,
+					  -1 /* TEXT's typlen */ ,
+					  false /* TEXT's typbyval */ ,
+					  TYPALIGN_INT /* TEXT's typalign */ ,
+					  &isnull);
+
+		if (isnull)
+			continue;
+
+		s = TextDatumGetCString(d);
+
+		/* ParseLongOption returns malloced storage */
+		ParseLongOption(s, &name, &value);
+		done = (strcmp(name, search_name) == 0);
+		
+		if (done && value) {
+			valuecopy = pstrdup(value);
+		}
+		if (name)
+			free(name);
+		if (value)
+			free(value);
+
+		if (done)
+			return valuecopy;
+	}
+	return NULL;
+}
+
 
 /*
  * Delete an entry from an option array.  The array parameter may be NULL
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index d5976ecbfb..24aa13f9e6 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -404,6 +404,7 @@ extern char *ExtractSetVariableArgs(VariableSetStmt *stmt);
 extern void ProcessGUCArray(ArrayType *array,
 							GucContext context, GucSource source, GucAction action);
 extern ArrayType *GUCArrayAdd(ArrayType *array, const char *name, const char *value);
+extern char * GUCArrayLookup(ArrayType *array, const char *search_name);
 extern ArrayType *GUCArrayDelete(ArrayType *array, const char *name);
 extern ArrayType *GUCArrayReset(ArrayType *array);
 
-- 
2.36.1

