On Thu, 9 Jun 2022 at 16:12, Peter Geoghegan <p...@bowt.ie> wrote: > > Presumably there is still significant value in detecting cases where > some user-defined code provably does the wrong thing. Especially by > targeting mistakes that experience has shown are relatively common. > That's what the search_path case seems like to me.
By "relatively common" I think we're talking "nigh universal". Afaics there are no warnings in the docs about worrying about search_path on IMMUTABLE functions. There is for SECURITY DEFINER but I have to admit I wasn't aware myself of all the gotchas described there. For that matter.... the gotchas are a bit .... convoluted.... If you leave out pg_catalog from search_path that's fine but if you leave out pg_temp that's a security disaster. If you put pg_catalog in it better be first or else it might be ok or might be a security issue but when you put pg_temp in it better be last or else it's *definitely* a disaster. $user is in search_path by default and that's fine for SECURITY DEFINER functions but it's a disaster for IMMUTABLE functions... I kind of feel like perhaps all the implicit stuff is unnecessary baroque frills. We should just put pg_temp and pg_catalog into the default postgresql.conf search_path and assume users will keep them there. And I'm not sure why we put pg_temp *first* -- I mean it sort of seems superficially sensible but it doesn't seem like there's any real reason to name your temporary tables the same as your permanent ones so why not just always add it last? I've attached a very WIP patch that implements the checks I'm leaning towards making (as elogs currently). They cause a ton of regression failures so probably we need to think about how to reduce the pain for users upgrading... Perhaps we should automatically fix up the current search patch and attach it to functions where necessary for users instead of just whingeing at them....
From 5cfccb714e7d9fd8f623700701e960abd54af25c Mon Sep 17 00:00:00 2001 From: Greg Stark <st...@mit.edu> 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
