From 1b4b575017a226011e6a0174c0a4c372015faa06 Mon Sep 17 00:00:00 2001 From: Jeff Davis 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; + + check_function_owner_trust (boolean) + + check_function_owner_trust configuration parameter + + + + + This variable controls whether to raise an error in lieu of executing + a SECURITY INVOKER function (see ) when that function is owned by an + untrusted user. When set to off (the default), + functions are executed normally regardless of the owner. When set to + on, an error is raised unless the SECURITY + INVOKER function is owned by the current user, or a user + that can to the current user. + + + + Setting this variable to on provides protection + against unexpectedly executing code written by another user, e.g. due + to a trigger (). + + + + default_table_access_method (string) 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