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

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

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

On 2023-01-12 19:29:43 -0800, Andres Freund wrote:

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.

Another reason security definer isn't a way to allow safe execution of lesser
privileged code:

superuser (andres):
# CREATE FUNCTION bleat_whoami() RETURNS text LANGUAGE plpgsql SECURITY INVOKER AS $$BEGIN RAISE NOTICE 'whoami: %', current_user;RETURN current_user;END;$$;
# REVOKE ALL ON FUNCTION bleat_whoami FROM PUBLIC;

unprivileged user:
$ CREATE FUNCTION secdef_with_default(foo text = bleat_whoami()) RETURNS text LANGUAGE plpgsql SECURITY DEFINER AS $$BEGIN RETURN 'secdef_with_default';END;$$;
$ SELECT secdef_with_default();
ERROR: 42501: permission denied for function bleat_whoami

superuser (andres):
# SELECT secdef_with_default();
NOTICE: 00000: whoami: andres
LOCATION: exec_stmt_raise, pl_exec.c:3893
┌─────────────────────┐
│ secdef_with_default │
├─────────────────────┤
│ secdef_with_default │
└─────────────────────┘
(1 row)

I.e. the default arguments where evaluated with the invoker's permissions, not
the definer's, despite being controlled by the less privileged user. Worsened
in this case by the fact that this allowed the less privileged user to call a
function they couldn't even call.

Greetings,

Andres Freund

Hi,

On Thu, 2023-01-12 at 19:29 -0800, Andres Freund wrote:

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 ;

That can be solved by creating the function in a schema where ordinary
users don't have USAGE:

CREATE TABLE trick_superuser(value text default admin.exec_su('ALTER
USER less_privs SUPERUSER'));
ERROR: permission denied for schema admin

An interesting case, but it looks more like a gotcha (which is solvable
with best practices); not a fundamental problem.

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.

There is not necessarily a GRANT hierarchy like you describe. The two
users can be peers each with comparable privileges that might make
grants to each other.

And the admin likely can switch into the user context of
the less privileged user to perform operations in a safer context.

How would the admin do that? The malicious UDF can just "RESET SESSION
AUTHORIZATION" to pop back out of the safer context.

If there's not a good way to do this safely now, then we should
probably provide one.

Regards,
--
Jeff Davis
PostgreSQL Contributor Team - AWS

Hi,

On 2023-01-12 23:38:50 -0800, Jeff Davis wrote:

On Thu, 2023-01-12 at 19:29 -0800, Andres Freund wrote:

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 ;

That can be solved by creating the function in a schema where ordinary
users don't have USAGE:

CREATE TABLE trick_superuser(value text default admin.exec_su('ALTER
USER less_privs SUPERUSER'));
ERROR: permission denied for schema admin

Doubtful. Leaving aside the practicalities of using dedicated schemas and
enforcing their use, there's plenty functions in pg_catalog that a less
privileged user can use to do bad things.

Just think of set_config(), pg_read_file(), lo_create(), binary_upgrade_*(),
pg_drop_replication_slot()...

If the default values get evaluated, this is arbitrary code exec, even if it
requires a few contortions. And the same is true for evaluating *any*
expression.

And the admin likely can switch into the user context of
the less privileged user to perform operations in a safer context.

How would the admin do that? The malicious UDF can just "RESET SESSION
AUTHORIZATION" to pop back out of the safer context.

I thought we had a reasonably convenient way, but now I am not sure
anymore. Might have been via a C helper function. It can be hacked together,
but this is an area that should be as unhacky as possible.

If there's not a good way to do this safely now, then we should
probably provide one.

Yea, particularly because we do have all the infrastructure for it
(c.f. SECURITY_LOCAL_USERID_CHANGE / SECURITY_RESTRICTED_OPERATION).

Greetings,

Andres Freund

On Thu, 2023-01-12 at 19:38 -0800, Andres Freund wrote:

I.e. the default arguments where evaluated with the invoker's
permissions, not
the definer's, despite being controlled by the less privileged user.

This is a very interesting case. It also involves tricking the
superuser into executing their own function with the attacker's inputs.
That part is the same as the other case. What's intriguing here is that
it shows the function can be SECURITY INVOKER, and that really means it
could be any builtin function as long as the types work out.

For example:
=> create function trick(l pg_lsn = pg_switch_wal()) returns int
language plpgsql security definer as $$ begin return 42; end; $$;

If the superuser executes that, even though it's a SECURITY DEFINER
function owned by an unprivileged user, it will still call
pg_switch_wal().

--
Jeff Davis
PostgreSQL Contributor Team - AWS

On Fri, 2023-01-13 at 00:16 -0800, Andres Freund wrote:

Just think of set_config(), pg_read_file(), lo_create(),
binary_upgrade_*(),
pg_drop_replication_slot()...

Thank you for walking through the details here. I missed it from your
first example because it was an extreme case -- a superuser writing an
eval() security definer function -- so the answer was to lock such a
dangerous function away. But more mild cases are the real problem,
because there's a lot of stuff in pg_catalog.*.

If the default values get evaluated, this is arbitrary code exec,
even if it
requires a few contortions. And the same is true for evaluating *any*
expression.

Right.

However, the normal use case for expressions (whether in a default
expression or check constraint or whatever) is very simple and doesn't
even involve table access. There must be a way to satisfy those simple
cases without opening up a vast attack surface area, and if we do, then
I think my proposal might look useful again.

--
Jeff Davis
PostgreSQL Contributor Team - AWS

Hi,

On 2023-01-13 10:04:19 -0800, Jeff Davis wrote:

However, the normal use case for expressions (whether in a default
expression or check constraint or whatever) is�very simple and doesn't
even involve table access. There must be a way to satisfy those simple
cases without opening up a vast attack surface area, and if we do, then
I think my proposal might look useful again.

I don't see how. I guess we could try to introduce a classification of "never
dangerous" functions (and thus operators). But that seems like a crapton of
work and hard to get right. And I think my examples pretty conclusively show
that security definer isn't a useful boundary to *reduce* privileges. So the
whole idea of preventing only security invoker functions just seems
non-viable.

I think the combination of
a) a setting that restricts evaluation of any non-trusted expressions,
independent of the origin
b) an easy way to execute arbitrary statements within
SECURITY_RESTRICTED_OPERATION

is promising though. In later steps We might be able to use a) to offer the
option to automatically switch to expression owners in specific places (if the
current user has the rights to do that).

An alternative to b would be a version SET ROLE that can't be undone. But I
think we'd just miss all the other things that are prevented by
SECURITY_RESTRICTED_OPERATION.

Greetings,

Andres Freund