Date: Wed, 28 Feb 2024 10:55:23 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: Jeff Davis <pgsql@j-davis.com>
Cc: pgsql-hackers@postgresql.org, Joe Conway <mail@joeconway.com>,
	Robert Haas <robertmhaas@gmail.com>
Subject: Re: MAINTAIN privilege -- what do we need to un-revert it?
Message-ID: <20240228165523.GA2435539@nathanxps13>
References: <d4ccaf3658cb3c281ec88c851a09733cd9482f22.camel@j-davis.com>
 <20240214190226.GA373350@nathanxps13>
 <4d497d82e4516037dc1082d5a62d199ea00d62e7.camel@j-davis.com>
 <20240223213055.GA1805646@nathanxps13>
 <196d1c86d43a147e777bb74159774f255dbe73e3.camel@j-davis.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <196d1c86d43a147e777bb74159774f255dbe73e3.camel@j-davis.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20240228165523.GA2435539%40nathanxps13>
Precedence: bulk

On Tue, Feb 27, 2024 at 04:22:34PM -0800, Jeff Davis wrote:
> Do we need a documentation update here? If so, where would be a good
> place?

I'm afraid I don't have a better idea than adding a short note in each
affected commands's page.

> On Fri, 2024-02-23 at 15:30 -0600, Nathan Bossart wrote:
>> I wonder if it's worth using PGC_S_INTERACTIVE or introducing a new
>> value
>> for these.
> 
> Did you have a particular concern about PGC_S_SESSION?

My only concern is that it could obscure the source of the search_path
change, which in turn might cause confusion when things fail.

> If it's less than PGC_S_SESSION, it won't work, because the caller's
> SET command will override it, and the same manipulation is possible.
> 
> And I don't think we want it higher than PGC_S_SESSION, otherwise the
> function can't set its own search_path, if needed.

Yeah, we would have to make it equivalent in priority to PGC_S_SESSION,
which would likely require a bunch of special logic.  I don't know if this
is worth it, and this seems like something that could pretty easily be
added in the future if it became necessary.

>> > +#define GUC_SAFE_SEARCH_PATH "pg_catalog, pg_temp"
>> 
>> Is including pg_temp actually safe?  I worry that a user could use
>> their
>> temporary schema to inject objects that would take the place of
>> non-schema-qualified stuff in functions.
> 
> pg_temp cannot (currently) be excluded. If it is omitted from the
> string, it will be placed *first* in the search_path, which is more
> dangerous.
> 
> pg_temp does not take part in function or operator resolution, which
> makes it safer than it first appears. There are potentially some risks
> around tables, but it's not typical to access a table in a function
> called as part of an index expression.
> 
> If we determine that pg_temp is actually unsafe to include, we need to
> do something like what I proposed here:
> 
> https://www.postgresql.org/message-id/a6865db287596c9c6ea12bdd9de87216cb5e7902.camel@j-davis.com

I don't doubt anything you've said, but I can't help but think that we
might as well handle the pg_temp risk, too.

Furthermore, I see that we use "" as a safe search_path for autovacuum and
fe_utils/connect.h.  Is there any way to unite these?  IIUC it might be
possible to combine the autovacuum and maintenance command cases (i.e.,
"!pg_temp"), but we might need to keep pg_temp for the frontend case.  I
think it's worth trying to add comments about why this setting is safe for
some cases but not others, too.

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