From 94fcda77f88b55a4c6e06a2e4367c1bd322c07b4 Mon Sep 17 00:00:00 2001 From: Greg Stark 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