Обсуждение: pgsql: Fix search_path to a safe value during maintenance operations.

Fix search_path to a safe value during maintenance operations.

While executing maintenance operations (ANALYZE, CLUSTER, REFRESH
MATERIALIZED VIEW, REINDEX, or VACUUM), set search_path to
'pg_catalog, pg_temp' to prevent inconsistent behavior.

Functions that are used for functional indexes, in index expressions,
or in materialized views and depend on a different search path must 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.

Discussion: https://postgr.es/m/e44327179e5c9015c8dda67351c04da552066017.camel%40j-davis.com
Reviewed-by: Greg Stark
Reviewed-by: Nathan Bossart

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/05e17373517114167d002494e004fa0aa32d1fd1

Modified Files
--------------
contrib/amcheck/verify_nbtree.c                             |  2 ++
src/backend/access/brin/brin.c                              |  2 ++
src/backend/catalog/index.c                                 |  8 ++++++++
src/backend/commands/analyze.c                              |  2 ++
src/backend/commands/cluster.c                              |  2 ++
src/backend/commands/indexcmds.c                            |  6 ++++++
src/backend/commands/matview.c                              |  2 ++
src/backend/commands/vacuum.c                               |  2 ++
src/bin/scripts/t/100_vacuumdb.pl                           |  4 ----
src/include/utils/guc.h                                     |  6 ++++++
src/test/modules/test_oat_hooks/expected/test_oat_hooks.out |  4 ++++
src/test/regress/expected/privileges.out                    | 12 ++++++------
src/test/regress/expected/vacuum.out                        |  2 +-
src/test/regress/sql/privileges.sql                         |  8 ++++----
src/test/regress/sql/vacuum.sql                             |  2 +-
15 files changed, 48 insertions(+), 16 deletions(-)

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)?

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

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.
>
> https://www.postgresql.org/message-id/A737B7A37273E048B164557ADEF4A58B53803F5A%40ntex2010i.host.magwien.gv.at

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

https://www.postgresql.org/message-id/16134.1456767564%40sss.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

Jeff Davis <pgsql@j-davis.com> writes:
> The most objection seems to be expressed most succinctly in this
> message:
> https://www.postgresql.org/message-id/16134.1456767564%40sss.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.

I agree that granting MAINTAIN implies that you trust the grantee
not to mess up your stats.

> 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?

Exactly.

            regards, tom lane

On Tue, 2023-06-13 at 16:23 -0400, Tom Lane wrote:
> What I'm concerned about is making such a fundamental semantics
> change
> post-beta1.

I have added the patch to the July CF for v17.

If someone does feel like something should be done for v16, David G.
Johnston posted one possibility here:

https://www.postgresql.org/message-id/CAKFQuwaVJkM9u+qpOaom2UkPE1sz0BASF-E5amxWPxncUhm4Hw@mail.gmail.com

But as Noah pointed out, there are other privileges that can be abused,
so a workaround for 16 might not be important if we have a likely fix
for MAINTAIN coming in 17.

Regards,
    Jeff Davis

On Thu, Jun 15, 2023 at 12:59 AM Jeff Davis <pgsql@j-davis.com> wrote:
> On Tue, 2023-06-13 at 16:23 -0400, Tom Lane wrote:
> > What I'm concerned about is making such a fundamental semantics
> > change
> > post-beta1.
>
> I have added the patch to the July CF for v17.
>
> If someone does feel like something should be done for v16, David G.
> Johnston posted one possibility here:
>
> https://www.postgresql.org/message-id/CAKFQuwaVJkM9u+qpOaom2UkPE1sz0BASF-E5amxWPxncUhm4Hw@mail.gmail.com
>
> But as Noah pointed out, there are other privileges that can be abused,
> so a workaround for 16 might not be important if we have a likely fix
> for MAINTAIN coming in 17.

Rather than is_superuser(userid) || userid == ownerid, I think that
the test should be has_privs_of_role(userid, ownerid).

I'm inclined to think that this is a real security issue and am not
very sanguine about waiting another year to fix it, but at the same
time, I'm somewhat worried that the proposed fix might be too narrow
or wrongly-shaped. I'm not too convinced that we've properly
understood what all of the problems in this area are. :-(

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

On Mon, 2023-06-19 at 16:03 -0400, Robert Haas wrote:
> I'm inclined to think that this is a real security issue and am not

Can you expand on that a bit? You mean a practical security issue for
the intended use cases?

> very sanguine about waiting another year to fix it, but at the same
> time, I'm somewhat worried that the proposed fix might be too narrow
> or wrongly-shaped. I'm not too convinced that we've properly
> understood what all of the problems in this area are. :-(

Would it be acceptable to document that the MAINTAIN privilege (along
with TRIGGER and, if I understand correctly, REFERENCES) carries
privilege escalation risk for the grantor?

Regards,
    Jeff Davis