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

[Wouter Verhelst <w@...112...>]

Hi Tuomas,

Thanks, applied.

On Thu, May 07, 2015 at 10:16:24AM +0300, Tuomas Räsänen wrote:
> 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
> 
> 
> ------------------------------------------------------------------------------
> One dashboard for servers and applications across Physical-Virtual-Cloud 
> Widest out-of-the-box monitoring support with 50+ applications
> Performance metrics, stats and reports that give you Actionable Insights
> Deep dive visibility with transaction tracing using APM Insight.
> http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
> _______________________________________________
> Nbd-general mailing list
> Nbd-general@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/nbd-general
> 

-- 
It is easy to love a country that is famous for chocolate and beer

  -- Barack Obama, speaking in Brussels, Belgium, 2014-03-26