Re: CREATE FUNCTION ... SEARCH { DEFAULT | SYSTEM | SESSION }

Hi,

On 2023-08-14 12:25:30 -0700, Jeff Davis wrote:
> On Sat, 2023-08-12 at 11:25 -0700, Andres Freund wrote:
> >
> > I'm not sure that anything based, directly or indirectly, on
> > search_path
> > really is a realistic way to get there.
>
> Can you explain a little more? I see what you mean generally, that
> search_path is an imprecise thing, and that it leaves room for
> ambiguity and mistakes.

It just doesn't seem to provide enough control and it's really painful for
users to manage. If you install a bunch of extensions into public - very very
common from what I have seen - you can't really remove public from the search
path. Which then basically makes all approaches of resolving any of the
security issues via search path pretty toothless.

> > I think that'd be pretty painful from a UX perspective. Having to
> > write
> > e.g. operators as operator(schema, op) just sucks as an experience.
>
> I'm not suggesting that the user fully-qualify everything; I'm
> suggesting that the include a "SET search_path" clause if they depend
> on anything other than pg_catalog.

I don't think that really works in practice, due to the very common practice
of installing extensions into the same schema as the application. Then that
schema needs to be in search path (if you don't want to schema qualify
everything), which leaves you wide open.

> > And with
> > extensions plenty of operators will live outside of pg_catalog, so
> > there is
> > plenty things that will need qualifying.
>
> In my proposal, that would still involve a "SET search_path TO
> myextension, pg_catalog, pg_temp".

myextension is typically public. Which means that there's zero protection due
to such a search path.

> >   And because of things like type
> > coercion search, which prefers "bettering fitting" coercions over
> > search path
> > order, you can't just put "less important" things later in search
> > path.
>
> I understand this introduces some ambiguity, but you just can't include
> schemas in the search_path that you don't trust, for similar reasons as
> $PATH. If you have a few objects you'd like to access in another user's
> schema, fully-qualify them.

I think the more common attack paths are things like tricking extension
scripts into evaluating arbitrary code, to gain "real superuser" privileges.

> > We can't just store the oids at the time, because that'd end up very
> > fragile -
> > tables/functions/... might be dropped and recreated etc and thus
> > change their
> > oid.
>
> Robert suggested something along those lines[1]. I won't rule it out,
> but I agree that there are quite a few things left to figure out.
>
> > But we could change the core PLs to rewrite all the queries (*) so
> > that
> > they schema qualify absolutely everything, including operators and
> > implicit
> > type casts.
>
> So not quite like "SET search_path FROM CURRENT": you resolve it to a
> specific "schemaname.objectname", but stop just short of resolving to a
> specific OID?
>
> An interesting compromise, but I'm not sure what the benefit is vs. SET
> search_path FROM CURRENT (or some defined search_path).

Search path does not reliably protect things involving "type matching". If you
have a better fitting cast, or a function call with parameters that won't need
coercion, later in search path, they'll win, even if there's another fit
earlier on.

IOW, search path is a bandaid for this kind of thing, at best.

If we instead store something that avoids the need for such search, the
"better fitting cast" logic wouldn't add these kind of security issues
anymore.

> > That way objects referenced by functions can still be replaced, but
> > search
> > path can't be used to "inject" objects in different schemas.
> > Obviously it
> > could lead to errors on some schema changes - e.g. changing a column
> > type
> > might mean that a relevant cast lives in a different place than with
> > the old
> > type - but I think that'll be quite rare. Perhaps we could offer a
> > ALTER
> > FUNCTION ... REFRESH REFERENCES; or such?
>
> Hmm. I feel like that's making things more complicated. I'd find it
> more straightforward to use something like Robert's approach of fully
> parsing something, and then have the REFRESH command reparse it when
> something needs updating. Or perhaps just create all of the dependency
> entries more like a view query and then auto-refresh.

Hm, I'm not quite sure I follow on what exactly you see as different here.

Greetings,

Andres Freund