Обсуждение: MAINTAIN privilege -- what do we need to un-revert it?
The MAINTAIN privilege was reverted during the 16 cycle because of the
potential for someone to play tricks with search_path.
For instance, if user foo does:
CREATE FUNCTION mod7(INT) RETURNS INT IMMUTABLE
LANGUAGE plpgsql AS $$ BEGIN RETURN mod($1, 7); END; $$;
CREATE TABLE x(i INT);
CREATE UNIQUE INDEX x_mod7_idx ON x (mod7(i));
GRANT MAINTAIN ON x TO bar;
Then user bar can create their own function named "bar.mod(int, int)",
and "SET search_path = bar, pg_catalog", and then issue a "REINDEX x"
and cause problems.
There are several factors required for that to be a problem:
1. foo hasn't used a "SET search_path" clause on their function
2. bar must have the privileges to create a function somewhere
3. bar must have privileges on table x
There's an argument that we should blame factor #1. Robert stated[1]
that users should use SET search_path clauses on their functions, even
SECURITY INVOKER functions. And I've added a search_path cache which
improves the performance enough to make that more reasonable to do
generally.
There's also an argument that #2 is to blame. Given the realities of
our system, best practice is that users shouldn't have the privileges
to create objects, even in their own schema, unless required. (Joe made
this suggestion in an offline discussion.)
There's also an arugment that #3 is not specific to the MAINTAIN
privilege. Clearly similar risks exist for other privileges, like
TRIGGER. And even the INSERT privilege, in the above example, would
allow bar to violate the unique constraint and corrupt the index[2].
If those arguments are still unconvincing, then the next idea is to fix
the search_path for all maintenance commands[3]. I tried this during
the 16 cycle, but due to timing issues it was also reverted. I can
proceed with this approach again, but I'd like a clear endorsement, in
case there were other reasons to doubt the approach.
Regards,
Jeff Davis
[1]
https://www.postgresql.org/message-id/CA+TgmoYEP40iBW-A9nPfDp8AhGoekPp3aPDFzTgBUrqmfCwZzQ@mail.gmail.com
[2]
https://www.postgresql.org/message-id/fff566293c9165c69bb4c555da1ac02c63660664.camel@j-davis.com
[3]
https://www.postgresql.org/message-id/e44327179e5c9015c8dda67351c04da552066017.camel@j-davis.com
On Wed, Feb 14, 2024 at 10:20:28AM -0800, Jeff Davis wrote: > If those arguments are still unconvincing, then the next idea is to fix > the search_path for all maintenance commands[3]. I tried this during > the 16 cycle, but due to timing issues it was also reverted. I can > proceed with this approach again, but I'd like a clear endorsement, in > case there were other reasons to doubt the approach. This seemed like the approach folks were most in favor of at the developer meeting a couple weeks ago [0]. At least, that was my interpretation of the discussion. BTW I have been testing reverting commit 151c22d (i.e., un-reverting MAINTAIN) every month or two, and last I checked, it still applies pretty cleanly. The only changes I've needed to make are to the catversion and to a hard-coded version in a test (16 -> 17). [0] https://wiki.postgresql.org/wiki/FOSDEM/PGDay_2024_Developer_Meeting#The_Path_to_un-reverting_the_MAINTAIN_privilege -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Wed, Feb 14, 2024 at 01:02:26PM -0600, Nathan Bossart wrote: > BTW I have been testing reverting commit 151c22d (i.e., un-reverting > MAINTAIN) every month or two, and last I checked, it still applies pretty > cleanly. The only changes I've needed to make are to the catversion and to > a hard-coded version in a test (16 -> 17). Posting to get some cfbot coverage. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Вложения
On Wed, 2024-02-14 at 13:02 -0600, Nathan Bossart wrote:
> This seemed like the approach folks were most in favor of at the
> developer
> meeting a couple weeks ago [0]. At least, that was my interpretation
> of
> the discussion.
Attached rebased version.
Note the changes in amcheck. It's creating functions and calling those
functions from the comparators, and so the comparators need to set the
search_path. I don't think that's terribly common, but does represent a
behavior change and could break something.
Regards,
Jef Davis
Вложения
(Apologies in advance for anything I'm bringing up that we've already
covered somewhere else.)
On Fri, Feb 16, 2024 at 04:03:55PM -0800, Jeff Davis wrote:
> Note the changes in amcheck. It's creating functions and calling those
> functions from the comparators, and so the comparators need to set the
> search_path. I don't think that's terribly common, but does represent a
> behavior change and could break something.
Why is this change needed? Is the idea to make amcheck follow the same
rules as maintenance commands to encourage folks to set up index functions
correctly? Or is amcheck similarly affected by search_path tricks?
> void
> InitializeSearchPath(void)
> {
> + /* Make the context we'll keep search path cache hashtable in */
> + SearchPathCacheContext = AllocSetContextCreate(TopMemoryContext,
> + "search_path processing cache",
> + ALLOCSET_DEFAULT_SIZES);
> +
> if (IsBootstrapProcessingMode())
> {
> /*
> @@ -4739,11 +4744,6 @@ InitializeSearchPath(void)
> }
> else
> {
> - /* Make the context we'll keep search path cache hashtable in */
> - SearchPathCacheContext = AllocSetContextCreate(TopMemoryContext,
> - "search_path processing cache",
> - ALLOCSET_DEFAULT_SIZES);
> -
What is the purpose of this change?
> + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
> + PGC_S_SESSION);
I wonder if it's worth using PGC_S_INTERACTIVE or introducing a new value
for these.
> +/*
> + * Safe search path when executing code as the table owner, such as during
> + * maintenance operations.
> + */
> +#define GUC_SAFE_SEARCH_PATH "pg_catalog, pg_temp"
Is including pg_temp actually safe? I worry that a user could use their
temporary schema to inject objects that would take the place of
non-schema-qualified stuff in functions.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
New version attached.
Do we need a documentation update here? If so, where would be a good
place?
On Fri, 2024-02-23 at 15:30 -0600, Nathan Bossart wrote:
> Why is this change needed? Is the idea to make amcheck follow the
> same
> rules as maintenance commands to encourage folks to set up index
> functions
> correctly?
amcheck is calling functions it defined, so in order to find those
functions it needs to set the right search path.
>
> What is the purpose of this [bootstrap-related] change?
DefineIndex() is called during bootstrap, and it's also a maintenance
command. I tried to handle the bootstrapping case, but I think it's
best to just guard it with a conditional. Done.
I also added Assert(!IsBootstrapProcessingMode()) in
assign_search_path().
> > + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH,
> > PGC_USERSET,
> > + PGC_S_SESSION);
>
> I wonder if it's worth using PGC_S_INTERACTIVE or introducing a new
> value
> for these.
> >
Did you have a particular concern about PGC_S_SESSION?
If it's less than PGC_S_SESSION, it won't work, because the caller's
SET command will override it, and the same manipulation is possible.
And I don't think we want it higher than PGC_S_SESSION, otherwise the
function can't set its own search_path, if needed.
> > +#define GUC_SAFE_SEARCH_PATH "pg_catalog, pg_temp"
>
> Is including pg_temp actually safe? I worry that a user could use
> their
> temporary schema to inject objects that would take the place of
> non-schema-qualified stuff in functions.
pg_temp cannot (currently) be excluded. If it is omitted from the
string, it will be placed *first* in the search_path, which is more
dangerous.
pg_temp does not take part in function or operator resolution, which
makes it safer than it first appears. There are potentially some risks
around tables, but it's not typical to access a table in a function
called as part of an index expression.
If we determine that pg_temp is actually unsafe to include, we need to
do something like what I proposed here:
https://www.postgresql.org/message-id/a6865db287596c9c6ea12bdd9de87216cb5e7902.camel@j-davis.com
before this change.
Regards,
Jeff Davis
Вложения
On Tue, Feb 27, 2024 at 04:22:34PM -0800, Jeff Davis wrote: > Do we need a documentation update here? If so, where would be a good > place? I'm afraid I don't have a better idea than adding a short note in each affected commands's page. > On Fri, 2024-02-23 at 15:30 -0600, Nathan Bossart wrote: >> I wonder if it's worth using PGC_S_INTERACTIVE or introducing a new >> value >> for these. > > Did you have a particular concern about PGC_S_SESSION? My only concern is that it could obscure the source of the search_path change, which in turn might cause confusion when things fail. > If it's less than PGC_S_SESSION, it won't work, because the caller's > SET command will override it, and the same manipulation is possible. > > And I don't think we want it higher than PGC_S_SESSION, otherwise the > function can't set its own search_path, if needed. Yeah, we would have to make it equivalent in priority to PGC_S_SESSION, which would likely require a bunch of special logic. I don't know if this is worth it, and this seems like something that could pretty easily be added in the future if it became necessary. >> > +#define GUC_SAFE_SEARCH_PATH "pg_catalog, pg_temp" >> >> Is including pg_temp actually safe? I worry that a user could use >> their >> temporary schema to inject objects that would take the place of >> non-schema-qualified stuff in functions. > > pg_temp cannot (currently) be excluded. If it is omitted from the > string, it will be placed *first* in the search_path, which is more > dangerous. > > pg_temp does not take part in function or operator resolution, which > makes it safer than it first appears. There are potentially some risks > around tables, but it's not typical to access a table in a function > called as part of an index expression. > > If we determine that pg_temp is actually unsafe to include, we need to > do something like what I proposed here: > > https://www.postgresql.org/message-id/a6865db287596c9c6ea12bdd9de87216cb5e7902.camel@j-davis.com I don't doubt anything you've said, but I can't help but think that we might as well handle the pg_temp risk, too. Furthermore, I see that we use "" as a safe search_path for autovacuum and fe_utils/connect.h. Is there any way to unite these? IIUC it might be possible to combine the autovacuum and maintenance command cases (i.e., "!pg_temp"), but we might need to keep pg_temp for the frontend case. I think it's worth trying to add comments about why this setting is safe for some cases but not others, too. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Wed, 2024-02-28 at 10:55 -0600, Nathan Bossart wrote:
> I'm afraid I don't have a better idea than adding a short note in
> each
> affected commands's page.
OK, that works for now.
Later we should also document that the functions are run as the table
owner.
> > On Fri, 2024-02-23 at 15:30 -0600, Nathan Bossart wrote:
> > > I wonder if it's worth using PGC_S_INTERACTIVE or introducing a
> > > new
> > > value
> > > for these.
> >
> > Did you have a particular concern about PGC_S_SESSION?
>
> My only concern is that it could obscure the source of the
> search_path
> change, which in turn might cause confusion when things fail.
That's a good point. AutoVacWorkerMain uses PGC_S_OVERRIDE, but it
doesn't have to worry about SET, because there's no real session.
The function SET clause uses PGC_S_SESSION. It's arguable whether
that's really the same source as a SET command, but it's definitely
closer.
>
> Yeah, we would have to make it equivalent in priority to
> PGC_S_SESSION,
> which would likely require a bunch of special logic.
I'm not clear on what problem that would solve.
> I don't doubt anything you've said, but I can't help but think that
> we
> might as well handle the pg_temp risk, too.
That sounds good to me, but I didn't get many replies in that last
thread. And although it solves the problem, it is a bit awkward.
Can we get some closure on whether that !pg_temp patch is the right
approach? That was just my first idea, and it would be good to hear
what others think.
> Furthermore, I see that we use "" as a safe search_path for
> autovacuum and
> fe_utils/connect.h. Is there any way to unite these?
We could have a single function like RestrictSearchPath(), which I
believe I had in some previous iteration. That would use the safest
search path (either excluding pg_temp or putting it at the end) and
PGC_S_SESSION, and then use it everywhere.
Regards,
Jeff Davis
On Wed, 2024-02-28 at 09:29 -0800, Jeff Davis wrote:
> On Wed, 2024-02-28 at 10:55 -0600, Nathan Bossart wrote:
> > I'm afraid I don't have a better idea than adding a short note in
> > each
> > affected commands's page.
>
> OK, that works for now.
Committed.
The only changes are documentation and test updates.
This is a behavior change, so it still carries some risk, though we've
had a lot of discussion and generally it seems to be worth it. If it
turns out worse than expected during beta, of course we can re-revert
it.
I will restate the risks here, which come basically from two places:
(1) Functions called from index expressions which rely on search_path
(and don't have a SET clause).
Such a function would have already been fairly broken before my commit,
because anyone accessing the table without the right search_path would
have seen an error or wrong results. And there is no means to set the
"right" search_path for autoanalyze or logical replication, so those
would not have worked with such a broken function before my commit, no
matter what.
That being said, surely some users did have such broken functions, and
with this commit, they will have to remedy them with a SET clause.
Fortunately, the performance impact of doing so has been greatly
reduced.
(2) Matierialized views which call functions that rely on search_path
(and don't have a SET clause).
This is arguably a worse kind of breakage because materialized views
are often refreshed only by the table owner, and it's easier to control
search_path when running REFRESH. Additionally, functions called from
materialized views are more likely to be "interesting" than functions
called from an index expression. However, the remedy is
straightforward: use a SET clause.
Regards,
Jeff Davis
On Mon, Mar 04, 2024 at 07:52:05PM -0800, Jeff Davis wrote:
> Committed.
Commit 2af07e2 wrote:
> --- a/src/backend/access/brin/brin.c
> +++ b/src/backend/access/brin/brin.c
> @@ -1412,6 +1412,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);
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"
While "not necessary for security", ExecCreateTableAs() should do it for the
same reason it calls NewGUCNestLevel().
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" 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. > While "not necessary for security", ExecCreateTableAs() should do it for the > same reason it calls NewGUCNestLevel(). +1. -- Michael
Вложения
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
Вложения
On Tue, Jul 09, 2024 at 05:47:36PM -0700, Jeff Davis wrote: > 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: > > 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? Worth a look. usercontext.c has an existing wrapper for a superuser process switching to an untrusted user. It could become the home for another wrapper targeting MAINTAIN-relevant callers. > > > 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? Those sound fine. Also fine: just adding a comment on why creation namespace considerations led to not doing it there.
On Thu, 2024-07-11 at 05:52 -0700, Noah Misch wrote:
> > 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?
>
> Those sound fine. Also fine: just adding a comment on why creation
> namespace
> considerations led to not doing it there.
Attached. 0002 separates the CREATE MATERIALIZED VIEW ... WITH DATA
into (effectively):
CREATE MATERIALIZED VIEW ... WITH NO DATA;
REFRESH MATERIALIZED VIEW ...;
Using refresh also achieves the stated goal more directly: to (mostly)
ensure that a subsequent REFRESH will succeed.
Note: the creation itself no longer executes in a security-restricted
context, but I don't think that's a problem. The only reason it's using
the security restricted context is so the following REFRESH will
succeed, right?
Regards,
Jeff Davis
Вложения
On Fri, Jul 12, 2024 at 02:50:52PM -0700, Jeff Davis wrote:
> On Thu, 2024-07-11 at 05:52 -0700, Noah Misch wrote:
> > > 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?
> >
> > Those sound fine. Also fine: just adding a comment on why creation
> > namespace
> > considerations led to not doing it there.
>
> Attached. 0002 separates the CREATE MATERIALIZED VIEW ... WITH DATA
> into (effectively):
>
> CREATE MATERIALIZED VIEW ... WITH NO DATA;
> REFRESH MATERIALIZED VIEW ...;
>
> Using refresh also achieves the stated goal more directly: to (mostly)
> ensure that a subsequent REFRESH will succeed.
>
> Note: the creation itself no longer executes in a security-restricted
> context, but I don't think that's a problem. The only reason it's using
> the security restricted context is so the following REFRESH will
> succeed, right?
Right, that's the only reason.
> @@ -346,13 +339,21 @@ 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);
> + RefreshMatViewStmt *refresh = makeNode(RefreshMatViewStmt);
>
> - /* Restore userid and security context */
> - SetUserIdAndSecContext(save_userid, save_sec_context);
> + refresh->relation = into->rel;
> + ExecRefreshMatView(refresh, pstate->p_sourcetext, NULL, qc);
> +
> + if (qc)
> + qc->commandTag = CMDTAG_SELECT;
> }
Since refresh->relation is a RangeVar, this departs from the standard against
repeated name lookups, from CVE-2014-0062 (commit 5f17304).
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
Вложения
On Fri, Jul 12, 2024 at 04:50:17PM -0700, Jeff Davis wrote: > 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? > +extern ObjectAddress RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent, > + const char *queryString, ParamListInfo params, > + QueryCompletion *qc); > Yes, that's an API design that avoids repeated name lookups.