From 67387349d2d84cdc2d7b6d7ab5d5824e5ccc89bf Mon Sep 17 00:00:00 2001 From: Jeff Davis Date: Thu, 29 Jun 2023 16:03:53 -0700 Subject: [PATCH] Restrict search_path for non-owners performing maintenance. When non-owners execute 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 addresses a 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. A previous fix 05e1737351 was deemed too large a change. This is a narrower fix that only affects non-owners, which previously could not execute maintenance commands. Per suggestion from David G. Johnston. Discussion: https://postgr.es/m/CAKFQuwaVJkM9u%2BqpOaom2UkPE1sz0BASF-E5amxWPxncUhm4Hw%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 | 5 +++++ src/backend/commands/analyze.c | 2 ++ src/backend/commands/cluster.c | 3 ++- src/backend/commands/indexcmds.c | 4 +++- src/backend/commands/matview.c | 2 ++ src/backend/commands/vacuum.c | 2 ++ src/backend/utils/init/usercontext.c | 17 +++++++++++++++++ src/include/utils/guc.h | 6 ++++++ src/include/utils/usercontext.h | 1 + 11 files changed, 44 insertions(+), 2 deletions(-) diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c index 94a9759322..1a77035e25 100644 --- a/contrib/amcheck/verify_nbtree.c +++ b/contrib/amcheck/verify_nbtree.c @@ -40,6 +40,7 @@ #include "utils/guc.h" #include "utils/memutils.h" #include "utils/snapmgr.h" +#include "utils/usercontext.h" PG_MODULE_MAGIC; @@ -281,6 +282,7 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed, SetUserIdAndSecContext(heaprel->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + RestrictSearchPath(save_userid, heaprel->rd_rel->relowner); } else { diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 3c6a956eaa..9e04fd6ecb 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -40,6 +40,7 @@ #include "utils/index_selfuncs.h" #include "utils/memutils.h" #include "utils/rel.h" +#include "utils/usercontext.h" /* @@ -1066,6 +1067,7 @@ brin_summarize_range(PG_FUNCTION_ARGS) SetUserIdAndSecContext(heapRel->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + RestrictSearchPath(save_userid, heapRel->rd_rel->relowner); } else { diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 352e43d0e6..9123e7e7b7 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -84,6 +84,7 @@ #include "utils/snapmgr.h" #include "utils/syscache.h" #include "utils/tuplesort.h" +#include "utils/usercontext.h" /* Potentially set by pg_upgrade_support functions */ Oid binary_upgrade_next_index_pg_class_oid = InvalidOid; @@ -1475,6 +1476,7 @@ index_concurrently_build(Oid heapRelationId, SetUserIdAndSecContext(heapRel->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + RestrictSearchPath(save_userid, heapRel->rd_rel->relowner); indexRelation = index_open(indexRelationId, RowExclusiveLock); @@ -3006,6 +3008,7 @@ index_build(Relation heapRelation, SetUserIdAndSecContext(heapRelation->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + RestrictSearchPath(save_userid, heapRelation->rd_rel->relowner); /* Set up initial progress report status */ { @@ -3341,6 +3344,7 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot) SetUserIdAndSecContext(heapRelation->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + RestrictSearchPath(save_userid, heapRelation->rd_rel->relowner); indexRelation = index_open(indexId, RowExclusiveLock); @@ -3601,6 +3605,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence, SetUserIdAndSecContext(heapRelation->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + RestrictSearchPath(save_userid, heapRelation->rd_rel->relowner); if (progress) { diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c index fc9a371f9b..cbe8350a7e 100644 --- a/src/backend/commands/analyze.c +++ b/src/backend/commands/analyze.c @@ -67,6 +67,7 @@ #include "utils/spccache.h" #include "utils/syscache.h" #include "utils/timestamp.h" +#include "utils/usercontext.h" /* Per-index data for ANALYZE */ @@ -348,6 +349,7 @@ do_analyze_rel(Relation onerel, VacuumParams *params, SetUserIdAndSecContext(onerel->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + RestrictSearchPath(save_userid, onerel->rd_rel->relowner); /* 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..efed6401ae 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -59,6 +59,7 @@ #include "utils/snapmgr.h" #include "utils/syscache.h" #include "utils/tuplesort.h" +#include "utils/usercontext.h" /* * This struct is used to pass around the information on tables to be @@ -355,7 +356,7 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params) SetUserIdAndSecContext(OldHeap->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); - + RestrictSearchPath(save_userid, OldHeap->rd_rel->relowner); /* * Since we may open a new transaction for each relation, we have to check * that the relation still is what we think it is. diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 9bc97e1fc2..f4dead0f48 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -69,7 +69,7 @@ #include "utils/regproc.h" #include "utils/snapmgr.h" #include "utils/syscache.h" - +#include "utils/usercontext.h" /* non-export function prototypes */ static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts); @@ -1300,6 +1300,7 @@ DefineIndex(Oid relationId, SetUserIdAndSecContext(childrel->rd_rel->relowner, child_save_sec_context | SECURITY_RESTRICTED_OPERATION); child_save_nestlevel = NewGUCNestLevel(); + RestrictSearchPath(child_save_userid, childrel->rd_rel->relowner); /* * Don't try to create indexes on foreign tables, though. Skip @@ -3750,6 +3751,7 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) SetUserIdAndSecContext(heapRel->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + RestrictSearchPath(save_userid, heapRel->rd_rel->relowner); /* 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..920703442b 100644 --- a/src/backend/commands/matview.c +++ b/src/backend/commands/matview.c @@ -45,6 +45,7 @@ #include "utils/rel.h" #include "utils/snapmgr.h" #include "utils/syscache.h" +#include "utils/usercontext.h" typedef struct @@ -179,6 +180,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString, SetUserIdAndSecContext(relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + RestrictSearchPath(save_userid, relowner); /* 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..58caa761ac 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -61,6 +61,7 @@ #include "utils/pg_rusage.h" #include "utils/snapmgr.h" #include "utils/syscache.h" +#include "utils/usercontext.h" /* @@ -2171,6 +2172,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, SetUserIdAndSecContext(rel->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + RestrictSearchPath(save_userid, rel->rd_rel->relowner); /* * If PROCESS_MAIN is set (the default), it's time to vacuum the main diff --git a/src/backend/utils/init/usercontext.c b/src/backend/utils/init/usercontext.c index dd9a0dd6a8..b62420b56e 100644 --- a/src/backend/utils/init/usercontext.c +++ b/src/backend/utils/init/usercontext.c @@ -90,3 +90,20 @@ RestoreUserContext(UserContext *context) AtEOXact_GUC(false, context->save_nestlevel); SetUserIdAndSecContext(context->save_userid, context->save_sec_context); } + +/* + * When potentially executing code on an object with the given owner, if + * userid does not have owner privileges, set a restricted safe search_path. + * + * Note: the caller is expected to save and restore the GUC nest level as + * necessary. + */ +void +RestrictSearchPath(Oid userid, Oid owner) +{ + if (has_privs_of_role(userid, owner)) + return; + + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); +} 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/include/utils/usercontext.h b/src/include/utils/usercontext.h index a8195c194d..d5422ce70d 100644 --- a/src/include/utils/usercontext.h +++ b/src/include/utils/usercontext.h @@ -22,5 +22,6 @@ typedef struct UserContext /* Function prototypes. */ extern void SwitchToUntrustedUser(Oid userid, UserContext *context); extern void RestoreUserContext(UserContext *context); +extern void RestrictSearchPath(Oid userid, Oid owner); #endif /* USERCONTEXT_H */ -- 2.34.1