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

diff --git a/ldpd/control.c b/ldpd/control.c
index 5c530e1b701163736f876a63206e7deccfb1f9dd..d40e0342c13a76011e0a112fec96145e2a783feb 100644 (file)
--- a/ldpd/control.c
+++ b/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 */

diff --git a/ldpd/control.h b/ldpd/control.h
index 32c49fdf8760cfd7782210ea1da5bc4092a24d39..0e66a1636aa88354fd83db989b961856cf05aa4f 100644 (file)
--- a/ldpd/control.h
+++ b/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_ */

diff --git a/ldpd/lde.c b/ldpd/lde.c
index c4a610b6d3f1f65bd45719d8727ac563b6f40a2b..ed8274bec789293619aba7336c91a39347633a9d 100644 (file)
--- a/ldpd/lde.c
+++ b/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,19 +180,36 @@ lde(const char *user, const char *group, u_short instance)
                fatal(NULL);
        imsg_init(&iev_main_sync->ibuf, LDPD_FD_SYNC);
 
+       /* create base configuration */
+       ldeconf = config_new_empty();
+
+       /* 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();
 
-       gettimeofday(&now, NULL);
-       global.uptime = now.tv_sec;
-
        /* Init synchronous zclient and label list */
-       zclient_sync_init(instance);
+       zclient_serv_path_set(init->zclient_serv_path);
+       zclient_sync_init(init->instance);
        lde_label_list_init();
-
-       /* Fetch next active thread. */
-       while (thread_fetch(master, &thread))
-               thread_call(&thread);
 }
 
 static 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)

diff --git a/ldpd/lde.h b/ldpd/lde.h
index 57791cd1b0c64ea5a033e349dd188c97736ee65a..3349d4ca000aeca4309e1eacbbe453a87d25bbf4 100644 (file)
--- a/ldpd/lde.h
+++ b/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);

diff --git a/ldpd/ldpd.c b/ldpd/ldpd.c
index 5fbdf52ec27f202a90fa1d60e828e0bf2c536dfe..bdf70973234559c47c333fd8c479e2d0c9219b4a 100644 (file)
--- a/ldpd/ldpd.c
+++ b/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,
-                           const char *, const char *, const char *, const char *);
+static pid_t            start_child(enum ldpd_process, char *, int, int);
 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, ctl_sock_custom_path,
-                           sizeof(ctl_sock_path));
+                       strlcpy(ctl_sock_path, optarg, 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);
-                       instance_char = optarg;
-                       if (instance < 1)
+                       init.instance = atoi(optarg);
+                       if (init.instance < 1)
                                exit(0);
                        break;
                case 'L':
@@ -267,8 +259,11 @@ main(int argc, char *argv[])
                }
        }
 
-       user = ldpd_privs.user;
-       group = ldpd_privs.group;
+       strlcpy(init.user, ldpd_privs.user, sizeof(init.user));
+       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],
-           user, group, ctl_sock_custom_path, instance_char);
+           pipe_parent2lde[1], pipe_parent2lde_sync[1]);
        ldpe_pid = start_child(PROC_LDP_ENGINE, saved_argv0,
-           pipe_parent2ldpe[1], pipe_parent2ldpe_sync[1],
-           user, group, ctl_sock_custom_path, instance_char);
+           pipe_parent2ldpe[1], pipe_parent2ldpe_sync[1]);
 
        /* 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,
-    const char *user, const char *group, const char *ctl_sock_custom_path,
-    const char *instance)
+start_child(enum ldpd_process p, char *argv0, int fd_async, int fd_sync)
 {
-       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);

diff --git a/ldpd/ldpd.h b/ldpd/ldpd.h
index 759f1d0da5586f3af021faef0a030e005e4c03f4..97239ed08edb1a009e02c3840ef741c3641399a8 100644 (file)
--- a/ldpd/ldpd.h
+++ b/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 *);

diff --git a/ldpd/ldpe.c b/ldpd/ldpe.c
index e82416276bb14b67d735378a108df04be4b9c5fa..1248d4f86eec3c841e764b23234fff44fc442c6e 100644 (file)
--- a/ldpd/ldpe.c
+++ b/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. */
-       while (thread_fetch(master, &thread))
-               thread_call(&thread);
+       accept_init();
 }
 
 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;
 

diff --git a/ldpd/ldpe.h b/ldpd/ldpe.h
index a3f41a8b9fa12f1914fb59e0d1eb1a8e2c10c97e..2c1ba46c6b7af727f272a6011570d1fae672242c 100644 (file)
--- a/ldpd/ldpe.h
+++ b/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 *,