On Tue, 2024-07-09 at 15:20 +0900, Michael Paquier wrote:
> On Sun, Jun 30, 2024 at 03:23:44PM -0700, Noah Misch wrote:
> > I've audited NewGUCNestLevel() calls that didn't get this
> > addition. Among
> > those, these need the addition:
> >
> > - Each in ComputeIndexAttrs() -- they arise when the caller is
> > DefineIndex()
> > - In DefineIndex(), after comment "changed a behavior-affecting
> > GUC"
Thank you for the report. Patch attached to address these missing call
sites.
> Hmm. Is RestrictSearchPath() something that we should advertise more
> strongly, thinking here about extensions that call NewGUCNestLevel()?
> That would be really easy to miss, and it could have bad
> consequences.
> I know that this is not something that's published in the release
> notes, but it looks like something sensible to have, though.
The pattern also involves SetUserIdAndSecContext(). Perhaps we could
come up with a wrapper function to better encapsulate the general
pattern?
> > While "not necessary for security", ExecCreateTableAs() should do
> > it for the
> > same reason it calls NewGUCNestLevel().
>
> +1.
Do you have a suggestion about how that should be done?
It's not trivial, because the both creates the table and populates it
in ExecutorRun. For table creation, we need to use the original
search_path, but we need to use the restricted search_path when
populating it.
I could try to refactor it into two statements and execute them
separately, or I could try to rewrite the statement to use a fully-
qualified destination table before execution. Thoughts?
Regards,
Jeff Davis
From 58331d25defa861a087e11755da987bce932ed1f 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 v1] 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
