Thread: 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
Attachment
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
Re: pgsql: Fix search_path to a safe value during maintenance operations.
From
"David G. Johnston"
Date:
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.
Re: pgsql: Fix search_path to a safe value during maintenance operations.
From
"David G. Johnston"
Date:
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?
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
Re: pgsql: Fix search_path to a safe value during maintenance operations.
From
"David G. Johnston"
Date:
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.
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. > > 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
Re: pgsql: Fix search_path to a safe value during maintenance operations.
From
"David G. Johnston"
Date:
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.
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