Re: Blocking execution of SECURITY INVOKER

Hi,

On 2023-01-11 18:16:32 -0800, Jeff Davis wrote:
> Motivation
> ==========
>
> SECURITY INVOKER is dangerous, particularly for administrators. There
> are numerous ways to put code in a place that's likely to be executed:
> triggers, views calling functions, logically-replicated tables, casts,
> search path and function resolution tricks, etc. If this code is run
> with the privileges of the invoker, then it provides an easy path to
> privilege escalation.

> We've addressed some of these risks, i.e. by offering better ways to
> control the search path, and by ignoring SECURITY INVOKER in some
> contexts (like maintenance commands). But it still leaves a lot of
> risks for administrators who want to do a SELECT or INSERT. And it
> limits major use cases, like logical replication (where the
> subscription owner must trust all table owners).

I'm very skeptical about this framing. On the one hand, you can do a lot of
mischief with security definer functions if they get privileged information as
well. But more importantly, just because a function is security definer,
doesn't mean it's safe to be called with attacker controlled input, and the
privilege check will be done with the rights of the admin in many of these
contexts.

And encouraging more security definer functions to be used will cause a lot of
other security issues.

However - I think the concept of more strict ownership checks is a good one. I
just don't think it's right to tie it to SECURITY INVOKER.

I think it'd be quite valuable to have a guc that prevents the execution of
any code that's not directly controlled by the privileged user. Not just
checking function ownership, but also checking ownership of the trigger
definition (i.e. table), check constraints, domain constraints, indexes with
expression columns / partial indexes, etc.

> Use Cases
> =========
>
> 1. If you are a superuser/admin working on a problem interactively, you
> can protect yourself against accidentally executing malicious code with
> your privileges.

In that case I think what's actually desirable is to simply execute no code
controlled by untrusted users. Even a security definer function can mess up
your day when called in the wrong situation, e.g. due to getting access to the
content of arguments (e.g. a trigger's row contents) or preventing an admin's
write from taking effect (by returning the relevant values from a trigger).

And not ever allowing execution of untrusted code in that situation IME
doesn't prevent desirable things.

> 2. You can set up logical replication subscriptions into tables owned
> by users you don't trust, as long as triggers (if needed) can be safely
> written as SECURITY DEFINER.

I think a much more promising path towards that is to add a feature to logical
replication that changes the execution context to the table owner while
applying those changes.

Using security definer functions for triggers opens up a significant new
attack surface, lots of code that previously didn't need to be safe against
any possible privilege escalation, now needs to be. Expanding the scope of
what needs to protect against privesc, is a BAD idea.

> 3. You can ensure that running an extension script doesn't somehow
> execute malicious code with superuser privileges.

It's not safe to allow executing secdef code in that context either. If a less
privileged user manages to get called in that context you don't want to
execute the code, even in a secdef, you want to error out, so the problem can
be detected and rectified.

> 4. Users can protect themselves from executing malicious code in cases
> where:
> a. role membership accurately describes the trust relationship
> already
> b. triggers, views-calling-UDFs, etc., (if any) can be safely written
> as SECURITY DEFINER

I don't think b) is true very often. And a) seems very problematic as well, as
particularly in "pseudo superuser" environments the most feasible way to
implement pseudo superusers is to automatically grant group membership to the
pseudo superuser. See also e5b8a4c098a.

> While that may not be every conceivable use case, it feels very useful
> to me.
>
> Even if you really don't like SECURITY DEFINER, points 1, 3, and 4(a)
> seem like wins. And there are a lot of cases where the user simply
> doesn't need triggers (etc.).

4 doesn't seem like a win, it's a requirement?

And as I said, for 1 and 3 I think it's way preferrable to error out.

> Future Work
> ===========
>
> In some cases, we should consider defaulting (or even forcing) this GUC
> to be true, such as in a subscription apply worker.
>
> This proposal may offer a path to allowing non-superusers to create
> event triggers.

That'd allow a less-privileged user to completely hobble the admin by erroring
out on all actions.

Greetings,

Andres Freund