On Thu, Sep 29, 2016 at 08:09:23PM +0200, Alexander Bluhm wrote:
> Hi,
>
> With this diff syslogd(8) does an exec on itself in the privileged
> parent process to reshuffle its memory layout.
>
> As syslogd only forks once, it does not really matter wether we
> fork+exec in the child or in the parent. To do it in the parent
> is easier as it has much less state.
>
> ok?
>
> bluhm
Your diffs looks good and you made me realize that I should use dup3()
instead of dup2() to create children socket.
Short explanation for outsiders: dup2(fd1, fd2) duplicates fd1 onto fd2
removing CLOEXEC flags, except if fd1 == fd2, then in that case the fd
will remain with CLOEXEC and things will not work. This is not the case
with httpd(8), relayd(8), ntpd(8) and switchd(8), but since code might
be copied around it would be good to fix this there.
I'm using this diff and it works in my default configuration, but since
I'm not familiar with syslogd I don't feel confortable giving oks here.
I made one comment inline in the snipped diff below.
>
> Index: usr.sbin/syslogd/privsep.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/privsep.c,v
> retrieving revision 1.61
> diff -u -p -r1.61 privsep.c
> --- usr.sbin/syslogd/privsep.c 28 Jun 2016 18:22:50 -0000 1.61
> +++ usr.sbin/syslogd/privsep.c 29 Sep 2016 17:55:03 -0000
> @@ -194,38 +162,87 @@ priv_init(char *conf, int numeric, int l
> if (fd_unix[i] != -1)
> close(fd_unix[i]);
>
> - /* Save the config file specified by the child process */
> - if (strlcpy(config_file, conf, sizeof config_file) >=
> sizeof(config_file))
> - errx(1, "config_file truncation");
> + if (dup3(socks[0], 3, 0) == -1)
> + err(1, "dup3 priv sock failed");
> + snprintf(childnum, sizeof(childnum), "%d", child_pid);
> + if ((privargv = reallocarray(NULL, argc + 3, sizeof(char *))) == NULL)
> + err(1, "alloc priv argv failed");
> + for (i = 0; i < argc; i++)
> + privargv[i] = argv[i];
> + privargv[i++] = "-P";
> + privargv[i++] = childnum;
> + privargv[i++] = NULL;
> + execv(privargv[0], privargv);
> + err(1, "exec priv '%s' failed", privargv[0]);
> +}
>
> - if (stat(config_file, &cf_info) < 0)
> - err(1, "stat config file failed");
> +__dead void
> +priv_exec(char *conf, int numeric, int child, int argc, char *argv[])
> +{
> + int i, fd, sock, cmd, addr_len, result, restart;
> + size_t path_len, protoname_len, hostname_len, servname_len;
> + char path[PATH_MAX], protoname[5];
> + char hostname[NI_MAXHOST], servname[NI_MAXSERV];
> + struct sockaddr_storage addr;
> + struct stat cf_info, cf_stat;
> + struct addrinfo hints, *res0;
> + struct sigaction sa;
>
> - /* Save whether or not the child can have access to getnameinfo(3) */
> - if (numeric > 0)
> - allow_getnameinfo = 0;
> - else
> - allow_getnameinfo = 1;
> + if (pledge("stdio rpath wpath cpath dns getpw sendfd id proc exec",
> + NULL) == -1)
> + err(1, "pledge priv");
> +
> + if (argc <= 2 || strcmp("-P", argv[argc - 2]) != 0)
> + errx(1, "exec without priv");
> + argv[argc -= 2] = NULL;
> +
> + sock = 3;
> + for (fd = 4; fd < 1024; fd++)
> + close(fd);
This could be replaced with "closefrom(4);".
> +
> + child_pid = child;
> +
> + memset(&sa, 0, sizeof(sa));
> + sigemptyset(&sa.sa_mask);
> + sa.sa_flags = SA_RESTART;
> + sa.sa_handler = SIG_DFL;
> + for (i = 1; i < _NSIG; i++)
> + sigaction(i, &sa, NULL);
> +
> + /* Pass TERM/HUP/INT/QUIT through to child, and accept CHLD */
> + sa.sa_handler = sig_pass_to_chld;
> + sigaction(SIGTERM, &sa, NULL);
> + sigaction(SIGHUP, &sa, NULL);
> + sigaction(SIGINT, &sa, NULL);
> + sigaction(SIGQUIT, &sa, NULL);
> + sa.sa_handler = sig_got_chld;
> + sa.sa_flags |= SA_NOCLDSTOP;
> + sigaction(SIGCHLD, &sa, NULL);
> +
> + setproctitle("[priv]");
> +
> + if (stat(conf, &cf_info) < 0)
> + err(1, "stat config file failed");
>
> TAILQ_INIT(&lognames);
> increase_state(STATE_CONFIG);
> restart = 0;
>
> while (cur_state < STATE_QUIT) {
> - if (may_read(socks[0], &cmd, sizeof(int)))
> + if (may_read(sock, &cmd, sizeof(int)))
> break;
> switch (cmd) {
> case PRIV_OPEN_TTY:
> logdebug("[priv]: msg PRIV_OPEN_TTY received\n");
> /* Expecting: length, path */
> - must_read(socks[0], &path_len, sizeof(size_t));
> + must_read(sock, &path_len, sizeof(size_t));
> if (path_len == 0 || path_len > sizeof(path))
> _exit(1);
> - must_read(socks[0], &path, path_len);
> + must_read(sock, &path, path_len);
> path[path_len - 1] = '\0';
> check_tty_name(path, sizeof(path));
> fd = open(path, O_WRONLY|O_NONBLOCK, 0);
> - send_fd(socks[0], fd);
> + send_fd(sock, fd);
> if (fd < 0)
> warnx("priv_open_tty failed");
> else
