pgsql: Fix search_path to a safe value during maintenance operations.

On Fri, 2023-06-09 at 20:54 +0000, Jeff Davis wrote:

Fix search_path to a safe value during maintenance operations.

Looks like this is causing pg_amcheck failures in the buildfarm.
Investigating...

Regards,
Jeff Davis

On Fri, 2023-06-09 at 15:16 -0700, Jeff Davis wrote:

On Fri, 2023-06-09 at 20:54 +0000, Jeff Davis wrote:

Fix search_path to a safe value during maintenance operations.

Looks like this is causing pg_amcheck failures in the buildfarm.
Investigating...

It looks related to bt_index_check_internal(), which is called by SQL
functions bt_index_check() and bt_index_parent_check(). SQL functions
can be called in parallel, so it raises the error:

ERROR: cannot set parameters during a parallel operation

because commit 05e1737351 added the SetConfigOption() line. Normally
those functions would not be called in parallel, but
debug_parallel_mode makes that happen.

Attached a patch to mark those functions as PARALLEL UNSAFE, which
fixes the problem.

Alternatively, I could just take out that line, as those SQL functions
are not controlled by the MAINTAIN privilege. But for consistency I
think it's a good idea to leave it in so that index functions are
called with the right search path for amcheck.

--
Jeff Davis
PostgreSQL Contributor Team - AWS

On Fri, 2023-06-09 at 17:37 -0700, Jeff Davis wrote:

Attached a patch to mark those functions as PARALLEL UNSAFE, which
fixes the problem.

On second thought, that might not be acceptable for amcheck, depending
on how its run.

I think it's OK if run by pg_amcheck, because that runs it on a single
index at a time in each connection, and parallelizes by opening more
connections.

But if some users are calling the check functions across many tables in
a single select statement (e.g. in the targetlist of a query on
pg_class), then they'll experience a regression.

Alternatively, I could just take out that line, as those SQL
functions
are not controlled by the MAINTAIN privilege. But for consistency I
think it's a good idea to leave it in so that index functions are
called with the right search path for amcheck.

If marking them PARALLEL UNSAFE is not acceptable, then this seems like
a fine solution.

Regards,
Jeff Davis

Jeff Davis <pgsql@j-davis.com> writes:

Attached a patch to mark those functions as PARALLEL UNSAFE, which
fixes the problem.

Alternatively, I could just take out that line, as those SQL functions
are not controlled by the MAINTAIN privilege. But for consistency I
think it's a good idea to leave it in so that index functions are
called with the right search path for amcheck.

I concur with the upthread objection that it is way too late in
the release cycle to be introducing a breaking change like this.
I request that you revert it.

regards, tom lane

On Sat, Jun 10, 2023 at 01:33:31AM -0400, Tom Lane wrote:

Jeff Davis <pgsql@j-davis.com> writes:

Attached a patch to mark those functions as PARALLEL UNSAFE, which
fixes the problem.

Alternatively, I could just take out that line, as those SQL functions
are not controlled by the MAINTAIN privilege. But for consistency I
think it's a good idea to leave it in so that index functions are
called with the right search path for amcheck.

I concur with the upthread objection that it is way too late in
the release cycle to be introducing a breaking change like this.
I request that you revert it.

The timing was not great, but this is fixing a purported defect in an older
v16 feature. If the MAINTAIN privilege is actually fine, we're all set for
v16. If MAINTAIN does have a material problem that $SUBJECT had fixed, we
should either revert MAINTAIN, un-revert $SUBJECT, or fix the problem a
different way.

On Mon, Jun 12, 2023 at 1:05 PM Noah Misch <noah@leadboat.com> wrote:

I concur with the upthread objection that it is way too late in
the release cycle to be introducing a breaking change like this.
I request that you revert it.

The timing was not great, but this is fixing a purported defect in an older
v16 feature. If the MAINTAIN privilege is actually fine, we're all set for
v16. If MAINTAIN does have a material problem that $SUBJECT had fixed, we
should either revert MAINTAIN, un-revert $SUBJECT, or fix the problem a
different way.

I wonder why this commit used pg_catalog, pg_temp rather than just the
empty string, as AutoVacWorkerMain does.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, 2023-06-12 at 13:33 -0400, Robert Haas wrote:

I wonder why this commit used pg_catalog, pg_temp rather than just
the
empty string, as AutoVacWorkerMain does.

I followed the rules here for "Writing SECURITY DEFINER Functions
Safely":

https://www.postgresql.org/docs/16/sql-createfunction.html

which suggests adding pg_temp at the end (otherwise it is searched
first by default).

It's not exactly like a SECURITY DEFINER function, but running a
maintenance command does switch to the table owner, so the risks are
similar.

I don't see a problem with the pg_temp schema in non-interactive
processes, because we trust the non-interactive processes.

Regards,
Jeff Davis

On Mon, 2023-06-12 at 13:05 -0400, Noah Misch wrote:

The timing was not great, but this is fixing a purported defect in an
older
v16 feature.  If the MAINTAIN privilege is actually fine, we're all
set for
v16.  If MAINTAIN does have a material problem that $SUBJECT had
fixed, we
should either revert MAINTAIN, un-revert $SUBJECT, or fix the problem
a
different way.

Someone with the MAINTAIN privilege on a table can use search_path
tricks against the table owner, if the code is susceptible, because
maintenance code runs with the privileges of the table owner.

I was concerned enough to bring it up on the -security list, and then
to -hackers followed by a commit (too late). But perhaps that was
paranoia: the practical risk is probably quite low, because a user with
the MAINTAIN privilege is likely to be highly trusted.

I'd like to hear from others on the topic about the relative risks of
shipping with/without the search_path changes.

I don't think a full revert of the MAINTAIN privilege is the right
thing -- the predefined role is very valuable and many other predefined
roles are much more dangerous than pg_maintain is.

Regards,
Jeff Davis

On Mon, Jun 12, 2023 at 5:40 PM Jeff Davis <pgsql@j-davis.com> wrote:

On Mon, 2023-06-12 at 13:05 -0400, Noah Misch wrote:

The timing was not great, but this is fixing a purported defect in an
older
v16 feature. If the MAINTAIN privilege is actually fine, we're all
set for
v16. If MAINTAIN does have a material problem that $SUBJECT had
fixed, we
should either revert MAINTAIN, un-revert $SUBJECT, or fix the problem
a
different way.

Someone with the MAINTAIN privilege on a table can use search_path
tricks against the table owner, if the code is susceptible, because
maintenance code runs with the privileges of the table owner.

Only change the search_path if someone other than the table owner or
superuser is running the command (which should only be possible via the new
MAINTAIN privilege)?

David J.

On Monday, June 12, 2023, David G. Johnston <david.g.johnston@gmail.com>
wrote:

On Mon, Jun 12, 2023 at 5:40 PM Jeff Davis <pgsql@j-davis.com> wrote:

On Mon, 2023-06-12 at 13:05 -0400, Noah Misch wrote:

The timing was not great, but this is fixing a purported defect in an
older
v16 feature. If the MAINTAIN privilege is actually fine, we're all
set for
v16. If MAINTAIN does have a material problem that $SUBJECT had
fixed, we
should either revert MAINTAIN, un-revert $SUBJECT, or fix the problem
a
different way.

Someone with the MAINTAIN privilege on a table can use search_path
tricks against the table owner, if the code is susceptible, because
maintenance code runs with the privileges of the table owner.

Only change the search_path if someone other than the table owner or
superuser is running the command (which should only be possible via the new
MAINTAIN privilege)?

On a related note, are we OK with someone using this privilege setting
their own default_statistics_target?

https://www.postgresql.org/docs/current/runtime-config-query.html#GUC-DEFAULT-STATISTICS-TARGET

My prior attempt to open up analyze had brought this up as a reason to
avoid having someone besides the table owner allowed to analyze the table.

David J.

On Mon, Jun 12, 2023 at 8:20 PM Jeff Davis <pgsql@j-davis.com> wrote:

I followed the rules here for "Writing SECURITY DEFINER Functions
Safely":

https://www.postgresql.org/docs/16/sql-createfunction.html

which suggests adding pg_temp at the end (otherwise it is searched
first by default).

Interesting. The issue of "what is a safe search path?" is more
nuanced than I would prefer. :-(

--
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Jun 12, 2023 at 05:39:40PM -0700, Jeff Davis wrote:

On Mon, 2023-06-12 at 13:05 -0400, Noah Misch wrote:

The timing was not great, but this is fixing a purported defect in an
older
v16 feature.� If the MAINTAIN privilege is actually fine, we're all
set for
v16.� If MAINTAIN does have a material problem that $SUBJECT had
fixed, we
should either revert MAINTAIN, un-revert $SUBJECT, or fix the problem
a
different way.

Someone with the MAINTAIN privilege on a table can use search_path
tricks against the table owner, if the code is susceptible, because
maintenance code runs with the privileges of the table owner.

I was concerned enough to bring it up on the -security list, and then
to -hackers followed by a commit (too late). But perhaps that was
paranoia: the practical risk is probably quite low, because a user with
the MAINTAIN privilege is likely to be highly trusted.

I'd like to hear from others on the topic about the relative risks of
shipping with/without the search_path changes.

I find shipping with the search_path change ($SUBJECT) to be lower risk
overall, though both are fairly low-risk. Expect no new errors in non-FULL
VACUUM, which doesn't run the relevant kinds of code. Tables not ready for
the search_path change in ANALYZE already cause errors in Autovacuum ANALYZE
and have since 2018-02 (CVE-2018-1058). Hence, $SUBJECT poses less
compatibility risk than the CVE-2018-1058 fix.

Best argument for shipping without $SUBJECT: we already have REFERENCES and
TRIGGER privilege that tend to let the grantee hijack the table owner's
account. Adding MAINTAIN to the list, while sad, is defensible. I still
prefer to ship with $SUBJECT, not without.

On Mon, 2023-06-12 at 17:50 -0700, David G. Johnston wrote:

Only change the search_path if someone other than the table owner or
superuser is running the command (which should only be possible via
the new MAINTAIN privilege)?

That sounds like a reasonable compromise, but a bit messy. If we do it
this way, is there hope to clean things up a bit in the future? These
special cases are quite difficult to document in a comprehensible way.

If others like this approach I'm fine with it.

Regards,
Jeff Davis

On Tue, 2023-06-13 at 11:24 -0400, Robert Haas wrote:

Interesting. The issue of "what is a safe search path?" is more
nuanced than I would prefer. :-(

As far as I can tell, search_path was designed as a convenience for
interactive queries, where a lot of these nuances make sense. But they
don't make sense as defaults for code inside functions, in my opinion.

Regards,
Jeff Davis

On Mon, 2023-06-12 at 18:31 -0700, David G. Johnston wrote:

On a related note, are we OK with someone using this privilege
setting their own default_statistics_target?

https://www.postgresql.org/docs/current/runtime-config-query.html#GUC-DEFAULT-STATISTICS-TARGET

My prior attempt to open up analyze had brought this up as a reason
to avoid having someone besides the table owner allowed to analyze
the table.

Thank you for bringing it up. I don't see a major concern there, but
please link to the prior discussion so I can see the objections.

Regards,
Jeff Davis

On Tue, Jun 13, 2023 at 12:54 PM Jeff Davis <pgsql@j-davis.com> wrote:

On Mon, 2023-06-12 at 18:31 -0700, David G. Johnston wrote:

On a related note, are we OK with someone using this privilege
setting their own default_statistics_target?

https://www.postgresql.org/docs/current/runtime-config-query.html#GUC-DEFAULT-STATISTICS-TARGET

My prior attempt to open up analyze had brought this up as a reason
to avoid having someone besides the table owner allowed to analyze
the table.

Thank you for bringing it up. I don't see a major concern there, but
please link to the prior discussion so I can see the objections.

This is the specific (first?) message I am recalling.

/messages/by-id/A737B7A37273E048B164557ADEF4A58B53803F5A@ntex2010i.host.magwien.gv.at

David J.

Noah Misch <noah@leadboat.com> writes:

Best argument for shipping without $SUBJECT: we already have REFERENCES and
TRIGGER privilege that tend to let the grantee hijack the table owner's
account. Adding MAINTAIN to the list, while sad, is defensible. I still
prefer to ship with $SUBJECT, not without.

What I'm concerned about is making such a fundamental semantics change
post-beta1. It'll basically invalidate any application compatibility
testing anybody might have done against beta1. I think this ship has
sailed as far as v16 is concerned, although we could reconsider it
in v17.

Also, I fail to see any connection to the MAINTAIN privilege: the
committed-and-reverted patch would break things whether the user
was making any use of that privilege or not. Thus, I do not accept
the idea that we're fixing something that's new in 16.

regards, tom lane

On Tue, 2023-06-13 at 13:22 -0700, David G. Johnston wrote:

This is the specific (first?) message I am recalling.

/messages/by-id/A737B7A37273E048B164557ADEF4A58B53803F5A@ntex2010i.host.magwien.gv.at

The most objection seems to be expressed most succinctly in this
message:

/messages/by-id/16134.1456767564@sss.pgh.pa.us

"if we allow non-owners to run ANALYZE, they'd be able to mess things
up by setting the stats target either much lower or much higher than
the table owner expected"

I have trouble seeing much of a problem here if there is an explicit
MAINTAIN privilege. If you grant someone MAINTAIN to someone, it's not
surprising that you need to coordinate maintenance-related settings
with that user; and if you don't, then it's not surprising that the
statistics could get messed up.

Perhaps the objections in that thread were because the proposal
involved inferring the privilege to ANALYZE from other privileges,
rather than having an explicit MAINTAIN privilege?

Regards,
Jeff Davis

On Tue, Jun 13, 2023 at 1:55 PM Jeff Davis <pgsql@j-davis.com> wrote:

Perhaps the objections in that thread were because the proposal
involved inferring the privilege to ANALYZE from other privileges,
rather than having an explicit MAINTAIN privilege?

That is true; but it seems worth being explicit whether we expect the user
to only be able to run "ANALYZE" using defaults (like auto-analyze would
do) or if this additional capability is assumed to be part of the grant. I
do imagine you'd want to be able to set the statistic target in order to do
vacuum --analyze-in-stages with a non-superuser.

David J.