On Fri, 2024-07-12 at 16:11 -0700, Noah Misch wrote:
> Since refresh->relation is a RangeVar, this departs from the standard
> against
> repeated name lookups, from CVE-2014-0062 (commit 5f17304).
Interesting, thank you.
I did a rough refactor and attached v3. Aside from cleanup issues, is
this what you had in mind?
Regards,
Jeff Davis
From cf1722bf0716897f42ce89282f361af4be56b60b Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Tue, 9 Jul 2024 11:12:46 -0700
Subject: [PATCH v3 1/2] Add missing RestrictSearchPath() calls.
Reported-by: Noah Misch
Backpatch-through: 17
Discussion: https://postgr.es/m/20240630222344.db.nmi...@google.com
---
src/backend/commands/indexcmds.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 2caab88aa58..c5a56c75f69 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1230,6 +1230,7 @@ DefineIndex(Oid tableId,
*/
AtEOXact_GUC(false, root_save_nestlevel);
root_save_nestlevel = NewGUCNestLevel();
+ RestrictSearchPath();
/* Add any requested comment */
if (stmt->idxcomment != NULL)
@@ -2027,6 +2028,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
{
SetUserIdAndSecContext(save_userid, save_sec_context);
*ddl_save_nestlevel = NewGUCNestLevel();
+ RestrictSearchPath();
}
}
@@ -2074,6 +2076,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
{
SetUserIdAndSecContext(save_userid, save_sec_context);
*ddl_save_nestlevel = NewGUCNestLevel();
+ RestrictSearchPath();
}
/*
@@ -2104,6 +2107,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
{
SetUserIdAndSecContext(save_userid, save_sec_context);
*ddl_save_nestlevel = NewGUCNestLevel();
+ RestrictSearchPath();
}
/*
--
2.34.1
From e1f89d13d46cdf040572fc1ce760d5ebc1cacc76 Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Fri, 12 Jul 2024 14:23:17 -0700
Subject: [PATCH v3 2/2] For materialized views, use REFRESH to load data
during creation.
Previously, CREATE MATERIALIZED VIEW ... WITH DATA populated the MV
during creation. Instead, use REFRESH, which locks down
security-restricted operations and restricts the
search_path. Otherwise, it's possible to create MVs that cannot be
refreshed.
Reported-by: Noah Misch
Backpatch-through: 17
Discussion: https://postgr.es/m/20240630222344.db.nmi...@google.com
---
src/backend/commands/createas.c | 33 +++++++++----------
src/backend/commands/matview.c | 43 +++++++++++++++----------
src/include/commands/matview.h | 3 ++
src/test/regress/expected/namespace.out | 4 +--
4 files changed, 47 insertions(+), 36 deletions(-)
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 62050f4dc59..54a554f4b67 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -225,10 +225,8 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
Query *query = castNode(Query, stmt->query);
IntoClause *into = stmt->into;
bool is_matview = (into->viewQuery != NULL);
+ bool do_refresh = false;
DestReceiver *dest;
- Oid save_userid = InvalidOid;
- int save_sec_context = 0;
- int save_nestlevel = 0;
ObjectAddress address;
List *rewritten;
PlannedStmt *plan;
@@ -263,18 +261,13 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
Assert(query->commandType == CMD_SELECT);
/*
- * 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.
+ * For materialized views, always skip data during table creation, and use
+ * REFRESH instead.
*/
if (is_matview)
{
- GetUserIdAndSecContext(&save_userid, &save_sec_context);
- SetUserIdAndSecContext(save_userid,
- save_sec_context | SECURITY_RESTRICTED_OPERATION);
- save_nestlevel = NewGUCNestLevel();
+ do_refresh = !into->skipData;
+ into->skipData = true;
}
if (into->skipData)
@@ -346,13 +339,19 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
PopActiveSnapshot();
}
- if (is_matview)
+ /*
+ * For materialized views, use REFRESH, which locks down
+ * security-restricted operations and restricts the search_path.
+ * Otherwise, one could create a materialized view not possible to
+ * refresh.
+ */
+ if (do_refresh)
{
- /* Roll back any GUC changes */
- AtEOXact_GUC(false, save_nestlevel);
+ RefreshMatViewByOid(address.objectId, false, false,
+ pstate->p_sourcetext, NULL, qc);
- /* Restore userid and security context */
- SetUserIdAndSecContext(save_userid, save_sec_context);
+ if (qc)
+ qc->commandTag = CMDTAG_SELECT;
}
return address;
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index ea05d4b224f..4b03e80404d 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -134,6 +134,28 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
ParamListInfo params, QueryCompletion *qc)
{
Oid matviewOid;
+ LOCKMODE lockmode;
+
+ /* Determine strength of lock needed. */
+ lockmode = stmt->concurrent ? ExclusiveLock : AccessExclusiveLock;
+
+ /*
+ * Get a lock until end of transaction.
+ */
+ matviewOid = RangeVarGetRelidExtended(stmt->relation,
+ lockmode, 0,
+ RangeVarCallbackMaintainsTable,
+ NULL);
+
+ return RefreshMatViewByOid(matviewOid, stmt->skipData, stmt->concurrent,
+ queryString, params, qc);
+}
+
+ObjectAddress
+RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
+ const char *queryString, ParamListInfo params,
+ QueryCompletion *qc)
+{
Relation matviewRel;
RewriteRule *rule;
List *actions;
@@ -143,25 +165,12 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
Oid OIDNewHeap;
DestReceiver *dest;
uint64 processed = 0;
- bool concurrent;
- LOCKMODE lockmode;
char relpersistence;
Oid save_userid;
int save_sec_context;
int save_nestlevel;
ObjectAddress address;
- /* Determine strength of lock needed. */
- concurrent = stmt->concurrent;
- lockmode = concurrent ? ExclusiveLock : AccessExclusiveLock;
-
- /*
- * Get a lock until end of transaction.
- */
- matviewOid = RangeVarGetRelidExtended(stmt->relation,
- lockmode, 0,
- RangeVarCallbackMaintainsTable,
- NULL);
matviewRel = table_open(matviewOid, NoLock);
relowner = matviewRel->rd_rel->relowner;
@@ -190,7 +199,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
errmsg("CONCURRENTLY cannot be used when the materialized view is not populated")));
/* Check that conflicting options have not been specified. */
- if (concurrent && stmt->skipData)
+ if (concurrent && skipData)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s and %s options cannot be used together",
@@ -275,7 +284,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
* Tentatively mark the matview as populated or not (this will roll back
* if we fail later).
*/
- SetMatViewPopulatedState(matviewRel, !stmt->skipData);
+ SetMatViewPopulatedState(matviewRel, !skipData);
/* Concurrent refresh builds new data in temp tablespace, and does diff. */
if (concurrent)
@@ -301,7 +310,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
dest = CreateTransientRelDestReceiver(OIDNewHeap);
/* Generate the data, if wanted. */
- if (!stmt->skipData)
+ if (!skipData)
processed = refresh_matview_datafill(dest, dataQuery, queryString);
/* Make the matview match the newly generated data. */
@@ -333,7 +342,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
* inserts and deletes it issues get counted by lower-level code.)
*/
pgstat_count_truncate(matviewRel);
- if (!stmt->skipData)
+ if (!skipData)
pgstat_count_heap_insert(matviewRel, processed);
}
diff --git a/src/include/commands/matview.h b/src/include/commands/matview.h
index 817b2ba0b6f..a226b2e68fb 100644
--- a/src/include/commands/matview.h
+++ b/src/include/commands/matview.h
@@ -25,6 +25,9 @@ extern void SetMatViewPopulatedState(Relation relation, bool newstate);
extern ObjectAddress ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
ParamListInfo params, QueryCompletion *qc);
+extern ObjectAddress RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
+ const char *queryString, ParamListInfo params,
+ QueryCompletion *qc);
extern DestReceiver *CreateTransientRelDestReceiver(Oid transientoid);
diff --git a/src/test/regress/expected/namespace.out b/src/test/regress/expected/namespace.out
index 7d36e9cc0c4..dbbda72d395 100644
--- a/src/test/regress/expected/namespace.out
+++ b/src/test/regress/expected/namespace.out
@@ -129,8 +129,8 @@ $$;
CREATE TABLE test_maint(i INT);
INSERT INTO test_maint VALUES (1), (2);
CREATE MATERIALIZED VIEW test_maint_mv AS SELECT fn(i) FROM test_maint;
-NOTICE: current search_path: test_maint_search_path
-NOTICE: current search_path: test_maint_search_path
+NOTICE: current search_path: pg_catalog, pg_temp
+NOTICE: current search_path: pg_catalog, pg_temp
-- the following commands should see search_path as pg_catalog, pg_temp
CREATE INDEX test_maint_idx ON test_maint_search_path.test_maint (fn(i));
NOTICE: current search_path: pg_catalog, pg_temp
--
2.34.1
