From 5cfccb714e7d9fd8f623700701e960abd54af25c Mon Sep 17 00:00:00 2001 From: Greg Stark Date: Mon, 13 Jun 2022 16:47:49 -0400 Subject: [PATCH] Add checks on search_path for IMMUTABLE and SECURITY DEFINER functions --- src/backend/commands/functioncmds.c | 76 +++++++++++++++++++++++++++++ src/backend/utils/misc/guc.c | 59 +++++++++++++++++++++- src/include/utils/guc.h | 1 + 3 files changed, 135 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c index 00a6d282cf..13d5c668b7 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 + */ +static void +verify_proconfig_value(ArrayType *a, char provolatile, bool prosecdef) { + const char *search_path = NULL; + + if (provolatile != PROVOLATILE_IMMUTABLE && !prosecdef) { + return; + } + + if (a != NULL) { + search_path = GUCArrayLookup(a, "search_path"); + } + + if (!search_path) + { + elog(WARNING, "IMMUTABLE and SECURITY DEFINER functions must have search_path set"); + } + + /* XXX This logic should perhaps be moved to namespace.c XXX */ + + char *rawname; + List *namelist; + ListCell *l; + /* Need a modifiable copy of namespace_search_path string */ + rawname = pstrdup(namespace_search_path); + + /* Parse string into list of identifiers */ + if (!SplitIdentifierString(rawname, ',', &namelist)) + { + /* syntax error in name list */ + /* this should not happen if GUC checked check_search_path */ + elog(ERROR, "invalid list syntax in proconfig"); + } + + bool has_dollar_user = false; + bool pg_catalog_first = true; + bool pg_temp_last = false; + bool is_first = true; + foreach(l, namelist) + { + char *curname = (char *) lfirst(l); + Oid namespaceId; + + /* It's not last unless it's last */ + 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->prosecdef, procForm->provolatile); + /* 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..bcfe8447d8 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,64 @@ GUCArrayAdd(ArrayType *array, const char *name, const char *value) return a; } +const 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; + + 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(s, &name, &value); + if (!name) + { + elog(WARNING, "parselongoption returned null name"); + continue; + } + else if (strcmp(name, search_name)) + { + continue; + } + else if (!value) + { + elog(WARNING, "parselongoption returned null value"); + return NULL; + continue; + } + /* free malloc'd strings immediately to avoid leak upon error */ + valuecopy = pstrdup(value); + free(name); + free(value); + pfree(s); + + 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..24dca3c7e6 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 const char * GUCArrayLookup(ArrayType *array, const char *search_name); extern ArrayType *GUCArrayDelete(ArrayType *array, const char *name); extern ArrayType *GUCArrayReset(ArrayType *array); -- 2.36.1