Horton

Re: Fix search_path for all maintenance commands

Jeff Davis <pgsql@j-davis.com>

View thread
From: Jeff Davis <pgsql@j-davis.com>
To: Nathan Bossart <nathandbossart@gmail.com>, Noah Misch <noah@leadboat.com>
Cc: "David G. Johnston" <david.g.johnston@gmail.com>, Gurjeet Singh <gurjeet@singh.im>, pgsql-hackers@postgresql.org, Robert Haas <robertmhaas@gmail.com>, Greg Stark <stark@mit.edu>, Tom Lane <tgl@sss.pgh.pa.us>
Date: 2023-07-21T22:32:43Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix search_path to a safe value during maintenance operations.

  2. Make relation-enumerating operations be security-restricted operations.

Attachments

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