Disallow RESET ROLE and RESET SESSION AUTHORIZATION inside security-definer

functions.

This extends the previous patch that forbade SETting these variables inside
security-definer functions.  RESET is equally a security hole, since it
would allow regaining privileges of the caller; furthermore it can trigger
Assert failures and perhaps other internal errors, since the code is not
expecting these variables to change in such contexts.  The previous patch
did not cover this case because assign hooks don't really have enough
information, so move the responsibility for preventing this into guc.c.

Problem discovered by Heikki Linnakangas.

Security: no CVE assigned yet, extends CVE-2007-6600

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 1374b65122939e03f9b86944f1116596fa128b42..ca6babb048a6608612ca6514776bd0095f2a5d14 100644 (file)
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -9,7 +9,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/commands/variable.c,v 1.130 2009/06/11 14:48:56 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/commands/variable.c,v 1.131 2009/09/03 22:08:05 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -717,21 +717,6 @@ assign_session_authorization(const char *value, bool doit, GucSource source)
                /* not a saved ID, so look it up */
                HeapTuple       roleTup;
 
-               if (InSecurityDefinerContext())
-               {
-                       /*
-                        * Disallow SET SESSION AUTHORIZATION inside a security definer
-                        * context.  We need to do this because when we exit the context,
-                        * GUC won't be notified, leaving things out of sync.  Note that
-                        * this test is positioned so that restoring a previously saved
-                        * setting isn't prevented.
-                        */
-                       ereport(GUC_complaint_elevel(source),
-                                       (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                                        errmsg("cannot set session authorization within security-definer function")));
-                       return NULL;
-               }
-
                if (!IsTransactionState())
                {
                        /*
@@ -838,24 +823,6 @@ assign_role(const char *value, bool doit, GucSource source)
                }
        }
 
-       if (roleid == InvalidOid && InSecurityDefinerContext())
-       {
-               /*
-                * Disallow SET ROLE inside a security definer context.  We need to do
-                * this because when we exit the context, GUC won't be notified,
-                * leaving things out of sync.  Note that this test is arranged so
-                * that restoring a previously saved setting isn't prevented.
-                *
-                * XXX it would be nice to allow this case in future, with the
-                * behavior being that the SET ROLE's effects end when the security
-                * definer context is exited.
-                */
-               ereport(GUC_complaint_elevel(source),
-                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                                errmsg("cannot set role within security-definer function")));
-               return NULL;
-       }
-
        if (roleid == InvalidOid &&
                strcmp(actual_rolename, "none") != 0)
        {

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 03e2408737e849fdc5ba036b9407f569b85bc2fc..70dcefbc315616986d924cab8f447eb3c04cca19 100644 (file)
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -10,7 +10,7 @@
  * Written by Peter Eisentraut <peter_e@gmx.net>.
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/utils/misc/guc.c,v 1.514 2009/08/31 19:41:00 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/utils/misc/guc.c,v 1.515 2009/09/03 22:08:05 tgl Exp $
  *
  *--------------------------------------------------------------------
  */
@@ -2321,7 +2321,7 @@ static struct config_string ConfigureNamesString[] =
                {"role", PGC_USERSET, UNGROUPED,
                        gettext_noop("Sets the current role."),
                        NULL,
-                       GUC_IS_NAME | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+                       GUC_IS_NAME | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_NOT_WHILE_SEC_DEF
                },
                &role_string,
                "none", assign_role, show_role
@@ -2332,7 +2332,7 @@ static struct config_string ConfigureNamesString[] =
                {"session_authorization", PGC_USERSET, UNGROUPED,
                        gettext_noop("Sets the session user name."),
                        NULL,
-                       GUC_IS_NAME | GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+                       GUC_IS_NAME | GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_NOT_WHILE_SEC_DEF
                },
                &session_authorization_string,
                NULL, assign_session_authorization, show_session_authorization
@@ -4661,6 +4661,32 @@ set_config_option(const char *name, const char *value,
                        break;
        }
 
+       /*
+        * Disallow changing GUC_NOT_WHILE_SEC_DEF values if we are inside a
+        * security-definer function.  We can reject this regardless of
+        * the context or source, mainly because sources that it might be
+        * reasonable to override for won't be seen while inside a function.
+        *
+        * Note: variables marked GUC_NOT_WHILE_SEC_DEF should probably be marked
+        * GUC_NO_RESET_ALL as well, because ResetAllOptions() doesn't check this.
+        *
+        * Note: this flag is currently used for "session_authorization" and
+        * "role".  We need to prohibit this because when we exit the sec-def
+        * context, GUC won't be notified, leaving things out of sync.
+        *
+        * XXX it would be nice to allow these cases in future, with the behavior
+        * being that the SET's effects end when the security definer context is
+        * exited.
+        */
+       if ((record->flags & GUC_NOT_WHILE_SEC_DEF) && InSecurityDefinerContext())
+       {
+               ereport(elevel,
+                               (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+                                errmsg("cannot set parameter \"%s\" within security-definer function",
+                                               name)));
+               return false;
+       }
+
        /*
         * Should we set reset/stacked values?  (If so, the behavior is not
         * transactional.)      This is done either when we get a default value from

diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 10eb70d01893c94bcb55cb27211df547f83277ff..6f4acdef70185aaaecf0a64541d6e534e2e5b229 100644 (file)
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -7,7 +7,7 @@
  * Copyright (c) 2000-2009, PostgreSQL Global Development Group
  * Written by Peter Eisentraut <peter_e@gmx.net>.
  *
- * $PostgreSQL: pgsql/src/include/utils/guc.h,v 1.103 2009/08/29 19:26:52 tgl Exp $
+ * $PostgreSQL: pgsql/src/include/utils/guc.h,v 1.104 2009/09/03 22:08:05 tgl Exp $
  *--------------------------------------------------------------------
  */
 #ifndef GUC_H
@@ -146,6 +146,8 @@ typedef enum
 #define GUC_UNIT_MIN                   0x4000  /* value is in minutes */
 #define GUC_UNIT_TIME                  0x7000  /* mask for MS, S, MIN */
 
+#define GUC_NOT_WHILE_SEC_DEF  0x8000  /* can't change inside sec-def func */
+
 /* GUC vars that are actually declared in guc.c, rather than elsewhere */
 extern bool log_duration;
 extern bool Debug_print_plan;