This is not gonna do

Started by Tom Laneover 22 years ago5 messages
#1Tom Lane
tgl@sss.pgh.pa.us

The recent change to make log_min_messages SUSET provokes the following
behavior:

$ export PGOPTIONS="-d 5"
$ psql
psql: FATAL: 'log_min_messages': permission denied
$

Considering that I *am* superuser, this is quite unacceptable.
If you don't want to revert the change, propose another solution.

regards, tom lane

#2Bruce Momjian
pgman@candle.pha.pa.us
In reply to: Tom Lane (#1)
1 attachment(s)
GUC --- prevent non-super user changes

Tom Lane wrote:

The recent change to make log_min_messages SUSET provokes the following
behavior:

$ export PGOPTIONS="-d 5"
$ psql
psql: FATAL: 'log_min_messages': permission denied
$

Considering that I *am* superuser, this is quite unacceptable.
If you don't want to revert the change, propose another solution.

Here is a proposed fix for the new SUSET of various variables. The
solution is to create a new GUC context called PGC_USERLIMIT, which
limits changes by non-super users. For example, non-super users can
turn on logging, but can't turn it off, and log_min_* logging can have
added output, but not less output.

The first part of the patch prevent client PGOPTIONS from lowering the
debug level. The second part adds this new GUC context, then allows it
to be set properly. The tests are in two parts --- the first prevents
non-super users from changing the value inappropriately, and the second
allows postgresql.conf changes to apply to existing backends, i.e. if
postgresql.conf turns logging off via SET, turning it on via
postgresql.conf should propogate to the client, because the client can't
turn something off that the admin wants turned on --- that is the tricky
part that we have to be able to handle the settings in any order.

Peter, how does this look? Is reset_val the proper value to test?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Attachments:

/pgpatches/userlimittext/plainDownload
Index: src/backend/tcop/postgres.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/tcop/postgres.c,v
retrieving revision 1.346
diff -c -c -r1.346 postgres.c
*** src/backend/tcop/postgres.c	27 May 2003 17:49:46 -0000	1.346
--- src/backend/tcop/postgres.c	11 Jun 2003 04:29:55 -0000
***************
*** 1921,1927 ****
  	bool		secure;
  	int			errs = 0;
  	int			debug_flag = 0;
! 	GucContext	ctx;
  	GucSource	gucsource;
  	char	   *tmp;
  	int			firstchar;
--- 1921,1927 ----
  	bool		secure;
  	int			errs = 0;
  	int			debug_flag = 0;
! 	GucContext	ctx, debug_context;
  	GucSource	gucsource;
  	char	   *tmp;
  	int			firstchar;
***************
*** 1996,2002 ****
  
  	/* all options are allowed until '-p' */
  	secure = true;
! 	ctx = PGC_POSTMASTER;
  	gucsource = PGC_S_ARGV;		/* initial switches came from command line */
  
  	while ((flag = getopt(argc, argv, "A:B:c:CD:d:Eef:FiNOPo:p:S:st:v:W:x:-:")) != -1)
--- 1996,2002 ----
  
  	/* all options are allowed until '-p' */
  	secure = true;
! 	ctx = debug_context = PGC_POSTMASTER;
  	gucsource = PGC_S_ARGV;		/* initial switches came from command line */
  
  	while ((flag = getopt(argc, argv, "A:B:c:CD:d:Eef:FiNOPo:p:S:st:v:W:x:-:")) != -1)
***************
*** 2033,2057 ****
  
  			case 'd':			/* debug level */
  				{
! 					debug_flag = atoi(optarg);
! 					/* Set server debugging level. */
! 					if (atoi(optarg) != 0)
  					{
! 						char	   *debugstr = palloc(strlen("debug") + strlen(optarg) + 1);
! 
! 						sprintf(debugstr, "debug%s", optarg);
! 						SetConfigOption("log_min_messages", debugstr, ctx, gucsource);
! 						pfree(debugstr);
! 
  					}
- 					else
- 						/*
- 						 * -d0 allows user to prevent postmaster debug
- 						 * from propagating to backend.  It would be nice
- 						 * to set it to the postgresql.conf value here.
- 						 */
- 						SetConfigOption("log_min_messages", "notice",
- 										ctx, gucsource);
  				}
  				break;
  
--- 2033,2066 ----
  
  			case 'd':			/* debug level */
  				{
! 					/*
! 					 *	Client option can't decrease debug level.
! 					 *	We have to do the test here because we group priv and client
! 					 *	set GUC calls below, after we know the final debug value.
! 					 */					   
! 					if (ctx != PGC_BACKEND || atoi(optarg) > debug_flag)
  					{
! 						debug_flag = atoi(optarg);
! 						debug_context = ctx;	/* save context for use below */
! 						/* Set server debugging level. */
! 						if (debug_flag != 0)
! 						{
! 							char	   *debugstr = palloc(strlen("debug") + strlen(optarg) + 1);
! 	
! 							sprintf(debugstr, "debug%s", optarg);
! 							SetConfigOption("log_min_messages", debugstr, ctx, gucsource);
! 							pfree(debugstr);
! 	
! 						}
! 						else
! 							/*
! 							 * -d0 allows user to prevent postmaster debug
! 							 * from propagating to backend.  It would be nice
! 							 * to set it to the postgresql.conf value here.
! 							 */
! 							SetConfigOption("log_min_messages", "notice",
! 											ctx, gucsource);
  					}
  				}
  				break;
  
***************
*** 2301,2320 ****
  
  
  	/*
! 	 * -d is not the same as setting
! 	 * log_min_messages because it enables other
! 	 * output options.
  	 */
  	if (debug_flag >= 1)
! 		SetConfigOption("log_connections", "true", ctx, gucsource);
  	if (debug_flag >= 2)
! 		SetConfigOption("log_statement", "true", ctx, gucsource);
  	if (debug_flag >= 3)
! 		SetConfigOption("debug_print_parse", "true", ctx, gucsource);
  	if (debug_flag >= 4)
! 		SetConfigOption("debug_print_plan", "true", ctx, gucsource);
  	if (debug_flag >= 5)
! 		SetConfigOption("debug_print_rewritten", "true", ctx, gucsource);
  
  	/*
  	 * Process any additional GUC variable settings passed in startup packet.
--- 2310,2328 ----
  
  
  	/*
! 	 * -d is not the same as setting log_min_messages because it enables
! 	 * other output options.
  	 */
  	if (debug_flag >= 1)
! 		SetConfigOption("log_connections", "true", debug_context, gucsource);
  	if (debug_flag >= 2)
! 		SetConfigOption("log_statement", "true", debug_context, gucsource);
  	if (debug_flag >= 3)
! 		SetConfigOption("debug_print_parse", "true", debug_context, gucsource);
  	if (debug_flag >= 4)
! 		SetConfigOption("debug_print_plan", "true", debug_context, gucsource);
  	if (debug_flag >= 5)
! 		SetConfigOption("debug_print_rewritten", "true", debug_context, gucsource);
  
  	/*
  	 * Process any additional GUC variable settings passed in startup packet.
Index: src/backend/utils/misc/guc.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/utils/misc/guc.c,v
retrieving revision 1.127
diff -c -c -r1.127 guc.c
*** src/backend/utils/misc/guc.c	28 May 2003 18:19:09 -0000	1.127
--- src/backend/utils/misc/guc.c	11 Jun 2003 04:30:01 -0000
***************
*** 406,416 ****
  	},
  
  	{
! 		{"log_statement", PGC_SUSET}, &log_statement,
  		false, NULL, NULL
  	},
  	{
! 		{"log_duration", PGC_SUSET}, &log_duration,
  		false, NULL, NULL
  	},
  	{
--- 406,416 ----
  	},
  
  	{
! 		{"log_statement", PGC_USERLIMIT}, &log_statement,
  		false, NULL, NULL
  	},
  	{
! 		{"log_duration", PGC_USERLIMIT}, &log_duration,
  		false, NULL, NULL
  	},
  	{
***************
*** 431,449 ****
  	},
  
  	{
! 		{"log_parser_stats", PGC_SUSET}, &log_parser_stats,
  		false, NULL, NULL
  	},
  	{
! 		{"log_planner_stats", PGC_SUSET}, &log_planner_stats,
  		false, NULL, NULL
  	},
  	{
! 		{"log_executor_stats", PGC_SUSET}, &log_executor_stats,
  		false, NULL, NULL
  	},
  	{
! 		{"log_statement_stats", PGC_SUSET}, &log_statement_stats,
  		false, NULL, NULL
  	},
  #ifdef BTREE_BUILD_STATS
--- 431,449 ----
  	},
  
  	{
! 		{"log_parser_stats", PGC_USERLIMIT}, &log_parser_stats,
  		false, NULL, NULL
  	},
  	{
! 		{"log_planner_stats", PGC_USERLIMIT}, &log_planner_stats,
  		false, NULL, NULL
  	},
  	{
! 		{"log_executor_stats", PGC_USERLIMIT}, &log_executor_stats,
  		false, NULL, NULL
  	},
  	{
! 		{"log_statement_stats", PGC_USERLIMIT}, &log_statement_stats,
  		false, NULL, NULL
  	},
  #ifdef BTREE_BUILD_STATS
***************
*** 797,803 ****
  	},
  
  	{
! 		{"log_min_error_statement", PGC_SUSET}, &log_min_error_statement_str,
  		"panic", assign_min_error_statement, NULL
  	},
  
--- 797,803 ----
  	},
  
  	{
! 		{"log_min_error_statement", PGC_USERLIMIT}, &log_min_error_statement_str,
  		"panic", assign_min_error_statement, NULL
  	},
  
***************
*** 884,890 ****
  	},
  
  	{
! 		{"log_min_messages", PGC_SUSET}, &log_min_messages_str,
  		"notice", assign_log_min_messages, NULL
  	},
  
--- 884,890 ----
  	},
  
  	{
! 		{"log_min_messages", PGC_USERLIMIT}, &log_min_messages_str,
  		"notice", assign_log_min_messages, NULL
  	},
  
***************
*** 1180,1185 ****
--- 1180,1189 ----
  					struct config_string *conf = (struct config_string *) gconf;
  					char	   *str;
  
+ 					Assert(conf->gen.context != PGC_USERLIMIT ||
+ 						   conf->assign_hook == assign_log_min_messages ||
+ 						   conf->assign_hook == assign_client_min_messages ||
+ 						   conf->assign_hook == assign_min_error_statement);
  					*conf->variable = NULL;
  					conf->reset_val = NULL;
  					conf->session_val = NULL;
***************
*** 1276,1282 ****
  		struct config_generic *gconf = guc_variables[i];
  
  		/* Don't reset non-SET-able values */
! 		if (gconf->context != PGC_SUSET && gconf->context != PGC_USERSET)
  			continue;
  		/* Don't reset if special exclusion from RESET ALL */
  		if (gconf->flags & GUC_NO_RESET_ALL)
--- 1280,1288 ----
  		struct config_generic *gconf = guc_variables[i];
  
  		/* Don't reset non-SET-able values */
! 		if (gconf->context != PGC_SUSET &&
! 			gconf->context != PGC_USERLIMIT &&
! 			gconf->context != PGC_USERSET)
  			continue;
  		/* Don't reset if special exclusion from RESET ALL */
  		if (gconf->flags & GUC_NO_RESET_ALL)
***************
*** 1810,1815 ****
--- 1816,1822 ----
  				return false;
  			}
  			break;
+ 		case PGC_USERLIMIT:	/* USERLIMIT permissions checked below */
  		case PGC_USERSET:
  			/* always okay */
  			break;
***************
*** 1861,1866 ****
--- 1868,1889 ----
  							 name);
  						return false;
  					}
+ 					/* Limit non-super user changes */
+ 					if (record->context == PGC_USERLIMIT &&
+ 						source > PGC_S_USERSTART &&
+ 						newval < conf->reset_val && !superuser())
+ 					{
+ 						elog(elevel, "'%s': permission denied\n"
+ 								"Only super-users can set this value to false.",
+ 								name);
+ 						return false;
+ 					}
+ 					/* Allow admin change to override user change */
+ 					if (!DoIt && record->context == PGC_USERLIMIT &&
+ 						source < PGC_S_USERSTART &&
+ 						record->reset_source > PGC_S_USERSTART &&
+ 						newval > conf->reset_val && !superuser())
+ 							DoIt = true;
  				}
  				else
  				{
***************
*** 1932,1937 ****
--- 1955,1976 ----
  							 name, newval, conf->min, conf->max);
  						return false;
  					}
+ 					/* Limit non-super user changes */
+ 					if (record->context == PGC_USERLIMIT &&
+ 						source > PGC_S_USERSTART &&
+ 						newval < conf->reset_val && !superuser())
+ 					{
+ 						elog(elevel, "'%s': permission denied\n"
+ 								"Only super-users can increase this value.",
+ 								name);
+ 						return false;
+ 					}
+ 					/* Allow admin change to override user change */
+ 					if (!DoIt && record->context == PGC_USERLIMIT &&
+ 						source < PGC_S_USERSTART &&
+ 						record->reset_source > PGC_S_USERSTART &&
+ 						newval > conf->reset_val && !superuser())
+ 							DoIt = true;
  				}
  				else
  				{
***************
*** 2003,2008 ****
--- 2042,2063 ----
  							 name, newval, conf->min, conf->max);
  						return false;
  					}
+ 					/* Limit non-super user changes */
+ 					if (record->context == PGC_USERLIMIT &&
+ 						source > PGC_S_USERSTART &&
+ 						newval < conf->reset_val && !superuser())
+ 					{
+ 						elog(elevel, "'%s': permission denied\n"
+ 								"Only super-users can increase this value.",
+ 								name);
+ 						return false;
+ 					}
+ 					/* Allow admin change to override user change */
+ 					if (!DoIt && record->context == PGC_USERLIMIT &&
+ 						source < PGC_S_USERSTART &&
+ 						record->reset_source > PGC_S_USERSTART &&
+ 						newval > conf->reset_val && !superuser())
+ 							DoIt = true;
  				}
  				else
  				{
***************
*** 2067,2072 ****
--- 2122,2151 ----
  						elog(elevel, "out of memory");
  						return false;
  					}
+ 
+ 					if (*conf->variable)
+ 					{
+ 						int old_int_value, new_int_value;
+ 
+ 						/* Limit non-super user changes */
+ 						assign_msglvl(&old_int_value, conf->reset_val, true, interactive);
+ 						assign_msglvl(&new_int_value, newval, true, interactive);
+ 						if (record->context == PGC_USERLIMIT &&
+ 							source > PGC_S_USERSTART &&
+ 							new_int_value > old_int_value && !superuser())
+ 						{
+ 							elog(elevel, "'%s': permission denied\n"
+ 										"Only super-users can increase this value.",
+ 										name);
+ 							return false;
+ 						}
+ 						/* Allow admin change to override user change */
+ 						if (!DoIt && record->context == PGC_USERLIMIT &&
+ 							source < PGC_S_USERSTART &&
+ 							record->reset_source > PGC_S_USERSTART &&
+ 							newval > conf->reset_val && !superuser())
+ 								DoIt = true;
+ 						}
  				}
  				else if (conf->reset_val)
  				{
***************
*** 3454,3457 ****
  
  
  #include "guc-file.c"
- 
--- 3533,3535 ----
Index: src/include/utils/guc.h
===================================================================
RCS file: /cvsroot/pgsql-server/src/include/utils/guc.h,v
retrieving revision 1.31
diff -c -c -r1.31 guc.h
*** src/include/utils/guc.h	6 May 2003 20:26:28 -0000	1.31
--- src/include/utils/guc.h	11 Jun 2003 04:30:02 -0000
***************
*** 48,53 ****
--- 48,56 ----
   * be set in the connection startup packet, because when it is processed
   * we don't yet know if the user is a superuser.
   *
+  * USERLIMIT options can only be manipulated in certain ways by
+  * non-super users.
+  *
   * USERSET options can be set by anyone any time.
   */
  typedef enum
***************
*** 57,62 ****
--- 60,66 ----
  	PGC_SIGHUP,
  	PGC_BACKEND,
  	PGC_SUSET,
+ 	PGC_USERLIMIT,
  	PGC_USERSET
  } GucContext;
  
***************
*** 76,86 ****
  	PGC_S_ENV_VAR = 1,			/* postmaster environment variable */
  	PGC_S_FILE = 2,				/* postgresql.conf */
  	PGC_S_ARGV = 3,				/* postmaster command line */
! 	PGC_S_DATABASE = 4,			/* per-database setting */
! 	PGC_S_USER = 5,				/* per-user setting */
! 	PGC_S_CLIENT = 6,			/* from client connection request */
! 	PGC_S_OVERRIDE = 7,			/* special case to forcibly set default */
! 	PGC_S_SESSION = 8			/* SET command */
  } GucSource;
  
  
--- 80,95 ----
  	PGC_S_ENV_VAR = 1,			/* postmaster environment variable */
  	PGC_S_FILE = 2,				/* postgresql.conf */
  	PGC_S_ARGV = 3,				/* postmaster command line */
! 	PGC_S_USERSTART=4,			/*
! 								 *	Settings below are controlled by users.
! 								 *	This is used by PGC_USERLIMT to prevent
! 								 *	non-super users from changing certain settings.
! 								 */
! 	PGC_S_DATABASE = 5,			/* per-database setting */
! 	PGC_S_USER = 6,				/* per-user setting */
! 	PGC_S_CLIENT = 7,			/* from client connection request */
! 	PGC_S_OVERRIDE = 8,			/* special case to forcibly set default */
! 	PGC_S_SESSION = 9			/* SET command */
  } GucSource;
  
  
#3Bruce Momjian
pgman@candle.pha.pa.us
In reply to: Bruce Momjian (#2)
Re: GUC --- prevent non-super user changes

Sorry, I forgot --- this should have gone only to patches.

---------------------------------------------------------------------------

Bruce Momjian wrote:

Tom Lane wrote:

The recent change to make log_min_messages SUSET provokes the following
behavior:

$ export PGOPTIONS="-d 5"
$ psql
psql: FATAL: 'log_min_messages': permission denied
$

Considering that I *am* superuser, this is quite unacceptable.
If you don't want to revert the change, propose another solution.

Here is a proposed fix for the new SUSET of various variables. The
solution is to create a new GUC context called PGC_USERLIMIT, which
limits changes by non-super users. For example, non-super users can
turn on logging, but can't turn it off, and log_min_* logging can have
added output, but not less output.

The first part of the patch prevent client PGOPTIONS from lowering the
debug level. The second part adds this new GUC context, then allows it
to be set properly. The tests are in two parts --- the first prevents
non-super users from changing the value inappropriately, and the second
allows postgresql.conf changes to apply to existing backends, i.e. if
postgresql.conf turns logging off via SET, turning it on via
postgresql.conf should propogate to the client, because the client can't
turn something off that the admin wants turned on --- that is the tricky
part that we have to be able to handle the settings in any order.

Peter, how does this look? Is reset_val the proper value to test?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
#4Robert Treat
xzilla@users.sourceforge.net
In reply to: Bruce Momjian (#2)
Re: [HACKERS] GUC --- prevent non-super user changes

On Wed, 2003-06-11 at 01:01, Bruce Momjian wrote:

Here is a proposed fix for the new SUSET of various variables. The
solution is to create a new GUC context called PGC_USERLIMIT, which
limits changes by non-super users. For example, non-super users can
turn on logging, but can't turn it off, and log_min_* logging can have
added output, but not less output.

Is there a danger here that users can crank logging up to the max and
either crash a server due to i/o load, or perhaps create enough "noise"
to cover tracks of something malicious?

Robert Treat
--
Build A Brighter Lamp :: Linux Apache {middleware} PostgreSQL

#5Bruce Momjian
pgman@candle.pha.pa.us
In reply to: Robert Treat (#4)
Re: [HACKERS] GUC --- prevent non-super user changes

Robert Treat wrote:

On Wed, 2003-06-11 at 01:01, Bruce Momjian wrote:

Here is a proposed fix for the new SUSET of various variables. The
solution is to create a new GUC context called PGC_USERLIMIT, which
limits changes by non-super users. For example, non-super users can
turn on logging, but can't turn it off, and log_min_* logging can have
added output, but not less output.

Is there a danger here that users can crank logging up to the max and
either crash a server due to i/o load, or perhaps create enough "noise"
to cover tracks of something malicious?

There perhaps is, but if they can connect to the database, I would think
there would be other worse things they can do.

The base problem is that we don't know if the person is a super user
until we connect to pg_shadow, and this is much later than when we
process the flags and PGOPTIONS packet, so the patch seemed like the
cleanest way to go. We could have delayed the setting of those
variables, but there are some variables that have to be set _before_ we
connect to pg_shadow, so it would get tricky.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073