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 6a856170d6c2aaa7ee5f90db8070444441fe2f66..4076312799d58eae7e86e5ea6b296c8f6470bd41 100644 (file)
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -9,7 +9,7 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/commands/variable.c,v 1.88.2.4 2008/01/03 21:25:33 tgl Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/commands/variable.c,v 1.88.2.5 2009/09/03 22:09:05 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -773,22 +773,6 @@ assign_session_authorization(const char *value, bool doit, bool interactive)
                /* not a saved ID, so look it up */
                HeapTuple       userTup;
 
-               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.
-                        */
-                       if (interactive)
-                               ereport(ERROR,
-                                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                                                errmsg("cannot set session authorization within security-definer function")));
-                       return NULL;
-               }
-
                if (!IsTransactionState())
                {
                        /*

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 227ecf62fdaffa2750dab40c7b4de1e8890d9e92..5e72d578913de977563d3b9d0a0a7dea2a23ab8d 100644 (file)
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -10,7 +10,7 @@
  * Written by Peter Eisentraut <[email protected]>.
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/utils/misc/guc.c,v 1.164.2.5 2006/05/21 20:11:58 tgl Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/utils/misc/guc.c,v 1.164.2.6 2009/09/03 22:09:05 tgl Exp $
  *
  *--------------------------------------------------------------------
  */
@@ -1511,7 +1511,7 @@ static struct config_string ConfigureNamesString[] =
                {"session_authorization", PGC_USERSET, UNGROUPED,
                        gettext_noop("Shows 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
@@ -2514,6 +2514,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".
+        * 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 report errors interactively? */
        interactive = (source >= PGC_S_SESSION);
 

diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h
index 553d3fc531b029a4397b2044116405957cca6715..d04dc7a41dcfc678d445933c21ebb9a4f0c52354 100644 (file)
--- a/src/include/utils/guc_tables.h
+++ b/src/include/utils/guc_tables.h
@@ -7,7 +7,7 @@
  *
  * Portions Copyright (c) 1996-2003, PostgreSQL Global Development Group
  *
- *       $Id: guc_tables.h,v 1.6.4.1 2006/02/12 22:33:29 tgl Exp $
+ *       $Id: guc_tables.h,v 1.6.4.2 2009/09/03 22:09:06 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -99,6 +99,8 @@ struct config_generic
 #define GUC_DISALLOW_IN_FILE   0x0040  /* can't set in postgresql.conf */
 #define GUC_IS_NAME                            0x0080  /* limit string to NAMEDATALEN-1 */
 
+#define GUC_NOT_WHILE_SEC_DEF  0x8000  /* can't change inside sec-def func */
+
 /* bit values in status field */
 #define GUC_HAVE_TENTATIVE     0x0001          /* tentative value is defined */
 #define GUC_HAVE_LOCAL         0x0002          /* a SET LOCAL has been executed */