Re: Blocking execution of SECURITY INVOKER

On Wed, 2023-01-11 at 19:33 -0800, Andres Freund wrote:

> and the
> privilege check will be done with the rights of the admin in many of
> these
> contexts.

Can you explain?

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

My proposal just gives some user foo a GUC to say "I am not accepting
the risk of eval()ing whatever arbitrary code finds its way in front of
me with all of my privileges".

If user foo sheds this security burden by setting the GUC, user bar may
then choose to write a trigger function as SECURITY DEFINER so that foo
can access bar's table. But that's the deal the two users struck -- foo
declined the burden, bar accepted it. Why do we want to prevent that
arrangement?

Right now, foo *always* has the burden and no opportunity to decline
it, and even a paranoid user can't figure out what code they will be
executing with a given command. That doesn't seem reasonable to me.

> 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.

Consider a canonical trigger example like:
https://wiki.postgresql.org/wiki/Audit_trigger or
https://github.com/2ndQuadrant/audit-trigger/blob/master/audit.sql

How can we make that secure for users that insert into the table with
the trigger if you don't differentiate between SECURITY INVOKER and
SECURITY DEFINER? If you allow neither, then it obviously won't work.
And if you allow both, then the owner of the table can change the
function to SECURITY INVOKER and the definition to be malicious a
millisecond before you insert a tuple.

I guess we currently say that anyone foolish enough to insert into a
table that they don't own deserves what they get. That's a weird thing
to say when we have such a fine-grained GRANT system and RLS.

> 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

That sounds like a mix of my proposal and Noah's. The way you've
phrased it seems overly strict though -- do you mean not even execute
untrusted expressions? And it seems to cut out maintenance commands,
which means it would be hard for administrators to use.

I'm OK considering these proposals. Anything that offers some safety.
But it seems like both your proposal and Noah's cut out huge amounts of
functionality unless you have unqualified trust.

> 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)

I don't see that as a problem. If you're inserting data in a table,
you'd expect the owner of the table to see that data and be able to
modify it as they see fit.

>  or preventing an admin's
> write from taking effect (by returning the relevant values from a
> trigger).

I don't see the problem here either. Even if we force the row to be
inserted, the table owner could just delete it.

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

I don't understand this statement.

>
> 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.

How is that different from SECURITY DEFINER?



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

My proposal does error out for SECURITY INVOKER, so I suppose you're
saying we should error out for SECURITY DEFINER as well? In the case of
1, I think that would prevent regular maintenance by an admin.

But for use case 3 (extension scripts), I think you're right, erroring
on any non-superuser-owned code is probably good.


--
Jeff Davis
PostgreSQL Contributor Team - AWS