Keep a master copy of the parameter block, to which all changes are applied,

and which is copied to the shared parameter block every time a parameter
changes as well as immediately before forking off a child.  This prevents a
hypothetical compromised child from changing the parent's idea of run-time
parameters (which would, for example, allow it to trick the the parent into
starting a new, hypothetically exploitable child with the attacker's choice
of uid / gid).

While I'm here, correct the use of the "volatile" qualifier - it is the
parmeter block itself which can change unpredictably, not the pointer.


git-svn-id: http://www.varnish-cache.org/svn/trunk/varnish-cache@1484 d4fa192b-c00b-0410-8231-f00ffab90ce4

bin/varnishd/heritage.h

@@ -121,7 +121,7 @@ struct params {
 	unsigned		ping_interval;
 };
 
-extern volatile struct params *params;
+extern struct params * volatile params;
 extern struct heritage heritage;
 
 void child_main(void);

bin/varnishd/mgt.h

@@ -52,6 +52,7 @@ void mgt_cli_stop_child(void);
 int mgt_cli_telnet(const char *T_arg);
 
 /* mgt_param.c */
+void MCF_ParamSync(void);
 void MCF_ParamInit(struct cli *);
 void MCF_ParamSet(struct cli *, const char *param, const char *val);
 

bin/varnishd/mgt_child.c

@@ -173,6 +173,7 @@ start_child(void)
 	AZ(pipe(&heritage.fds[0]));
 	AZ(pipe(&heritage.fds[2]));
 	AZ(pipe(child_fds));
+	MCF_ParamSync();
 	i = fork();
 	if (i < 0)
 		errx(1, "Could not fork child");

bin/varnishd/mgt_param.c

@@ -59,6 +59,8 @@ struct parspec {
 	const char	*units;
 };
 
+static struct params master;
+
 /*--------------------------------------------------------------------*/
 
 static void
@@ -150,26 +152,26 @@ tweak_user(struct cli *cli, struct parspec *par, const char *arg)
 			cli_result(cli, CLIS_PARAM);
 			return;
 		}
-		if (params->user)
-			free(params->user);
-		params->user = strdup(pw->pw_name);
-		AN(params->user);
-		params->uid = pw->pw_uid;
+		if (master.user)
+			free(master.user);
+		master.user = strdup(pw->pw_name);
+		AN(master.user);
+		master.uid = pw->pw_uid;
 
 		/* set group to user's primary group */
-		if (params->group)
-			free(params->group);
+		if (master.group)
+			free(master.group);
 		if ((gr = getgrgid(pw->pw_gid)) != NULL &&
 		    (gr = getgrnam(gr->gr_name)) != NULL &&
 		    gr->gr_gid == pw->pw_gid) {
-			params->group = strdup(gr->gr_name);
-			AN(params->group);
+			master.group = strdup(gr->gr_name);
+			AN(master.group);
 		}
-		params->gid = pw->pw_gid;
-	} else if (params->user) {
-		cli_out(cli, "%s (%d)", params->user, (int)params->uid);
+		master.gid = pw->pw_gid;
+	} else if (master.user) {
+		cli_out(cli, "%s (%d)", master.user, (int)master.uid);
 	} else {
-		cli_out(cli, "%d", (int)params->uid);
+		cli_out(cli, "%d", (int)master.uid);
 	}
 }
 
@@ -187,15 +189,15 @@ tweak_group(struct cli *cli, struct parspec *par, const char *arg)
 			cli_result(cli, CLIS_PARAM);
 			return;
 		}
-		if (params->group)
-			free(params->group);
-		params->group = strdup(gr->gr_name);
-		AN(params->group);
-		params->gid = gr->gr_gid;
-	} else if (params->group) {
-		cli_out(cli, "%s (%d)", params->group, (int)params->gid);
+		if (master.group)
+			free(master.group);
+		master.group = strdup(gr->gr_name);
+		AN(master.group);
+		master.gid = gr->gr_gid;
+	} else if (master.group) {
+		cli_out(cli, "%s (%d)", master.group, (int)master.gid);
 	} else {
-		cli_out(cli, "%d", (int)params->gid);
+		cli_out(cli, "%d", (int)master.gid);
 	}
 }
 
@@ -206,7 +208,7 @@ tweak_default_ttl(struct cli *cli, struct parspec *par, const char *arg)
 {
 
 	(void)par;
-	tweak_generic_uint(cli, &params->default_ttl, arg, 0, UINT_MAX);
+	tweak_generic_uint(cli, &master.default_ttl, arg, 0, UINT_MAX);
 }
 
 /*--------------------------------------------------------------------*/
@@ -216,7 +218,7 @@ tweak_thread_pools(struct cli *cli, struct parspec *par, const char *arg)
 {
 
 	(void)par;
-	tweak_generic_uint(cli, &params->wthread_pools, arg,
+	tweak_generic_uint(cli, &master.wthread_pools, arg,
 	    1, UINT_MAX);
 }
 
@@ -228,8 +230,8 @@ tweak_thread_pool_min(struct cli *cli, struct parspec *par, const char *arg)
 {
 
 	(void)par;
-	tweak_generic_uint(cli, &params->wthread_min, arg,
-	    0, params->wthread_max);
+	tweak_generic_uint(cli, &master.wthread_min, arg,
+	    0, master.wthread_max);
 }
 
 /*--------------------------------------------------------------------*/
@@ -239,8 +241,8 @@ tweak_thread_pool_max(struct cli *cli, struct parspec *par, const char *arg)
 {
 
 	(void)par;
-	tweak_generic_uint(cli, &params->wthread_max, arg,
-	    params->wthread_min, UINT_MAX);
+	tweak_generic_uint(cli, &master.wthread_max, arg,
+	    master.wthread_min, UINT_MAX);
 }
 
 /*--------------------------------------------------------------------*/
@@ -250,7 +252,7 @@ tweak_thread_pool_timeout(struct cli *cli, struct parspec *par, const char *arg)
 {
 
 	(void)par;
-	tweak_generic_timeout(cli, &params->wthread_timeout, arg);
+	tweak_generic_timeout(cli, &master.wthread_timeout, arg);
 }
 
 /*--------------------------------------------------------------------*/
@@ -260,7 +262,7 @@ tweak_overflow_max(struct cli *cli, struct parspec *par, const char *arg)
 {
 
 	(void)par;
-	tweak_generic_uint(cli, &params->overflow_max, arg, 0, UINT_MAX);
+	tweak_generic_uint(cli, &master.overflow_max, arg, 0, UINT_MAX);
 }
 
 /*--------------------------------------------------------------------*/
@@ -270,7 +272,7 @@ tweak_http_workspace(struct cli *cli, struct parspec *par, const char *arg)
 {
 
 	(void)par;
-	tweak_generic_uint(cli, &params->mem_workspace, arg,
+	tweak_generic_uint(cli, &master.mem_workspace, arg,
 	    1024, UINT_MAX);
 }
 
@@ -280,7 +282,7 @@ static void
 tweak_sess_timeout(struct cli *cli, struct parspec *par, const char *arg)
 {
 	(void)par;
-	tweak_generic_timeout(cli, &params->sess_timeout, arg);
+	tweak_generic_timeout(cli, &master.sess_timeout, arg);
 }
 
 /*--------------------------------------------------------------------*/
@@ -289,7 +291,7 @@ static void
 tweak_pipe_timeout(struct cli *cli, struct parspec *par, const char *arg)
 {
 	(void)par;
-	tweak_generic_timeout(cli, &params->pipe_timeout, arg);
+	tweak_generic_timeout(cli, &master.pipe_timeout, arg);
 }
 
 /*--------------------------------------------------------------------*/
@@ -298,7 +300,7 @@ static void
 tweak_send_timeout(struct cli *cli, struct parspec *par, const char *arg)
 {
 	(void)par;
-	tweak_generic_timeout(cli, &params->send_timeout, arg);
+	tweak_generic_timeout(cli, &master.send_timeout, arg);
 }
 
 /*--------------------------------------------------------------------*/
@@ -308,7 +310,7 @@ tweak_auto_restart(struct cli *cli, struct parspec *par, const char *arg)
 {
 
 	(void)par;
-	tweak_generic_bool(cli, &params->auto_restart, arg);
+	tweak_generic_bool(cli, &master.auto_restart, arg);
 }
 
 /*--------------------------------------------------------------------*/
@@ -318,7 +320,7 @@ tweak_fetch_chunksize(struct cli *cli, struct parspec *par, const char *arg)
 {
 
 	(void)par;
-	tweak_generic_uint(cli, &params->fetch_chunksize, arg,
+	tweak_generic_uint(cli, &master.fetch_chunksize, arg,
 	    4, UINT_MAX / 1024);
 }
 
@@ -330,7 +332,7 @@ tweak_sendfile_threshold(struct cli *cli, struct parspec *par, const char *arg)
 {
 
 	(void)par;
-	tweak_generic_uint(cli, &params->sendfile_threshold, arg, 0, UINT_MAX);
+	tweak_generic_uint(cli, &master.sendfile_threshold, arg, 0, UINT_MAX);
 }
 #endif /* HAVE_SENDFILE */
 
@@ -340,7 +342,7 @@ static void
 tweak_vcl_trace(struct cli *cli, struct parspec *par, const char *arg)
 {
 	(void)par;
-	tweak_generic_bool(cli, &params->vcl_trace, arg);
+	tweak_generic_bool(cli, &master.vcl_trace, arg);
 }
 
 /*--------------------------------------------------------------------*/
@@ -369,9 +371,9 @@ tweak_listen_address(struct cli *cli, struct parspec *par, const char *arg)
 	if (arg == NULL) {
 		/* Quote the string if we have more than one socket */
 		if (heritage.nsocks > 1)
-			cli_out(cli, "\"%s\"", params->listen_address);
+			cli_out(cli, "\"%s\"", master.listen_address);
 		else
-			cli_out(cli, "%s", params->listen_address);
+			cli_out(cli, "%s", master.listen_address);
 		return;
 	}
 
@@ -422,9 +424,9 @@ tweak_listen_address(struct cli *cli, struct parspec *par, const char *arg)
 		return;
 	}
 
-	free(params->listen_address);
-	params->listen_address = strdup(arg);
-	AN(params->listen_address);
+	free(master.listen_address);
+	master.listen_address = strdup(arg);
+	AN(master.listen_address);
 
 	clean_listen_sock_head(&heritage.socks);
 	heritage.nsocks = 0;
@@ -443,7 +445,7 @@ static void
 tweak_listen_depth(struct cli *cli, struct parspec *par, const char *arg)
 {
 	(void)par;
-	tweak_generic_uint(cli, &params->listen_depth, arg, 0, UINT_MAX);
+	tweak_generic_uint(cli, &master.listen_depth, arg, 0, UINT_MAX);
 }
 
 /*--------------------------------------------------------------------*/
@@ -452,7 +454,7 @@ static void
 tweak_srcaddr_hash(struct cli *cli, struct parspec *par, const char *arg)
 {
 	(void)par;
-	tweak_generic_uint(cli, &params->srcaddr_hash, arg, 63, UINT_MAX);
+	tweak_generic_uint(cli, &master.srcaddr_hash, arg, 63, UINT_MAX);
 }
 
 /*--------------------------------------------------------------------*/
@@ -461,7 +463,7 @@ static void
 tweak_srcaddr_ttl(struct cli *cli, struct parspec *par, const char *arg)
 {
 	(void)par;
-	tweak_generic_uint(cli, &params->srcaddr_ttl, arg, 0, UINT_MAX);
+	tweak_generic_uint(cli, &master.srcaddr_ttl, arg, 0, UINT_MAX);
 }
 
 /*--------------------------------------------------------------------*/
@@ -470,7 +472,7 @@ static void
 tweak_backend_http11(struct cli *cli, struct parspec *par, const char *arg)
 {
 	(void)par;
-	tweak_generic_bool(cli, &params->backend_http11, arg);
+	tweak_generic_bool(cli, &master.backend_http11, arg);
 }
 
 /*--------------------------------------------------------------------*/
@@ -479,7 +481,7 @@ static void
 tweak_client_http11(struct cli *cli, struct parspec *par, const char *arg)
 {
 	(void)par;
-	tweak_generic_bool(cli, &params->client_http11, arg);
+	tweak_generic_bool(cli, &master.client_http11, arg);
 }
 
 /*--------------------------------------------------------------------*/
@@ -488,7 +490,7 @@ static void
 tweak_ping_interval(struct cli *cli, struct parspec *par, const char *arg)
 {
 	(void)par;
-	tweak_generic_uint(cli, &params->ping_interval, arg, 0, UINT_MAX);
+	tweak_generic_uint(cli, &master.ping_interval, arg, 0, UINT_MAX);
 }
 
 /*--------------------------------------------------------------------*/
@@ -732,6 +734,15 @@ mcf_param_show(struct cli *cli, char **av, void *priv)
 
 /*--------------------------------------------------------------------*/
 
+void
+MCF_ParamSync(void)
+{
+	if (params != &master)
+		*params = master;
+}
+
+/*--------------------------------------------------------------------*/
+
 void
 MCF_ParamSet(struct cli *cli, const char *param, const char *val)
 {
@@ -745,6 +756,7 @@ MCF_ParamSet(struct cli *cli, const char *param, const char *val)
 	}
 	cli_result(cli, CLIS_PARAM);
 	cli_out(cli, "Unknown paramter \"%s\".", param);
+	MCF_ParamSync();
 }
 
 
@@ -771,4 +783,5 @@ MCF_ParamInit(struct cli *cli)
 		if (cli->result != CLIS_OK)
 			return;
 	}
+	params = &master;
 }

bin/varnishd/varnishd.c

@@ -66,7 +66,7 @@
 #endif
 
 struct heritage heritage;
-volatile struct params *params;
+struct params * volatile params;
 
 /*--------------------------------------------------------------------*/
 
@@ -407,7 +407,6 @@ main(int argc, char *argv[])
 	const char *T_arg = NULL;
 	unsigned C_flag = 0;
 	char *p;
-	struct params param;
 	struct cli cli[1];
 	struct pidfh *pfh = NULL;
 
@@ -419,23 +418,7 @@ main(int argc, char *argv[])
 	XXXAN(cli[0].sb);
 	cli[0].result = CLIS_OK;
 
-	/*
-	 * Set up a temporary param block until VSL_MgtInit() can
-	 * replace with shmem backed structure version.
-	 *
-	 * XXX: I wonder if it would be smarter to inform the child process
-	 * XXX: about param changes via CLI rather than keeping the param
-	 * XXX: block in shared memory.  It would give us the advantage
-	 * XXX: of having the CLI thread be able to take action on the
-	 * XXX: change.
-	 * XXX: For now live with the harmless flexelint warning this causes:
-	 * XXX: varnishd.c 393 Info 789: Assigning address of auto variable
-	 * XXX:    'param' to static
-	 */
-
 	TAILQ_INIT(&heritage.socks);
-	memset(&param, 0, sizeof param);
-	params = &param;
 	mgt_vcc_init();
 
 	MCF_ParamInit(cli);