Fix search_path for all maintenance commands

On Fri, 26 May 2023 at 19:22, Jeff Davis <pgsql@j-davis.com> wrote:

Maintenance commands (ANALYZE, CLUSTER, REFRESH MATERIALIZED VIEW,
REINDEX, and VACUUM) currently run as the table owner, and as a
SECURITY_RESTRICTED_OPERATION.

I propose that we also fix the search_path to "pg_catalog, pg_temp"
when running maintenance commands, for two reasons:

1. Make the behavior of maintenance commands more consistent because
they'd always have the same search_path.

What exactly would this impact? Offhand... expression indexes where
the functions in the expression (which would already be schema
qualified) themselves reference other objects without schema
qualification?

So this would negatively affect someone who was using such a dangerous
function definition but was careful to always use the same search_path
on it. Perhaps someone who had created an expression index on their
own table in their own schema calling their own functions in their own
schema. As long as nobody else ever calls it that would work but this
would cause superuser to no longer be able to reindex it even if
superuser set the same search_path?

I guess that's pretty narrow and a reasonable thing to desupport.
Users could just mark those functions with search_path or schema
qualify the object references in them. Perhaps we should also be
picking up cases like that sooner so users realize they've created a
footgun for themselves?

--
greg

On Thu, Jun 08, 2023 at 06:08:08PM -0400, Greg Stark wrote:

I guess that's pretty narrow and a reasonable thing to desupport.
Users could just mark those functions with search_path or schema
qualify the object references in them. Perhaps we should also be
picking up cases like that sooner so users realize they've created a
footgun for themselves?

I'm inclined to agree that this is reasonable to desupport. Relying on the
search_path for the cases Greg describes already seems rather fragile, so
I'm skeptical that forcing a safe one for maintenance commands would make
things significantly worse. At least, it sounds like the right trade-off
based on Jeff's note about privilege escalation risks.

I bet we could skip forcing the search_path for maintenance commands run as
the table owner, but such a discrepancy seems likely to cause far more
confusion than anything else.

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

On Thu, 2023-06-08 at 21:55 -0700, Nathan Bossart wrote:

On Thu, Jun 08, 2023 at 06:08:08PM -0400, Greg Stark wrote:

I guess that's pretty narrow and a reasonable thing to desupport.
Users could just mark those functions with search_path or schema
qualify the object references in them. Perhaps we should also be
picking up cases like that sooner so users realize they've created
a
footgun for themselves?

Many cases will be picked up, for instance CREATE INDEX will error if
the safe search path is not good enough.

I'm inclined to agree that this is reasonable to desupport.

Committed.

I bet we could skip forcing the search_path for maintenance commands
run as
the table owner, but such a discrepancy seems likely to cause far
more
confusion than anything else.

Agreed.

Regards,
Jeff Davis

On Fri, Jun 9, 2023 at 2:00 PM Jeff Davis <pgsql@j-davis.com> wrote:

On Thu, 2023-06-08 at 21:55 -0700, Nathan Bossart wrote:

On Thu, Jun 08, 2023 at 06:08:08PM -0400, Greg Stark wrote:

I guess that's pretty narrow and a reasonable thing to desupport.
Users could just mark those functions with search_path or schema
qualify the object references in them. Perhaps we should also be
picking up cases like that sooner so users realize they've created
a
footgun for themselves?

Many cases will be picked up, for instance CREATE INDEX will error if
the safe search path is not good enough.

I'm inclined to agree that this is reasonable to desupport.

Committed.

I'm not sure if mine is a valid concern, and it has been a long time
since I looked at the search_path's and switching Role's implications
(CVE-2009-4136) so pardon my ignorance.

It feels a bit late in release cycle to introduce this breaking
change. If they depended on search_path, people and utilities that use
these maintenance commands may see failures. Although I can't think of
a scenario where such a failure may cause an outage, sometimes these
maintenance operations are performed while the users are staring down
the barrel of a gun (imminent danger of running out of space, bad
statistics causing absurd query plans, etc.). So, if not directly, a
failure of these operations may indirectly cause an outage.

I feel more thought needs to be given to the impact of this change,
and we should to give others more time for feedback.

Short of that, it'd be prudent to allow the user to somehow fall back
to old behaviour; a command-line option, or GUC, etc. That way we can
mark the old behaviour "deprecated", with a workaround for those who
may desperately need it, and in another release or so, finally pull
the plug on old behaviour.

My 2bits.

Best regards,
Gurjeet
http://Gurje.et

On Fri, 2023-06-09 at 16:29 -0700, Gurjeet Singh wrote:

I'm not sure if mine is a valid concern, and it has been a long time
since I looked at the search_path's and switching Role's implications
(CVE-2009-4136) so pardon my ignorance.

It feels a bit late in release cycle to introduce this breaking
change.

That's a valid concern. It just needs to be weighed against the
potential problems of running maintenance code with different search
paths, and the interaction with the new MAINTAIN privilege.

I feel more thought needs to be given to the impact of this change,
and we should to give others more time for feedback.

For context, I initially posted to the -security list in case it needed
to be addressed there, and got some feedback on the patch before
posting to -hackers two weeks ago. So it has been seen by at least 4
people.

But I'm happy to hear more input and I'll backtrack if necessary.

Here are my thoughts:

Lazy VACUUM is by far the most important for the overall system. It's
unaffected by this change; see comment in vacuum_rel():

/*
* Switch to the table owner's userid, so that any index functions
are run
* as that user. Also lock down security-restricted operations and
* arrange to make GUC variable changes local to this command. (This
is
* unnecessary, but harmless, for lazy VACUUM.)
*/

REINDEX, CLUSTER, and VACUUM FULL are potentially affected because of
index functions, but only if the index functions are quite broken (or
at least very fragile) already.

REFRESH MATERIALIZED VIEW is the most likely to be affected because it
is more likely to call "interesting" functions and the author may not
anticipate a different search path.

A normal dump/reload cycle for upgrade testing will catch these
problems because it will create indexes after loading the data
(DefineIndex sets the search path), and it will also call REFRESH
MATERIALIZED VIEW. If using pg_upgrade instead, a post-upgrade ANALYZE
will catch index function problems, but I suppose not MV problems.

So there is some risk to this change. It feels fairly narrow to me, but
non-zero. Perhaps we can do more?

Short of that, it'd be prudent to allow the user to somehow fall back
to old behaviour; a command-line option, or GUC, etc. That way we can
mark the old behaviour "deprecated", with a workaround for those who
may desperately need it, and in another release or so, finally pull
the plug on old behaviour.

That sounds wise, though others may not like the idea of a GUC just for
this change.

Regards,
Jeff Davis

On Fri, 2023-05-26 at 16:21 -0700, Jeff Davis wrote:

Maintenance commands (ANALYZE, CLUSTER, REFRESH MATERIALIZED VIEW,
REINDEX, and VACUUM) currently run as the table owner, and as a
SECURITY_RESTRICTED_OPERATION.

I propose that we also fix the search_path to "pg_catalog, pg_temp"
when running maintenance commands

New patch attached.

We need this patch for several reasons:

* If you have a functional index, and the function depends on the
search_path, then it's easy to corrupt your index if you (or a
superuser) run a REINDE/CLUSTER with the wrong search_path.

* The MAINTAIN privilege needs a safe search_path, and was reverted
from 16 because the search_path in 16 is not restricted.

* In general, it's a good idea for things like functional indexes and
materialized views to be independent of the search_path.

* The search_path is already restricted in some other contexts, like
logical replication and autoanalyze.

Others have raised some concerns though:

* It might break for users who have a functional index where the
function implicitly depends on a search_path containing a namespace
other than pg_catalog. My opinion is that such functional indexes are
conceptually broken and we need to desupport them, and there will be
some breakage, but I'm open to suggestion about how we minimize that (a
compatibility GUC or something?).

* The fix might not go far enough or might be in the wrong place. I'm
open to suggestion here, too. Maybe we can make it part of the general
function call mechanism, and can be overridden by explicitly setting
the function search path? Or maybe we need new syntax where the
function can acquire the search path from the session explicitly, but
uses a safe search path by default?

--
Jeff Davis
PostgreSQL Contributor Team - AWS

On Thu, 6 Jul 2023 at 21:39, Jeff Davis <pgsql@j-davis.com> wrote:

I apologize in advance if anything I’ve written below is either too obvious
or too crazy or misinformed to belong here. I hope I have something to say
that is on point, but feel unsure what makes sense to say.

* It might break for users who have a functional index where the

function implicitly depends on a search_path containing a namespace
other than pg_catalog. My opinion is that such functional indexes are
conceptually broken and we need to desupport them, and there will be
some breakage, but I'm open to suggestion about how we minimize that (a
compatibility GUC or something?).

I agree this is OK. If somebody has an index whole meaning depends on the
search_path, then the best that can be said is that their database hasn't
been corrupted yet. At the same time, I can see that somebody would get
upset if they couldn't upgrade their database because of this. Maybe
pg_upgrade could apply "SET search_path TO pg_catalog, pg_temp" to any
function used in a functional index that doesn't have a search_path setting
of its own? (BEGIN ATOMIC functions count, if I understand correctly, as
having a search_path setting, because the lookups happen at definition time)

Now I'm doing more reading and I'm worried about SET TIME ZONE (or more
precisely, its absence) and maybe some other ones.

* The fix might not go far enough or might be in the wrong place. I'm

open to suggestion here, too. Maybe we can make it part of the general
function call mechanism, and can be overridden by explicitly setting
the function search path? Or maybe we need new syntax where the
function can acquire the search path from the session explicitly, but
uses a safe search path by default?

Change it so by default each function gets handled as if "SET search_path
FROM CURRENT" was applied to it? That's what I do for all my functions
(maybe hurting performance?). Expand on my pg_upgrade idea above by
applying it to all functions?

I feel that this may tie into other behaviour issues where to me it is
obvious that the expected behaviour should be different from the actual
behaviour. If a view calls a function, shouldn't it be called in the
context of the view's definer/owner? It's weird that I can write a view
that filters a table for users of the view, but as soon as the view calls
functions they run in the security context of the user of the view. Are
views security definers or not? Similar comment for triggers. Also as far
as I can tell there is no way for a security definer function to determine
who (which user) invoked it. So I can grant/deny access to run a particular
function using permissions, but I can't have the supposed security definer
define security for different callers.

Is the fundamental problem that we now find ourselves wanting to do things
that require different defaults to work smoothly? On some level I suspect
we want lexical scoping, which is what most of us have in our programming
languages, in the database; but the database has many elements of dynamic
scoping, and changing that is both a compatibility break and requires
significant changes in the way the database is designed.

Hi,

On Thu, 2023-07-06 at 23:22 -0400, Isaac Morland wrote:

Maybe pg_upgrade could apply "SET search_path TO pg_catalog, pg_temp"
to any function used in a functional index that doesn't have a
search_path setting of its own?

I don't think we want to go down the road of trying to solve this at
upgrade time.

Now I'm doing more reading and I'm worried about SET TIME ZONE (or
more precisely, its absence) and maybe some other ones.

That's a good point that it's not limited to search_path, but
search_path is by far the biggest problem.

For one thing, functions affected by TimeZone or other GUCs are
typically marked STABLE, and can't be used in index expressions. Also,
search_path affects a lot more functions.
 

Change it so by default each function gets handled as if "SET
search_path FROM CURRENT" was applied to it?

Yes, that's one idea, along with some syntax to get the old behavior
(inherit search_path at runtime) if you want.

It feels weird to make search_path too special in the syntax though. If
we want a general solution, we could do something like:

CREATE FUNCTION ...
[DEPENDS ON CONFIGURATION {NONE|{some_guc}[, ...]}]
[CONFIGURATION IS {STATIC|DYNAMIC}]

where STATIC means "all of the GUC dependencies are SET FROM CURRENT
unless specified otherwise" and DYNAMIC means "all of the GUC
dependencies come from the session at runtime unless specified
otherwise".

The default would be "DEPENDS CONFIGURATION search_path CONFIGURATION
IS STATIC".

That would make search_path special only because, by default, every
function would depend on it. Which I think summarizes the reason
search_path really is special.

That also opens up opportunities to do other things we might want to
do:

* have a compatibility GUC to set the default back to DYNAMIC
* track other dependencies of functions better ("DEPENDS ON TABLE
...")
* provide better error messages, like "can't use function xyz in
index expression because it depends on configuration parameter foo"
* be more consistent about using STABLE to mean that the function
depends on a snapshot, rather than overloading it for GUC dependencies

The question is, given that the acute problem is search_path, do we
want to invent all of the syntax above? Are there other use cases for
it, or should we just focus on search_path?

That's what I do for all my functions (maybe hurting performance?).

It doesn't look cheap, although I think we could optimize it.

If a view calls a function, shouldn't it be called in the context of
the view's definer/owner?

Yeah, there are a bunch of problems along those lines. I don't know if
we can solve them all in one release.

Is the fundamental problem that we now find ourselves wanting to do
things that require different defaults to work smoothly? On some
level I suspect we want lexical scoping, which is what most of us
have in our programming languages, in the database; but the database
has many elements of dynamic scoping, and changing that is both a
compatibility break and requires significant changes in the way the
database is designed.

Does that suggest another approach?

Regards,
Jeff Davis

On Thu, 2023-07-06 at 18:39 -0700, Jeff Davis wrote:

* The fix might not go far enough or might be in the wrong place. I'm
open to suggestion here, too. Maybe we can make it part of the
general
function call mechanism, and can be overridden by explicitly setting
the function search path? Or maybe we need new syntax where the
function can acquire the search path from the session explicitly, but
uses a safe search path by default?

I'm inclined to proceed with the current approach here, which is to
just fix search_path for maintenance commands. Other approaches may be
possible, but:

(a) We already special-case the way functions are executed for
maintenance commands in other ways -- we run the functions as the table
owner (even SECURITY INVOKER functions) and run them as a
SECURITY_RESTRICTED_OPERATION. Setting the search_path to a safe value
seems like a natural extension of that; and

(b) The lack of a safe search path is blocking other useful features,
such as the MAINTAIN privilege; and

(c) I don't see other proposals, aside from a few ideas I put forward
here[1]/messages/by-id/6781cc79580c464a63fc0a1343637ed2b2b0cf09.camel@j-davis.com, which didn't get any responses.

The current approach seemed to get support from Noah, Nathan, and Greg
Stark.

Concerns were raised by Gurjeet, Tom, and Robert in the 16 cycle; but
I'm not sure whether those objections were specific to the 16 cycle or
whether they are objections to the approach entirely. Comments?

Regards,
Jeff Davis

[1]: /messages/by-id/6781cc79580c464a63fc0a1343637ed2b2b0cf09.camel@j-davis.com
/messages/by-id/6781cc79580c464a63fc0a1343637ed2b2b0cf09.camel@j-davis.com

On Thu, Jul 13, 2023 at 11:56 AM Jeff Davis <pgsql@j-davis.com> wrote:

The current approach seemed to get support from Noah, Nathan, and Greg
Stark.

Concerns were raised by Gurjeet, Tom, and Robert in the 16 cycle; but

I didn't see Tom's or Robert's concerns raised in this thread. I see
now that for some reason there are two threads with slightly different
subjects. I'll catch-up on that, as well.

The other thread's subject: "pgsql: Fix search_path to a safe value
during maintenance operations"

I'm not sure whether those objections were specific to the 16 cycle or
whether they are objections to the approach entirely. Comments?

The approach seems good to me. My concern is with this change's
potential to cause an extended database outage. Hence sending it out
as part of v16, without any escape hatch for the DBA, is my objection.

It it were some commercial database, we would have simply introduced a
hidden event, or a GUC, with default off. But a GUC for this feels too
heavy handed.

Perhaps we can provide an escape hatch as follows (warning, pseudocode ahead).

if (first_element(search_path) != "dont_override_search_patch_for_maintenance")
SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, ...);

So, if someone desperately wants to just plow through the maintenance
command, and are not ready or able to fix their dependence on their
search_path, the community can show them this escape-hatch.

Best regards,
Gurjeet
http://Gurje.et

On Thu, Jul 13, 2023 at 12:54 PM Gurjeet Singh <gurjeet@singh.im> wrote:

The approach seems good to me. My concern is with this change's
potential to cause an extended database outage. Hence sending it out
as part of v16, without any escape hatch for the DBA, is my objection.

If this is limited to MAINT, which I'm in support of, there is no need for
an "escape hatch". A prerequisite for leveraging the new feature is that
you fix the code so it conforms to the new way of doing things.

Tom's opinion was a general dislike for differing behavior in different
situations. I dislike it too, on purist grounds, but would rather do this
than not make any forward progress because we made a poor decision in the
past. And I'm against simply breaking the past without any recourse as what
we did for pg_dump/pg_restore still bothers me.

David J.

On Thu, Jul 13, 2023 at 1:37 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

I'm against simply breaking the past without any recourse as what we did for pg_dump/pg_restore still bothers me.

I'm sure this is tangential, but can you please provide some
context/links to the change you're referring to here.

Best regards,
Gurjeet
http://Gurje.et

On Thu, Jul 13, 2023 at 2:00 PM Gurjeet Singh <gurjeet@singh.im> wrote:

On Thu, Jul 13, 2023 at 1:37 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

I'm against simply breaking the past without any recourse as what we

did for pg_dump/pg_restore still bothers me.

I'm sure this is tangential, but can you please provide some
context/links to the change you're referring to here.

Here is the instigating issue and a discussion thread on the aftermath:

https://wiki.postgresql.org/wiki/A_Guide_to_CVE-2018-1058%3A_Protect_Your_Search_Path

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

David J.

On Thu, 2023-07-13 at 13:37 -0700, David G. Johnston wrote:

If this is limited to MAINT, which I'm in support of, there is no
need for an "escape hatch".  A prerequisite for leveraging the new
feature is that you fix the code so it conforms to the new way of
doing things.

The current patch is not limited to exercise of the MAINTAIN privilege.

Tom's opinion was a general dislike for differing behavior in
different situations.  I dislike it too, on purist grounds, but would
rather do this than not make any forward progress because we made a
poor decision in the past.

I believe the opinion you're referring to is here:

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

Which was a reaction to another version of my patch which implemented
your idea to limit the changes to the MAINTAIN privilege.

I agree with you that we should be practical here. The end goal is to
move users away from using functions that both (a) implicitly depend on
the search_path; and (b) are called implicitly as a side-effect of
accessing a table. Whatever is the fastest and smoothest way to get
there is fine with me.

And I'm against simply breaking the past without any recourse as what
we did for pg_dump/pg_restore still bothers me.

Is a GUC the solution here? Gurjeet called it heavy-handed, and I see
the point that carrying such a GUC forever would be unpleasant. But if
it reduces the risk of breakage (or at least offers an escape hatch)
then it may be wise, and hopefully we can remove it later.

Regards,
Jeff Davis

On Thu, Jul 13, 2023 at 02:07:27PM -0700, David G. Johnston wrote:

On Thu, Jul 13, 2023 at 2:00 PM Gurjeet Singh <gurjeet@singh.im> wrote:

On Thu, Jul 13, 2023 at 1:37 PM David G. Johnston <david.g.johnston@gmail.com> wrote:

I'm against simply breaking the past without any recourse as what we

did for pg_dump/pg_restore still bothers me.

I'm sure this is tangential, but can you please provide some
context/links to the change you're referring to here.

Here is the instigating issue and a discussion thread on the aftermath:
https://wiki.postgresql.org/wiki/A_Guide_to_CVE-2018-1058%3A_Protect_Your_Search_Path
/messages/by-id/13033.1531517020@sss.pgh.pa.us

I don't blame you for feeling bothered about it. A benefit of having done it
is that we gained insight into the level of pain it caused. If it had been
sufficiently painful, someone would have quickly added an escape hatch. Five
years later, nobody has added one.

The 2018 security fixes instigated many function repairs that $SUBJECT would
otherwise instigate. That wasn't too painful. The net new pain of $SUBJECT
will be less, since the 2018 security fixes prepared the path. Hence, I
remain +1 for the latest Davis proposal.

On Sat, Jul 15, 2023 at 02:13:33PM -0700, Noah Misch wrote:

The 2018 security fixes instigated many function repairs that $SUBJECT would
otherwise instigate. That wasn't too painful. The net new pain of $SUBJECT
will be less, since the 2018 security fixes prepared the path. Hence, I
remain +1 for the latest Davis proposal.

I concur.

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

On Mon, 2023-07-17 at 10:58 -0700, Nathan Bossart wrote:

On Sat, Jul 15, 2023 at 02:13:33PM -0700, Noah Misch wrote:

The 2018 security fixes instigated many function repairs that
$SUBJECT would
otherwise instigate.  That wasn't too painful.  The net new pain of
$SUBJECT
will be less, since the 2018 security fixes prepared the path. 
Hence, I
remain +1 for the latest Davis proposal.

I concur.

Based on feedback, I plan to commit soon.

Tom's objection seemed specific to v16, and Robert's concern seemed to
be about having the MAINTAIN privilege without this patch. If I missed
any objections to this patch, please let me know.

If we hear about breakage that suggests we need an escape hatch GUC, we
have time to add one later.

I remain open to considering more complete fixes for the search_path
problems.

Regards,
Jeff Davis

On Mon, 2023-07-17 at 12:16 -0700, Jeff Davis wrote:

Based on feedback, I plan to commit soon.

Attached is a new version.

Changes:

* Also switch the search_path during CREATE MATERIALIZED VIEW, so that
it's consistent with REFRESH. As a part of this change, I slightly
reordered things in ExecCreateTableAs() so that the skipData path
returns early without entering the SECURITY_RESTRICTED_OPERATION. I
don't think that's a problem because (a) that is one place where
SECURITY_RESTRICTED_OPERATION is not used for security, but rather for
consistency; and (b) that path doesn't go through rewriter, planner, or
executor anyway so I don't see why it would matter.

* Use GUC_ACTION_SAVE rather than GUC_ACTION_SET. That was a problem
with the previous patch for index functions executed in parallel
workers, which can happen calling SQL functions from pg_amcheck.

* I used a wrapper function RestrictSearchPath() rather than calling
set_config_option() directly. That provides a nice place in case we
need to add a compatibility GUC to disable it.

Question:

Why do we switch to the table owner and use
SECURITY_RESTRICTED_OPERATION in DefineIndex(), when we will switch in
index_build (etc.) anyway? Similarly, why do we switch in vacuum_rel(),
when it doesn't matter for lazy vacuum and we will switch in
cluster_rel() and do_analyze_rel() anyway?

For now, I left the extra calls to RestrictSearchPath() in for
consistency with the switches to the table owner.

Regards,
Jeff Davis

On Fri, Jul 21, 2023 at 03:32:43PM -0700, Jeff Davis wrote:

Why do we switch to the table owner and use
SECURITY_RESTRICTED_OPERATION in DefineIndex(), when we will switch in
index_build (etc.) anyway?

Commit a117ceb added that, and it added some test cases that behaved
differently without that.

Similarly, why do we switch in vacuum_rel(),
when it doesn't matter for lazy vacuum and we will switch in
cluster_rel() and do_analyze_rel() anyway?

It conforms to the "as soon as possible after locking the relation" coding
rule that commit a117ceb wrote into miscinit.c. That provides future
proofing.