[Nbd] [PATCH] nbd-server: fix unsafe signal handling

[Tuomas Räsänen <tuomasjjrasanen@...1261...>]

Previously, signal handlers themselves were entered only once, but they called
posixly unsafe, non-reentrant, functions, such as syslog(). If a signal was
caught in the middle of the execution of such function, consequences were
undefined. In practice, nbd-server was observed to deadlock during the execution
of sigchld_handler() with following kind of system call trace:

  sendto(7, "<31>Mar 30 15:26:26 nbd_server[1"..., 59, MSG_NOSIGNAL, NULL, 0) = 59
  --- SIGCHLD (Child exited) @ 0 (0) ---
  wait4(-1, [{WIFEXITED(s) && WEXITSTATUS(s) == 0}], WNOHANG, NULL) = 14367
  futex(0x7f8881903734, FUTEX_WAIT_PRIVATE, 2, NULL

Unsafe signal handlers made nbd-server vulnerable to denial-of-service
attacks. The vulnerability is identified as

  CVE-2015-0847 nbd-server denial of service due to unsafe signal handlers

This patch fixes the problem by moving all signal handling logic away from the
signal handler context to the main loop. Signal handlers just set atomic flags
whenever a signal is received and hence are safe to call at any point.

Furthermore, select() is replaced with pselect() to avoid race conditions with
signal flag tests.

Signed-off-by: Tuomas Räsänen <tuomasjjrasanen@...1261...>
---
 nbd-server.c | 108 +++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 82 insertions(+), 26 deletions(-)

diff --git a/nbd-server.c b/nbd-server.c
index 3075a0e..e036a52 100644
--- a/nbd-server.c
+++ b/nbd-server.c
@@ -168,6 +168,16 @@ char default_authname[] = SYSCONFDIR "/nbd-server/allow"; /**< default name of a
 
 #include <nbdsrv.h>
 
+static volatile sig_atomic_t is_sigchld_caught; /**< Flag set by
+						     SIGCHLD handler
+						     to mark a child
+						     exit */
+
+static volatile sig_atomic_t is_sigterm_caught; /**< Flag set by
+						     SIGTERM handler
+						     to mark a exit
+						     request */
+
 static volatile sig_atomic_t is_sighup_caught; /**< Flag set by SIGHUP
                                                     handler to mark a
                                                     reconfiguration
@@ -930,27 +940,16 @@ GArray* parse_cfile(gchar* f, struct generic_conf *const genconf, bool expect_ge
 }
 
 /**
- * Signal handler for SIGCHLD
+ * Handle SIGCHLD by setting atomically a flag which will be evaluated in the
+ * main loop of the root server process. This allows us to separate the signal
+ * catching from th actual task triggered by SIGCHLD and hence processing in the
+ * interrupt context is kept as minimial as possible.
+ *
  * @param s the signal we're handling (must be SIGCHLD, or something
  * is severely wrong)
  **/
-void sigchld_handler(int s) {
-        int status;
-	int* i;
-	pid_t pid;
-
-	while((pid=waitpid(-1, &status, WNOHANG)) > 0) {
-		if(WIFEXITED(status)) {
-			msg(LOG_INFO, "Child exited with %d", WEXITSTATUS(status));
-		}
-		i=g_hash_table_lookup(children, &pid);
-		if(!i) {
-			msg(LOG_INFO, "SIGCHLD received for an unknown child with PID %ld", (long)pid);
-		} else {
-			DEBUG("Removing %d from the list of children", pid);
-			g_hash_table_remove(children, &pid);
-		}
-	}
+static void sigchld_handler(const int s G_GNUC_UNUSED) {
+        is_sigchld_caught = 1;
 }
 
 /**
@@ -968,15 +967,16 @@ void killchild(gpointer key, gpointer value, gpointer user_data) {
 }
 
 /**
- * Handle SIGTERM and dispatch it to our children
+ * Handle SIGTERM by setting atomically a flag which will be evaluated in the
+ * main loop of the root server process. This allows us to separate the signal
+ * catching from th actual task triggered by SIGTERM and hence processing in the
+ * interrupt context is kept as minimial as possible.
+ *
  * @param s the signal we're handling (must be SIGTERM, or something
  * is severely wrong).
  **/
-void sigterm_handler(int s) {
-	g_hash_table_foreach(children, killchild, NULL);
-	unlink(pidfname);
-
-	exit(EXIT_SUCCESS);
+static void sigterm_handler(const int s G_GNUC_UNUSED) {
+	is_sigterm_caught = 1;
 }
 
 /**
@@ -2065,9 +2065,12 @@ spawn_child()
                 goto out;
         }
         /* Child */
+
+        /* Child's signal disposition is reset to default. */
         signal(SIGCHLD, SIG_DFL);
         signal(SIGTERM, SIG_DFL);
         signal(SIGHUP, SIG_DFL);
+        sigemptyset(&oldset);
 out:
         sigprocmask(SIG_SETMASK, &oldset, NULL);
         return pid;
@@ -2261,9 +2264,12 @@ handle_oldstyle_connection(GArray *const servers, SERVER *const serve)
 			goto handle_connection_out;
 		}
 		/* child */
+
+		/* Child's signal disposition is reset to default. */
 		signal(SIGCHLD, SIG_DFL);
 		signal(SIGTERM, SIG_DFL);
 		signal(SIGHUP, SIG_DFL);
+		sigemptyset(&oldset);
 		sigprocmask(SIG_SETMASK, &oldset, NULL);
 
 		g_hash_table_destroy(children);
@@ -2367,6 +2373,8 @@ void serveloop(GArray* servers) {
 	int max;
 	fd_set mset;
 	fd_set rset;
+	sigset_t blocking_mask;
+	sigset_t original_mask;
 
 	/* 
 	 * Set up the master fd_set. The set of descriptors we need
@@ -2389,7 +2397,56 @@ void serveloop(GArray* servers) {
 		FD_SET(sock, &mset);
 		max=sock>max?sock:max;
 	}
+
+	/* Construct a signal mask which is used to make signal testing and
+	 * receiving an atomic operation to ensure no signal is received between
+	 * tests and blocking pselect(). */
+	if (sigemptyset(&blocking_mask) == -1)
+		err("failed to initialize blocking_mask: %m");
+
+	if (sigaddset(&blocking_mask, SIGCHLD) == -1)
+		err("failed to add SIGCHLD to blocking_mask: %m");
+
+	if (sigaddset(&blocking_mask, SIGHUP) == -1)
+		err("failed to add SIGHUP to blocking_mask: %m");
+
+	if (sigaddset(&blocking_mask, SIGTERM) == -1)
+		err("failed to add SIGTERM to blocking_mask: %m");
+
+	if (sigprocmask(SIG_BLOCK, &blocking_mask, &original_mask) == -1)
+	    err("failed to block signals: %m");
+
 	for(;;) {
+		if (is_sigterm_caught) {
+			is_sigterm_caught = 0;
+
+			g_hash_table_foreach(children, killchild, NULL);
+			unlink(pidfname);
+
+			exit(EXIT_SUCCESS);
+		}
+
+		if (is_sigchld_caught) {
+			int status;
+			int* i;
+			pid_t pid;
+
+			is_sigchld_caught = 0;
+
+			while ((pid=waitpid(-1, &status, WNOHANG)) > 0) {
+				if (WIFEXITED(status)) {
+					msg(LOG_INFO, "Child exited with %d", WEXITSTATUS(status));
+				}
+				i = g_hash_table_lookup(children, &pid);
+				if (!i) {
+					msg(LOG_INFO, "SIGCHLD received for an unknown child with PID %ld", (long)pid);
+				} else {
+					DEBUG("Removing %d from the list of children", pid);
+					g_hash_table_remove(children, &pid);
+				}
+			}
+		}
+
                 /* SIGHUP causes the root server process to reconfigure
                  * itself and add new export servers for each newly
                  * found export configuration group, i.e. spawn new
@@ -2424,8 +2481,7 @@ void serveloop(GArray* servers) {
                 }
 
 		memcpy(&rset, &mset, sizeof(fd_set));
-		if(select(max+1, &rset, NULL, NULL, NULL)>0) {
-
+		if (pselect(max + 1, &rset, NULL, NULL, NULL, &original_mask) > 0) {
 			DEBUG("accept, ");
 			for(i=0; i < modernsocks->len; i++) {
 				int sock = g_array_index(modernsocks, int, i);
-- 
2.1.4