On Thu, 2023-06-29 at 11:19 -0400, Robert Haas wrote:
> Yeah. I mean, as things stand, it seems like giving someone the
> MAINTAIN privilege will be sufficient to allow them to escalate to
> the
> table owner if there are any expression indexes involved. That seems
> like a real problem. We shouldn't ship a new feature with a built-in
> security hole like that.
Let's take David's suggestion[1] then, and only restrict the search
path for those without owner privileges on the object.
That would mean no behavior change unless using the MAINTAIN privilege,
which is new, so no breakage. And if someone is using the MAINTAIN
privilege, they wouldn't be able to abuse the search_path, so it would
close the hole.
Patch attached (created a bit quickly, but seems to work).
Regards,
Jeff Davis
[1]
https://postgr.es/m/CAKFQuwaVJkM9u%2BqpOaom2UkPE1sz0BASF-E5amxWPxncUhm4Hw%40mail.gmail.com
From 67387349d2d84cdc2d7b6d7ab5d5824e5ccc89bf Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
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
