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).
Note that, in the SQL spec, SECURITY DEFINER is the default, which may
be due to some of the dangers of SECURITY INVOKER. (SECURITY DEFINER
carries its own risks, of course, especially if the definer is highly
privileged.)
Prior work
==========
https://www.postgresql.org/message-id/19327.1533748538%40sss.pgh.pa.us
The above thread came up with a couple solutions to express a trust
relationship between users (via GUC or DDL). I'm happy if that
discussion continues, but it appeared to trail off.
My new proposal is different (and simpler, I believe) in two ways:
First, my proposal is only concerned with SECURITY INVOKER functions
and executing arbitrary code written by untrusted users. There's still
the potential for mischief without using SECURITY INVOKER (e.g. if the
search path isn't properly controlled), but it's a different kind of
problem. This narrower problem scope makes my proposal less invasive.
Second, my proposal doesn't establish a new trust relationship. If the
SECURITY INVOKER function is owned by a user that can SET ROLE to you,
you trust it; otherwise not.
Proposal
========
New boolean GUC check_function_owner_trust, default false.
If check_function_owner_trust=true, throw an error if you try to
execute a function that is SECURITY INVOKER and owned by a user other
than you or someone that can SET ROLE to you.
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.
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.
3. You can ensure that running an extension script doesn't somehow
execute malicious code with superuser privileges.
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
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.).
Extensions
==========
Some extensions might create and extension-specific user that owns lots
of SECURITY INVOKER functions. If this GUC is set, other users wouldn't
be able to call those functions.
Our contrib extensions don't seem do that, and all the tests for them
pass without modification (even when the GUC is true).
For extensions that do create extension-specific users that own
SECURITY INVOKER functions, this GUC alone won't work. Trying to
capture that use case as well could involve more discussion (involving
extension authors) and may result in an extension-specific trust
proposal, so I'm considering that out of scope.
Loose Ends
==========
Do we need to check security-invoker views? I don't think it's nearly
as important, because views can't write data. A security-invoker view
read from a security definer function uses the privileges of the
function owner, so I don't see an obvious way to abuse a security
invoker view, but perhaps I'm not creative enough.
Also, Noah's patch did things differently from mine in a few places,
and I need to work out whether I missed something. I may have to add a
check for the range types "subtype_diff" function, for instance.
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.
We may want to provide SECURITY PUBLIC or SECURITY NONE (or even
"SECURITY AS <role>"?), which would execute a function with minimal
privileges, and further reduce the need for executing untrusted
SECURITY INVOKER code.
Another idea is to have READ ONLY functions which would be another way
to make SECURITY INVOKER safer.
--
Jeff Davis
PostgreSQL Contributor Team - AWS
From 1b4b575017a226011e6a0174c0a4c372015faa06 Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Wed, 11 Jan 2023 15:33:47 -0800
Subject: [PATCH v1] Introduce GUC check_function_owner_trust.
If set to true, blocks execution of SECURITY INVOKER functions unless
the function owner can SET ROLE to the current user.
---
doc/src/sgml/config.sgml | 26 ++++++
src/backend/catalog/aclchk.c | 13 +++
src/backend/optimizer/util/clauses.c | 2 +
src/backend/utils/fmgr/fmgr.c | 10 +++
src/backend/utils/misc/guc_tables.c | 10 +++
src/backend/utils/misc/postgresql.conf.sample | 1 +
src/include/utils/acl.h | 1 +
src/include/utils/guc.h | 1 +
src/test/regress/expected/privileges.out | 85 +++++++++++++++++++
src/test/regress/sql/privileges.sql | 57 +++++++++++++
10 files changed, 206 insertions(+)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 2fec613484..fdfb732a6e 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8707,6 +8707,32 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
</listitem>
</varlistentry>
+ <varlistentry id="guc-check-function-owner-trust" xreflabel="check_function_owner_trust">
+ <term><varname>check_function_owner_trust</varname> (<type>boolean</type>)
+ <indexterm>
+ <primary><varname>check_function_owner_trust</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ This variable controls whether to raise an error in lieu of executing
+ a <literal>SECURITY INVOKER</literal> function (see <xref
+ linkend="sql-createfunction"/>) when that function is owned by an
+ untrusted user. When set to <literal>off</literal> (the default),
+ functions are executed normally regardless of the owner. When set to
+ <literal>on</literal>, an error is raised unless the <literal>SECURITY
+ INVOKER</literal> function is owned by the current user, or a user
+ that can <xref linkend="sql-set-role"/> to the current user.
+ </para>
+
+ <para>
+ Setting this variable to <literal>on</literal> provides protection
+ against unexpectedly executing code written by another user, e.g. due
+ to a trigger (<xref linkend="triggers"/>).
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-default-table-access-method" xreflabel="default_table_access_method">
<term><varname>default_table_access_method</varname> (<type>string</type>)
<indexterm>
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index cc6e260908..734c95a3f4 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -3977,6 +3977,19 @@ pg_largeobject_aclcheck_snapshot(Oid lobj_oid, Oid roleid, AclMode mode,
return ACLCHECK_NO_PRIV;
}
+/*
+ * For SECURITY INVOKER functions, check that the calling user trusts the
+ * function owner enough to run the code.
+ */
+bool
+function_owner_trust(Oid proowner)
+{
+ if (!check_function_owner_trust)
+ return true;
+
+ return member_can_set_role(proowner, GetUserId());
+}
+
/*
* Generic ownership check for an object
*/
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index aa584848cf..9678f55052 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -4412,6 +4412,7 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid,
funcform->prokind != PROKIND_FUNCTION ||
funcform->prosecdef ||
funcform->proretset ||
+ !function_owner_trust(funcform->proowner) ||
funcform->prorettype == RECORDOID ||
!heap_attisnull(func_tuple, Anum_pg_proc_proconfig, NULL) ||
funcform->pronargs != list_length(args))
@@ -4996,6 +4997,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
funcform->prorettype == VOIDOID ||
funcform->prosecdef ||
!funcform->proretset ||
+ !function_owner_trust(funcform->proowner) ||
list_length(fexpr->args) != funcform->pronargs ||
!heap_attisnull(func_tuple, Anum_pg_proc_proconfig, NULL))
{
diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c
index 3f64161760..3985f42290 100644
--- a/src/backend/utils/fmgr/fmgr.c
+++ b/src/backend/utils/fmgr/fmgr.c
@@ -214,6 +214,16 @@ fmgr_info_cxt_security(Oid functionId, FmgrInfo *finfo, MemoryContext mcxt,
return;
}
+ /* SECURITY INVOKER; check that we trust the function owner */
+ if (!ignore_security && !function_owner_trust(procedureStruct->proowner))
+ ereport(ERROR,
+ (errmsg("cannot execute function %s owned by untrusted user %s",
+ NameStr(procedureStruct->proname),
+ GetUserNameFromId(procedureStruct->proowner, false)),
+ errdetail("When check_function_owner_trust is on, "
+ "SECURITY INVOKER functions must be owned by "
+ "a role that can SET ROLE to the calling user.")));
+
switch (procedureStruct->prolang)
{
case INTERNALlanguageId:
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 92545b4958..5a99b66fdc 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -479,6 +479,7 @@ bool log_btree_build_stats = false;
char *event_source;
bool row_security;
+bool check_function_owner_trust = false;
bool check_function_bodies = true;
/*
@@ -1576,6 +1577,15 @@ struct config_bool ConfigureNamesBool[] =
true,
NULL, NULL, NULL
},
+ {
+ {"check_function_owner_trust", PGC_USERSET, CLIENT_CONN_STATEMENT,
+ gettext_noop("Check that a SECURITY INVOKER function's owner is trusted before executing."),
+ NULL
+ },
+ &check_function_owner_trust,
+ false,
+ NULL, NULL, NULL
+ },
{
{"check_function_bodies", PGC_USERSET, CLIENT_CONN_STATEMENT,
gettext_noop("Check routine bodies during CREATE FUNCTION and CREATE PROCEDURE."),
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index c2ada92054..f37a9c3bba 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -684,6 +684,7 @@
#default_toast_compression = 'pglz' # 'pglz' or 'lz4'
#temp_tablespaces = '' # a list of tablespace names, '' uses
# only default tablespace
+#check_function_owner_trust = off
#check_function_bodies = on
#default_transaction_isolation = 'read committed'
#default_transaction_read_only = off
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index f8e1238fa2..87b775041a 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -258,6 +258,7 @@ extern AclResult pg_parameter_aclcheck(const char *name, Oid roleid,
AclMode mode);
extern AclResult pg_largeobject_aclcheck_snapshot(Oid lobj_oid, Oid roleid,
AclMode mode, Snapshot snapshot);
+extern bool function_owner_trust(Oid proowner);
extern void aclcheck_error(AclResult aclerr, ObjectType objtype,
const char *objectname);
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index ba89d013e6..cd210f5fcc 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -249,6 +249,7 @@ extern PGDLLIMPORT bool log_executor_stats;
extern PGDLLIMPORT bool log_statement_stats;
extern PGDLLIMPORT bool log_btree_build_stats;
+extern PGDLLIMPORT bool check_function_owner_trust;
extern PGDLLIMPORT bool check_function_bodies;
extern PGDLLIMPORT bool session_auth_is_superuser;
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 34c26a804a..5dafbdebb5 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -359,6 +359,91 @@ SELECT * FROM atest1; -- ok
1 | two
(2 rows)
+-- test that we trust the owner of SECURITY INVOKER functions
+RESET SESSION AUTHORIZATION;
+GRANT regress_priv_user1 TO regress_priv_user2;
+SET SESSION AUTHORIZATION regress_priv_user2;
+CREATE FUNCTION testf_trust1() RETURNS INT SECURITY INVOKER
+ LANGUAGE sql AS $$ SELECT 42 $$;
+CREATE FUNCTION testf_trust2() RETURNS INT SECURITY INVOKER
+ LANGUAGE plpgsql AS $$ BEGIN RETURN 42; END; $$;
+CREATE PROCEDURE testp_trust3() SECURITY INVOKER
+ LANGUAGE plpgsql AS $$ BEGIN PERFORM 42; END; $$;
+CREATE FUNCTION testf_trust4() RETURNS INT SECURITY DEFINER
+ LANGUAGE plpgsql AS $$ BEGIN RETURN 42; END; $$;
+CREATE VIEW testv_trust1 WITH (security_invoker = true)
+ AS SELECT testf_trust4(), testf_trust1();
+CREATE VIEW testv_trust2 WITH (security_invoker = false)
+ AS SELECT testf_trust2(), testf_trust4();
+GRANT SELECT ON testv_trust1 TO regress_priv_user1, regress_priv_user7;
+GRANT SELECT ON testv_trust2 TO regress_priv_user1, regress_priv_user7;
+-- should succeed; regress_priv_user2 is a member of regress_priv_user1
+SET SESSION AUTHORIZATION regress_priv_user1;
+SET check_function_owner_trust = true;
+SELECT testf_trust1();
+ testf_trust1
+--------------
+ 42
+(1 row)
+
+SELECT testf_trust2();
+ testf_trust2
+--------------
+ 42
+(1 row)
+
+CALL testp_trust3();
+SELECT testf_trust4();
+ testf_trust4
+--------------
+ 42
+(1 row)
+
+SELECT * FROM testv_trust1;
+ testf_trust4 | testf_trust1
+--------------+--------------
+ 42 | 42
+(1 row)
+
+SELECT * FROM testv_trust2;
+ testf_trust2 | testf_trust4
+--------------+--------------
+ 42 | 42
+(1 row)
+
+SET SESSION AUTHORIZATION regress_priv_user7;
+SET check_function_owner_trust = true;
+SELECT testf_trust1(); -- fails
+ERROR: cannot execute function testf_trust1 owned by untrusted user regress_priv_user2
+DETAIL: When check_function_owner_trust is on, SECURITY INVOKER functions must be owned by a role that can SET ROLE to the calling user.
+SELECT testf_trust2(); -- fails
+ERROR: cannot execute function testf_trust2 owned by untrusted user regress_priv_user2
+DETAIL: When check_function_owner_trust is on, SECURITY INVOKER functions must be owned by a role that can SET ROLE to the calling user.
+CALL testp_trust3(); -- fails
+ERROR: cannot execute function testp_trust3 owned by untrusted user regress_priv_user2
+DETAIL: When check_function_owner_trust is on, SECURITY INVOKER functions must be owned by a role that can SET ROLE to the calling user.
+SELECT testf_trust4();
+ testf_trust4
+--------------
+ 42
+(1 row)
+
+SELECT * FROM testv_trust1; -- fails
+ERROR: cannot execute function testf_trust1 owned by untrusted user regress_priv_user2
+DETAIL: When check_function_owner_trust is on, SECURITY INVOKER functions must be owned by a role that can SET ROLE to the calling user.
+SELECT * FROM testv_trust2; -- fails
+ERROR: cannot execute function testf_trust2 owned by untrusted user regress_priv_user2
+DETAIL: When check_function_owner_trust is on, SECURITY INVOKER functions must be owned by a role that can SET ROLE to the calling user.
+SET SESSION AUTHORIZATION regress_priv_user2;
+DROP VIEW testv_trust1;
+DROP VIEW testv_trust2;
+DROP FUNCTION testf_trust1();
+DROP FUNCTION testf_trust2();
+DROP PROCEDURE testp_trust3();
+DROP FUNCTION testf_trust4();
+RESET SESSION AUTHORIZATION;
+REVOKE regress_priv_user1 FROM regress_priv_user2;
+SET check_function_owner_trust TO DEFAULT;
-- test leaky-function protections in selfuncs
-- regress_priv_user1 will own a table and provide views for it.
SET SESSION AUTHORIZATION regress_priv_user1;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 39aa0b4ecf..57fd93a573 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -265,6 +265,63 @@ bar true
\.
SELECT * FROM atest1; -- ok
+-- test that we trust the owner of SECURITY INVOKER functions
+
+RESET SESSION AUTHORIZATION;
+GRANT regress_priv_user1 TO regress_priv_user2;
+
+SET SESSION AUTHORIZATION regress_priv_user2;
+
+CREATE FUNCTION testf_trust1() RETURNS INT SECURITY INVOKER
+ LANGUAGE sql AS $$ SELECT 42 $$;
+CREATE FUNCTION testf_trust2() RETURNS INT SECURITY INVOKER
+ LANGUAGE plpgsql AS $$ BEGIN RETURN 42; END; $$;
+CREATE PROCEDURE testp_trust3() SECURITY INVOKER
+ LANGUAGE plpgsql AS $$ BEGIN PERFORM 42; END; $$;
+CREATE FUNCTION testf_trust4() RETURNS INT SECURITY DEFINER
+ LANGUAGE plpgsql AS $$ BEGIN RETURN 42; END; $$;
+
+CREATE VIEW testv_trust1 WITH (security_invoker = true)
+ AS SELECT testf_trust4(), testf_trust1();
+CREATE VIEW testv_trust2 WITH (security_invoker = false)
+ AS SELECT testf_trust2(), testf_trust4();
+
+GRANT SELECT ON testv_trust1 TO regress_priv_user1, regress_priv_user7;
+GRANT SELECT ON testv_trust2 TO regress_priv_user1, regress_priv_user7;
+
+-- should succeed; regress_priv_user2 is a member of regress_priv_user1
+SET SESSION AUTHORIZATION regress_priv_user1;
+SET check_function_owner_trust = true;
+
+SELECT testf_trust1();
+SELECT testf_trust2();
+CALL testp_trust3();
+SELECT testf_trust4();
+SELECT * FROM testv_trust1;
+SELECT * FROM testv_trust2;
+
+SET SESSION AUTHORIZATION regress_priv_user7;
+SET check_function_owner_trust = true;
+
+SELECT testf_trust1(); -- fails
+SELECT testf_trust2(); -- fails
+CALL testp_trust3(); -- fails
+SELECT testf_trust4();
+SELECT * FROM testv_trust1; -- fails
+SELECT * FROM testv_trust2; -- fails
+
+SET SESSION AUTHORIZATION regress_priv_user2;
+DROP VIEW testv_trust1;
+DROP VIEW testv_trust2;
+DROP FUNCTION testf_trust1();
+DROP FUNCTION testf_trust2();
+DROP PROCEDURE testp_trust3();
+DROP FUNCTION testf_trust4();
+
+RESET SESSION AUTHORIZATION;
+REVOKE regress_priv_user1 FROM regress_priv_user2;
+
+SET check_function_owner_trust TO DEFAULT;
-- test leaky-function protections in selfuncs
--
2.34.1
