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