Re: Fix search_path for all maintenance commands

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