Subject: Re: Fix search_path for all maintenance commands

From: Jeff Davis <[email-masked]>
Date: Thu, 06 Jul 2023 18:39:27 -0700

On Fri, 2023-05-26 at 16:21 -0700, Jeff Davis wrote:
> Maintenance commands (ANALYZE, CLUSTER, REFRESH MATERIALIZED VIEW,
> REINDEX, and VACUUM) currently run as the table owner, and as a
> SECURITY_RESTRICTED_OPERATION.
> 
> I propose that we also fix the search_path to "pg_catalog, pg_temp"
> when running maintenance commands

New patch attached.

We need this patch for several reasons:

* If you have a functional index, and the function depends on the
search_path, then it's easy to corrupt your index if you (or a
superuser) run a REINDE/CLUSTER with the wrong search_path.

* The MAINTAIN privilege needs a safe search_path, and was reverted
from 16 because the search_path in 16 is not restricted.

* In general, it's a good idea for things like functional indexes and
materialized views to be independent of the search_path.

* The search_path is already restricted in some other contexts, like
logical replication and autoanalyze.

Others have raised some concerns though:

* It might break for users who have a functional index where the
function implicitly depends on a search_path containing a namespace
other than pg_catalog. My opinion is that such functional indexes are
conceptually broken and we need to desupport them, and there will be
some breakage, but I'm open to suggestion about how we minimize that (a
compatibility GUC or something?).

* The fix might not go far enough or might be in the wrong place. I'm
open to suggestion here, too. Maybe we can make it part of the general
function call mechanism, and can be overridden by explicitly setting
the function search path? Or maybe we need new syntax where the
function can acquire the search path from the session explicitly, but
uses a safe search path by default?

-- 
Jeff Davis
PostgreSQL Contributor Team - AWS

From 46dbbf86e927e1872004c37f651e6a5b5c62bdd0 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email-masked]>
Date: Thu, 6 Jul 2023 13:06:22 -0700
Subject: [PATCH v5] Restrict search_path when performing maintenance.

When executing maintenance operations (ANALYZE, CLUSTER, REFRESH
MATERIALIZED VIEW, REINDEX, or VACUUM), set search_path to
'pg_catalog, pg_temp'.

Functions that are used for functional indexes, in index expressions,
or in materialized views and depend on a different search path should
be declared with CREATE FUNCTION ... SET search_path='...'.

This change was previously committed as 05e1737351, but was not
acceptable for 16 and reverted.

Discussion: https://postgr.es/m/CA%2BTgmoZVCHERUkXhAMT2Er-sKBc5C6_iX%2BTpxxivBevDHzq2TQ%40mail.gmail.com
Discussion: https://postgr.es/m/e44327179e5c9015c8dda67351c04da552066017.camel%40j-davis.com
---
 contrib/amcheck/verify_nbtree.c                      |  2 ++
 src/backend/access/brin/brin.c                       |  2 ++
 src/backend/catalog/index.c                          |  8 ++++++++
 src/backend/commands/analyze.c                       |  2 ++
 src/backend/commands/cluster.c                       |  2 ++
 src/backend/commands/indexcmds.c                     |  6 ++++++
 src/backend/commands/matview.c                       |  2 ++
 src/backend/commands/vacuum.c                        |  2 ++
 src/bin/scripts/t/100_vacuumdb.pl                    |  4 ----
 src/include/utils/guc.h                              |  6 ++++++
 .../test_oat_hooks/expected/test_oat_hooks.out       |  4 ++++
 src/test/regress/expected/privileges.out             | 12 ++++++------
 src/test/regress/expected/vacuum.out                 |  2 +-
 src/test/regress/sql/privileges.sql                  |  8 ++++----
 src/test/regress/sql/vacuum.sql                      |  2 +-
 15 files changed, 48 insertions(+), 16 deletions(-)

diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index 94a9759322..35035967f9 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -281,6 +281,8 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed,
 		SetUserIdAndSecContext(heaprel->rd_rel->relowner,
 							   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 		save_nestlevel = NewGUCNestLevel();
+		SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+						PGC_S_SESSION);
 	}
 	else
 	{
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 3c6a956eaa..11e78cb62c 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -1066,6 +1066,8 @@ brin_summarize_range(PG_FUNCTION_ARGS)
 		SetUserIdAndSecContext(heapRel->rd_rel->relowner,
 							   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 		save_nestlevel = NewGUCNestLevel();
+		SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+						PGC_S_SESSION);
 	}
 	else
 	{
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 67b743e251..64127a894f 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1476,6 +1476,8 @@ index_concurrently_build(Oid heapRelationId,
 	SetUserIdAndSecContext(heapRel->rd_rel->relowner,
 						   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 	save_nestlevel = NewGUCNestLevel();
+	SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+					PGC_S_SESSION);
 
 	indexRelation = index_open(indexRelationId, RowExclusiveLock);
 
@@ -3007,6 +3009,8 @@ index_build(Relation heapRelation,
 	SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
 						   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 	save_nestlevel = NewGUCNestLevel();
+	SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+					PGC_S_SESSION);
 
 	/* Set up initial progress report status */
 	{
@@ -3343,6 +3347,8 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
 	SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
 						   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 	save_nestlevel = NewGUCNestLevel();
+	SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+					PGC_S_SESSION);
 
 	indexRelation = index_open(indexId, RowExclusiveLock);
 
@@ -3603,6 +3609,8 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
 	SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
 						   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 	save_nestlevel = NewGUCNestLevel();
+	SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+					PGC_S_SESSION);
 
 	if (progress)
 	{
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 9c8413eef2..c71051eaf2 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -348,6 +348,8 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
 	SetUserIdAndSecContext(onerel->rd_rel->relowner,
 						   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 	save_nestlevel = NewGUCNestLevel();
+	SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+					PGC_S_SESSION);
 
 	/* measure elapsed time iff autovacuum logging requires it */
 	if (IsAutoVacuumWorkerProcess() && params->log_min_duration >= 0)
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 03b24ab90f..393c732315 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -355,6 +355,8 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
 	SetUserIdAndSecContext(OldHeap->rd_rel->relowner,
 						   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 	save_nestlevel = NewGUCNestLevel();
+	SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+					PGC_S_SESSION);
 
 	/*
 	 * Since we may open a new transaction for each relation, we have to check
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 403f5fc143..8f182c1493 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -575,6 +575,8 @@ DefineIndex(Oid relationId,
 	int			root_save_nestlevel;
 
 	root_save_nestlevel = NewGUCNestLevel();
+	SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+					PGC_S_SESSION);
 
 	/*
 	 * Some callers need us to run with an empty default_tablespace; this is a
@@ -1300,6 +1302,8 @@ DefineIndex(Oid relationId,
 				SetUserIdAndSecContext(childrel->rd_rel->relowner,
 									   child_save_sec_context | SECURITY_RESTRICTED_OPERATION);
 				child_save_nestlevel = NewGUCNestLevel();
+				SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+								PGC_S_SESSION);
 
 				/*
 				 * Don't try to create indexes on foreign tables, though. Skip
@@ -3767,6 +3771,8 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 		SetUserIdAndSecContext(heapRel->rd_rel->relowner,
 							   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 		save_nestlevel = NewGUCNestLevel();
+		SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+						PGC_S_SESSION);
 
 		/* determine safety of this index for set_indexsafe_procflags */
 		idx->safe = (indexRel->rd_indexprs == NIL &&
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index f9a3bdfc3a..9114a7924c 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -179,6 +179,8 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
 	SetUserIdAndSecContext(relowner,
 						   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 	save_nestlevel = NewGUCNestLevel();
+	SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+					PGC_S_SESSION);
 
 	/* Make sure it is a materialized view. */
 	if (matviewRel->rd_rel->relkind != RELKIND_MATVIEW)
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 7fe6a54c06..6dd0f1e77a 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -2171,6 +2171,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 	SetUserIdAndSecContext(rel->rd_rel->relowner,
 						   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 	save_nestlevel = NewGUCNestLevel();
+	SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+					PGC_S_SESSION);
 
 	/*
 	 * If PROCESS_MAIN is set (the default), it's time to vacuum the main
diff --git a/src/bin/scripts/t/100_vacuumdb.pl b/src/bin/scripts/t/100_vacuumdb.pl
index eccfcc54a1..f91b5127a8 100644
--- a/src/bin/scripts/t/100_vacuumdb.pl
+++ b/src/bin/scripts/t/100_vacuumdb.pl
@@ -109,15 +109,11 @@ $node->safe_psql(
   CREATE FUNCTION f1(int) RETURNS int LANGUAGE SQL AS 'SELECT f0($1)';
   CREATE TABLE funcidx (x int);
   INSERT INTO funcidx VALUES (0),(1),(2),(3);
-  CREATE INDEX i0 ON funcidx ((f1(x)));
   CREATE SCHEMA "Foo";
   CREATE TABLE "Foo".bar(id int);
 |);
 $node->command_ok([qw|vacuumdb -Z --table="need""q(uot"(")x") postgres|],
 	'column list');
-$node->command_fails(
-	[qw|vacuumdb -Zt funcidx postgres|],
-	'unqualified name via functional index');
 
 $node->command_fails(
 	[ 'vacuumdb', '--analyze', '--table', 'vactable(c)', 'postgres' ],
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index d5253c7ed2..edd82a551b 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -201,6 +201,12 @@ typedef enum
 
 #define GUC_QUALIFIER_SEPARATOR '.'
 
+/*
+ * Safe search path when executing code as the table owner, such as during
+ * maintenance operations.
+ */
+#define GUC_SAFE_SEARCH_PATH "pg_catalog, pg_temp"
+
 /*
  * Bit values in "flags" of a GUC variable.  Note that these don't appear
  * on disk, so we can reassign their values freely.
diff --git a/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out b/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out
index f80373aecc..effdc49145 100644
--- a/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out
+++ b/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out
@@ -89,11 +89,15 @@ NOTICE:  in object access: superuser finished create (subId=0x0) [internal]
 NOTICE:  in process utility: superuser finished CREATE TABLE
 CREATE INDEX regress_test_table_t_idx ON regress_test_table (t);
 NOTICE:  in process utility: superuser attempting CREATE INDEX
+NOTICE:  in object access: superuser attempting namespace search (subId=0x0) [no report on violation, allowed]
+NOTICE:  in object access: superuser finished namespace search (subId=0x0) [no report on violation, allowed]
 NOTICE:  in object access: superuser attempting create (subId=0x0) [explicit]
 NOTICE:  in object access: superuser finished create (subId=0x0) [explicit]
 NOTICE:  in process utility: superuser finished CREATE INDEX
 GRANT SELECT ON Table regress_test_table TO public;
 NOTICE:  in process utility: superuser attempting GRANT
+NOTICE:  in object access: superuser attempting namespace search (subId=0x0) [no report on violation, allowed]
+NOTICE:  in object access: superuser finished namespace search (subId=0x0) [no report on violation, allowed]
 NOTICE:  in process utility: superuser finished GRANT
 CREATE FUNCTION regress_test_func (t text) RETURNS text AS $$
 	SELECT $1;
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 3e4dfcc2ec..6457da9531 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -1769,7 +1769,7 @@ SET SESSION AUTHORIZATION regress_sro_user;
 CREATE FUNCTION unwanted_grant() RETURNS void LANGUAGE sql AS
 	'GRANT regress_priv_group2 TO regress_sro_user';
 CREATE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS
-	'DECLARE c CURSOR WITH HOLD FOR SELECT unwanted_grant(); SELECT true';
+	'DECLARE c CURSOR WITH HOLD FOR SELECT public.unwanted_grant(); SELECT true';
 -- REFRESH of this MV will queue a GRANT at end of transaction
 CREATE MATERIALIZED VIEW sro_mv AS SELECT mv_action() WITH NO DATA;
 REFRESH MATERIALIZED VIEW sro_mv;
@@ -1783,12 +1783,12 @@ SET SESSION AUTHORIZATION regress_sro_user;
 -- INSERT to this table will queue a GRANT at end of transaction
 CREATE TABLE sro_trojan_table ();
 CREATE FUNCTION sro_trojan() RETURNS trigger LANGUAGE plpgsql AS
-	'BEGIN PERFORM unwanted_grant(); RETURN NULL; END';
+	'BEGIN PERFORM public.unwanted_grant(); RETURN NULL; END';
 CREATE CONSTRAINT TRIGGER t AFTER INSERT ON sro_trojan_table
     INITIALLY DEFERRED FOR EACH ROW EXECUTE PROCEDURE sro_trojan();
 -- Now, REFRESH will issue such an INSERT, queueing the GRANT
 CREATE OR REPLACE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS
-	'INSERT INTO sro_trojan_table DEFAULT VALUES; SELECT true';
+	'INSERT INTO public.sro_trojan_table DEFAULT VALUES; SELECT true';
 REFRESH MATERIALIZED VIEW sro_mv;
 ERROR:  cannot fire deferred trigger within security-restricted operation
 CONTEXT:  SQL function "mv_action" statement 1
@@ -1800,15 +1800,15 @@ BEGIN; SET CONSTRAINTS ALL IMMEDIATE; REFRESH MATERIALIZED VIEW sro_mv; COMMIT;
 ERROR:  permission denied to grant role "regress_priv_group2"
 DETAIL:  Only roles with the ADMIN option on role "regress_priv_group2" may grant this role.
 CONTEXT:  SQL function "unwanted_grant" statement 1
-SQL statement "SELECT unwanted_grant()"
-PL/pgSQL function sro_trojan() line 1 at PERFORM
+SQL statement "SELECT public.unwanted_grant()"
+PL/pgSQL function public.sro_trojan() line 1 at PERFORM
 SQL function "mv_action" statement 1
 -- REFRESH MATERIALIZED VIEW CONCURRENTLY use of eval_const_expressions()
 SET SESSION AUTHORIZATION regress_sro_user;
 CREATE FUNCTION unwanted_grant_nofail(int) RETURNS int
 	IMMUTABLE LANGUAGE plpgsql AS $$
 BEGIN
-	PERFORM unwanted_grant();
+	PERFORM public.unwanted_grant();
 	RAISE WARNING 'owned';
 	RETURN 1;
 EXCEPTION WHEN OTHERS THEN
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index 4def90b805..330fcd884c 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -64,7 +64,7 @@ CLUSTER vaccluster;
 CREATE FUNCTION do_analyze() RETURNS VOID VOLATILE LANGUAGE SQL
 	AS 'ANALYZE pg_am';
 CREATE FUNCTION wrap_do_analyze(c INT) RETURNS INT IMMUTABLE LANGUAGE SQL
-	AS 'SELECT $1 FROM do_analyze()';
+	AS 'SELECT $1 FROM public.do_analyze()';
 CREATE INDEX ON vaccluster(wrap_do_analyze(i));
 INSERT INTO vaccluster VALUES (1), (2);
 ANALYZE vaccluster;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 134809e8cc..e961748d79 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -1177,7 +1177,7 @@ SET SESSION AUTHORIZATION regress_sro_user;
 CREATE FUNCTION unwanted_grant() RETURNS void LANGUAGE sql AS
 	'GRANT regress_priv_group2 TO regress_sro_user';
 CREATE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS
-	'DECLARE c CURSOR WITH HOLD FOR SELECT unwanted_grant(); SELECT true';
+	'DECLARE c CURSOR WITH HOLD FOR SELECT public.unwanted_grant(); SELECT true';
 -- REFRESH of this MV will queue a GRANT at end of transaction
 CREATE MATERIALIZED VIEW sro_mv AS SELECT mv_action() WITH NO DATA;
 REFRESH MATERIALIZED VIEW sro_mv;
@@ -1188,12 +1188,12 @@ SET SESSION AUTHORIZATION regress_sro_user;
 -- INSERT to this table will queue a GRANT at end of transaction
 CREATE TABLE sro_trojan_table ();
 CREATE FUNCTION sro_trojan() RETURNS trigger LANGUAGE plpgsql AS
-	'BEGIN PERFORM unwanted_grant(); RETURN NULL; END';
+	'BEGIN PERFORM public.unwanted_grant(); RETURN NULL; END';
 CREATE CONSTRAINT TRIGGER t AFTER INSERT ON sro_trojan_table
     INITIALLY DEFERRED FOR EACH ROW EXECUTE PROCEDURE sro_trojan();
 -- Now, REFRESH will issue such an INSERT, queueing the GRANT
 CREATE OR REPLACE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS
-	'INSERT INTO sro_trojan_table DEFAULT VALUES; SELECT true';
+	'INSERT INTO public.sro_trojan_table DEFAULT VALUES; SELECT true';
 REFRESH MATERIALIZED VIEW sro_mv;
 \c -
 REFRESH MATERIALIZED VIEW sro_mv;
@@ -1204,7 +1204,7 @@ SET SESSION AUTHORIZATION regress_sro_user;
 CREATE FUNCTION unwanted_grant_nofail(int) RETURNS int
 	IMMUTABLE LANGUAGE plpgsql AS $$
 BEGIN
-	PERFORM unwanted_grant();
+	PERFORM public.unwanted_grant();
 	RAISE WARNING 'owned';
 	RETURN 1;
 EXCEPTION WHEN OTHERS THEN
diff --git a/src/test/regress/sql/vacuum.sql b/src/test/regress/sql/vacuum.sql
index 51d7b1fecc..0b63ef8dc6 100644
--- a/src/test/regress/sql/vacuum.sql
+++ b/src/test/regress/sql/vacuum.sql
@@ -49,7 +49,7 @@ CLUSTER vaccluster;
 CREATE FUNCTION do_analyze() RETURNS VOID VOLATILE LANGUAGE SQL
 	AS 'ANALYZE pg_am';
 CREATE FUNCTION wrap_do_analyze(c INT) RETURNS INT IMMUTABLE LANGUAGE SQL
-	AS 'SELECT $1 FROM do_analyze()';
+	AS 'SELECT $1 FROM public.do_analyze()';
 CREATE INDEX ON vaccluster(wrap_do_analyze(i));
 INSERT INTO vaccluster VALUES (1), (2);
 ANALYZE vaccluster;
-- 
2.34.1

---

From: Isaac Morland <[email-masked]>
Date: Thu, 6 Jul 2023 23:22:13 -0400

On Thu, 6 Jul 2023 at 21:39, Jeff Davis <[email-masked]> wrote:

I apologize in advance if anything I’ve written below is either too obvious
or too crazy or misinformed to belong here. I hope I have something to say
that is on point, but feel unsure what makes sense to say.

> compatibility GUC or something?).

I agree this is OK. If somebody has an index whole meaning depends on the
search_path, then the best that can be said is that their database hasn't
been corrupted yet. At the same time, I can see that somebody would get
upset if they couldn't upgrade their database because of this. Maybe
pg_upgrade could apply "SET search_path TO pg_catalog, pg_temp" to any
function used in a functional index that doesn't have a search_path setting
of its own? (BEGIN ATOMIC functions count, if I understand correctly, as
having a search_path setting, because the lookups happen at definition time)

Now I'm doing more reading and I'm worried about SET TIME ZONE (or more
precisely, its absence) and maybe some other ones.

> uses a safe search path by default?

Change it so by default each function gets handled as if "SET search_path
FROM CURRENT" was applied to it? That's what I do for all my functions
(maybe hurting performance?). Expand on my pg_upgrade idea above by
applying it to all functions?

I feel that this may tie into other behaviour issues where to me it is
obvious that the expected behaviour should be different from the actual
behaviour. If a view calls a function, shouldn't it be called in the
context of the view's definer/owner? It's weird that I can write a view
that filters a table for users of the view, but as soon as the view calls
functions they run in the security context of the user of the view. Are
views security definers or not? Similar comment for triggers. Also as far
as I can tell there is no way for a security definer function to determine
who (which user) invoked it. So I can grant/deny access to run a particular
function using permissions, but I can't have the supposed security definer
define security for different callers.

Is the fundamental problem that we now find ourselves wanting to do things
that require different defaults to work smoothly? On some level I suspect
we want lexical scoping, which is what most of us have in our programming
languages, in the database; but the database has many elements of dynamic
scoping, and changing that is both a compatibility break and requires
significant changes in the way the database is designed.

---

From: Jeff Davis <[email-masked]>
Date: Fri, 07 Jul 2023 08:42:01 -0700

Hi,

On Thu, 2023-07-06 at 23:22 -0400, Isaac Morland wrote:
> Maybe pg_upgrade could apply "SET search_path TO pg_catalog, pg_temp"
> to any function used in a functional index that doesn't have a
> search_path setting of its own?

I don't think we want to go down the road of trying to solve this at
upgrade time.

> Now I'm doing more reading and I'm worried about SET TIME ZONE (or
> more precisely, its absence) and maybe some other ones.

That's a good point that it's not limited to search_path, but
search_path is by far the biggest problem.

For one thing, functions affected by TimeZone or other GUCs are
typically marked STABLE, and can't be used in index expressions. Also,
search_path affects a lot more functions.
 
> Change it so by default each function gets handled as if "SET
> search_path FROM CURRENT" was applied to it?

Yes, that's one idea, along with some syntax to get the old behavior
(inherit search_path at runtime) if you want.

It feels weird to make search_path too special in the syntax though. If
we want a general solution, we could do something like:

  CREATE FUNCTION ...
    [DEPENDS ON CONFIGURATION {NONE|{some_guc}[, ...]}]
    [CONFIGURATION IS {STATIC|DYNAMIC}]

where STATIC means "all of the GUC dependencies are SET FROM CURRENT
unless specified otherwise" and DYNAMIC means "all of the GUC
dependencies come from the session at runtime unless specified
otherwise".

The default would be "DEPENDS CONFIGURATION search_path CONFIGURATION
IS STATIC".

That would make search_path special only because, by default, every
function would depend on it. Which I think summarizes the reason
search_path really is special.

That also opens up opportunities to do other things we might want to
do:

  * have a compatibility GUC to set the default back to DYNAMIC
  * track other dependencies of functions better ("DEPENDS ON TABLE
...")
  * provide better error messages, like "can't use function xyz in
index expression because it depends on configuration parameter foo"
  * be more consistent about using STABLE to mean that the function
depends on a snapshot, rather than overloading it for GUC dependencies

The question is, given that the acute problem is search_path, do we
want to invent all of the syntax above? Are there other use cases for
it, or should we just focus on search_path?

>  That's what I do for all my functions (maybe hurting performance?).

It doesn't look cheap, although I think we could optimize it.

> If a view calls a function, shouldn't it be called in the context of
> the view's definer/owner?

Yeah, there are a bunch of problems along those lines. I don't know if
we can solve them all in one release.

> Is the fundamental problem that we now find ourselves wanting to do
> things that require different defaults to work smoothly? On some
> level I suspect we want lexical scoping, which is what most of us
> have in our programming languages, in the database; but the database
> has many elements of dynamic scoping, and changing that is both a
> compatibility break and requires significant changes in the way the
> database is designed.

Does that suggest another approach?

Regards,
	Jeff Davis

---

From: Jeff Davis <[email-masked]>
Date: Thu, 13 Jul 2023 11:56:00 -0700

> uses a safe search path by default?

I'm inclined to proceed with the current approach here, which is to
just fix search_path for maintenance commands. Other approaches may be
possible, but:

 (a) We already special-case the way functions are executed for
maintenance commands in other ways -- we run the functions as the table
owner (even SECURITY INVOKER functions) and run them as a
SECURITY_RESTRICTED_OPERATION. Setting the search_path to a safe value
seems like a natural extension of that; and

 (b) The lack of a safe search path is blocking other useful features,
such as the MAINTAIN privilege; and

 (c) I don't see other proposals, aside from a few ideas I put forward
here[1], which didn't get any responses.

The current approach seemed to get support from Noah, Nathan, and Greg
Stark.

Concerns were raised by Gurjeet, Tom, and Robert in the 16 cycle; but
I'm not sure whether those objections were specific to the 16 cycle or
whether they are objections to the approach entirely. Comments?

Regards,
	Jeff Davis

[1]
https://www.postgresql.org/message-id/6781cc79580c464a63fc0a1343637ed2b2b0cf09.camel%40j-davis.com

---

From: Gurjeet Singh <[email-masked]>
Date: Thu, 13 Jul 2023 12:54:26 -0700

> Concerns were raised by Gurjeet, Tom, and Robert in the 16 cycle; but

I didn't see Tom's or Robert's concerns raised in this thread. I see
now that for some reason there are two threads with slightly different
subjects. I'll catch-up on that, as well.

The other thread's subject: "pgsql: Fix search_path to a safe value
during maintenance operations"

> whether they are objections to the approach entirely. Comments?

The approach seems good to me. My concern is with this change's
potential to cause an extended database outage. Hence sending it out
as part of v16, without any escape hatch for the DBA, is my objection.

It it were some commercial database, we would have simply introduced a
hidden event, or a GUC, with default off. But a GUC for this feels too
heavy handed.

Perhaps we can provide an escape hatch as follows (warning, pseudocode ahead).

> if (first_element(search_path) != "dont_override_search_patch_for_maintenance")
>      SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, ...);

So, if someone desperately wants to just plow through the maintenance
command, and are not ready or able to fix their dependence on their
search_path, the community can show them this escape-hatch.

Best regards,
Gurjeet
http://Gurje.et

---

From: "David G. Johnston" <[email-masked]>
Date: Thu, 13 Jul 2023 13:37:24 -0700

> as part of v16, without any escape hatch for the DBA, is my objection.
If this is limited to MAINT, which I'm in support of, there is no need for
an "escape hatch".  A prerequisite for leveraging the new feature is that
you fix the code so it conforms to the new way of doing things.

Tom's opinion was a general dislike for differing behavior in different
situations.  I dislike it too, on purist grounds, but would rather do this
than not make any forward progress because we made a poor decision in the
past. And I'm against simply breaking the past without any recourse as what
we did for pg_dump/pg_restore still bothers me.

David J.

---

From: Gurjeet Singh <[email-masked]>
Date: Thu, 13 Jul 2023 14:00:29 -0700

On Thu, Jul 13, 2023 at 1:37 PM David G. Johnston
<[email-masked]> wrote:
>
>  I'm against simply breaking the past without any recourse as what we did for pg_dump/pg_restore still bothers me.

I'm sure this is tangential, but can you please provide some
context/links to the change you're referring to here.

Best regards,
Gurjeet
http://Gurje.et

---

From: "David G. Johnston" <[email-masked]>
Date: Thu, 13 Jul 2023 14:07:27 -0700

On Thu, Jul 13, 2023 at 2:00 PM Gurjeet Singh <[email-masked]> wrote:

> >  I'm against simply breaking the past without any recourse as what we
> did for pg_dump/pg_restore still bothers me.
>
Here is the instigating issue and a discussion thread on the aftermath:

https://wiki.postgresql.org/wiki/A_Guide_to_CVE-2018-1058%3A_Protect_Your_Search_Path

https://www.postgresql.org/message-id/flat/13033.1531517020%40sss.pgh.pa.us#2aa2e25816d899d62f168926e3ff17b1

David J.

---

From: Jeff Davis <[email-masked]>
Date: Thu, 13 Jul 2023 15:19:42 -0700

On Thu, 2023-07-13 at 13:37 -0700, David G. Johnston wrote:
> If this is limited to MAINT, which I'm in support of, there is no
> need for an "escape hatch".  A prerequisite for leveraging the new
> feature is that you fix the code so it conforms to the new way of
> doing things.

The current patch is not limited to exercise of the MAINTAIN privilege.

> Tom's opinion was a general dislike for differing behavior in
> different situations.  I dislike it too, on purist grounds, but would
> rather do this than not make any forward progress because we made a
> poor decision in the past.

I believe the opinion you're referring to is here:

https://www.postgresql.org/message-id/[email-masked]

Which was a reaction to another version of my patch which implemented
your idea to limit the changes to the MAINTAIN privilege.

I agree with you that we should be practical here. The end goal is to
move users away from using functions that both (a) implicitly depend on
the search_path; and (b) are called implicitly as a side-effect of
accessing a table. Whatever is the fastest and smoothest way to get
there is fine with me.

> And I'm against simply breaking the past without any recourse as what

Is a GUC the solution here? Gurjeet called it heavy-handed, and I see
the point that carrying such a GUC forever would be unpleasant. But if
it reduces the risk of breakage (or at least offers an escape hatch)
then it may be wise, and hopefully we can remove it later.

Regards,
	Jeff Davis

---

From: Noah Misch <[email-masked]>
Date: Sat, 15 Jul 2023 14:13:33 -0700

On Thu, Jul 13, 2023 at 02:07:27PM -0700, David G. Johnston wrote:
> > On Thu, Jul 13, 2023 at 1:37 PM David G. Johnston <[email-masked]> wrote:

I don't blame you for feeling bothered about it.  A benefit of having done it
is that we gained insight into the level of pain it caused.  If it had been
sufficiently painful, someone would have quickly added an escape hatch.  Five
years later, nobody has added one.

The 2018 security fixes instigated many function repairs that $SUBJECT would
otherwise instigate.  That wasn't too painful.  The net new pain of $SUBJECT
will be less, since the 2018 security fixes prepared the path.  Hence, I
remain +1 for the latest Davis proposal.