Bruce Momjian wrote:
> Alvaro Herrera wrote:
> > Tom Lane wrote:
> > > Alvaro Herrera <alvhe...@commandprompt.com> writes:
> > > > Tom Lane wrote:
> > > >> It looks to me like the code in AlterSetting() will allow an ordinary
> > > >> user to blow away all settings for himself. Even those that are for
> > > >> SUSET variables and were presumably set for him by a superuser. Isn't
> > > >> this a security hole? I would expect that an unprivileged user should
> > > >> not be able to change such settings, not even to the extent of
> > > >> reverting to the installation-wide default.
> > >
> > > > Yes, it is, but this is not a new hole. This works just fine in 8.4
> > > > too:
> > >
> > > So I'd argue for changing it in 8.4 too.
> >
> > Understood. I'm starting to look at what this requires.
>
> Any progress on this?
I have come up with the attached patch. I haven't tested it fully yet,
and I need to backport it. The gist of it is: we can't simply remove
the pg_db_role_setting tuple, we need to ask GUC to reset the settings
array, for which it checks superuser-ness on each setting.
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Index: src/backend/catalog/pg_db_role_setting.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/catalog/pg_db_role_setting.c,v
retrieving revision 1.3
diff -c -p -r1.3 pg_db_role_setting.c
*** src/backend/catalog/pg_db_role_setting.c 26 Feb 2010 02:00:37 -0000 1.3
--- src/backend/catalog/pg_db_role_setting.c 18 Mar 2010 15:43:14 -0000
*************** AlterSetting(Oid databaseid, Oid roleid,
*** 49,55 ****
/*
* There are three cases:
*
! * - in RESET ALL, simply delete the pg_db_role_setting tuple (if any)
*
* - in other commands, if there's a tuple in pg_db_role_setting, update
* it; if it ends up empty, delete it
--- 49,56 ----
/*
* There are three cases:
*
! * - in RESET ALL, request GUC to reset the settings array and update the
! * catalog if there's anything left, delete it otherwise
*
* - in other commands, if there's a tuple in pg_db_role_setting, update
* it; if it ends up empty, delete it
*************** AlterSetting(Oid databaseid, Oid roleid,
*** 60,66 ****
if (setstmt->kind == VAR_RESET_ALL)
{
if (HeapTupleIsValid(tuple))
! simple_heap_delete(rel, &tuple->t_self);
}
else if (HeapTupleIsValid(tuple))
{
--- 61,101 ----
if (setstmt->kind == VAR_RESET_ALL)
{
if (HeapTupleIsValid(tuple))
! {
! ArrayType *new = NULL;
! Datum datum;
! bool isnull;
!
! datum = heap_getattr(tuple, Anum_pg_db_role_setting_setconfig,
! RelationGetDescr(rel), &isnull);
!
! if (!isnull)
! new = GUCArrayReset(DatumGetArrayTypeP(datum));
!
! if (new)
! {
! Datum repl_val[Natts_pg_db_role_setting];
! bool repl_null[Natts_pg_db_role_setting];
! bool repl_repl[Natts_pg_db_role_setting];
! HeapTuple newtuple;
!
! memset(repl_repl, false, sizeof(repl_repl));
!
! repl_val[Anum_pg_db_role_setting_setconfig - 1] =
! PointerGetDatum(new);
! repl_repl[Anum_pg_db_role_setting_setconfig - 1] = true;
! repl_null[Anum_pg_db_role_setting_setconfig - 1] = false;
!
! newtuple = heap_modify_tuple(tuple, RelationGetDescr(rel),
! repl_val, repl_null, repl_repl);
! simple_heap_update(rel, &tuple->t_self, newtuple);
!
! /* Update indexes */
! CatalogUpdateIndexes(rel, newtuple);
! }
! else
! simple_heap_delete(rel, &tuple->t_self);
! }
}
else if (HeapTupleIsValid(tuple))
{
Index: src/backend/utils/misc/guc.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/utils/misc/guc.c,v
retrieving revision 1.543
diff -c -p -r1.543 guc.c
*** src/backend/utils/misc/guc.c 26 Feb 2010 02:01:14 -0000 1.543
--- src/backend/utils/misc/guc.c 18 Mar 2010 15:39:15 -0000
*************** ParseLongOption(const char *string, char
*** 7099,7105 ****
/*
! * Handle options fetched from pg_database.datconfig, pg_authid.rolconfig,
* pg_proc.proconfig, etc. Caller must specify proper context/source/action.
*
* The array parameter must be an array of TEXT (it must not be NULL).
--- 7099,7105 ----
/*
! * Handle options fetched from pg_db_role_setting.setconfig,
* pg_proc.proconfig, etc. Caller must specify proper context/source/action.
*
* The array parameter must be an array of TEXT (it must not be NULL).
*************** ProcessGUCArray(ArrayType *array,
*** 7151,7156 ****
--- 7151,7157 ----
free(name);
if (value)
free(value);
+ pfree(s);
}
}
*************** GUCArrayDelete(ArrayType *array, const c
*** 7285,7290 ****
--- 7286,7370 ----
&& val[strlen(name)] == '=')
continue;
+
+ /* else add it to the output array */
+ if (newarray)
+ {
+ newarray = array_set(newarray, 1, &index,
+ d,
+ false,
+ -1 /* varlenarray */ ,
+ -1 /* TEXT's typlen */ ,
+ false /* TEXT's typbyval */ ,
+ 'i' /* TEXT's typalign */ );
+ }
+ else
+ newarray = construct_array(&d, 1,
+ TEXTOID,
+ -1, false, 'i');
+
+ index++;
+ }
+
+ 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
+ * those that are PGC_USERSET
+ */
+ ArrayType *
+ GUCArrayReset(ArrayType *array)
+ {
+ ArrayType *newarray;
+ int i;
+ int index;
+
+ /* if array is currently null, nothing to do */
+ if (!array)
+ return NULL;
+
+ /* if we're superuser, we can delete everything */
+ if (superuser())
+ return NULL;
+
+ newarray = NULL;
+ index = 1;
+
+ for (i = 1; i <= ARR_DIMS(array)[0]; i++)
+ {
+ Datum d;
+ char *val;
+ char *eqsgn;
+ bool isnull;
+ struct config_generic *gconf;
+
+ d = array_ref(array, 1, &i,
+ -1 /* varlenarray */ ,
+ -1 /* TEXT's typlen */ ,
+ false /* TEXT's typbyval */ ,
+ 'i' /* TEXT's typalign */ ,
+ &isnull);
+
+ if (isnull)
+ continue;
+ val = TextDatumGetCString(d);
+
+ 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)
+ continue;
+
+ /* XXX do we need to worry about database owner? */
+
/* else add it to the output array */
if (newarray)
{
*************** GUCArrayDelete(ArrayType *array, const c
*** 7302,7307 ****
--- 7382,7388 ----
-1, false, 'i');
index++;
+ pfree(val);
}
return newarray;
Index: src/include/utils/guc.h
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/include/utils/guc.h,v
retrieving revision 1.112
diff -c -p -r1.112 guc.h
*** src/include/utils/guc.h 26 Jan 2010 16:33:40 -0000 1.112
--- src/include/utils/guc.h 17 Mar 2010 20:51:40 -0000
*************** extern void ProcessGUCArray(ArrayType *a
*** 284,289 ****
--- 284,290 ----
GucContext context, GucSource source, GucAction action);
extern ArrayType *GUCArrayAdd(ArrayType *array, const char *name, const char *value);
extern ArrayType *GUCArrayDelete(ArrayType *array, const char *name);
+ extern ArrayType *GUCArrayReset(ArrayType *array);
extern int GUC_complaint_elevel(GucSource source);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
