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, for two reasons:
1. Make the behavior of maintenance commands more consistent because
they'd always have the same search_path.
2. Now that we have the MAINTAIN privilege in 16, privileged non-
superusers can execute maintenance commands on other users' tables.
That raises the possibility that a user with MAINTAIN privilege may be
able to use search_path tricks to escalate privileges to the table
owner. The MAINTAIN privilege is only given to highly-privileged users,
but there's still some risk. For this reason I also propose that it
goes in v16.
There's one interesting part: in the code path for creating a
materialized view, ExecCreateTableAs() has this comment:
/*
* For materialized views, lock down security-restricted operations and
* arrange to make GUC variable changes local to this command. This is
* not necessary for security, but this keeps the behavior similar to
* REFRESH MATERIALIZED VIEW. Otherwise, one could create a
materialized
* view not possible to refresh.
*/
My patch doesn't address this ExecCreateTableAs() check. To do so, we
would need to set the search path after DefineRelation(), otherwise it
will try to create the object in pg_catalog. But DefineRelation() is
happening at execution time, well after we entered the
SECURITY_RESTRICTED_OPERATION, and it doesn't seem good to separate the
SECURITY_RESTRICTED_OPERATION from setting search_path.
This ExecCreateTableAs() check doesn't seem terribly important, so I
don't think it's necessary to improve it as a part of this patch (it
won't be perfect anyway: functions can behave inconsistently for all
kinds of reasons). But I'm open to suggestion if someone knows a good
way to do it.
--
Jeff Davis
PostgreSQL Contributor Team - AWS
From 5c6d707a88c887641d551ed9a6983c74d6a82a7a Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Tue, 18 Apr 2023 10:45:51 -0700
Subject: [PATCH] Fix search_path to a safe value during maintenance
operations.
While executing maintenance operations (ANALYZE, CLUSTER, REFRESH
MATERIALIZED VIEW, REINDEX, or VACUUM), set search_path to
'pg_catalog, pg_temp' to prevent inconsistent behavior.
Functions that are used for functional indexes, in index expressions,
or in materialized views and depend on a different search path must be
declared with CREATE FUNCTION ... SET search_path='...'.
This change addresses a small security risk introduced in commit
60684dd834, where a role with MAINTAIN privileges on a table may be
able to escalate privileges to the table owner. That commit is not yet
part of any release, so no need to backpatch.
---
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 6979aff727..2b5c8a300b 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -282,6 +282,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 352e43d0e6..e8337041ac 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1475,6 +1475,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);
@@ -3006,6 +3008,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 */
{
@@ -3341,6 +3345,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);
@@ -3601,6 +3607,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 52ef462dba..2fdd180497 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 369fea7c04..c94a3e20d6 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 a5168c9f09..a7c6a3dc7a 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
@@ -3753,6 +3757,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 a843f9ad92..c082611797 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -2172,6 +2172,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 3cf4ac8c9e..997c5c57a5 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 41e020cf20..454821c16b 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
