POC for a function trust mechanism - Mailing list pgsql-hackers

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;