]> granicus.if.org Git - postgresql/commitdiff
Enforce superuser permissions checks during ALTER ROLE/DATABASE SET, rather
authorTom Lane <tgl@sss.pgh.pa.us>
Wed, 21 Apr 2010 20:54:19 +0000 (20:54 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Wed, 21 Apr 2010 20:54:19 +0000 (20:54 +0000)
than during define_custom_variable().  This entails rejecting an ALTER
command if the target variable doesn't have a known (non-placeholder)
definition, unless the calling user is superuser.  When the variable *is*
known, we can correctly apply the rule that only superusers can issue ALTER
for SUSET parameters.  This allows define_custom_variable to apply ALTER's
values for SUSET parameters at module load time, secure in the knowledge
that only a superuser could have set the ALTER value.  This change fixes a
longstanding gotcha in the usage of SUSET-level custom parameters; which
is a good thing to fix now that plpgsql defines such a parameter.

doc/src/sgml/ref/alter_role.sgml
src/backend/utils/misc/guc.c

index 3a2504cd0a0c3e5207a314850e46a1fa11988a82..dfd7b7c405c2038917730d912377f0628efdf0ef 100644 (file)
@@ -1,5 +1,5 @@
 <!--
-$PostgreSQL: pgsql/doc/src/sgml/ref/alter_role.sgml,v 1.16 2010/04/03 07:22:57 petere Exp $
+$PostgreSQL: pgsql/doc/src/sgml/ref/alter_role.sgml,v 1.17 2010/04/21 20:54:19 tgl Exp $
 PostgreSQL documentation
 -->
 
@@ -24,7 +24,7 @@ PostgreSQL documentation
 ALTER ROLE <replaceable class="PARAMETER">name</replaceable> [ [ WITH ] <replaceable class="PARAMETER">option</replaceable> [ ... ] ]
 
 <phrase>where <replaceable class="PARAMETER">option</replaceable> can be:</phrase>
-    
+
       SUPERUSER | NOSUPERUSER
     | CREATEDB | NOCREATEDB
     | CREATEROLE | NOCREATEROLE
@@ -33,7 +33,7 @@ ALTER ROLE <replaceable class="PARAMETER">name</replaceable> [ [ WITH ] <replace
     | LOGIN | NOLOGIN
     | CONNECTION LIMIT <replaceable class="PARAMETER">connlimit</replaceable>
     | [ ENCRYPTED | UNENCRYPTED ] PASSWORD '<replaceable class="PARAMETER">password</replaceable>'
-    | VALID UNTIL '<replaceable class="PARAMETER">timestamp</replaceable>' 
+    | VALID UNTIL '<replaceable class="PARAMETER">timestamp</replaceable>'
 
 ALTER ROLE <replaceable class="PARAMETER">name</replaceable> RENAME TO <replaceable>new_name</replaceable>
 
@@ -54,7 +54,7 @@ ALTER ROLE <replaceable class="PARAMETER">name</replaceable> [ IN DATABASE <repl
 
   <para>
    The first variant of this command listed in the synopsis can change
-   many of the role attributes that can be specified in 
+   many of the role attributes that can be specified in
    <xref linkend="sql-createrole">.
    (All the possible attributes are covered,
    except that there are no options for adding or removing memberships; use
@@ -79,20 +79,24 @@ ALTER ROLE <replaceable class="PARAMETER">name</replaceable> [ IN DATABASE <repl
    password is <literal>MD5</>-encrypted.
   </para>
 
-  <para> 
-   The remaining variants change a role's session default for a configuration variable 
-   for all databases or, when the <literal>IN DATABASE</literal> clause is specified,
-   for the named database. Whenever the role subsequently
+  <para>
+   The remaining variants change a role's session default for a configuration
+   variable, either for all databases or, when the <literal>IN
+   DATABASE</literal> clause is specified, only for sessions in
+   the named database. Whenever the role subsequently
    starts a new session, the specified value becomes the session
    default, overriding whatever setting is present in
    <filename>postgresql.conf</> or has been received from the postgres
-   command line. This only happens at login time, so configuration
-   settings associated with a role to which you've <xref
-   linkend="sql-set-role"> will be ignored. Settings set to
-   a role directly are overridden by any database specific settings attached to a role.
+   command line. This only happens at login time; executing
+   <xref linkend="sql-set-role"> or
+   <xref linkend="sql-set-session-authorization"> does not cause new
+   configuration values to be set.
+   Settings set for all databases are overridden by database-specific settings
+   attached to a role.
    Superusers can change anyone's session defaults. Roles having
    <literal>CREATEROLE</> privilege can change defaults for non-superuser
-   roles. Certain variables cannot be set this way, or can only be
+   roles. Ordinary roles can only set defaults for themselves.
+   Certain configuration variables cannot be set this way, or can only be
    set if a superuser issues the command.
   </para>
  </refsect1>
@@ -169,14 +173,15 @@ ALTER ROLE <replaceable class="PARAMETER">name</replaceable> [ IN DATABASE <repl
         <literal>RESET ALL</literal> to clear all role-specific settings.
         <literal>SET FROM CURRENT</> saves the session's current value of
         the parameter as the role-specific value.
-        If used in conjunction with <literal>IN DATABASE</literal>, the configuration
+        If <literal>IN DATABASE</literal> is specified, the configuration
         parameter is set or removed for the given role and database only.
        </para>
 
        <para>
-        Role-specific variable setting take effect only at login;
-        <xref linkend="sql-set-role">
-        does not process role-specific variable settings.
+        Role-specific variable settings take effect only at login;
+        <xref linkend="sql-set-role"> and
+        <xref linkend="sql-set-session-authorization">
+        do not process role-specific variable settings.
        </para>
 
        <para>
@@ -210,8 +215,8 @@ ALTER ROLE <replaceable class="PARAMETER">name</replaceable> [ IN DATABASE <repl
    in cleartext, and it might also be logged in the client's command
    history or the server log.  <xref linkend="app-psql">
    contains a command
-   <command>\password</command> that can be used to safely change a
-   role's password.
+   <command>\password</command> that can be used to change a
+   role's password without exposing the cleartext password.
   </para>
 
   <para>
@@ -276,8 +281,8 @@ ALTER ROLE worker_bee SET maintenance_work_mem = 100000;
   </para>
 
   <para>
-    Give a role a non-default, database-specific setting of the 
-  <xref linkend="guc-client-min-messages"> parameter:
+   Give a role a non-default, database-specific setting of the
+   <xref linkend="guc-client-min-messages"> parameter:
 
 <programlisting>
 ALTER ROLE fred IN DATABASE devel SET client_min_messages = DEBUG;
@@ -287,7 +292,7 @@ ALTER ROLE fred IN DATABASE devel SET client_min_messages = DEBUG;
 
  <refsect1>
   <title>Compatibility</title>
-    
+
   <para>
    The <command>ALTER ROLE</command> statement is a
    <productname>PostgreSQL</productname> extension.
index 15ca9c1b2242402fc71b0ba5fec372e14ab56a6d..98261e10e4025ec0d951bfbc2aef95ce5dadf474 100644 (file)
@@ -10,7 +10,7 @@
  * Written by Peter Eisentraut <peter_e@gmx.net>.
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/utils/misc/guc.c,v 1.549 2010/04/20 11:15:06 rhaas Exp $
+ *       $PostgreSQL: pgsql/src/backend/utils/misc/guc.c,v 1.550 2010/04/21 20:54:19 tgl Exp $
  *
  *--------------------------------------------------------------------
  */
@@ -2864,6 +2864,8 @@ static void ShowGUCConfigOption(const char *name, DestReceiver *dest);
 static void ShowAllGUCConfig(DestReceiver *dest);
 static char *_ShowOption(struct config_generic * record, bool use_units);
 static bool is_newvalue_equal(struct config_generic * record, const char *newvalue);
+static bool validate_option_array_item(const char *name, const char *value,
+                                                                          bool skipIfNoPermissions);
 
 
 /*
@@ -5474,14 +5476,15 @@ flatten_set_variable_args(const char *name, List *args)
        if (args == NIL)
                return NULL;
 
-       /* Else get flags for the variable */
-       record = find_option(name, true, ERROR);
-       if (record == NULL)
-               ereport(ERROR,
-                               (errcode(ERRCODE_UNDEFINED_OBJECT),
-                          errmsg("unrecognized configuration parameter \"%s\"", name)));
-
-       flags = record->flags;
+       /*
+        * Get flags for the variable; if it's not known, use default flags.
+        * (Caller might throw error later, but not our business to do so here.)
+        */
+       record = find_option(name, false, WARNING);
+       if (record)
+               flags = record->flags;
+       else
+               flags = 0;
 
        /* Complain if list input and non-list variable */
        if ((flags & GUC_LIST_INPUT) == 0 &&
@@ -5870,12 +5873,27 @@ define_custom_variable(struct config_generic * variable)
                        else
                                phcontext = PGC_SIGHUP;
                        break;
+
                case PGC_S_DATABASE:
                case PGC_S_USER:
                case PGC_S_DATABASE_USER:
+                       /*
+                        * The existing value came from an ALTER ROLE/DATABASE SET command.
+                        * We can assume that at the time the command was issued, we
+                        * checked that the issuing user was superuser if the variable
+                        * requires superuser privileges to set.  So it's safe to
+                        * use SUSET context here.
+                        */
+                       phcontext = PGC_SUSET;
+                       break;
+
                case PGC_S_CLIENT:
                case PGC_S_SESSION:
                default:
+                       /*
+                        * We must assume that the value came from an untrusted user,
+                        * even if the current_user is a superuser.
+                        */
                        phcontext = PGC_USERSET;
                        break;
        }
@@ -7180,7 +7198,7 @@ ProcessGUCArray(ArrayType *array,
 ArrayType *
 GUCArrayAdd(ArrayType *array, const char *name, const char *value)
 {
-       const char *varname;
+       struct config_generic *record;
        Datum           datum;
        char       *newval;
        ArrayType  *a;
@@ -7188,15 +7206,15 @@ GUCArrayAdd(ArrayType *array, const char *name, const char *value)
        Assert(name);
        Assert(value);
 
-       /* test if the option is valid */
-       set_config_option(name, value,
-                                         superuser() ? PGC_SUSET : PGC_USERSET,
-                                         PGC_S_TEST, GUC_ACTION_SET, false);
+       /* test if the option is valid and we're allowed to set it */
+       (void) validate_option_array_item(name, value, false);
 
-       /* convert name to canonical spelling, so we can use plain strcmp */
-       (void) GetConfigOptionByName(name, &varname);
-       name = varname;
+       /* normalize name (converts obsolete GUC names to modern spellings) */
+       record = find_option(name, false, WARNING);
+       if (record)
+               name = record->name;
 
+       /* build new item for array */
        newval = palloc(strlen(name) + 1 + strlen(value) + 1);
        sprintf(newval, "%s=%s", name, value);
        datum = CStringGetTextDatum(newval);
@@ -7227,6 +7245,8 @@ GUCArrayAdd(ArrayType *array, const char *name, const char *value)
                        if (isnull)
                                continue;
                        current = TextDatumGetCString(d);
+
+                       /* check for match up through and including '=' */
                        if (strncmp(current, newval, strlen(name) + 1) == 0)
                        {
                                index = i;
@@ -7259,21 +7279,20 @@ GUCArrayAdd(ArrayType *array, const char *name, const char *value)
 ArrayType *
 GUCArrayDelete(ArrayType *array, const char *name)
 {
-       const char *varname;
+       struct config_generic *record;
        ArrayType  *newarray;
        int                     i;
        int                     index;
 
        Assert(name);
 
-       /* test if the option is valid */
-       set_config_option(name, NULL,
-                                         superuser() ? PGC_SUSET : PGC_USERSET,
-                                         PGC_S_TEST, GUC_ACTION_SET, false);
+       /* test if the option is valid and we're allowed to set it */
+       (void) validate_option_array_item(name, NULL, false);
 
-       /* convert name to canonical spelling, so we can use plain strcmp */
-       (void) GetConfigOptionByName(name, &varname);
-       name = varname;
+       /* normalize name (converts obsolete GUC names to modern spellings) */
+       record = find_option(name, false, WARNING);
+       if (record)
+               name = record->name;
 
        /* if array is currently null, then surely nothing to delete */
        if (!array)
@@ -7303,10 +7322,8 @@ GUCArrayDelete(ArrayType *array, const char *name)
                        && val[strlen(name)] == '=')
                        continue;
 
-
                /* else add it to the output array */
                if (newarray)
-               {
                        newarray = array_set(newarray, 1, &index,
                                                                 d,
                                                                 false,
@@ -7314,7 +7331,6 @@ GUCArrayDelete(ArrayType *array, const char *name)
                                                                 -1 /* TEXT's typlen */ ,
                                                                 false /* TEXT's typbyval */ ,
                                                                 'i' /* TEXT's typalign */ );
-               }
                else
                        newarray = construct_array(&d, 1,
                                                                           TEXTOID,
@@ -7326,6 +7342,7 @@ GUCArrayDelete(ArrayType *array, const char *name)
        return newarray;
 }
 
+
 /*
  * Given a GUC array, delete all settings from it that our permission
  * level allows: if superuser, delete them all; if regular user, only
@@ -7342,7 +7359,7 @@ GUCArrayReset(ArrayType *array)
        if (!array)
                return NULL;
 
-       /* if we're superuser, we can delete everything */
+       /* if we're superuser, we can delete everything, so just do it */
        if (superuser())
                return NULL;
 
@@ -7355,7 +7372,6 @@ GUCArrayReset(ArrayType *array)
                char       *val;
                char       *eqsgn;
                bool            isnull;
-               struct config_generic *gconf;
 
                d = array_ref(array, 1, &i,
                                          -1 /* varlenarray */ ,
@@ -7363,7 +7379,6 @@ GUCArrayReset(ArrayType *array)
                                          false /* TEXT's typbyval */ ,
                                          'i' /* TEXT's typalign */ ,
                                          &isnull);
-
                if (isnull)
                        continue;
                val = TextDatumGetCString(d);
@@ -7371,20 +7386,12 @@ GUCArrayReset(ArrayType *array)
                eqsgn = strchr(val, '=');
                *eqsgn = '\0';
 
-               gconf = find_option(val, false, WARNING);
-               if (!gconf)
-                       continue;
-
-               /* note: superuser-ness was already checked above */
-               /* skip entry if OK to delete */
-               if (gconf->context == PGC_USERSET)
+               /* skip if we have permission to delete it */
+               if (validate_option_array_item(val, NULL, true))
                        continue;
 
-               /* XXX do we need to worry about database owner? */
-
                /* else add it to the output array */
                if (newarray)
-               {
                        newarray = array_set(newarray, 1, &index,
                                                                 d,
                                                                 false,
@@ -7392,7 +7399,6 @@ GUCArrayReset(ArrayType *array)
                                                                 -1 /* TEXT's typlen */ ,
                                                                 false /* TEXT's typbyval */ ,
                                                                 'i' /* TEXT's typalign */ );
-               }
                else
                        newarray = construct_array(&d, 1,
                                                                           TEXTOID,
@@ -7405,6 +7411,89 @@ GUCArrayReset(ArrayType *array)
        return newarray;
 }
 
+/*
+ * Validate a proposed option setting for GUCArrayAdd/Delete/Reset.
+ *
+ * name is the option name.  value is the proposed value for the Add case,
+ * or NULL for the Delete/Reset cases.  If skipIfNoPermissions is true, it's
+ * not an error to have no permissions to set the option.
+ *
+ * Returns TRUE if OK, FALSE if skipIfNoPermissions is true and user does not
+ * have permission to change this option (all other error cases result in an
+ * error being thrown).
+ */
+static bool
+validate_option_array_item(const char *name, const char *value,
+                                                  bool skipIfNoPermissions)
+
+{
+       struct config_generic *gconf;
+
+       /*
+        * There are three cases to consider:
+        *
+        * name is a known GUC variable.  Check the value normally, check
+        * permissions normally (ie, allow if variable is USERSET, or if it's
+        * SUSET and user is superuser).
+        *
+        * name is not known, but exists or can be created as a placeholder
+        * (implying it has a prefix listed in custom_variable_classes).
+        * We allow this case if you're a superuser, otherwise not.  Superusers
+        * are assumed to know what they're doing.  We can't allow it for other
+        * users, because when the placeholder is resolved it might turn out to
+        * be a SUSET variable; define_custom_variable assumes we checked that.
+        *
+        * name is not known and can't be created as a placeholder.  Throw error,
+        * unless skipIfNoPermissions is true, in which case return FALSE.
+        * (It's tempting to allow this case to superusers, if the name is
+        * qualified but not listed in custom_variable_classes.  That would
+        * ease restoring of dumps containing ALTER ROLE/DATABASE SET.  However,
+        * it's not clear that this usage justifies such a loss of error checking.
+        * You can always fix custom_variable_classes before you restore.)
+        */
+       gconf = find_option(name, true, WARNING);
+       if (!gconf)
+       {
+               /* not known, failed to make a placeholder */
+               if (skipIfNoPermissions)
+                       return false;
+               ereport(ERROR,
+                               (errcode(ERRCODE_UNDEFINED_OBJECT),
+                                errmsg("unrecognized configuration parameter \"%s\"", name)));
+       }
+
+       if (gconf->flags & GUC_CUSTOM_PLACEHOLDER)
+       {
+               /*
+                * We cannot do any meaningful check on the value, so only permissions
+                * are useful to check.
+                */
+               if (superuser())
+                       return true;
+               if (skipIfNoPermissions)
+                       return false;
+               ereport(ERROR,
+                               (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+                                errmsg("permission denied to set parameter \"%s\"", name)));
+       }
+
+       /* manual permissions check so we can avoid an error being thrown */
+       if (gconf->context == PGC_USERSET)
+               /* ok */ ;
+       else if (gconf->context == PGC_SUSET && superuser())
+               /* ok */ ;
+       else if (skipIfNoPermissions)
+               return false;
+       /* if a permissions error should be thrown, let set_config_option do it */
+
+       /* test for permissions and valid option value */
+       set_config_option(name, value,
+                                         superuser() ? PGC_SUSET : PGC_USERSET,
+                                         PGC_S_TEST, GUC_ACTION_SET, false);
+
+       return true;
+}
+
 
 /*
  * assign_hook and show_hook subroutines