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(at)j-davis(dot)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(at)j-davis(dot)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(at)leadboat(dot)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(at)j-davis(dot)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(dot)g(dot)johnston(at)gmail(dot)com>
wrote:

> On Mon, Jun 12, 2023 at 5:40 PM Jeff Davis <pgsql(at)j-davis(dot)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(at)j-davis(dot)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(at)j-davis(dot)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.

https://www.postgresql.org/message-id/A737B7A37273E048B164557ADEF4A58B53803F5A%40ntex2010i.host.magwien.gv.at

David J.

Noah Misch <noah(at)leadboat(dot)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

On Tue, Jun 13, 2023 at 1:55 PM Jeff Davis <pgsql(at)j-davis(dot)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(at)j-davis(dot)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(at)j-davis(dot)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

[ emerges from hibernation ]

On Mon, Jun 19, 2023 at 6:58 PM Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> 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?

Yeah. I mean, as things stand, it seems like giving someone the
MAINTAIN privilege will be sufficient to allow them to escalate to the
table owner if there are any expression indexes involved. That seems
like a real problem. We shouldn't ship a new feature with a built-in
security hole like that.

I was pretty outraged when I realized that we'd been shipping releases
for years where CREATEROLE let you grab superuser because you could
just grant yourself pg_execute_server_program and then go to town.
Ideally, that hazard should have been identified and fixed in some way
before introducing pg_execute_server_program. I don't know whether the
hazard wasn't realized at the time or whether somebody somehow
convinced themselves that that was OK, but it clearly isn't.

Now we're proposing to ship a brand-new feature with a hole that we
definitely already know exists. I can't understand that at all. Should
we just go file the CVE against ourselves right now, then? Seriously,
what are we doing?

If we're not going to fix the feature so that it doesn't break the
security model, we should probably just revert it. I don't understand
at all the idea of shipping something that we 100% know is broken.

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

That's clearly better than nothing, but also seems like it's pretty
clearly the wrong approach. If somebody electrocutes themselves on the
toaster in the break room, you don't stick a sign on the side of it
that says "this toaster will electrocute you if you try to use it" and
then call it good. You either fix or replace the toaster, or at the
very least throw it out, or at the VERY least unplug it. I am failing
to understand how this situation is any different.

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

On 2023-06-29 Th 11:19, Robert Haas wrote:
>
> Now we're proposing to ship a brand-new feature with a hole that we
> definitely already know exists. I can't understand that at all. Should
> we just go file the CVE against ourselves right now, then? Seriously,
> what are we doing?
>
> If we're not going to fix the feature so that it doesn't break the
> security model, we should probably just revert it. I don't understand
> at all the idea of shipping something that we 100% know is broken.
>
>

+1

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

On Thu, Jun 29, 2023 at 11:19:38AM -0400, Robert Haas wrote:
> [ emerges from hibernation ]

Welcome back.

> If we're not going to fix the feature so that it doesn't break the
> security model, we should probably just revert it. I don't understand
> at all the idea of shipping something that we 100% know is broken.

Given Jeff's commit followed the precedent set by the fix for
CVE-2018-1058, I'm inclined to think he was on the right track. Perhaps a
more targeted fix, such as only changing search_path when the command is
not run by the table owner (as suggested upthread [0]) is worth
considering.

[0] https://postgr.es/m/CAKFQuwaVJkM9u%2BqpOaom2UkPE1sz0BASF-E5amxWPxncUhm4Hw%40mail.gmail.com

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, 2023-06-29 at 11:19 -0400, Robert Haas wrote:
> Yeah. I mean, as things stand, it seems like giving someone the
> MAINTAIN privilege will be sufficient to allow them to escalate to
> the
> table owner if there are any expression indexes involved. That seems
> like a real problem. We shouldn't ship a new feature with a built-in
> security hole like that.

Let's take David's suggestion[1] then, and only restrict the search
path for those without owner privileges on the object.

That would mean no behavior change unless using the MAINTAIN privilege,
which is new, so no breakage. And if someone is using the MAINTAIN
privilege, they wouldn't be able to abuse the search_path, so it would
close the hole.

Patch attached (created a bit quickly, but seems to work).

Regards,
Jeff Davis

[1]
https://postgr.es/m/CAKFQuwaVJkM9u%2BqpOaom2UkPE1sz0BASF-E5amxWPxncUhm4Hw%40mail.gmail.com

Jeff Davis <pgsql(at)j-davis(dot)com> writes:
> On Thu, 2023-06-29 at 11:19 -0400, Robert Haas wrote:
>> We shouldn't ship a new feature with a built-in
>> security hole like that.

> Let's take David's suggestion[1] then, and only restrict the search
> path for those without owner privileges on the object.

I think that's a seriously awful kluge. It will mean that things behave
differently for the owner than for MAINTAIN grantees, which pretty much
destroys the use-case for that privilege, as well as being very confusing
and hard to debug. Yes, *if* you're careful about search path cleanliness
then you can make it work, but that will be a foot-gun forevermore.

(I'm also less than convinced that this is sufficient to remove all
security hazards. One pretty obvious question is do we really want
superusers to be treated as owners, rather than MAINTAIN grantees,
for this purpose.)

I'm leaning to Robert's thought that we need to revert this for now,
and think harder about how to make it work cleanly and safely.

regards, tom lane

On Thu, Jun 29, 2023 at 08:53:56PM -0400, Tom Lane wrote:
> I'm leaning to Robert's thought that we need to revert this for now,
> and think harder about how to make it work cleanly and safely.

Since it sounds like this is headed towards a revert, here's a patch for
removing MAINTAIN and pg_maintain.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, 2023-06-29 at 20:53 -0400, Tom Lane wrote:
> I think that's a seriously awful kluge.  It will mean that things
> behave
> differently for the owner than for MAINTAIN grantees, which pretty
> much
> destroys the use-case for that privilege, as well as being very
> confusing
> and hard to debug.

In version 15, try this:

CREATE USER foo;
CREATE SCHEMA foo AUTHORIZATION foo;
CREATE USER bar;
CREATE SCHEMA bar AUTHORIZATION bar;
\c - foo
CREATE FUNCTION foo.mod10(INT) RETURNS INT IMMUTABLE
LANGUAGE plpgsql AS $$ BEGIN RETURN mod($1,10); END; $$;
CREATE TABLE t(i INT);
-- units digit must be unique
CREATE UNIQUE INDEX t_idx ON t (foo.mod10(i));
INSERT INTO t VALUES(7); -- success
INSERT INTO t VALUES(17); -- fails
GRANT USAGE ON SCHEMA foo TO bar;
GRANT INSERT ON t TO bar;
\c - bar
CREATE FUNCTION bar.mod(INT, INT) RETURNS INT IMMUTABLE
LANGUAGE plpgsql AS $$ BEGIN RETURN $1 + 1000000; END; $$;
SET search_path = bar, pg_catalog;
INSERT INTO foo.t VALUES(7); -- succeeds
\c - foo
SELECT * FROM t;
i
---
7
7
(2 rows)

I'm not sure that everyone in this thread realizes just how broken it
is to depend on search_path in a functional index at all. And doubly so
if it depends on a schema other than pg_catalog in the search_path.

Let's also not forget that logical replication always uses
search_path=pg_catalog, so if you depend on a different search_path for
any function attached to the table (not just functional indexes, also
functions inside expressions or trigger functions), then those are
already broken in version 15. And if a superuser is executing
maintenance commands, there's little reason to think they'll have the
same search path as the user that created the table.

At some point in the very near future (though I realize that point may
come after version 16), we need to lock down the search path in a lot
of cases (not just maintenance commands), and I don't see any way
around that.

Regards,
Jeff Davis

On Thu, Jun 29, 2023 at 10:09:21PM -0700, Nathan Bossart wrote:
> On Thu, Jun 29, 2023 at 08:53:56PM -0400, Tom Lane wrote:
>> I'm leaning to Robert's thought that we need to revert this for now,
>> and think harder about how to make it work cleanly and safely.
>
> Since it sounds like this is headed towards a revert, here's a patch for
> removing MAINTAIN and pg_maintain.

I will revert this next week unless opinions change before then. I'm
currently planning to revert on both master and REL_16_STABLE, but another
option would be to keep it on master while we sort out the remaining
issues. Thoughts?

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Fri, Jun 30, 2023 at 11:35:46AM -0700, Nathan Bossart wrote:
> On Thu, Jun 29, 2023 at 10:09:21PM -0700, Nathan Bossart wrote:
> > On Thu, Jun 29, 2023 at 08:53:56PM -0400, Tom Lane wrote:
> >> I'm leaning to Robert's thought that we need to revert this for now,
> >> and think harder about how to make it work cleanly and safely.

Another dimension of compromise could be to make MAINTAIN affect fewer
commands in v16. Un-revert the part of commit 05e1737 affecting just the
commands it still affects. For example, limit MAINTAIN and the 05e1737
behavior change to VACUUM, ANALYZE, and REINDEX. Don't worry about VACUUM or
ANALYZE failing under commit 05e1737, since they would have been failing under
autovacuum since 2018. A problem index expression breaks both autoanalyze and
REINDEX, hence the inclusion of REINDEX. The already-failing argument doesn't
apply to CLUSTER or REFRESH MATERIALIZED VIEW, so those commands could join
MAINTAIN in v17.

> > Since it sounds like this is headed towards a revert, here's a patch for
> > removing MAINTAIN and pg_maintain.
>
> I will revert this next week unless opinions change before then. I'm
> currently planning to revert on both master and REL_16_STABLE, but another
> option would be to keep it on master while we sort out the remaining
> issues. Thoughts?

From my reading of the objections, I think they're saying that commit 05e1737
arrived too late and that MAINTAIN is unacceptable without commit 05e1737. I
think you'd conform to all objections by pushing the revert to v16 and pushing
a roll-forward of commit 05e1737 to master.

On Sun, Jul 02, 2023 at 08:57:31PM -0700, Noah Misch wrote:
> Another dimension of compromise could be to make MAINTAIN affect fewer
> commands in v16. Un-revert the part of commit 05e1737 affecting just the
> commands it still affects. For example, limit MAINTAIN and the 05e1737
> behavior change to VACUUM, ANALYZE, and REINDEX. Don't worry about VACUUM or
> ANALYZE failing under commit 05e1737, since they would have been failing under
> autovacuum since 2018. A problem index expression breaks both autoanalyze and
> REINDEX, hence the inclusion of REINDEX. The already-failing argument doesn't
> apply to CLUSTER or REFRESH MATERIALIZED VIEW, so those commands could join
> MAINTAIN in v17.

I'm open to compromise if others are, but I'm skeptical that folks will be
okay with too much fancy footwork this late in the game.

Anyway, IMO your argument could extend to CLUSTER and REFRESH, too. If
we're willing to change behavior under the assumption that autovacuum
would've been failing since 2018, then why wouldn't we be willing to change
it everywhere? I suppose someone could have been manually vacuuming with a
special search_path for 5 years to avoid needing to schema-qualify their
index expressions (and would then be surprised that CLUSTER/REFRESH no
longer work), but limiting MAINTAIN to VACUUM, etc. would still break their
use-case, right?

> From my reading of the objections, I think they're saying that commit 05e1737
> arrived too late and that MAINTAIN is unacceptable without commit 05e1737. I
> think you'd conform to all objections by pushing the revert to v16 and pushing
> a roll-forward of commit 05e1737 to master.

Okay, I'll adjust my plans accordingly.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Mon, Jul 03, 2023 at 11:19:14AM -0700, Nathan Bossart wrote:
> On Sun, Jul 02, 2023 at 08:57:31PM -0700, Noah Misch wrote:
> > Another dimension of compromise could be to make MAINTAIN affect fewer
> > commands in v16. Un-revert the part of commit 05e1737 affecting just the
> > commands it still affects. For example, limit MAINTAIN and the 05e1737
> > behavior change to VACUUM, ANALYZE, and REINDEX. Don't worry about VACUUM or
> > ANALYZE failing under commit 05e1737, since they would have been failing under
> > autovacuum since 2018. A problem index expression breaks both autoanalyze and
> > REINDEX, hence the inclusion of REINDEX. The already-failing argument doesn't
> > apply to CLUSTER or REFRESH MATERIALIZED VIEW, so those commands could join
> > MAINTAIN in v17.
>
> I'm open to compromise if others are, but I'm skeptical that folks will be
> okay with too much fancy footwork this late in the game.

Got it.

> Anyway, IMO your argument could extend to CLUSTER and REFRESH, too. If
> we're willing to change behavior under the assumption that autovacuum
> would've been failing since 2018, then why wouldn't we be willing to change
> it everywhere? I suppose someone could have been manually vacuuming with a
> special search_path for 5 years to avoid needing to schema-qualify their
> index expressions (and would then be surprised that CLUSTER/REFRESH no
> longer work), but limiting MAINTAIN to VACUUM, etc. would still break their
> use-case, right?

Yes, limiting MAINTAIN to VACUUM would still break a site that has used manual
VACUUM to work around associated loss of autovacuum. I'm not sympathetic to a
user who neglected to benefit from the last five years of prep time on this
issue as it affects VACUUM and ANALYZE. REFRESH runs more than index
expressions, e.g. function calls in the targetlist of the materialized view
query. Those targetlist expressions haven't been putting ERRORs in the log
during autovacuum, so REFRESH hasn't had the sort of advance warning that
VACUUM and ANALYZE got.

On Thu, 2023-06-29 at 22:09 -0700, Nathan Bossart wrote:
> On Thu, Jun 29, 2023 at 08:53:56PM -0400, Tom Lane wrote:
> > I'm leaning to Robert's thought that we need to revert this for
> > now,
> > and think harder about how to make it work cleanly and safely.
>
> Since it sounds like this is headed towards a revert, here's a patch
> for
> removing MAINTAIN and pg_maintain.

It was difficult to review standalone, so I tried a quick version
myself and ended up with very similar results. The only substantial
difference was that I put back:

+ if (!vacuum_is_relation_owner(relid, classForm,
options))
+ continue;

in get_all_vacuum_rels() whereas your patch left it out -- double-check
that we're doing the right thing there.

Also remember to bump the catversion. Other than that, it looks good to
me.

Regards,
Jeff Davis

On Thu, Jul 06, 2023 at 12:55:14AM -0700, Jeff Davis wrote:
> It was difficult to review standalone, so I tried a quick version
> myself and ended up with very similar results.

Thanks for taking a look.

> The only substantial
> difference was that I put back:
>
>
> + if (!vacuum_is_relation_owner(relid, classForm,
> options))
> + continue;
>
>
> in get_all_vacuum_rels() whereas your patch left it out -- double-check
> that we're doing the right thing there.

The privilege check was moved in d46a979, which I think still makes sense,
so I left it there. That might be why it looks like I removed it.

> Also remember to bump the catversion. Other than that, it looks good to
> me.

Will do.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Here is a new version of the patch that I think is ready for commit (except
it still needs a catversion bump). The only real difference from v1 is in
AdjustUpgrade.pm. From my cross-version pg_upgrade testing, I believe we
can remove the other "privilege-set discrepancy" rule as well.

Since MAINTAIN will no longer exist in v16, we'll also need the following
change applied to v17devel:

diff --git a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
index 843f65b448..d435812c06 100644
--- a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
+++ b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
@@ -274,7 +274,7 @@ sub adjust_old_dumpfile
$dump = _mash_view_qualifiers($dump);
}

- if ($old_version >= 14 && $old_version < 16)
+ if ($old_version >= 14 && $old_version < 17)
{
# Fix up some privilege-set discrepancies.
$dump =~

On Thu, Jul 06, 2023 at 10:20:04AM -0700, Nathan Bossart wrote:
> On Thu, Jul 06, 2023 at 12:55:14AM -0700, Jeff Davis wrote:
>> Also remember to bump the catversion. Other than that, it looks good to
>> me.
>
> Will do.

Since we are only reverting from v16, the REL_16_STABLE catversion will be
bumped ahead of the one on master. AFAICT that is okay, but there is also
a chance that someone bumps the catversion on master to the same value.
I'm not sure if this is problem is worth worrying about, but I thought I'd
raise it just in case. I could bump the catversion on master to the
following value to help prevent this scenario, but I'm not wild about
adding unnecessary catversion bumps.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, 2023-07-06 at 22:14 -0700, Nathan Bossart wrote:
> Since we are only reverting from v16, the REL_16_STABLE catversion
> will be
> bumped ahead of the one on master.

I don't object to you doing it this way, but FWIW, I'd just revert in
both branches to avoid this kind of weirdness.

Also I'm not quite sure how quickly my search_path fix will be
committed. Hopefully soon, because the current state is not great, but
it's hard for me to say for sure.

Regards,
Jeff Davis

On Fri, Jul 07, 2023 at 09:22:22AM -0700, Jeff Davis wrote:
> On Thu, 2023-07-06 at 22:14 -0700, Nathan Bossart wrote:
>> Since we are only reverting from v16, the REL_16_STABLE catversion
>> will be
>> bumped ahead of the one on master.
>
> I don't object to you doing it this way, but FWIW, I'd just revert in
> both branches to avoid this kind of weirdness.
>
> Also I'm not quite sure how quickly my search_path fix will be
> committed. Hopefully soon, because the current state is not great, but
> it's hard for me to say for sure.

Yeah, I guess I should just revert it in both. Given your fix will
hopefully be committed soon, I was hoping to avoid reverting and
un-reverting in quick succession to prevent affecting git-blame too much.

I found an example of a post-beta2 revert on both master and a stable
branch where Tom set the catversions to different values (20b6847,
e256312). I'll do the same here.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Fri, Jul 07, 2023 at 09:57:10AM -0700, Nathan Bossart wrote:
> Yeah, I guess I should just revert it in both. Given your fix will
> hopefully be committed soon, I was hoping to avoid reverting and
> un-reverting in quick succession to prevent affecting git-blame too much.
>
> I found an example of a post-beta2 revert on both master and a stable
> branch where Tom set the catversions to different values (20b6847,
> e256312). I'll do the same here.

reverted

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Fri, Jun 30, 2023 at 3:41 AM Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> I'm not sure that everyone in this thread realizes just how broken it
> is to depend on search_path in a functional index at all. And doubly so
> if it depends on a schema other than pg_catalog in the search_path.
>
> Let's also not forget that logical replication always uses
> search_path=pg_catalog, so if you depend on a different search_path for
> any function attached to the table (not just functional indexes, also
> functions inside expressions or trigger functions), then those are
> already broken in version 15. And if a superuser is executing
> maintenance commands, there's little reason to think they'll have the
> same search path as the user that created the table.
>
> At some point in the very near future (though I realize that point may
> come after version 16), we need to lock down the search path in a lot
> of cases (not just maintenance commands), and I don't see any way
> around that.

I agree. I think there are actually two interrelated problems here.

One is that virtually all code needs to run with the originally
intended search_path rather than some search_path chosen at another
time and maybe by a different user. If not, it's going to break, or
compromise security, depending on the situation. The other is that
running arbitrary code written by somebody else as yourself is
basically instant death, from a security perspective.

It's a little hard to imagine a world in which these problems don't
exist at all, but it somehow feels like the design of the system
pushes you toward doing this stuff incorrectly rather than doing it
correctly. For instance, you can imagine a system where when you run
CREATE OR REPLACE FUNCTION, the prevailing search_path is captured and
automatically included in proconfig. Then the default behavior would
be to run functions and procedures with the search_path that was in
effect when they were created, rather than what we actually have,
where it's the one in effect at execution time as it is currently.

It's a little harder to imagine something similar around all the user
switching behavior, just because we have so many ways of triggering
arbitrary code execution: views, triggers, event triggers, expression
indexes, constraints, etc. But you can maybe imagine a system where
all code associated with a table is run as the table owner in all
cases, regardless of SECURITY INVOKER/DEFINER, which I think would at
least close some holes.

The difficulty is that it's a bit hard to imagine making these kinds
of definitional changes now, because they'd probably be breaking
changes for pretty significant numbers of users. But on the other
hand, if we don't start thinking about systemic changes here, it feels
like we're just playing whack-a-mole.

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

On 7/31/23 12:53, Robert Haas wrote:
> On Fri, Jun 30, 2023 at 3:41 AM Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
>> I'm not sure that everyone in this thread realizes just how broken it
>> is to depend on search_path in a functional index at all. And doubly so
>> if it depends on a schema other than pg_catalog in the search_path.
>>
>> Let's also not forget that logical replication always uses
>> search_path=pg_catalog, so if you depend on a different search_path for
>> any function attached to the table (not just functional indexes, also
>> functions inside expressions or trigger functions), then those are
>> already broken in version 15. And if a superuser is executing
>> maintenance commands, there's little reason to think they'll have the
>> same search path as the user that created the table.
>>
>> At some point in the very near future (though I realize that point may
>> come after version 16), we need to lock down the search path in a lot
>> of cases (not just maintenance commands), and I don't see any way
>> around that.
>
> I agree. I think there are actually two interrelated problems here.
>
> One is that virtually all code needs to run with the originally
> intended search_path rather than some search_path chosen at another
> time and maybe by a different user. If not, it's going to break, or
> compromise security, depending on the situation. The other is that
> running arbitrary code written by somebody else as yourself is
> basically instant death, from a security perspective.

I agree too.

But the analysis of the issue needs to go one step further. Even if the
search_path does not change from the originally intended one, a newly
created function can shadow the intended one based on argument coercion
rules.

--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Mon, Jul 31, 2023 at 1:18 PM Joe Conway <mail(at)joeconway(dot)com> wrote:
> But the analysis of the issue needs to go one step further. Even if the
> search_path does not change from the originally intended one, a newly
> created function can shadow the intended one based on argument coercion
> rules.

Yeah, this is a complicated issue. As the system works today, if you
include in your search_path a schema to which some other user can
write, you are pretty much agreeing to execute code provided by that
user. If that user has strictly greater privileges than you, e.g. they
are the super-user, then that's fine, because they can compromise your
account anyway, but otherwise, you're probably doomed. Not only can
they try to capture references with similarly-named objects, they can
also do things like create objects whose names are common
mis-spellings of the objects that are supposed to be there and hope
you access the wrong one by mistake. Maybe there are other attacks as
well, but even if not, I think it's already a pretty hopeless
situation. I think the UNIX equivalent would be including a directory
in your PATH that is world-writable and hoping your account will stay
secure. Not very likely.

We have already taken an important step in terms of preventing this
attack in commit b073c3ccd06e4cb845e121387a43faa8c68a7b62, which
removed PUBLIC CREATE from the public schema. Before that, we were
shipping a configuration analogous to a UNIX system where /usr/bin was
world-writable -- something which no actual UNIX system has likely
done any time in the last 40 years, because it's so clearly insane. We
could maybe go a step further by changing the default search_path to
not even include public, to further discourage people from using that
as a catch-all where everybody can just dump their objects. Right now,
anybody can revert that change with a single GRANT statement, and if
we removed public from the default search_path as well, people would
have one extra step to restore that insecure configuration. I don't
know such a change is worthwhile, though. It would still be super-easy
for users to create insecure configurations: as soon as user A can
write a schema and user B has it in the search_path, B is in a lot of
trouble if A turns out to be malicious.

One thing we might be able to do to prevent that sort of thing is to
have a feature to prevent "accidental" code execution, as in the
"function trust" mechanism proposed previously. Say I trust all users
who can SET ROLE to me and/or who inherit my privileges. Additionally
I can decide to trust users who do neither of those things by some
sort of explicit declaration. If I don't trust a user then if I do
anything that would cause code supplied by that user to get executed,
it just errors out:

ERROR: role "rhaas" should not execute arbitrary code provided by role "jconway"
HINT: If this should be allowed, use the TRUST command to permit it.

Even if we do this, I still think we need the kinds of fixes that I
mentioned earlier. An error like this is fine if you're trying to do
something to a table owned by another user and they've got a surprise
trigger on there or something, but a maintenance command like VACUUM
should find a way to work that is both secure and non-astonishing. And
we probably also still need to find ways to control search_path in a
lot more widely than we do today. Otherwise, even if stuff is
technically secure, it may just not work.

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

On Mon, 2023-07-31 at 16:06 -0400, Robert Haas wrote:
> if you
> include in your search_path a schema to which some other user can
> write, you are pretty much agreeing to execute code provided by that
> user.

Agreed on all counts here. I don't think it's reasonable for us to try
to make such a setup secure, and I don't think users have much need for
such a setup anyway.

> One thing we might be able to do to prevent that sort of thing is to
> have a feature to prevent "accidental" code execution, as in the
> "function trust" mechanism proposed previously. Say I trust all users
> who can SET ROLE to me and/or who inherit my privileges. Additionally
> I can decide to trust users who do neither of those things by some
> sort of explicit declaration. If I don't trust a user then if I do
> anything that would cause code supplied by that user to get executed,
> it just errors out:
>
> ERROR: role "rhaas" should not execute arbitrary code provided by
> role "jconway"
> HINT: If this should be allowed, use the TRUST command to permit it.

+1, though I'm not sure we need an extensive trust mechanism beyond
what we already have with the SET ROLE privilege.

> And
> we probably also still need to find ways to control search_path in a
> lot more widely than we do today. Otherwise, even if stuff is
> technically secure, it may just not work.

+1.

Regards,
Jeff Davis

On Mon, 2023-07-31 at 12:53 -0400, Robert Haas wrote:
> I agree. I think there are actually two interrelated problems here.
>
> One is that virtually all code needs to run with the originally
> intended search_path rather than some search_path chosen at another
> time and maybe by a different user. If not, it's going to break, or
> compromise security, depending on the situation. The other is that
> running arbitrary code written by somebody else as yourself is
> basically instant death, from a security perspective.

Good framing.

The search_path is a particularly nasty problem in our system because
it means that users can't even trust the code that they write
themselves! A function author has no way to know how their own function
will behave under a different search_path.

> It's a little hard to imagine a world in which these problems don't
> exist at all, but it somehow feels like the design of the system
> pushes you toward doing this stuff incorrectly rather than doing it
> correctly. For instance, you can imagine a system where when you run
> CREATE OR REPLACE FUNCTION, the prevailing search_path is captured
> and
> automatically included in proconfig.

Capturing the environment is not ideal either, in my opinion. It makes
it easy to carelessly depend on a schema that others might not have
USAGE privileges on, which would then create a runtime problem for
other callers. Also, I don't think we could just depend on the raw
search_path, we'd need to do some processing for $user, and there are
probably a few other annoyances.

It's one possibility and we don't have a lot of great options, so I
don't want to rule it out though. If nothing else it could be a
transition path to something better.

> But you can maybe imagine a system where
> all code associated with a table is run as the table owner in all
> cases, regardless of SECURITY INVOKER/DEFINER, which I think would at
> least close some holes.
>
> The difficulty is that it's a bit hard to imagine making these kinds
> of definitional changes now, because they'd probably be breaking
> changes for pretty significant numbers of users.

I believe we can get close to a good model with minimal breakage. And
when we make the problem small enough I believe other solutions will
emerge. We will probably have to hedge with some compatibility GUCs.

> But on the other
> hand, if we don't start thinking about systemic changes here, it
> feels
> like we're just playing whack-a-mole.

Exactly. If we can agree on where we're going then I think we can get
there.

Regards,
Jeff Davis

On Mon, 2023-07-31 at 13:17 -0400, Joe Conway wrote:
> But the analysis of the issue needs to go one step further. Even if
> the
> search_path does not change from the originally intended one, a newly
> created function can shadow the intended one based on argument
> coercion
> rules.

There are quite a few issues going down this path:

* The set of objects in each schema can change. Argument coercion is a
particularly subtle one, but there are other ways that it could find
the wrong object. The temp namespace also has some subtle issues.

* Schema USAGE privileges may vary over time or from caller to caller,
affecting which items in the search path are searched at all. The same
goes if theres an object access hook in place.

* $user should be resolved to a specific schema (or perhaps not in some
cases?)

* There are other GUCs and environment that can affect function
behavior. Is it worth trying to lock those down?

I agree that each of these is some potential problem, but these are
much smaller problems than allowing the caller to have complete control
over the search_path.

Regards,
Jeff Davis

On Mon, Jul 31, 2023 at 5:15 PM Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> > ERROR: role "rhaas" should not execute arbitrary code provided by
> > role "jconway"
> > HINT: If this should be allowed, use the TRUST command to permit it.
>
> +1, though I'm not sure we need an extensive trust mechanism beyond
> what we already have with the SET ROLE privilege.

FWIW, I think it would be a good idea. It might not be absolutely
mandatory but I think it would be smart.

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

On Mon, Jul 31, 2023 at 6:10 PM Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> Capturing the environment is not ideal either, in my opinion. It makes
> it easy to carelessly depend on a schema that others might not have
> USAGE privileges on, which would then create a runtime problem for
> other callers. Also, I don't think we could just depend on the raw
> search_path, we'd need to do some processing for $user, and there are
> probably a few other annoyances.
>
> It's one possibility and we don't have a lot of great options, so I
> don't want to rule it out though. If nothing else it could be a
> transition path to something better.

Here is my thought about this. Right now, we basically do one of two
things. In some cases, we parse statements when they're submitted, and
then store the resulting node trees. In such cases, references are
fixed: the statements will always refer to the objects to which they
referred previously. In functions and procedures, except for the new
BEGIN ATOMIC stuff, we just store the statements as a string and they
get parsed at execution time. Then, the problem arises of statements
being possibly parsed in an environment that differs from the original
one. It can differ either by search_path being different so that we
look in different schemas, or, as you point out here, if the contents
of the schemas themselves have been modified.

I think that a lot of people would like it if we moved more in the
direction of parsing statements at object definition time. Possibly
because EDB deals with a lot of people coming from Oracle, I've heard
a lot of complaints about the PG behavior. However, there are some
fairly significant problems with that idea. First, it would break some
use cases, such as creating a temporary table and then running DML
commands on it, or more generally any use case where a function or
procedure might need to reference objects that don't exist at time of
definition. Second, while we have clear separation of parsing and
execution for queries, the same is not true for DDL commands; it's not
the case, I believe, that you can parse an arbitrary DDL command such
that all object references are resolved, and then later execute it.
We'd need to change a bunch of code to get there. Third, we'd have to
deal with dependency loops: right now, because functions and
procedures don't parse their bodies at definition time, they also
don't depend on the objects that they are going to end up accessing,
which means that a function or procedure can be restored by pg_dump
without worrying about whether those objects exist yet. That would
have to change, and that would mean creating dependency loops for
pg_dump, which we'd have to then find a way to break. I'm not trying
to say that any of these problems are intractable, but I do think
changing stuff like this would be quite a bit of work -- and that's
assuming the user impact was judged to be acceptable, which I'm not at
all sure that it would be. We'd certainly need to provide some
workaround for people who want to do stuff like create and use
temporary tables inside of a function or procedure.

Now, if we don't go in the direction of resolving everything at parse
time, then I think capturing search_path is probably the next best
thing, or at least the next best thing that I've thought up so far. It
doesn't hold constant the meaning of the code to the same degree that
parsing at definition time would do, but it gets us closer to that
than the status quo. Crucially, if the user is using a secure
search_path, then any changes to the meaning of the code that captures
that search_path will have to be made by that user or someone with a
superset of their privileges, which is a lot less serious than what
you get when there's no search_path setting at all, where the *caller*
can change the meaning of the called code. That is not, however, to
say that this idea is really good enough. To be honest, I think it's a
bit of a kludge, and dropping a kludge on top of our entire user base
and maybe also breaking a lot of things is not particularly where I
want to be. I just don't have an idea I like better at the moment.

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

On Tue, Aug 1, 2023 at 10:42 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> Now, if we don't go in the direction of resolving everything at parse
> time, then I think capturing search_path is probably the next best
> thing, or at least the next best thing that I've thought up so far.

I'd much rather strongly encourage the user to write a safe and
self-sufficient function definition. Specifically, if there is no
search_path attached to the function then the search path that will be in
place will be temp + pg_catalog only. Though I wonder whether it would be
advantageous to allow a function to have a temporary schema separate from
the session-scoped one...

They can use ALTER FUNCTION and the existing "FROM CURRENT" specification
to get back to current behavior if desired.

Going further, I could envision an "explicit" mode that both performs a
parse-time check for object existence and optionally reports an error if
the lookup happened via an inexact match - forcing lots more type casts to
occur (we'd probably need to invent something to say "must match the
anyelement function signature") but ensuring at parse time you've correctly
identified everything you intend to be using. Sure, the meanings of those
things could change but the surface is much smaller than plugging a new
function that matches earlier in the lookup resolution process.

David J.

On Tue, 2023-08-01 at 11:16 -0700, David G. Johnston wrote:
> They can use ALTER FUNCTION and the existing "FROM CURRENT"
> specification to get back to current behavior if desired.

The current behavior is that the search_path comes from the environment
each execution. FROM CURRENT saves the search_path at definition time
and uses that each execution.

Regards,
Jeff Davis

On Tue, Aug 1, 2023 at 2:38 PM Jeff Davis <pgsql(at)j-davis(dot)com> wrote:

> On Tue, 2023-08-01 at 11:16 -0700, David G. Johnston wrote:
> > They can use ALTER FUNCTION and the existing "FROM CURRENT"
> > specification to get back to current behavior if desired.
>
> The current behavior is that the search_path comes from the environment
> each execution. FROM CURRENT saves the search_path at definition time
> and uses that each execution.
>
>
Right...I apparently misread "create" as "the" in "when CREATE FUNCTION is
executed".

The overall point stands, it just requires defining a similar "FROM
SESSION" to allow for explicitly specifying the current default (missing)
behavior.

David J.

On Tue, 2023-08-01 at 13:41 -0400, Robert Haas wrote:
> In functions and procedures, except for the new
> BEGIN ATOMIC stuff, we just store the statements as a string and they
> get parsed at execution time.

...

> I think that a lot of people would like it if we moved more in the
> direction of parsing statements at object definition time.

Do you mean that we'd introduce some BEGIN ATOMIC version of plpgsql
(and other trusted languages)?

> However, there are some
> fairly significant problems with that idea.

To satisfy intended use cases of things like default expressions, CHECK
constraints, index expressions, etc., there is no need to call
functions that would be subject to the problems you list.

One problem is that functions are too general a concept -- we have no
idea whether the user is trying to do something simple or trying to do
something "interesting".

> Now, if we don't go in the direction of resolving everything at parse
> time,

It would be useful to pursue this approach, but I don't think it will
be enough. We still need to solve our search_path problems.

> then I think capturing search_path is probably the next best
> thing,

+0.5.

> To be honest, I think it's
> a
> bit of a kludge, and dropping a kludge on top of our entire user base
> and maybe also breaking a lot of things is not particularly where I
> want to be. I just don't have an idea I like better at the moment.

We will also be fixing things for a lot of users who just haven't run
into a problem *yet* (or perhaps did, and just don't know it).

Regards,
Jeff Davis

On Tue, 2023-08-01 at 14:47 -0700, David G. Johnston wrote:
> The overall point stands, it just requires defining a similar "FROM
> SESSION" to allow for explicitly specifying the current default
> (missing) behavior.

That sounds useful as a way to future-proof function definitions that
intend to use the session search_path.

It seems like we're moving in the direction of search_path defaulting
to FROM CURRENT (probably with a long road of compatibility GUCs to
minimize breakage...) and everything else defaulting to FROM SESSION
(as before)?

Regards,
Jeff Davis