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