Feedback-ID: id4a34324:Fastmail
Date: Thu, 12 Jan 2023 19:29:43 -0800
From: Andres Freund <andres@anarazel.de>
To: Jeff Davis <pgsql@j-davis.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: Blocking execution of SECURITY INVOKER
Message-ID: <20230113032943.iyxdu7bnxe4cmbld@awork3.anarazel.de>
References: <75b0dbb55e9febea54c441efff8012a6d2cb5bd7.camel@j-davis.com>
 <20230112033355.u5tiyr2bmuoc4jf4@awork3.anarazel.de>
 <b65c9660147b283f06e71569fe7fd36222b5aec6.camel@j-davis.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b65c9660147b283f06e71569fe7fd36222b5aec6.camel@j-davis.com>
Archived-At: <https://www.postgresql.org/message-id/20230113032943.iyxdu7bnxe4cmbld%40awork3.anarazel.de>
Precedence: bulk

Hi,

On 2023-01-12 18:40:30 -0800, Jeff Davis wrote:
> 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?

If the less-privileged user does *not* have execution rights to a security
definer function, but somehow can trick the more-privileged user into calling
the function for them, e.g. by using it as the default expression of a column,
the less-privileged user can escalate to the permissions of the security
definer function.

superuser:
# CREATE FUNCTION exec_su(p_sql text) RETURNS text LANGUAGE plpgsql SECURITY DEFINER AS $$BEGIN RAISE NOTICE 'executing %', p_sql; EXECUTE p_sql;RETURN 'p_sql';END;$$;
# REVOKE ALL ON FUNCTION exec_su FROM PUBLIC ;

unprivileged user:
$ SELECT exec_su('ALTER USER less_privs SUPERUSER');
ERROR:  42501: permission denied for function exec_su
$ CREATE TABLE trick_superuser(value text default exec_su('ALTER USER less_privs SUPERUSER'));

superuser:
# INSERT INTO trick_superuser DEFAULT VALUES;
NOTICE:  00000: executing ALTER USER less_privs SUPERUSER


This case would *not* be prevented by your proposed GUC, unless I miss
something major. The superuser trusts itself and thus the exec_su() function.


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

Because it afaict doesn't provide any meaningfully increased security
guarantees (see above), and opens up new ways of attacking, because while
granting execute on a security definer function is low risk, granting execute
on security invoker functions is very high risk, but required for triggers etc
to work.


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

I agree it's not reasonable - I just don't see the proposal moving the bar.


The proposal to not trust any expressions controlled by untrusted users at
least allows to prevent execution of code, even if it doesn't provide a way to
execute the code in a safe manner.  Given that we don't have the former, it
seems foolish to shoot for the latter.


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

As shown above, triggers are simply not a relevant boundary when a more
privileged user accesses a table controlled by a less privileged user.

And yes, of course an audit function needs to be security definer. But that's
independent of whether it's safe for a more privileged user modify table
contents.


> I guess we currently say that anyone foolish enough to insert into a
> table that they don't own deserves what they get.

I agree that we have a problem that we should address. I just don't think your
solution works.


> That's a weird thing to say when we have such a fine-grained GRANT system
> and RLS.

That's a non-sequitur imo. Particularly when RLS you'd not allow
less-privileged users to create any objects, with the possible exception of
temp tables. The point of the grant system is for a privileged user to safely
allow a less privileged user to perform a safe subset of actions. That's just
a separate angle than allowing safe access for a more privileged user to to
objects controlled by a less privileged user.


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

Yes, I mean every expression. As show above, as soon as there is *any*
expression controlled by a less privileged user is executed, the game is lost.

I don't think that prevents all maintenance btw - for things like reindex we
switch to the object owner for evaluation via SetUserIdAndSecContext(). After
checking whether the current user is allowed to that kind of thing.

But for things like default expressions, generated columns etc, I just don't
see an alternative to erroring out when we'd otherwise evaluate an expression
that's controlled by a less privileged user.  The admin can alter the table
definition / drop it, if requried.


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

It's not a huge problem if a server admin gets an error while evaluating a
less-privileged-expression, because that's not commonly something that an
admin needs to do. And the admin likely can switch into the user context of
the less privileged user to perform operations in a safer context.


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

It protect against vastly more things, see the default expression example.


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

What regular maintenance would be prevented? And would it be safe to execute
said code as superuser?

Greetings,

Andres Freund