Re: syslogd block signals

[Jeremie Courreges-Anglas]

Alexander Bluhm  writes:

> On Mon, Dec 26, 2016 at 04:54:54PM -0700, Theo de Raadt wrote:
>> I think programs should only block the absolutely critical things, and this
>> is overreach.
>
> Yes, blocking SIGINT and SIGQUIT is not clever.  I thought there
> were races with SIGCHLD and SIGTERM where only one process would
> survive.  But races don't exist as parent and child communicate
> over the socketpair and terminate if the other one dies.
>
> I have seen problems with SIGHUP during debugging, instead of
> restarting syslogd died.  I think this could also happen during
> normal operation.
>
> - edit syslog.conf
> - send SIGHUP
> - syslogd execs itself
> - newsyslog rotates logfile and sends SIGHUP
> - syslogd dies as signal handlers are not installed yet
>
> So I suggest to block SIGHUP during startup.

ok jca@

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: syslogd block signals

[Theo de Raadt]

Yes, that's much better.  It solves the problem, without trying to
solve other problems which don't exist.

> On Mon, Dec 26, 2016 at 04:54:54PM -0700, Theo de Raadt wrote:
> > I think programs should only block the absolutely critical things, and this
> > is overreach.
> 
> Yes, blocking SIGINT and SIGQUIT is not clever.  I thought there
> were races with SIGCHLD and SIGTERM where only one process would
> survive.  But races don't exist as parent and child communicate
> over the socketpair and terminate if the other one dies.
> 
> I have seen problems with SIGHUP during debugging, instead of
> restarting syslogd died.  I think this could also happen during
> normal operation.
> 
> - edit syslog.conf
> - send SIGHUP
> - syslogd execs itself
> - newsyslog rotates logfile and sends SIGHUP
> - syslogd dies as signal handlers are not installed yet
> 
> So I suggest to block SIGHUP during startup.
> 
> bluhm
> 
> Index: usr.sbin/syslogd/privsep.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/privsep.c,v
> retrieving revision 1.65
> diff -u -p -r1.65 privsep.c
> --- usr.sbin/syslogd/privsep.c27 Dec 2016 19:16:24 -  1.65
> +++ usr.sbin/syslogd/privsep.c30 Dec 2016 18:24:00 -
> @@ -175,6 +175,7 @@ priv_exec(char *conf, int numeric, int c
>   struct stat cf_info, cf_stat;
>   struct addrinfo hints, *res0;
>   struct sigaction sa;
> + sigset_t sigmask;
>  
>   if (pledge("stdio rpath wpath cpath dns getpw sendfd id proc exec",
>   NULL) == -1)
> @@ -209,6 +210,10 @@ priv_exec(char *conf, int numeric, int c
>   setproctitle("[priv]");
>   logdebug("[priv]: fork+exec done\n");
>  
> + sigemptyset();
> + if (sigprocmask(SIG_SETMASK, , NULL) == -1)
> + err(1, "sigprocmask priv");
> +
>   if (stat(conf, _info) < 0)
>   err(1, "stat config file failed");
>  
> @@ -409,6 +414,10 @@ priv_exec(char *conf, int numeric, int c
>   int status;
>  
>   waitpid(child_pid, , 0);
> + sigemptyset();
> + sigaddset(, SIGHUP);
> + if (sigprocmask(SIG_SETMASK, , NULL) == -1)
> + err(1, "sigprocmask exec");
>   execvp(argv[0], argv);
>   err(1, "exec restart '%s' failed", argv[0]);
>   }
> Index: usr.sbin/syslogd/syslogd.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
> retrieving revision 1.225
> diff -u -p -r1.225 syslogd.c
> --- usr.sbin/syslogd/syslogd.c27 Dec 2016 19:16:24 -  1.225
> +++ usr.sbin/syslogd/syslogd.c30 Dec 2016 17:55:37 -
> @@ -354,6 +354,7 @@ main(int argc, char *argv[])
>   struct event*ev_klog, *ev_sendsys, *ev_udp, *ev_udp6,
>   *ev_bind, *ev_listen, *ev_tls, *ev_unix,
>   *ev_hup, *ev_int, *ev_quit, *ev_term, *ev_mark;
> + sigset_t sigmask;
>   const char  *errstr;

Re: syslogd block signals

[Alexander Bluhm]

On Mon, Dec 26, 2016 at 04:54:54PM -0700, Theo de Raadt wrote:
> I think programs should only block the absolutely critical things, and this
> is overreach.

Yes, blocking SIGINT and SIGQUIT is not clever.  I thought there
were races with SIGCHLD and SIGTERM where only one process would
survive.  But races don't exist as parent and child communicate
over the socketpair and terminate if the other one dies.

I have seen problems with SIGHUP during debugging, instead of
restarting syslogd died.  I think this could also happen during
normal operation.

- edit syslog.conf
- send SIGHUP
- syslogd execs itself
- newsyslog rotates logfile and sends SIGHUP
- syslogd dies as signal handlers are not installed yet

So I suggest to block SIGHUP during startup.

bluhm

Index: usr.sbin/syslogd/privsep.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/privsep.c,v
retrieving revision 1.65
diff -u -p -r1.65 privsep.c
--- usr.sbin/syslogd/privsep.c  27 Dec 2016 19:16:24 -  1.65
+++ usr.sbin/syslogd/privsep.c  30 Dec 2016 18:24:00 -
@@ -175,6 +175,7 @@ priv_exec(char *conf, int numeric, int c
struct stat cf_info, cf_stat;
struct addrinfo hints, *res0;
struct sigaction sa;
+   sigset_t sigmask;
 
if (pledge("stdio rpath wpath cpath dns getpw sendfd id proc exec",
NULL) == -1)
@@ -209,6 +210,10 @@ priv_exec(char *conf, int numeric, int c
setproctitle("[priv]");
logdebug("[priv]: fork+exec done\n");
 
+   sigemptyset();
+   if (sigprocmask(SIG_SETMASK, , NULL) == -1)
+   err(1, "sigprocmask priv");
+
if (stat(conf, _info) < 0)
err(1, "stat config file failed");
 
@@ -409,6 +414,10 @@ priv_exec(char *conf, int numeric, int c
int status;
 
waitpid(child_pid, , 0);
+   sigemptyset();
+   sigaddset(, SIGHUP);
+   if (sigprocmask(SIG_SETMASK, , NULL) == -1)
+   err(1, "sigprocmask exec");
execvp(argv[0], argv);
err(1, "exec restart '%s' failed", argv[0]);
}
Index: usr.sbin/syslogd/syslogd.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
retrieving revision 1.225
diff -u -p -r1.225 syslogd.c
--- usr.sbin/syslogd/syslogd.c  27 Dec 2016 19:16:24 -  1.225
+++ usr.sbin/syslogd/syslogd.c  30 Dec 2016 17:55:37 -
@@ -354,6 +354,7 @@ main(int argc, char *argv[])
struct event*ev_klog, *ev_sendsys, *ev_udp, *ev_udp6,
*ev_bind, *ev_listen, *ev_tls, *ev_unix,
*ev_hup, *ev_int, *ev_quit, *ev_term, *ev_mark;
+   sigset_t sigmask;
const char  *errstr;
char*p;
int  ch, i;
@@ -361,6 +362,12 @@ main(int argc, char *argv[])
int  fd_ctlsock, fd_klog, fd_sendsys, fd_bind, fd_listen;
int *fd_unix;
 
+   /* block signal until handler is set up */
+   sigemptyset();
+   sigaddset(, SIGHUP);
+   if (sigprocmask(SIG_SETMASK, , NULL) == -1)
+   err(1, "sigprocmask block");
+
if ((path_unix = malloc(sizeof(*path_unix))) == NULL)
err(1, "malloc %s", _PATH_LOG);
path_unix[0] = _PATH_LOG;
@@ -839,6 +846,10 @@ main(int argc, char *argv[])
 
logmsg(LOG_SYSLOG|LOG_INFO, "syslogd: start", LocalHostName, ADDDATE);
logdebug("syslogd: started\n");
+
+   sigemptyset();
+   if (sigprocmask(SIG_SETMASK, , NULL) == -1)
+   err(1, "sigprocmask unblock");
 
event_dispatch();
/* NOTREACHED */