POC for a function trust mechanism

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

POC for a function trust mechanism

Tom Lane-2
This is sort of a counter-proposal to Noah's discussion of search path
security checking in <[hidden email]>.
(There's no technical reason we couldn't do both things, but I think
this'd be more useful to most people.)

Some back story here is that the PG security team has been aware of the
issues in CVE-2018-1058 for an embarrassing number of years, and we'd
been vainly working to find a fix that was both non-invasive to users
and practical to back-patch.  Eventually our hands were forced by an
outside security researcher who discovered some of those problems, and
naturally wanted to publish on a fairly short time scale.  So we ended
up with the decidedly not non-invasive approach of locking down
search_path in especially critical places, and otherwise telling people
that they had to worry about this themselves.  Of the various ideas that
we'd kicked around and not been able to finish, the one that seemed most
promising to me was to invent a "function trust" mechanism.

The core idea here is to prevent security problems not by changing an
application's rules for operator/function name resolution, but by
detecting an attempted compromise and preventing the trojan-horse code
from being executed.  Essentially, a user or application is to declare
a list of roles that it trusts functions owned by, and the system will
then refuse to execute functions owned by other not-trusted roles.
So, if $badguy creates a trojan-horse operator and manages to capture
a call from your SQL code, he'll nonetheless not be able to execute
code as you.

To reduce the overhead of the mechanism and chance of unintentionally
breaking things, superuser-owned functions (particularly, all built-in
functions) are always trusted by everybody.  A superuser who wants to
become you can do so trivially, with no need for a trojan horse, so
this restriction isn't creating any new security hole.

The things that we hadn't resolved, which is why this didn't get further
than POC stage, were

(1) What's the mechanism for declaring trust?  In this POC, it's just
a GUC that you can set to a list of role names, with $user for yourself
and "public" if you want to trust everybody.  It's not clear if that's
good enough, or if we want something a bit more locked-down.

(2) Is trust transitive?  Where and how would the list of trusted roles
change?  Arguably, if you call a SECURITY DEFINER function, then once
you've decided that you trust the function owner, actual execution of the
function should use the function owner's list of trusted roles not yours.
With the GUC approach, it'd be necessary for SECURITY DEFINER functions
to implement this with a "SET trusted_roles" clause, much as they now
have to do with search_path.  That's possible but it's again not very
non-invasive, so we'd been speculating about automating this more.
If we had, say, a catalog that provided the desired list of trusted roles
for every role, then we could imagine implementing that context change
automatically.  Likewise, stuff like autovacuum or REINDEX would want
to run with the table owner's list of trusted roles, but the GUC approach
doesn't really provide enough infrastructure to know what to do there.

So we'd kind of decided that the GUC solution wasn't good enough, but
it didn't seem like catalog additions would be feasible as a back-patched
security fix, which is why this didn't go anywhere.  But it could work
as a new feature.

Anyway, I had written a small POC that did use a GUC for this, and just
checked function calls without any attempts to switch the active
trusted_roles setting in places like autovacuum.  I've rebased it up to
HEAD and here it is.

                        regards, tom lane


diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 9a754da..ea24bc3 100644
*** a/src/backend/commands/variable.c
--- b/src/backend/commands/variable.c
*************** show_role(void)
*** 950,952 ****
--- 950,1075 ----
  /* Otherwise we can just use the GUC string */
  return role_string ? role_string : "none";
  }
+
+
+ /*
+  * TRUSTED_ROLES
+  */
+
+ /* XXX this needs to be replaced with some more complex cache structure */
+ static const char *trusted_roles = "";
+
+ /*
+  * check_trusted_roles: GUC check_hook for trusted_roles
+  */
+ bool
+ check_trusted_roles(char **newval, void **extra, GucSource source)
+ {
+ char   *rawname;
+ List   *namelist;
+
+ /* Need a modifiable copy of string */
+ rawname = pstrdup(*newval);
+
+ /* Parse string into list of identifiers */
+ if (!SplitIdentifierString(rawname, ',', &namelist))
+ {
+ /* syntax error in name list */
+ GUC_check_errdetail("List syntax is invalid.");
+ pfree(rawname);
+ list_free(namelist);
+ return false;
+ }
+
+ /*
+ * No additional checks are possible now, because we might not be inside a
+ * transaction; so we're done.
+ */
+
+ pfree(rawname);
+ list_free(namelist);
+
+ return true;
+ }
+
+ /*
+  * assign_trusted_roles: GUC assign_hook for trusted_roles
+  */
+ void
+ assign_trusted_roles(const char *newval, void *extra)
+ {
+ /* Update the active value */
+ trusted_roles = newval;
+ }
+
+ /*
+  * role_trusts_role: does caller trust callee?
+  *
+  * Basically, this checks whether callee is a member of any role listed
+  * in trusted_roles; "$user" is resolved as being the caller.  However,
+  * if callee is a superuser, it'll be trusted regardless of the GUC.
+  */
+ bool
+ role_trusts_role(Oid caller, Oid callee)
+ {
+ bool result = false;
+ char   *rawname;
+ List   *namelist;
+ ListCell   *lc;
+
+ /* Superusers are always trusted by everybody */
+ if (superuser_arg(callee))
+ return true;
+
+ /* Need a modifiable copy of string */
+ rawname = pstrdup(trusted_roles);
+
+ /* Parse string into list of identifiers */
+ if (!SplitIdentifierString(rawname, ',', &namelist))
+ {
+ /* syntax error in name list */
+ /* this should not happen if GUC checked check_trusted_roles */
+ elog(ERROR, "invalid list syntax");
+ }
+
+ /* Examine each identifier */
+ foreach(lc, namelist)
+ {
+ char   *curname = (char *) lfirst(lc);
+ Oid roleId;
+
+ /* Check special cases */
+ if (strcmp(curname, "$user") == 0)
+ {
+ /* Take this as the caller */
+ roleId = caller;
+ }
+ else if (strcmp(curname, "public") == 0)
+ {
+ /* We should trust everybody */
+ result = true;
+ break;
+ }
+ else
+ {
+ /* Normal role name, look it up */
+ roleId = get_role_oid(curname, true);
+ /* As with search_path, ignore unknown names for ease of use */
+ if (!OidIsValid(roleId))
+ continue;
+ }
+
+ /* No point in repeating superuserness check */
+ if (is_member_of_role_nosuper(callee, roleId))
+ {
+ /* We should trust callee */
+ result = true;
+ break;
+ }
+ }
+
+ pfree(rawname);
+ list_free(namelist);
+
+ return result;
+ }
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index a04ad6e..444d1af 100644
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
***************
*** 26,31 ****
--- 26,32 ----
  #include "catalog/pg_operator.h"
  #include "catalog/pg_proc.h"
  #include "catalog/pg_type.h"
+ #include "commands/variable.h"
  #include "executor/executor.h"
  #include "executor/functions.h"
  #include "funcapi.h"
*************** simplify_function(Oid funcid, Oid result
*** 4063,4068 ****
--- 4064,4081 ----
  *args_p = args;
  }
 
+ /*
+ * Don't try to simplify an untrusted function; evaluate_function() would
+ * throw an error, but we'd rather postpone that failure to execution.  We
+ * mustn't inline either, because that would bypass the trust check.  (XXX
+ * maybe we should do the ACL check here too, to postpone that failure?)
+ */
+ if (!role_trusts_role(GetUserId(), func_form->proowner))
+ {
+ ReleaseSysCache(func_tuple);
+ return NULL;
+ }
+
  /* Now attempt simplification of the function call proper. */
 
  newexpr = evaluate_function(funcid, result_type, result_typmod,
*************** inline_set_returning_function(PlannerInf
*** 5035,5041 ****
  funcform->prorettype == VOIDOID ||
  funcform->prosecdef ||
  !funcform->proretset ||
! !heap_attisnull(func_tuple, Anum_pg_proc_proconfig, NULL))
  {
  ReleaseSysCache(func_tuple);
  return NULL;
--- 5048,5055 ----
  funcform->prorettype == VOIDOID ||
  funcform->prosecdef ||
  !funcform->proretset ||
! !heap_attisnull(func_tuple, Anum_pg_proc_proconfig, NULL) ||
! !role_trusts_role(GetUserId(), funcform->proowner))
  {
  ReleaseSysCache(func_tuple);
  return NULL;
diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c
index 6cbbd5b..32dee72 100644
*** a/src/backend/utils/fmgr/fmgr.c
--- b/src/backend/utils/fmgr/fmgr.c
***************
*** 18,27 ****
--- 18,29 ----
  #include "access/tuptoaster.h"
  #include "catalog/pg_language.h"
  #include "catalog/pg_proc.h"
+ #include "commands/variable.h"
  #include "executor/functions.h"
  #include "lib/stringinfo.h"
  #include "miscadmin.h"
  #include "nodes/nodeFuncs.h"
+ #include "parser/parse_func.h"
  #include "pgstat.h"
  #include "utils/acl.h"
  #include "utils/builtins.h"
*************** fmgr_info_cxt_security(Oid functionId, F
*** 164,170 ****
  if ((fbp = fmgr_isbuiltin(functionId)) != NULL)
  {
  /*
! * Fast path for builtin functions: don't bother consulting pg_proc
  */
  finfo->fn_nargs = fbp->nargs;
  finfo->fn_strict = fbp->strict;
--- 166,173 ----
  if ((fbp = fmgr_isbuiltin(functionId)) != NULL)
  {
  /*
! * Fast path for builtin functions: don't bother consulting pg_proc,
! * and assume the function is trusted.
  */
  finfo->fn_nargs = fbp->nargs;
  finfo->fn_strict = fbp->strict;
*************** fmgr_info_cxt_security(Oid functionId, F
*** 186,191 ****
--- 189,212 ----
  finfo->fn_retset = procedureStruct->proretset;
 
  /*
+ * Check to see if the current user trusts the owner of the function.  We
+ * can skip this when recursing, since it was checked already.  If we do
+ * throw an error, go to some lengths to identify the function exactly.
+ */
+ if (!ignore_security &&
+ !role_trusts_role(GetUserId(), procedureStruct->proowner))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("role %s does not trust role %s, which owns function %s.%s",
+ GetUserNameFromId(GetUserId(), false),
+ GetUserNameFromId(procedureStruct->proowner, false),
+ get_namespace_name(procedureStruct->pronamespace),
+ funcname_signature_string(NameStr(procedureStruct->proname),
+  procedureStruct->pronargs,
+  NIL,
+  procedureStruct->proargtypes.values))));
+
+ /*
  * If it has prosecdef set, non-null proconfig, or if a plugin wants to
  * hook function entry/exit, use fmgr_security_definer call handler ---
  * unless we are being called again by fmgr_security_definer or
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index c5ba149..4f7399d 100644
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*************** static char *client_encoding_string;
*** 508,513 ****
--- 508,514 ----
  static char *datestyle_string;
  static char *locale_collate;
  static char *locale_ctype;
+ static char *trusted_roles_string;
  static char *server_encoding_string;
  static char *server_version_string;
  static int server_version_num;
*************** static struct config_string ConfigureNam
*** 3493,3498 ****
--- 3494,3510 ----
  },
 
  {
+ {"trusted_roles", PGC_USERSET, CLIENT_CONN_STATEMENT,
+ gettext_noop("Only functions owned by roles in this list will be executed."),
+ NULL,
+ GUC_LIST_INPUT | GUC_LIST_QUOTE
+ },
+ &trusted_roles_string,
+ "\"$user\"",
+ check_trusted_roles, assign_trusted_roles, NULL
+ },
+
+ {
  /* Can't be set in postgresql.conf */
  {"server_encoding", PGC_INTERNAL, CLIENT_CONN_LOCALE,
  gettext_noop("Sets the server (database) character set encoding."),
diff --git a/src/backend/utils/misc/superuser.c b/src/backend/utils/misc/superuser.c
index fbe83c9..07329ec 100644
*** a/src/backend/utils/misc/superuser.c
--- b/src/backend/utils/misc/superuser.c
*************** superuser_arg(Oid roleid)
*** 63,70 ****
  if (OidIsValid(last_roleid) && last_roleid == roleid)
  return last_roleid_is_super;
 
! /* Special escape path in case you deleted all your users. */
! if (!IsUnderPostmaster && roleid == BOOTSTRAP_SUPERUSERID)
  return true;
 
  /* OK, look up the information in pg_authid */
--- 63,78 ----
  if (OidIsValid(last_roleid) && last_roleid == roleid)
  return last_roleid_is_super;
 
! /*
! * Quick out for the bootstrap superuser, too.  Aside from making trust
! * checks for builtin functions fast, this provides a special escape path
! * in case you deleted all your users.  Standalone backends hardwire their
! * user OID as BOOTSTRAP_SUPERUSERID, and this check ensures that that OID
! * will be given superuser privileges, even if there is no underlying
! * catalog entry.  (This means you can't usefully revoke the bootstrap
! * superuser's superuserness, but that seems fine.)
! */
! if (roleid == BOOTSTRAP_SUPERUSERID)
  return true;
 
  /* OK, look up the information in pg_authid */
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 9baf7b2..b64e08d 100644
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
*************** setup_connection(Archive *AH, const char
*** 1119,1124 ****
--- 1119,1131 ----
  }
 
  /*
+ * If supported, disable trust for all non-superuser-owned functions.
+ * Rather than trying to track which minor versions trusted_roles was
+ * introduced in, issue the SET unconditionally, and ignore any error.
+ */
+ PQclear(PQexec(conn, "SET trusted_roles = ''"));
+
+ /*
  * Start transaction-snapshot mode transaction to dump consistent data.
  */
  ExecuteSqlStatement(AH, "BEGIN");
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index eb29d31..6ff1edd 100644
*** a/src/bin/pg_dump/pg_dumpall.c
--- b/src/bin/pg_dump/pg_dumpall.c
*************** connectDatabase(const char *dbname, cons
*** 1725,1730 ****
--- 1725,1738 ----
 
  PQclear(executeQuery(conn, ALWAYS_SECURE_SEARCH_PATH_SQL));
 
+ /*
+ * If supported, disable trust for all non-superuser-owned functions (not
+ * that there should be any in pg_catalog, but let's be paranoid).  Rather
+ * than trying to track which minor versions trusted_roles was introduced
+ * in, issue the SET unconditionally, and ignore any error.
+ */
+ PQclear(PQexec(conn, "SET trusted_roles = ''"));
+
  return conn;
  }
 
diff --git a/src/include/commands/variable.h b/src/include/commands/variable.h
index 4ea3b02..425bf1a 100644
*** a/src/include/commands/variable.h
--- b/src/include/commands/variable.h
*************** extern void assign_session_authorization
*** 36,40 ****
--- 36,43 ----
  extern bool check_role(char **newval, void **extra, GucSource source);
  extern void assign_role(const char *newval, void *extra);
  extern const char *show_role(void);
+ extern bool check_trusted_roles(char **newval, void **extra, GucSource source);
+ extern void assign_trusted_roles(const char *newval, void *extra);
+ extern bool role_trusts_role(Oid caller, Oid callee);
 
  #endif /* VARIABLE_H */
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index ac8968d..c1abb60 100644
*** a/src/test/regress/expected/privileges.out
--- b/src/test/regress/expected/privileges.out
*************** CREATE FUNCTION leak(integer,integer) RE
*** 197,202 ****
--- 197,203 ----
    LANGUAGE plpgsql immutable;
  CREATE OPERATOR <<< (procedure = leak, leftarg = integer, rightarg = integer,
                       restrict = scalarltsel);
+ SET trusted_roles = "$user", regress_priv_user1;  -- allow execution of leak()
  -- view with leaky operator
  CREATE VIEW atest12v AS
    SELECT * FROM atest12 WHERE b <<< 5;
*************** EXPLAIN (COSTS OFF) SELECT * FROM atest1
*** 281,286 ****
--- 282,288 ----
  -- clean up (regress_priv_user1's objects are all dropped later)
  DROP FUNCTION leak2(integer, integer) CASCADE;
  NOTICE:  drop cascades to operator >>>(integer,integer)
+ RESET trusted_roles;
  -- groups
  SET SESSION AUTHORIZATION regress_priv_user3;
  CREATE TABLE atest3 (one int, two int, three int);
*************** CREATE FUNCTION priv_testfunc4(boolean)
*** 674,679 ****
--- 676,682 ----
    AS 'select col1 from atest2 where col2 = $1;'
    LANGUAGE sql SECURITY DEFINER;
  GRANT EXECUTE ON FUNCTION priv_testfunc4(boolean) TO regress_priv_user3;
+ SET trusted_roles = "$user", regress_priv_user1;  -- allow execution of priv_testfunc4
  SET SESSION AUTHORIZATION regress_priv_user2;
  SELECT priv_testfunc1(5), priv_testfunc2(5); -- ok
   priv_testfunc1 | priv_testfunc2
*************** SELECT priv_testagg1(x) FROM (VALUES (1)
*** 719,724 ****
--- 722,728 ----
  (1 row)
 
  CALL priv_testproc1(6); -- ok
+ RESET trusted_roles;
  DROP FUNCTION priv_testfunc1(int); -- fail
  ERROR:  must be owner of function priv_testfunc1
  DROP AGGREGATE priv_testagg1(int); -- fail
*************** SELECT has_table_privilege('regress_priv
*** 1176,1181 ****
--- 1180,1186 ----
  SET SESSION AUTHORIZATION regress_priv_user4;
  CREATE FUNCTION dogrant_ok() RETURNS void LANGUAGE sql SECURITY DEFINER AS
  'GRANT regress_priv_group2 TO regress_priv_user5';
+ SET trusted_roles = "$user", regress_priv_user4;  -- allow execution of dogrant_ok
  GRANT regress_priv_group2 TO regress_priv_user5; -- ok: had ADMIN OPTION
  SET ROLE regress_priv_group2;
  GRANT regress_priv_group2 TO regress_priv_user5; -- fails: SET ROLE suspended privilege
*************** ERROR:  must have admin option on role "
*** 1203,1208 ****
--- 1208,1214 ----
  CONTEXT:  SQL function "dogrant_fails" statement 1
  DROP FUNCTION dogrant_fails();
  SET SESSION AUTHORIZATION regress_priv_user4;
+ RESET trusted_roles;
  DROP FUNCTION dogrant_ok();
  REVOKE regress_priv_group2 FROM regress_priv_user5;
  -- has_sequence_privilege tests
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index f7f3bbb..434676e 100644
*** a/src/test/regress/sql/privileges.sql
--- b/src/test/regress/sql/privileges.sql
*************** CREATE FUNCTION leak(integer,integer) RE
*** 143,148 ****
--- 143,149 ----
    LANGUAGE plpgsql immutable;
  CREATE OPERATOR <<< (procedure = leak, leftarg = integer, rightarg = integer,
                       restrict = scalarltsel);
+ SET trusted_roles = "$user", regress_priv_user1;  -- allow execution of leak()
 
  -- view with leaky operator
  CREATE VIEW atest12v AS
*************** EXPLAIN (COSTS OFF) SELECT * FROM atest1
*** 187,192 ****
--- 188,195 ----
  -- clean up (regress_priv_user1's objects are all dropped later)
  DROP FUNCTION leak2(integer, integer) CASCADE;
 
+ RESET trusted_roles;
+
 
  -- groups
 
*************** CREATE FUNCTION priv_testfunc4(boolean)
*** 462,467 ****
--- 465,471 ----
    AS 'select col1 from atest2 where col2 = $1;'
    LANGUAGE sql SECURITY DEFINER;
  GRANT EXECUTE ON FUNCTION priv_testfunc4(boolean) TO regress_priv_user3;
+ SET trusted_roles = "$user", regress_priv_user1;  -- allow execution of priv_testfunc4
 
  SET SESSION AUTHORIZATION regress_priv_user2;
  SELECT priv_testfunc1(5), priv_testfunc2(5); -- ok
*************** SELECT priv_testfunc1(5); -- ok
*** 481,486 ****
--- 485,492 ----
  SELECT priv_testagg1(x) FROM (VALUES (1), (2), (3)) _(x); -- ok
  CALL priv_testproc1(6); -- ok
 
+ RESET trusted_roles;
+
  DROP FUNCTION priv_testfunc1(int); -- fail
  DROP AGGREGATE priv_testagg1(int); -- fail
  DROP PROCEDURE priv_testproc1(int); -- fail
*************** SELECT has_table_privilege('regress_priv
*** 748,753 ****
--- 754,761 ----
  SET SESSION AUTHORIZATION regress_priv_user4;
  CREATE FUNCTION dogrant_ok() RETURNS void LANGUAGE sql SECURITY DEFINER AS
  'GRANT regress_priv_group2 TO regress_priv_user5';
+ SET trusted_roles = "$user", regress_priv_user4;  -- allow execution of dogrant_ok
+
  GRANT regress_priv_group2 TO regress_priv_user5; -- ok: had ADMIN OPTION
  SET ROLE regress_priv_group2;
  GRANT regress_priv_group2 TO regress_priv_user5; -- fails: SET ROLE suspended privilege
*************** SELECT dogrant_fails(); -- fails: no s
*** 766,771 ****
--- 774,780 ----
  DROP FUNCTION dogrant_fails();
 
  SET SESSION AUTHORIZATION regress_priv_user4;
+ RESET trusted_roles;
  DROP FUNCTION dogrant_ok();
  REVOKE regress_priv_group2 FROM regress_priv_user5;
 
Reply | Threaded
Open this post in threaded view
|

Re: POC for a function trust mechanism

Bruce Momjian
On Wed, Aug  8, 2018 at 01:15:38PM -0400, Tom Lane wrote:
> This is sort of a counter-proposal to Noah's discussion of search path
> security checking in <[hidden email]>.
> (There's no technical reason we couldn't do both things, but I think
> this'd be more useful to most people.)

Yes, the query from Noah below confirmed that schema qualification just
isn't a realistic approach:

        CREATE FUNCTION latitude(earth)
        RETURNS float8
        LANGUAGE SQL
        IMMUTABLE STRICT
        PARALLEL SAFE
        AS 'SELECT CASE
        WHEN @cube_schema(at)(dot)cube_ll_coord($1, 3) OPERATOR(pg_catalog./)
        @extschema(at)(dot)earth() OPERATOR(pg_catalog.<) -1 THEN -90::pg_catalog.float8
        WHEN @cube_schema(at)(dot)cube_ll_coord($1, 3) OPERATOR(pg_catalog./)
        @extschema(at)(dot)earth() OPERATOR(pg_catalog.>) 1 THEN 90::pg_catalog.float8
        ELSE pg_catalog.degrees(pg_catalog.asin(@cube_schema(at)(dot)cube_ll_coord($1, 3)
        OPERATOR(pg_catalog./) @extschema(at)(dot)earth()))
        END';

Of course, with the limitations of backpatching and security-only
discussion, that was the best we could do in the past.

> The core idea here is to prevent security problems not by changing an
> application's rules for operator/function name resolution, but by
> detecting an attempted compromise and preventing the trojan-horse code
> from being executed.  Essentially, a user or application is to declare
> a list of roles that it trusts functions owned by, and the system will
> then refuse to execute functions owned by other not-trusted roles.
> So, if $badguy creates a trojan-horse operator and manages to capture
> a call from your SQL code, he'll nonetheless not be able to execute
> code as you.

Yes, this is the only reasonable approach I can think of.

> To reduce the overhead of the mechanism and chance of unintentionally
> breaking things, superuser-owned functions (particularly, all built-in
> functions) are always trusted by everybody.  A superuser who wants to
> become you can do so trivially, with no need for a trojan horse, so
> this restriction isn't creating any new security hole.

Agreed.

> The things that we hadn't resolved, which is why this didn't get further
> than POC stage, were
>
> (1) What's the mechanism for declaring trust?  In this POC, it's just
> a GUC that you can set to a list of role names, with $user for yourself
> and "public" if you want to trust everybody.  It's not clear if that's
> good enough, or if we want something a bit more locked-down.

Yes, works for me.

> (2) Is trust transitive?  Where and how would the list of trusted roles
> change?  Arguably, if you call a SECURITY DEFINER function, then once
> you've decided that you trust the function owner, actual execution of the
> function should use the function owner's list of trusted roles not yours.
> With the GUC approach, it'd be necessary for SECURITY DEFINER functions
> to implement this with a "SET trusted_roles" clause, much as they now
> have to do with search_path.  That's possible but it's again not very
> non-invasive, so we'd been speculating about automating this more.
> If we had, say, a catalog that provided the desired list of trusted roles
> for every role, then we could imagine implementing that context change
> automatically.  Likewise, stuff like autovacuum or REINDEX would want
> to run with the table owner's list of trusted roles, but the GUC approach
> doesn't really provide enough infrastructure to know what to do there.

I can't think of any other places we do transitive permissions, except
for role membership.  I don't see the logic in adding such transitivity
to function/operator calls, or even a per-function GUC.  I assume most
sites have a small number of extensions installed by a predefined group
of users, usually superusers. If there is a larger group, a group role
should be created and those people put in the role, and the group role
trusted.

--
  Bruce Momjian  <[hidden email]>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

Reply | Threaded
Open this post in threaded view
|

Re: POC for a function trust mechanism

David Kohn


On Thu, Aug 9, 2018 at 12:11 PM Bruce Momjian <[hidden email]> wrote:
...

> The things that we hadn't resolved, which is why this didn't get further
> than POC stage, were
>
> (1) What's the mechanism for declaring trust?  In this POC, it's just
> a GUC that you can set to a list of role names, with $user for yourself
> and "public" if you want to trust everybody.  It's not clear if that's
> good enough, or if we want something a bit more locked-down.

Yes, works for me.

> (2) Is trust transitive?  Where and how would the list of trusted roles
> change?  Arguably, if you call a SECURITY DEFINER function, then once
> you've decided that you trust the function owner, actual execution of the
> function should use the function owner's list of trusted roles not yours.
> With the GUC approach, it'd be necessary for SECURITY DEFINER functions
> to implement this with a "SET trusted_roles" clause, much as they now
> have to do with search_path.  That's possible but it's again not very
> non-invasive, so we'd been speculating about automating this more.
> If we had, say, a catalog that provided the desired list of trusted roles
> for every role, then we could imagine implementing that context change
> automatically.  Likewise, stuff like autovacuum or REINDEX would want
> to run with the table owner's list of trusted roles, but the GUC approach
> doesn't really provide enough infrastructure to know what to do there.

I can't think of any other places we do transitive permissions, except
for role membership.  I don't see the logic in adding such transitivity
to function/operator calls, or even a per-function GUC.  I assume most
sites have a small number of extensions installed by a predefined group
of users, usually superusers. If there is a larger group, a group role
should be created and those people put in the role, and the group role
trusted.

I am wondering how this will interact with the inheritance of roles. For instance, if two users are members of the same role, and one creates a function the expectation would be that other users in the same role will not trust that function. However, do I trust functions that are owned by the roles that I am a member of? Or do I have to list any nested roles explicitly? If the former, I suppose we'd have to modify how alter function set owner works. It is currently allowed for roles that you are a member of (and would then create a security hole). However, not trusting functions owned by roles that I am a member of seems to also be a bit counterintuitive. 
Best,
David
Reply | Threaded
Open this post in threaded view
|

Re: POC for a function trust mechanism

Bruce Momjian
On Thu, Aug  9, 2018 at 02:12:41PM -0400, David Kohn wrote:

> On Thu, Aug 9, 2018 at 12:11 PM Bruce Momjian <[hidden email]> wrote:
>     I can't think of any other places we do transitive permissions, except
>     for role membership.  I don't see the logic in adding such transitivity
>     to function/operator calls, or even a per-function GUC.  I assume most
>     sites have a small number of extensions installed by a predefined group
>     of users, usually superusers. If there is a larger group, a group role
>     should be created and those people put in the role, and the group role
>     trusted.
>
>
> I am wondering how this will interact with the inheritance of roles. For
> instance, if two users are members of the same role, and one creates a function
> the expectation would be that other users in the same role will not trust that
> function.

Well, right now, if you want to give members of a role rights to
something, you have to specifically grant rights to that role.  I would
assume the same thing would happen here --- if you want to trust a group
role, you have to mention that group role in the GUC list (not
function-level GUC).

Do we allow any GUC on a function?  Would not allowing this be confusing?

If we did transitive permissions, I could trust someone, and that person
could call a function of someone else they trust, and after a while you
don't know who you are trusting, which is why I think complex setups
like that are unwise.

> However, do I trust functions that are owned by the roles that I am a
> member of? Or do I have to list any nested roles explicitly? If the former, I
> suppose we'd have to modify how alter function set owner works. It is currently
> allowed for roles that you are a member of (and would then create a security
> hole). However, not trusting functions owned by roles that I am a member of
> seems to also be a bit counterintuitive.

Well, if someone adds me to the 'bad' role, do I have any control over
that?  Seems someone adding me to their role is not something I am
requesting.  Let's look at the docs on GRANT ROLE:

   GRANT on Roles
       This variant of the GRANT command grants membership in a role to
       one or more other roles. Membership in a role is significant because
       it conveys the privileges granted to a role to each of its members.

       If WITH ADMIN OPTION is specified, the member can in turn grant
       membership in the role to others, and revoke membership in the role
       as well. Without the admin option, ordinary users cannot do that. A
       role is not considered to hold WITH ADMIN OPTION on itself, but it
       may grant or revoke membership in itself from a database session
       where the session user matches the role. Database superusers can
       grant or revoke membership in any role to anyone. Roles having
       CREATEROLE privilege can grant or revoke membership in any role
       that is not a superuser.

       Unlike the case with privileges, membership in a role cannot be
       granted to PUBLIC. Note also that this form of the command does
       not allow the noise word GROUP.

The point is that it is granting the role _access_ to something, not
something trust that the role accepts.  The WITH ADMIN OPTION would
allow ordinary users to add roles for whoever they want to attack.

Basically, as it is now, someone adding me to their role membership has
no downside for me.  To trust my own role membership adds a downside to
role membership that I don't think we want to do --- it makes role
membership too complex in what it grants _and_ trusts.

--
  Bruce Momjian  <[hidden email]>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

Reply | Threaded
Open this post in threaded view
|

Re: POC for a function trust mechanism

Nico Williams
In reply to this post by Tom Lane-2
On Wed, Aug 08, 2018 at 01:15:38PM -0400, Tom Lane wrote:
> This is sort of a counter-proposal to Noah's discussion of search path
> security checking in <[hidden email]>.
> (There's no technical reason we couldn't do both things, but I think
> this'd be more useful to most people.)

So, this is why I always fully-qualify all references to functions,
tables, etc.  I also always set a search_path on each function just in
case I accidentally leave a non-fully-qualified symbol.

I would like to have a way to request that all non-fully-qualified
symbols be resolved at function create/replace time and that the
resolution results be made permanent for the function.  If I have
several schemas in a search_path at function definition time, this would
not allow me to move dependencies around without replacing the
dependents -- that's OK for me.

Nico
--

Reply | Threaded
Open this post in threaded view
|

Re: POC for a function trust mechanism

David Kohn
In reply to this post by Bruce Momjian


On Thu, Aug 9, 2018 at 3:04 PM Bruce Momjian <[hidden email]> wrote:


Well, right now, if you want to give members of a role rights to
something, you have to specifically grant rights to that role.  I would
assume the same thing would happen here --- if you want to trust a group
role, you have to mention that group role in the GUC list (not
function-level GUC).
Sure, but if I grant execute on a function to a role, members of that role will be able to execute that function. Now, each member will (potentially) need to update their trust list before doing that. Which seems a bit odd. Or will I be able to modify the some sort of default trust list of the group role? If not, it seems like it could be an administrative nightmare, if so there are potential issues with who is allowed to modify the list of trusted users that then gets inherited. 
...

Basically, as it is now, someone adding me to their role membership has
no downside for me.  To trust my own role membership adds a downside to
role membership that I don't think we want to do --- it makes role
membership too complex in what it grants _and_ trusts.

Makes sense, and I can see how that could get out of hand in terms of figuring out who you trust. I guess I don't know of other cases where this concept of trusting comes about in our current permissions system? And it seems to introduce a lot of odd cases where you end up with a sort of permissions error or I guess a trust error in this case. 

One possibility that might help this would be to only use the check this if a) the user who created the function isn't in the trust list and b) there is a function with the same name and equivalent argument classes that would be called if you weren't to call the untrusted user's function. So it is only used for disambiguation. 

Best,
David
Reply | Threaded
Open this post in threaded view
|

Re: POC for a function trust mechanism

Bruce Momjian
On Thu, Aug  9, 2018 at 04:01:09PM -0400, David Kohn wrote:

>
>
> On Thu, Aug 9, 2018 at 3:04 PM Bruce Momjian <[hidden email]> wrote:
>
>
>
>     Well, right now, if you want to give members of a role rights to
>     something, you have to specifically grant rights to that role.  I would
>     assume the same thing would happen here --- if you want to trust a group
>     role, you have to mention that group role in the GUC list (not
>     function-level GUC).
>
> Sure, but if I grant execute on a function to a role, members of that role will
> be able to execute that function. Now, each member will (potentially) need to
> update their trust list before doing that. Which seems a bit odd. Or will I be

Look at your wording above, "I grant execute" --- you are opening up
permissions to them.  There is no approval on their part that signifies
they should trust you.  If I open permissions for a file on my web
server, it doesn't mean people should trust me more than before.

> able to modify the some sort of default trust list of the group role? If not,
> it seems like it could be an administrative nightmare, if so there are
> potential issues with who is allowed to modify the list of trusted users that
> then gets inherited.

We certainly don't want to double-down on extending trust by allowing
someone to modify someone else's trusted role list.  Practically, if you
are opening up permissions to someone, you will need to create a group
that you both belong to first, and have them trust the group, or they
can trust you directly.  The benefit of a group role is that other
people can be added to that group without having to modify the trusted
role list.  Basically, the function caller is trusting whoever controls
membership to that group role.  This is different from having someone
trusting a role just because they were added to it (perhaps without
their knowledge).


>     Basically, as it is now, someone adding me to their role membership has
>     no downside for me.  To trust my own role membership adds a downside to
>     role membership that I don't think we want to do --- it makes role
>     membership too complex in what it grants _and_ trusts.
>
> Makes sense, and I can see how that could get out of hand in terms of figuring
> out who you trust. I guess I don't know of other cases where this concept of
> trusting comes about in our current permissions system? And it seems to
> introduce a lot of odd cases where you end up with a sort of permissions error
> or I guess a trust error in this case. 

Yep.

> One possibility that might help this would be to only use the check this if a)
> the user who created the function isn't in the trust list and b) there is a
> function with the same name and equivalent argument classes that would be
> called if you weren't to call the untrusted user's function. So it is only used
> for disambiguation. 

You can't do that because if you do, someone creating a ambiguous
function would cause a denial-of-service attack --- better to fail at
the time of the first function, when you are likely watching the output.

--
  Bruce Momjian  <[hidden email]>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

Reply | Threaded
Open this post in threaded view
|

Re: POC for a function trust mechanism

David Kohn
We certainly don't want to double-down on extending trust by allowing
someone to modify someone else's trusted role list.  Practically, if you
are opening up permissions to someone, you will need to create a group
that you both belong to first, and have them trust the group, or they
can trust you directly.  The benefit of a group role is that other
people can be added to that group without having to modify the trusted
role list.  Basically, the function caller is trusting whoever controls
membership to that group role.  This is different from having someone
trusting a role just because they were added to it (perhaps without
their knowledge).

I think if you trust a group role you are implicitly trusting any members of that group, because one can always alter the function owner from themselves to the group role, because they are a member of that group. So what you'd need to do is create a special group role that only owned the functions and then not make anyone an actual member of that group, but you could trust that group role. Then a separate group role that everyone would be a member of and you'd do grants from the first role to the second. 
So for what you proposed, if you are opening up permissions to someone by using a role that you are both members of, then you implicitly open up permissions from them to you as well. 

Anyway, I guess all of this seems to introduce a lot more complexity into an already complex permissions management system...is this all about the public schema? Can we just make create function/operator etc something you have to grant even in the public schema? It seems like that could be significantly more user friendly than this. Or otherwise, would functions owned by the database or schema owner be exempt from this? Because there are many setups where people try to avoid superuser usage by creating database or schema owner users who can do things like create function, which a normal users can now use. Would checks be skipped if the function call is schema qualified because then there's no reasonable way to think that someone is being fooled about which function they are executing? 

Best,
David
Reply | Threaded
Open this post in threaded view
|

Re: POC for a function trust mechanism

Isaac Morland
On 9 August 2018 at 18:18, David Kohn <[hidden email]> wrote:

Anyway, I guess all of this seems to introduce a lot more complexity into an already complex permissions management system...is this all about the public schema? Can we just make create function/operator etc something you have to grant even in the public schema? It seems like that could be significantly more user friendly than this.

Already true, if you do:

REVOKE CREATE ON SCHEMA public FROM PUBLIC;

Which I do, in all my databases, and which is probably a good idea in most scenarios.
 
Or otherwise, would functions owned by the database or schema owner be exempt from this? Because there are many setups where people try to avoid superuser usage by creating database or schema owner users who can do things like create function, which a normal users can now use. Would checks be skipped if the function call is schema qualified because then there's no reasonable way to think that someone is being fooled about which function they are executing? 

At present, permissions are completely separate from ownership: your ability to use an object does not depend on who owns what (I believe you can even revoke your own rights to use your own stuff). I suspect changing this is probably not a good idea.
Reply | Threaded
Open this post in threaded view
|

Re: POC for a function trust mechanism

Bruce Momjian
In reply to this post by David Kohn
On Thu, Aug  9, 2018 at 06:18:16PM -0400, David Kohn wrote:

>     We certainly don't want to double-down on extending trust by allowing
>     someone to modify someone else's trusted role list.  Practically, if you
>     are opening up permissions to someone, you will need to create a group
>     that you both belong to first, and have them trust the group, or they
>     can trust you directly.  The benefit of a group role is that other
>     people can be added to that group without having to modify the trusted
>     role list.  Basically, the function caller is trusting whoever controls
>     membership to that group role.  This is different from having someone
>     trusting a role just because they were added to it (perhaps without
>     their knowledge).
>
>
> I think if you trust a group role you are implicitly trusting any members of
> that group, because one can always alter the function owner from themselves to
> the group role, because they are a member of that group. So what you'd need to
> do is create a special group role that only owned the functions and then not
> make anyone an actual member of that group, but you could trust that group
> role. Then a separate group role that everyone would be a member of and you'd

Good point.  I think you are right that if you trust a role, you trust
whoever controls role membership to add only trust-worthy people --- that
seems obvious because any member can create functions you will trust,
even if they don't change the function owner.

> do grants from the first role to the second. 
> So for what you proposed, if you are opening up permissions to someone by using
> a role that you are both members of, then you implicitly open up permissions
> from them to you as well. 

I don't see the value in that.  My point is that you can't trust anyone
in a role you are a member of, by default --- you have to make that
decision as a user.

> Anyway, I guess all of this seems to introduce a lot more complexity into an
> already complex permissions management system...is this all about the public
> schema? Can we just make create function/operator etc something you have to
> grant even in the public schema? It seems like that could be significantly more
> user friendly than this. Or otherwise, would functions owned by the database or

I think 95% of users are proabably creating these things as the
accessing user or super-user.

> schema owner be exempt from this? Because there are many setups where people
> try to avoid superuser usage by creating database or schema owner users who can
> do things like create function, which a normal users can now use. Would checks
> be skipped if the function call is schema qualified because then there's no
> reasonable way to think that someone is being fooled about which function they
> are executing? 

I think most setups will be pretty simple.

--
  Bruce Momjian  <[hidden email]>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

Reply | Threaded
Open this post in threaded view
|

Re: POC for a function trust mechanism

Robert Haas
In reply to this post by Tom Lane-2
On Wed, Aug 8, 2018 at 1:15 PM, Tom Lane <[hidden email]> wrote:
> that they had to worry about this themselves.  Of the various ideas that
> we'd kicked around and not been able to finish, the one that seemed most
> promising to me was to invent a "function trust" mechanism.

In the interest of giving credit where credit is due, I went back and
researched this.  I discovered that you were the first one to suggest
this idea, and that Noah was the first to produce a completed patch
for it.  That was in 2013.

(Also in 2013, I was arguing that the first thing we should do is fix
pg_dump and our in-core tools to be secure against this sort of
attack.  Noah had a patch for it, in 2013.  Five years later, that was
indeed the first thing we did.  We should have done it back then.)

> The things that we hadn't resolved, which is why this didn't get further
> than POC stage, were
>
> (1) What's the mechanism for declaring trust?  In this POC, it's just
> a GUC that you can set to a list of role names, with $user for yourself
> and "public" if you want to trust everybody.  It's not clear if that's
> good enough, or if we want something a bit more locked-down.

Back in 2013, I pointed out that we need to consider the case where
the database owner has done ALTER DATABASE .. SET search_path =
'malevolence', hoping that the superuser will log in and do something
that allows control to be captured. If trusted_roles is merely a GUC,
then the database owner could set that, too.  If we want this solution
to be water-tight, I think we need a way to make very sure that a user
never uses a trust setting configured by someone else.  We could
implement a special rule (as you proposed back at the time) that
limits the ability to ALTER DATABASE .. SET trusted_roles, but I'm a
little worried that there may be other ways that one user could end up
using a trusted_roles setting configured by somebody else.  I wonder
whether thoroughly isolating the manner of configuring trust from the
GUC system might make it easier to avoid unpredictable interactions.

> (2) Is trust transitive?  Where and how would the list of trusted roles
> change?  Arguably, if you call a SECURITY DEFINER function, then once
> you've decided that you trust the function owner, actual execution of the
> function should use the function owner's list of trusted roles not yours.
> With the GUC approach, it'd be necessary for SECURITY DEFINER functions
> to implement this with a "SET trusted_roles" clause, much as they now
> have to do with search_path.  That's possible but it's again not very
> non-invasive, so we'd been speculating about automating this more.
> If we had, say, a catalog that provided the desired list of trusted roles
> for every role, then we could imagine implementing that context change
> automatically.  Likewise, stuff like autovacuum or REINDEX would want
> to run with the table owner's list of trusted roles, but the GUC approach
> doesn't really provide enough infrastructure to know what to do there.

Yeah, I think these are all good points.  It seems natural for trust
to be a property of a role, for just the reasons that you mention.
However, there does also seem to be a use case for varying it
temporarily on a per-session or per-function basis, and I'm not
exactly sure how to cater to those needs.  I have a feeling that it
would be really useful to attach a listed of trusted roles, and for
that matter also a search path, to the *lexical scope* of a function.

> So we'd kind of decided that the GUC solution wasn't good enough, but
> it didn't seem like catalog additions would be feasible as a back-patched
> security fix, which is why this didn't go anywhere.  But it could work
> as a new feature.

Check.

> Anyway, I had written a small POC that did use a GUC for this, and just
> checked function calls without any attempts to switch the active
> trusted_roles setting in places like autovacuum.  I've rebased it up to
> HEAD and here it is.

I wonder if Noah would like to rebase and post his version also.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Previous Thread Next Thread