[master] 0311c78 Replace alien FD's with /dev/null rather than just closing them

[Poul-Henning Kamp]

commit 0311c78353cbacbec73a155807d3777749dd0802
Author: Poul-Henning Kamp <phk at FreeBSD.org>
Date:   Tue Feb 2 11:51:14 2016 +0000

    Replace alien FD's with /dev/null rather than just closing them
    
    When we fork the worker process, we close all filedescriptors we
    have not explictly marked for it to inherit, for security reasons.
    
    Operating system libraries may have open filedescriptors (see
    end*ent(3)) and there is no way to chase these down.
    
    At least on OSX something related to DNS lookups leaves such
    a FD around, and when that code later discovers the FD doesn't
    work, it closes it, even though it no longer owns it.
    
    In ticket 1841, that happens to be FD7 which is one of our kqueue FDs.
    
    Normally such library code should set 'close-on-exec' status with
    fcntl(2) but that doesn't seem to be the case here, and this bit
    of wisdom seems neglegted about 50/50, so it probably wouldn't
    help us to examine this.
    
    The fix here is to close the FDs, and replace them with a FD open
    to /dev/null, so that there is no risk of information leak, but we
    don't reuse the FD for something else until the library has properly
    closed it.
    
    Fixes #1841

diff --git a/bin/varnishd/mgt/mgt_child.c b/bin/varnishd/mgt/mgt_child.c
index ae01a91..8cedeaf 100644
--- a/bin/varnishd/mgt/mgt_child.c
+++ b/bin/varnishd/mgt/mgt_child.c
@@ -295,7 +295,7 @@ mgt_launch_child(struct cli *cli)
 	unsigned u;
 	char *p;
 	struct vev *e;
-	int i, cp[2];
+	int i, j, k, cp[2];
 	struct sigaction sa;
 
 	if (child_state != CH_STOPPED && child_state != CH_DIED)
@@ -351,13 +351,29 @@ mgt_launch_child(struct cli *cli)
 		assert(dup2(heritage.std_fd, STDOUT_FILENO) == STDOUT_FILENO);
 		assert(dup2(heritage.std_fd, STDERR_FILENO) == STDERR_FILENO);
 
-		/* Close anything we shouldn't know about */
+		/*
+		 * Close all FDs the child shouldn't know about
+		 *
+		 * We cannot just close these filedescriptors, some random
+		 * library routine might miss it later on and wantonly close
+		 * a FD we use at that point in time. (See bug #1841).
+		 * We close the FD and replace it with /dev/null instead,
+		 * That prevents security leakage, and gives the library
+		 * code a valid FD to close when it discovers the changed
+		 * circumstances.
+		 */
 		closelog();
 
 		for (i = STDERR_FILENO + 1; i < CLOSE_FD_UP_TO; i++) {
 			if (vbit_test(fd_map, i))
 				continue;
-			(void)(close(i) == 0);
+			if (close(i) == 0) {
+				k = open("/dev/null", O_RDONLY);
+				assert(k >= 0);
+				j = dup2(k, i);
+				assert(j == i);
+				AZ(close(k));
+			}
 		}
 #ifdef HAVE_SETPROCTITLE
 		setproctitle("Varnish-Chld %s", heritage.name);