ldpd: simplify initialization of the child processes

In order to have separate ASLR/cookies per process, ldpd calls exec()
in the child processes after fork() (this is also known as the fork+exec
model).

This is an important security feature but it makes the initialization
of the child processes a bit more complicated as they're not a copy of
the parent anymore, so all parameters given via command line are lost.

To solve this problem, we were creating an argv array by hand with all
necessary parameters and providing it to the exec() syscall. This works
but it's a very ugly solution. This patch introduces a different approach
to solve the problem: send an IMSG_INIT message to the child processes
with all parameters they need in order to initialize properly. This
makes adding additional initialization parameters much more convenient
and less error prone.

Signed-off-by: Renato Westphal <renato@opensourcerouting.org>

Conflicts:
	ldpd/ldpd.c
	ldpd/ldpd.h

ldpd/control.c

@@ -37,7 +37,7 @@ struct ctl_conns	 ctl_conns;
 static int		 control_fd;
 
 int
-control_init(void)
+control_init(char *path)
 {
 	struct sockaddr_un	 s_un;
 	int			 fd;
@@ -51,28 +51,28 @@ control_init(void)
 
 	memset(&s_un, 0, sizeof(s_un));
 	s_un.sun_family = AF_UNIX;
-	strlcpy(s_un.sun_path, ctl_sock_path, sizeof(s_un.sun_path));
+	strlcpy(s_un.sun_path, path, sizeof(s_un.sun_path));
 
-	if (unlink(ctl_sock_path) == -1)
+	if (unlink(path) == -1)
 		if (errno != ENOENT) {
-			log_warn("%s: unlink %s", __func__, ctl_sock_path);
+			log_warn("%s: unlink %s", __func__, path);
 			close(fd);
 			return (-1);
 		}
 
 	old_umask = umask(S_IXUSR|S_IXGRP|S_IWOTH|S_IROTH|S_IXOTH);
 	if (bind(fd, (struct sockaddr *)&s_un, sizeof(s_un)) == -1) {
-		log_warn("%s: bind: %s", __func__, ctl_sock_path);
+		log_warn("%s: bind: %s", __func__, path);
 		close(fd);
 		umask(old_umask);
 		return (-1);
 	}
 	umask(old_umask);
 
-	if (chmod(ctl_sock_path, S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP) == -1) {
+	if (chmod(path, S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP) == -1) {
 		log_warn("%s: chmod", __func__);
 		close(fd);
-		(void)unlink(ctl_sock_path);
+		(void)unlink(path);
 		return (-1);
 	}
 
@@ -93,11 +93,11 @@ control_listen(void)
 }
 
 void
-control_cleanup(void)
+control_cleanup(char *path)
 {
 	accept_del(control_fd);
 	close(control_fd);
-	unlink(ctl_sock_path);
+	unlink(path);
 }
 
 /* ARGSUSED */

ldpd/control.h

@@ -29,9 +29,9 @@ TAILQ_HEAD(ctl_conns, ctl_conn);
 
 extern struct ctl_conns ctl_conns;
 
-int	control_init(void);
+int	control_init(char *);
 int	control_listen(void);
-void	control_cleanup(void);
+void	control_cleanup(char *);
 int	control_imsg_relay(struct imsg *);
 
 #endif	/* _CONTROL_H_ */

ldpd/lde.c

@@ -152,12 +152,9 @@ zclient_sync_init(u_short instance)
 
 /* label decision engine */
 void
-lde(const char *user, const char *group, u_short instance)
+lde(void)
 {
 	struct thread		 thread;
-	struct timeval		 now;
-
-	ldeconf = config_new_empty();
 
 #ifdef HAVE_SETPROCTITLE
 	setproctitle("label decision engine");
@@ -165,18 +162,6 @@ lde(const char *user, const char *group, u_short instance)
 	ldpd_process = PROC_LDE_ENGINE;
 	log_procname = log_procnames[PROC_LDE_ENGINE];
 
-	/* drop privileges */
-	if (user)
-		lde_privs.user = user;
-	if (group)
-		lde_privs.group = group;
-	zprivs_init(&lde_privs);
-
-#ifdef HAVE_PLEDGE
-	if (pledge("stdio recvfd unix", NULL) == -1)
-		fatal("pledge");
-#endif
-
 	master = thread_master_create();
 
 	/* setup signal handler */
@@ -195,21 +180,38 @@ lde(const char *user, const char *group, u_short instance)
 		fatal(NULL);
 	imsg_init(&iev_main_sync->ibuf, LDPD_FD_SYNC);
 
-	/* start the LIB garbage collector */
+	/* create base configuration */
-	lde_gc_start_timer();
+	ldeconf = config_new_empty();
-
-	gettimeofday(&now, NULL);
-	global.uptime = now.tv_sec;
-
-	/* Init synchronous zclient and label list */
-	zclient_sync_init(instance);
-	lde_label_list_init();
 
 	/* Fetch next active thread. */
 	while (thread_fetch(master, &thread))
 		thread_call(&thread);
 }
 
+void
+lde_init(struct ldpd_init *init)
+{
+	/* drop privileges */
+	if (init->user)
+		lde_privs.user = init->user;
+	if (init->group)
+		lde_privs.group = init->group;
+	zprivs_init(&lde_privs);
+
+#ifdef HAVE_PLEDGE
+	if (pledge("stdio recvfd unix", NULL) == -1)
+		fatal("pledge");
+#endif
+
+	/* start the LIB garbage collector */
+	lde_gc_start_timer();
+
+	/* Init synchronous zclient and label list */
+	zclient_serv_path_set(init->zclient_serv_path);
+	zclient_sync_init(init->instance);
+	lde_label_list_init();
+}
+
 static void
 lde_shutdown(void)
 {
@@ -553,6 +555,14 @@ lde_dispatch_parent(struct thread *thread)
 			iev_ldpe->handler_write = ldp_write_handler;
 			iev_ldpe->ev_write = NULL;
 			break;
+		case IMSG_INIT:
+			if (imsg.hdr.len != IMSG_HEADER_SIZE +
+			    sizeof(struct ldpd_init))
+				fatalx("INIT imsg with wrong len");
+
+			memcpy(&init, imsg.data, sizeof(init));
+			lde_init(&init);
+			break;
 		case IMSG_RECONF_CONF:
 			if ((nconf = malloc(sizeof(struct ldpd_conf))) ==
 			    NULL)

ldpd/lde.h

@@ -139,7 +139,8 @@ extern struct nbr_tree	 lde_nbrs;
 extern struct thread	*gc_timer;
 
 /* lde.c */
-void		 lde(const char *, const char *, u_short instance);
+void		 lde(void);
+void		 lde_init(struct ldpd_init *);
 int		 lde_imsg_compose_parent(int, pid_t, void *, uint16_t);
 int		 lde_imsg_compose_ldpe(int, uint32_t, pid_t, void *, uint16_t);
 int		 lde_acl_check(char *, int, union ldpd_addr *, uint8_t);

ldpd/ldpd.c

@@ -44,8 +44,7 @@
 #include "libfrr.h"
 
 static void		 ldpd_shutdown(void);
-static pid_t		 start_child(enum ldpd_process, char *, int, int,
+static pid_t		 start_child(enum ldpd_process, char *, int, int);
-			    const char *, const char *, const char *, const char *);
 static int		 main_dispatch_ldpe(struct thread *);
 static int		 main_dispatch_lde(struct thread *);
 static int		 main_imsg_send_ipc_sockets(struct imsgbuf *,
@@ -77,6 +76,7 @@ DEFINE_QOBJ_TYPE(l2vpn)
 DEFINE_QOBJ_TYPE(ldpd_conf)
 
 struct ldpd_global	 global;
+struct ldpd_init	 init;
 struct ldpd_conf	*ldpd_conf, *vty_conf;
 
 static struct imsgev	*iev_ldpe, *iev_ldpe_sync;
@@ -200,12 +200,7 @@ main(int argc, char *argv[])
 	int			 lflag = 0, eflag = 0;
 	int			 pipe_parent2ldpe[2], pipe_parent2ldpe_sync[2];
 	int			 pipe_parent2lde[2], pipe_parent2lde_sync[2];
-	char			*ctl_sock_custom_path = NULL;
 	char			*ctl_sock_name;
-	const char		*user = NULL;
-	const char		*group = NULL;
-	u_short			 instance = 0;
-	const char		*instance_char = NULL;
 
 	ldpd_process = PROC_MAIN;
 	log_procname = log_procnames[ldpd_process];
@@ -242,17 +237,14 @@ main(int argc, char *argv[])
 				 * sensible config
 				 */
 				ctl_sock_name = (char *)LDPD_SOCKET;
-			ctl_sock_custom_path = optarg;
+			strlcpy(ctl_sock_path, optarg, sizeof(ctl_sock_path));
-			strlcpy(ctl_sock_path, ctl_sock_custom_path,
-			    sizeof(ctl_sock_path));
 			strlcat(ctl_sock_path, "/", sizeof(ctl_sock_path));
 			strlcat(ctl_sock_path, ctl_sock_name,
 			    sizeof(ctl_sock_path));
 			break;
 		case 'n':
-			instance = atoi(optarg);
+			init.instance = atoi(optarg);
-			instance_char = optarg;
+			if (init.instance < 1)
-			if (instance < 1)
 				exit(0);
 			break;
 		case 'L':
@@ -267,8 +259,11 @@ main(int argc, char *argv[])
 		}
 	}
 
-	user = ldpd_privs.user;
+	strlcpy(init.user, ldpd_privs.user, sizeof(init.user));
-	group = ldpd_privs.group;
+	strlcpy(init.group, ldpd_privs.group, sizeof(init.group));
+	strlcpy(init.ctl_sock_path, ctl_sock_path, sizeof(init.ctl_sock_path));
+	strlcpy(init.zclient_serv_path, zclient_serv_path_get(),
+	    sizeof(init.zclient_serv_path));
 
 	argc -= optind;
 	argv += optind;
@@ -286,9 +281,9 @@ main(int argc, char *argv[])
 	    LOG_CONS | LOG_NDELAY | LOG_PID, LOG_DAEMON);
 
 	if (lflag)
-		lde(user, group, instance);
+		lde();
 	else if (eflag)
-		ldpe(user, group, ctl_sock_path);
+		ldpe();
 
 	if (socketpair(AF_UNIX, SOCK_STREAM, PF_UNSPEC, pipe_parent2ldpe) == -1)
 		fatal("socketpair");
@@ -317,11 +312,9 @@ main(int argc, char *argv[])
 
 	/* start children */
 	lde_pid = start_child(PROC_LDE_ENGINE, saved_argv0,
-	    pipe_parent2lde[1], pipe_parent2lde_sync[1],
+	    pipe_parent2lde[1], pipe_parent2lde_sync[1]);
-	    user, group, ctl_sock_custom_path, instance_char);
 	ldpe_pid = start_child(PROC_LDP_ENGINE, saved_argv0,
-	    pipe_parent2ldpe[1], pipe_parent2ldpe_sync[1],
+	    pipe_parent2ldpe[1], pipe_parent2ldpe_sync[1]);
-	    user, group, ctl_sock_custom_path, instance_char);
 
 	/* drop privileges */
 	zprivs_init(&ldpd_privs);
@@ -389,6 +382,7 @@ main(int argc, char *argv[])
 
 	if (main_imsg_send_ipc_sockets(&iev_ldpe->ibuf, &iev_lde->ibuf))
 		fatal("could not establish imsg links");
+	main_imsg_compose_both(IMSG_INIT, &init, sizeof(init));
 	main_imsg_compose_both(IMSG_DEBUG_UPDATE, &ldp_debug,
 	    sizeof(ldp_debug));
 	main_imsg_send_config(ldpd_conf);
@@ -453,11 +447,9 @@ ldpd_shutdown(void)
 }
 
 static pid_t
-start_child(enum ldpd_process p, char *argv0, int fd_async, int fd_sync,
+start_child(enum ldpd_process p, char *argv0, int fd_async, int fd_sync)
-    const char *user, const char *group, const char *ctl_sock_custom_path,
-    const char *instance)
 {
-	char	*argv[13];
+	char	*argv[3];
 	int	 argc = 0;
 	pid_t	 pid;
 
@@ -488,29 +480,6 @@ start_child(enum ldpd_process p, char *argv0, int fd_async, int fd_sync,
 		argv[argc++] = (char *)"-E";
 		break;
 	}
-	if (user) {
-		argv[argc++] = (char *)"-u";
-		argv[argc++] = (char *)user;
-	}
-	if (group) {
-		argv[argc++] = (char *)"-g";
-		argv[argc++] = (char *)group;
-	}
-	if (ctl_sock_custom_path) {
-		argv[argc++] = (char *)"--ctl_socket";
-		argv[argc++] = (char *)ctl_sock_custom_path;
-	}
-	/* zclient serv path */
-#ifdef HAVE_TCP_ZEBRA
-#else
-	argv[argc++] = (char *)"-z";
-	argv[argc++] = (char *)zclient_serv_path_get();
-#endif
-	/* instance */
-	if (instance) {
-		argv[argc++] = (char *)"-n";
-		argv[argc++] = (char *)instance;
-	}
 	argv[argc++] = NULL;
 
 	execvp(argv0, argv);

ldpd/ldpd.h

@@ -147,7 +147,16 @@ enum imsg_type {
 	IMSG_LOG,
 	IMSG_ACL_CHECK,
 	IMSG_GET_LABEL_CHUNK,
-	IMSG_RELEASE_LABEL_CHUNK
+	IMSG_RELEASE_LABEL_CHUNK,
+	IMSG_INIT
+};
+
+struct ldpd_init {
+	char		 user[256];
+	char		 group[256];
+	char		 ctl_sock_path[MAXPATHLEN];
+	char		 zclient_serv_path[MAXPATHLEN];
+	u_short		 instance;
 };
 
 union ldpd_addr {
@@ -510,7 +519,6 @@ struct ldpd_af_global {
 struct ldpd_global {
 	int			 cmd_opts;
 	int			 sighup;
-	time_t			 uptime;
 	struct in_addr		 rtr_id;
 	struct ldpd_af_global	 ipv4;
 	struct ldpd_af_global	 ipv6;
@@ -655,6 +663,7 @@ struct ctl_pw {
 
 extern struct ldpd_conf		*ldpd_conf, *vty_conf;
 extern struct ldpd_global	 global;
+extern struct ldpd_init		 init;
 
 /* parse.y */
 struct ldpd_conf	*parse_config(char *);

ldpd/ldpe.c

@@ -103,47 +103,17 @@ static struct quagga_signal_t ldpe_signals[] =
 
 /* label distribution protocol engine */
 void
-ldpe(const char *user, const char *group, const char *ctl_path)
+ldpe(void)
 {
 	struct thread		 thread;
 
-	leconf = config_new_empty();
-
 #ifdef HAVE_SETPROCTITLE
 	setproctitle("ldp engine");
 #endif
 	ldpd_process = PROC_LDP_ENGINE;
 	log_procname = log_procnames[ldpd_process];
 
-	LIST_INIT(&global.addr_list);
-	RB_INIT(&global.adj_tree);
-	TAILQ_INIT(&global.pending_conns);
-	if (inet_pton(AF_INET, AllRouters_v4, &global.mcast_addr_v4) != 1)
-		fatal("inet_pton");
-	if (inet_pton(AF_INET6, AllRouters_v6, &global.mcast_addr_v6) != 1)
-		fatal("inet_pton");
-#ifdef __OpenBSD__
-	global.pfkeysock = pfkey_init();
-#endif
-
-	/* drop privileges */
-	if (user)
-		ldpe_privs.user = user;
-	if (group)
-		ldpe_privs.group = group;
-	zprivs_init(&ldpe_privs);
-
-	strlcpy(ctl_sock_path, ctl_path, sizeof(ctl_sock_path));
-	if (control_init() == -1)
-		fatalx("control socket setup failed");
-
-#ifdef HAVE_PLEDGE
-	if (pledge("stdio cpath inet mcast recvfd", NULL) == -1)
-		fatal("pledge");
-#endif
-
   	master = thread_master_create();
-	accept_init();
 
 	/* setup signal handler */
 	signal_init(master, array_size(ldpe_signals), ldpe_signals);
@@ -161,7 +131,45 @@ ldpe(const char *user, const char *group, const char *ctl_path)
 		fatal(NULL);
 	imsg_init(&iev_main_sync->ibuf, LDPD_FD_SYNC);
 
+	/* create base configuration */
+	leconf = config_new_empty();
+
+	/* Fetch next active thread. */
+	while (thread_fetch(master, &thread))
+		thread_call(&thread);
+}
+
+void
+ldpe_init(struct ldpd_init *init)
+{
+	/* drop privileges */
+	if (init->user)
+		ldpe_privs.user = init->user;
+	if (init->group)
+		ldpe_privs.group = init->group;
+	zprivs_init(&ldpe_privs);
+
+	/* listen on ldpd control socket */
+	strlcpy(ctl_sock_path, init->ctl_sock_path, sizeof(ctl_sock_path));
+	if (control_init(ctl_sock_path) == -1)
+		fatalx("control socket setup failed");
+	TAILQ_INIT(&ctl_conns);
+	control_listen();
+
+#ifdef HAVE_PLEDGE
+	if (pledge("stdio cpath inet mcast recvfd", NULL) == -1)
+		fatal("pledge");
+#endif
+
+	LIST_INIT(&global.addr_list);
+	RB_INIT(&global.adj_tree);
+	TAILQ_INIT(&global.pending_conns);
+	if (inet_pton(AF_INET, AllRouters_v4, &global.mcast_addr_v4) != 1)
+		fatal("inet_pton");
+	if (inet_pton(AF_INET6, AllRouters_v6, &global.mcast_addr_v6) != 1)
+		fatal("inet_pton");
 #ifdef __OpenBSD__
+	global.pfkeysock = pfkey_init();
 	if (sysdep.no_pfkey == 0)
 		pfkey_ev = thread_add_read(master, ldpe_dispatch_pfkey,
 		    NULL, global.pfkeysock);
@@ -175,16 +183,10 @@ ldpe(const char *user, const char *group, const char *ctl_path)
 	global.ipv6.ldp_edisc_socket = -1;
 	global.ipv6.ldp_session_socket = -1;
 
-	/* listen on ldpd control socket */
-	TAILQ_INIT(&ctl_conns);
-	control_listen();
-
 	if ((pkt_ptr = calloc(1, IBUF_READ_SIZE)) == NULL)
 		fatal(__func__);
 
-	/* Fetch next active thread. */
+	accept_init();
-	while (thread_fetch(master, &thread))
-		thread_call(&thread);
 }
 
 static void
@@ -203,7 +205,7 @@ ldpe_shutdown(void)
 	msgbuf_clear(&iev_main_sync->ibuf.w);
 	close(iev_main_sync->ibuf.fd);
 
-	control_cleanup();
+	control_cleanup(ctl_sock_path);
 	config_clear(leconf);
 
 #ifdef __OpenBSD__
@@ -352,6 +354,14 @@ ldpe_dispatch_main(struct thread *thread)
 			iev_lde->handler_write = ldp_write_handler;
 			iev_lde->ev_write = NULL;
 			break;
+		case IMSG_INIT:
+			if (imsg.hdr.len != IMSG_HEADER_SIZE +
+			    sizeof(struct ldpd_init))
+				fatalx("INIT imsg with wrong len");
+
+			memcpy(&init, imsg.data, sizeof(init));
+			ldpe_init(&init);
+			break;
 		case IMSG_CLOSE_SOCKETS:
 			af = imsg.hdr.peerid;
 

ldpd/ldpe.h

@@ -195,7 +195,8 @@ int	 tlv_decode_fec_elm(struct nbr *, struct ldp_msg *, char *,
 	    uint16_t, struct map *);
 
 /* ldpe.c */
-void		 ldpe(const char *, const char *, const char *);
+void		 ldpe(void);
+void		 ldpe_init(struct ldpd_init *);
 int		 ldpe_imsg_compose_parent(int, pid_t, void *,
 		    uint16_t);
 int		 ldpe_imsg_compose_lde(int, uint32_t, pid_t, void *,