On Sat, Oct 01, 2016 at 07:41:13PM +0200, Rafael Zalamena wrote:
> This could be replaced with "closefrom(4);".
Updated diff:
- use closefrom(2)
- use execvp(3) to allow starting syslogd(8) without full path
- add a debug message to make testing easier
ok?
bluhm
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 -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 2 Oct 2016 20:57:10 -0000
@@ -2,6 +2,7 @@
/*
* Copyright (c) 2003 Anil Madhavapeddy <a...@recoil.org>
+ * Copyright (c) 2016 Alexander Bluhm <bl...@openbsd.org>
*
* Permission to use, copy, modify, and distribute this software for any
* purpose with or without fee is hereby granted, provided that the above
@@ -71,9 +72,6 @@ enum cmd_types {
static int priv_fd = -1;
static volatile pid_t child_pid = -1;
-static char config_file[PATH_MAX];
-static struct stat cf_info;
-static int allow_getnameinfo = 0;
static volatile sig_atomic_t cur_state = STATE_INIT;
/* Queue for the allowed logfiles */
@@ -94,25 +92,12 @@ static void must_read(int, void *, size_
static void must_write(int, void *, size_t);
static int may_read(int, void *, size_t);
-int
-priv_init(char *conf, int numeric, int lockfd, int nullfd, char *argv[])
+void
+priv_init(int lockfd, int nullfd, int argc, char *argv[])
{
- int i, fd, socks[2], 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_stat;
+ int i, socks[2];
struct passwd *pw;
- struct addrinfo hints, *res0;
- struct sigaction sa;
-
- 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);
+ char childnum[11], **privargv;
/* Create sockets */
if (socketpair(AF_LOCAL, SOCK_STREAM, PF_UNSPEC, socks) == -1)
@@ -141,12 +126,9 @@ priv_init(char *conf, int numeric, int l
err(1, "setresuid() failed");
close(socks[0]);
priv_fd = socks[1];
- return 0;
+ return;
}
-
- if (pledge("stdio rpath wpath cpath dns getpw sendfd id proc exec",
- NULL) == -1)
- err(1, "pledge priv");
+ close(socks[1]);
if (!Debug) {
close(lockfd);
@@ -157,20 +139,6 @@ priv_init(char *conf, int numeric, int l
if (nullfd > 2)
close(nullfd);
- /* Father */
- /* 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]");
- close(socks[1]);
-
/* Close descriptors that only the unpriv child needs */
if (fd_ctlconn != -1)
close(fd_ctlconn);
@@ -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;
+ execvp(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;
+
+ if (pledge("stdio rpath wpath cpath dns getpw sendfd id proc exec",
+ NULL) == -1)
+ err(1, "pledge priv");
- /* Save whether or not the child can have access to getnameinfo(3) */
- if (numeric > 0)
- allow_getnameinfo = 0;
- else
- allow_getnameinfo = 1;
+ if (argc <= 2 || strcmp("-P", argv[argc - 2]) != 0)
+ errx(1, "exec without priv");
+ argv[argc -= 2] = NULL;
+
+ sock = 3;
+ 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]");
+ logdebug("[priv]: fork+exec done\n");
+
+ 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
@@ -237,10 +254,10 @@ priv_init(char *conf, int numeric, int l
logdebug("[priv]: msg PRIV_OPEN_%s received\n",
cmd == PRIV_OPEN_PIPE ? "PIPE" : "LOG");
/* 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_log_name(path, sizeof(path));
@@ -251,7 +268,7 @@ priv_init(char *conf, int numeric, int l
else
errx(1, "invalid cmd");
- send_fd(socks[0], fd);
+ send_fd(sock, fd);
if (fd < 0)
warnx("priv_open_log failed");
else
@@ -261,7 +278,7 @@ priv_init(char *conf, int numeric, int l
case PRIV_OPEN_UTMP:
logdebug("[priv]: msg PRIV_OPEN_UTMP received\n");
fd = open(_PATH_UTMP, O_RDONLY|O_NONBLOCK, 0);
- send_fd(socks[0], fd);
+ send_fd(sock, fd);
if (fd < 0)
warnx("priv_open_utmp failed");
else
@@ -270,9 +287,9 @@ priv_init(char *conf, int numeric, int l
case PRIV_OPEN_CONFIG:
logdebug("[priv]: msg PRIV_OPEN_CONFIG received\n");
- stat(config_file, &cf_info);
- fd = open(config_file, O_RDONLY|O_NONBLOCK, 0);
- send_fd(socks[0], fd);
+ stat(conf, &cf_info);
+ fd = open(conf, O_RDONLY|O_NONBLOCK, 0);
+ send_fd(sock, fd);
if (fd < 0)
warnx("priv_open_config failed");
else
@@ -281,16 +298,16 @@ priv_init(char *conf, int numeric, int l
case PRIV_CONFIG_MODIFIED:
logdebug("[priv]: msg PRIV_CONFIG_MODIFIED received\n");
- if (stat(config_file, &cf_stat) < 0 ||
+ if (stat(conf, &cf_stat) < 0 ||
timespeccmp(&cf_info.st_mtimespec,
&cf_stat.st_mtimespec, <) ||
cf_info.st_size != cf_stat.st_size) {
logdebug("config file modified: restarting\n");
restart = result = 1;
- must_write(socks[0], &result, sizeof(int));
+ must_write(sock, &result, sizeof(int));
} else {
result = 0;
- must_write(socks[0], &result, sizeof(int));
+ must_write(sock, &result, sizeof(int));
}
break;
@@ -303,25 +320,25 @@ priv_init(char *conf, int numeric, int l
case PRIV_GETADDRINFO:
logdebug("[priv]: msg PRIV_GETADDRINFO received\n");
/* Expecting: len, proto, len, host, len, serv */
- must_read(socks[0], &protoname_len, sizeof(size_t));
+ must_read(sock, &protoname_len, sizeof(size_t));
if (protoname_len == 0 ||
protoname_len > sizeof(protoname))
_exit(1);
- must_read(socks[0], &protoname, protoname_len);
+ must_read(sock, &protoname, protoname_len);
protoname[protoname_len - 1] = '\0';
- must_read(socks[0], &hostname_len, sizeof(size_t));
+ must_read(sock, &hostname_len, sizeof(size_t));
if (hostname_len == 0 ||
hostname_len > sizeof(hostname))
_exit(1);
- must_read(socks[0], &hostname, hostname_len);
+ must_read(sock, &hostname, hostname_len);
hostname[hostname_len - 1] = '\0';
- must_read(socks[0], &servname_len, sizeof(size_t));
+ must_read(sock, &servname_len, sizeof(size_t));
if (servname_len == 0 ||
servname_len > sizeof(servname))
_exit(1);
- must_read(socks[0], &servname, servname_len);
+ must_read(sock, &servname, servname_len);
servname[servname_len - 1] = '\0';
memset(&hints, 0, sizeof(hints));
@@ -356,34 +373,34 @@ priv_init(char *conf, int numeric, int l
i = getaddrinfo(hostname, servname, &hints, &res0);
if (i != 0 || res0 == NULL) {
addr_len = 0;
- must_write(socks[0], &addr_len, sizeof(int));
+ must_write(sock, &addr_len, sizeof(int));
} else {
/* Just send the first address */
i = res0->ai_addrlen;
- must_write(socks[0], &i, sizeof(int));
- must_write(socks[0], res0->ai_addr, i);
+ must_write(sock, &i, sizeof(int));
+ must_write(sock, res0->ai_addr, i);
freeaddrinfo(res0);
}
break;
case PRIV_GETNAMEINFO:
logdebug("[priv]: msg PRIV_GETNAMEINFO received\n");
- if (!allow_getnameinfo)
+ if (numeric)
errx(1, "rejected attempt to getnameinfo");
/* Expecting: length, sockaddr */
- must_read(socks[0], &addr_len, sizeof(int));
+ must_read(sock, &addr_len, sizeof(int));
if (addr_len <= 0 || (size_t)addr_len > sizeof(addr))
_exit(1);
- must_read(socks[0], &addr, addr_len);
+ must_read(sock, &addr, addr_len);
if (getnameinfo((struct sockaddr *)&addr, addr_len,
hostname, sizeof(hostname), NULL, 0,
NI_NOFQDN|NI_NAMEREQD|NI_DGRAM) != 0) {
addr_len = 0;
- must_write(socks[0], &addr_len, sizeof(int));
+ must_write(sock, &addr_len, sizeof(int));
} else {
addr_len = strlen(hostname) + 1;
- must_write(socks[0], &addr_len, sizeof(int));
- must_write(socks[0], hostname, addr_len);
+ must_write(sock, &addr_len, sizeof(int));
+ must_write(sock, hostname, addr_len);
}
break;
default:
@@ -392,23 +409,23 @@ priv_init(char *conf, int numeric, int l
}
}
- close(socks[0]);
+ close(sock);
/* Unlink any domain sockets that have been opened */
for (i = 0; i < nunix; i++)
- if (fd_unix[i] != -1)
- (void)unlink(path_unix[i]);
- if (path_ctlsock != NULL && fd_ctlsock != -1)
+ (void)unlink(path_unix[i]);
+ if (path_ctlsock != NULL)
(void)unlink(path_ctlsock);
if (restart) {
- int r;
+ int status;
- wait(&r);
+ waitpid(child_pid, &status, 0);
execvp(argv[0], argv);
+ err(1, "exec restart '%s' failed", argv[0]);
}
unlink(_PATH_LOGPID);
- _exit(0);
+ exit(0);
}
static int
Index: usr.sbin/syslogd/syslogd.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
retrieving revision 1.215
diff -u -p -u -p -r1.215 syslogd.c
--- usr.sbin/syslogd/syslogd.c 23 Sep 2016 15:46:39 -0000 1.215
+++ usr.sbin/syslogd/syslogd.c 29 Sep 2016 16:25:28 -0000
@@ -208,6 +208,7 @@ int Initialized = 0; /* set when we have
int MarkInterval = 20 * 60; /* interval between marks in seconds */
int MarkSeq = 0; /* mark sequence number */
+int PrivChild = 0; /* Exec the privileged parent process */
int SecureMode = 1; /* when true, speak only unix domain socks */
int NoDNS = 0; /* when true, will refrain from doing DNS
lookups */
int IncludeHostname = 0; /* include RFC 3164 style hostnames when
forwarding */
@@ -358,7 +359,7 @@ main(int argc, char *argv[])
int ch, i;
int lockpipe[2] = { -1, -1}, pair[2], nullfd, fd;
- while ((ch = getopt(argc, argv, "46a:C:c:dFf:hK:k:m:np:S:s:T:U:uV"))
+ while ((ch = getopt(argc, argv, "46a:C:c:dFf:hK:k:m:nP:p:S:s:T:U:uV"))
!= -1)
switch (ch) {
case '4': /* disable IPv6 */
@@ -405,6 +406,11 @@ main(int argc, char *argv[])
case 'n': /* don't do DNS lookups */
NoDNS = 1;
break;
+ case 'P': /* used internally, exec the parent */
+ PrivChild = strtonum(optarg, 2, INT_MAX, &errstr);
+ if (errstr)
+ errx(1, "priv child %s: %s", errstr, optarg);
+ break;
case 'p': /* path */
path_unix[0] = optarg;
break;
@@ -441,7 +447,7 @@ main(int argc, char *argv[])
default:
usage();
}
- if ((argc -= optind) != 0)
+ if (argc != optind)
usage();
if (Debug)
@@ -459,6 +465,9 @@ main(int argc, char *argv[])
}
}
+ if (PrivChild > 1)
+ priv_exec(ConfFile, NoDNS, PrivChild, argc, argv);
+
consfile.f_type = F_CONSOLE;
(void)strlcpy(consfile.f_un.f_fname, ctty,
sizeof(consfile.f_un.f_fname));
@@ -691,8 +700,7 @@ main(int argc, char *argv[])
}
/* Privilege separation begins here */
- if (priv_init(ConfFile, NoDNS, lockpipe[1], nullfd, argv) < 0)
- errx(1, "unable to privsep");
+ priv_init(lockpipe[1], nullfd, argc, argv);
if (pledge("stdio unix inet recvfd", NULL) == -1)
err(1, "pledge");
Index: usr.sbin/syslogd/syslogd.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.h,v
retrieving revision 1.23
diff -u -p -u -p -r1.23 syslogd.h
--- usr.sbin/syslogd/syslogd.h 23 Oct 2015 16:28:52 -0000 1.23
+++ usr.sbin/syslogd/syslogd.h 29 Sep 2016 16:25:58 -0000
@@ -21,7 +21,8 @@
#include <sys/uio.h>
/* Privilege separation */
-int priv_init(char *, int, int, int, char **);
+void priv_init(int, int, int, char **);
+__dead void priv_exec(char *, int, int, int, char **);
int priv_open_tty(const char *);
int priv_open_log(const char *);
FILE *priv_open_utmp(void);
