Обсуждение: POC for a function trust mechanism

This is sort of a counter-proposal to Noah's discussion of search path
security checking in <20180805080441.GH1688868@rfd.leadboat.com>.
(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;

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 <20180805080441.GH1688868@rfd.leadboat.com>.
> (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  <bruce@momjian.us>        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 +

On Thu, Aug  9, 2018 at 02:12:41PM -0400, David Kohn wrote:
> On Thu, Aug 9, 2018 at 12:11 PM Bruce Momjian <bruce@momjian.us> 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  <bruce@momjian.us>        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 +

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 <20180805080441.GH1688868@rfd.leadboat.com>.
> (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
-- 

On Thu, Aug  9, 2018 at 04:01:09PM -0400, David Kohn wrote:
> 
> 
> On Thu, Aug 9, 2018 at 3:04 PM Bruce Momjian <bruce@momjian.us> 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  <bruce@momjian.us>        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 +

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  <bruce@momjian.us>        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 +

On Wed, Aug 8, 2018 at 1:15 PM, Tom Lane <tgl@sss.pgh.pa.us> 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

On Sun, Aug 12, 2018 at 10:40:30PM -0400, Robert Haas wrote:
> On Wed, Aug 8, 2018 at 1:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > 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.

Yep.  A GUC is great for src/bin sessions, since many of those applications
consistently want tight trust settings.

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

Sure, attached (last merge at 757c518).  The owner_trustcheck() header comment
is a good starting point for reading it.  Tom Lane later asserted that it's
okay to perform the function trust check in fmgr_info_cxt_security() instead
of doing it in most fmgr_info() callers and some *FunctionCall* callers.
While I suspected he was right, I have not made that change in this rebase.
(I also haven't audited every fmgr_info() call added after -v1.)  When I
shared -v1 with the security team, I included the following submission notes:

===

Here's an implementation.  Design decisions:

1. A role trusts itself implicitly

2. Default pg_auth_trust entry for "public TRUST public"

This effectively disables the new restrictions; concerned administrators will
test their applications with it removed, then remove it.

3. Default pg_auth_trust entry for "public TRUST <bootstrap superuser>"

Almost everyone will leave this untouched.

4. Changing trust for a role requires ADMIN OPTION on the role.

Trust is the next step down from granting role membership.  ("ALTER ROLE
public TRUST ..." does require superuser.)

5. Non-inheritance of trust

Trust is like a reverse permission; I find it helpful to think of "ALTER ROLE
foo TRUST bar" as "GRANT may_supply_functions_to_foo TO bar".  It would be
reasonable to have that trust relationship be effective for INHERITS members
of bar; after all, they could always "ALTER FUNCTION ... OWNER TO bar".  For
now, trust relationships are not subject to inheritance.  This keeps things
simpler to understand and poses no major downsides.  For similar reasons, I
have not made a role trust its own members implicitly.

6. Non-transitivity of trust

If I trust only alice, alice trusts only bob, and I call alice's function that
calls bob's function, should the call succeed?  This is a tough one.  In favor
of "yes", this choice would allow alice to refactor her function without
needing her callers to revise their trust settings.  That is, she could switch
from bob's function to carol's function, and her callers would never notice.
My trust in alice is probably misplaced if she chooses her friends badly.

In favor of "no", making the trust relationship transitive seems to mix two
otherwise-separate concepts: alice's willingness to run code as herself and
alice's willingness to certify third-party functions to her friends.  In
particular, current defects in the system are at odds with conflating those
concepts.  Everyone should trust the bootstrap superuser, but it currently
owns functions like integer_pl_date that are not careful in what they call.
That closed the issue for me; trust is not transitive by default.  Even so, I
think there would be value in a facility for requesting trust transitivity in
specific situations.

7. (Lack of) negative trust entries

There's no way to opt out of a PUBLIC trust relationship; until a superuser
removes the "public TRUST public" entry, no user can achieve anything with
ALTER USER [NO] TRUST.  I considered adding the concept of a negative trust
relationship to remedy this problem; it could also be used to remove the
implicit self-trust for testing purposes.  I have refrained; I don't know
whether the benefits would pay for the extra user-facing complexity.

8. Applicable roles during trust checks

We had examined whether the check could be looser for SECURITY DEFINER
functions, since those can't perform arbitrary actions as the caller.  I
described some challenges then, but a deeper look turned up more:  a SECURITY
DEFINER function can do things like "SET search_path = ..." that affect the
caller.  Consequently, I concluded that all roles on the active call stack
should have a stake in whether a particular function owner is permitted.  Each
trust check walks the call stack and checks every role represented there; if
any lacks the trust relationships to approve the newly-contemplated function
call, the call is rejected.

The stack traversal may and does stop at the edge of a security-restricted
operation; since those restrictions must prevent important session changes,
the stack entries active before the operation started need not be concerned
with the code running therein.  A CREATE FUNCTION option specifying voluntary
use of a security-restricted context would provide the "facility for
requesting trust transitivity" mentioned earlier.

To make walking the role stack possible, I redid the API for changing the
current user ID.  SetUserIdAndSecContext() gives way to PushTransientUser()
and PopTransientUser(); xact.c has its own little API for unwinding this stack
at (sub)transaction abort.  This patch just removes the old APIs, but that
probably wouldn't cut it for the back branches; I do have a design sketch for
reintroducing backward-compatible versions.

9. Trust checks on other object types

Despite trust checks on functions, hostile users can make mischief with
something like "CREATE OPERATOR * (PROCEDURE = int4pl, LEFTARG = int4,
RIGHTARG = int4)".  Therefore, I also caused trust checks to be enforced on
operators themselves.  As I opine in the comments at owner_trustcheck(), where
to stop is more a judgement call than a science.


The following work remains TODO:
- Documentation for the new concepts, syntax and system catalog
- pg_dumpall support
- Testing the performance impact and perhaps adding a cache
- Backward-compatible versions of removed miscinit.c functions