[PATCH] improved chroot handling

[Tony Finch]

There are a couple of niggles with the sandboxing of the unprivileged
child in the privsep code: the empty directory causes namespace pollution,
and it requires care to ensure that it is set up properly and remains set
up properly. The patch below (against the portable OpenSSH, although the
patch against the OpenBSD version is very similar) replaces the fixed
empty directory with one that is created on demand and is immediately
removed after the child process has chdir()ed and chroot()ed into it.
This ensures that the directory is in a known-safe state and that no-one
(not even root) can mess it up.

Index: pathnames.h
===================================================================
RCS file: /home/ncvs/src/crypto/openssh-portable/pathnames.h,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 pathnames.h
--- pathnames.h	24 Jun 2002 22:46:13 -0000	1.1.1.1
+++ pathnames.h	26 Jun 2002 17:58:59 -0000
@@ -145,11 +145,6 @@
 #define _PATH_SFTP_SERVER		"/usr/libexec/sftp-server"
 #endif
 
-/* chroot directory for unprivileged user when UsePrivilegeSeparation=yes */
-#ifndef _PATH_PRIVSEP_CHROOT_DIR
-#define _PATH_PRIVSEP_CHROOT_DIR	"/var/empty"
-#endif
-
 #ifndef _PATH_LS
 #define _PATH_LS			"ls"
 #endif
Index: sshd.c
===================================================================
RCS file: /home/ncvs/src/crypto/openssh-portable/sshd.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 sshd.c
--- sshd.c	24 Jun 2002 22:46:20 -0000	1.1.1.1
+++ sshd.c	26 Jun 2002 18:00:25 -0000
@@ -545,14 +545,9 @@
 	memset(pw->pw_passwd, 0, strlen(pw->pw_passwd));
 	endpwent();
 
-	/* Change our root directory*/
-	if (chroot(_PATH_PRIVSEP_CHROOT_DIR) == -1)
-		fatal("chroot(\"%s\"): %s", _PATH_PRIVSEP_CHROOT_DIR,
-		    strerror(errno));
-	if (chdir("/") == -1)
-		fatal("chdir(\"/\"): %s", strerror(errno));
-
-	/* Drop our privileges */
+	/* Change our root directory and drop privileges */
+	if (chroot(".") < 0)
+		fatal("chroot(): %s\n", strerror(errno));
 	debug3("privsep user:group %u:%u", (u_int)pw->pw_uid,
 	    (u_int)pw->pw_gid);
 	do_setusercontext(pw);
@@ -561,6 +556,7 @@
 static Authctxt*
 privsep_preauth(void)
 {
+	char emptydir[] = "/var/tmp/sshd.XXXXXXXXXX";
 	Authctxt *authctxt = NULL;
 	int status;
 	pid_t pid;
@@ -570,12 +566,31 @@
 	/* Store a pointer to the kex for later rekeying */
 	pmonitor->m_pkex = &xxx_kex;
 
+	/*
+	 * We create a safe environment for the child by creating an empty
+	 * directory into which the child chroots, and the parent prevents
+	 * others from fooling around with it by removing the directory. We do
+	 * it this way because the child can't remove its own current working
+	 * directory (except on some systems by giving an absolute path to
+	 * rmdir, but it is highly dependent on the OS and filesystem). We
+	 * create the directory in /var/tmp in order that we are more likely
+	 * to get a well-behaved disk filesystem.
+	 */
+	if (mkdtemp(emptydir) == NULL)
+		fatal("mkdtemp(\"%s\"): %s", emptydir, strerror(errno));
+
 	pid = fork();
 	if (pid == -1) {
 		fatal("fork of unprivileged child failed");
 	} else if (pid != 0) {
 		debug2("Network child is on pid %ld", (long)pid);
 
+		/* Wait for the child to chdir then remove the directory */
+		if (read(pmonitor->m_recvfd, &status, 1) < 0)
+			fatal("read(): %s", strerror(errno));
+		if (rmdir(emptydir) < 0)
+			fatal("rmdir(\"%s\"): %s", emptydir, strerror(errno));
+
 		close(pmonitor->m_recvfd);
 		authctxt = monitor_child_preauth(pmonitor);
 		close(pmonitor->m_sendfd);
@@ -591,6 +606,10 @@
 	} else {
 		/* child */
 
+		if (chdir(emptydir) == -1)
+			fatal("chdir(\"%s\"): %s", emptydir, strerror(errno));
+		if (write(pmonitor->m_sendfd, &status, 1) < 0)
+			fatal("write(): %s", strerror(errno));
 		close(pmonitor->m_sendfd);
 
 		/* Demote the child */
@@ -1008,10 +1027,6 @@
 		if ((pw = getpwnam(SSH_PRIVSEP_USER)) == NULL)
 			fatal("Privilege separation user %s does not exist",
 			    SSH_PRIVSEP_USER);
-		if ((stat(_PATH_PRIVSEP_CHROOT_DIR, &st) == -1) ||
-		    (S_ISDIR(st.st_mode) == 0))
-			fatal("Missing privilege separation directory: %s",
-			    _PATH_PRIVSEP_CHROOT_DIR);
 	}
 
 	/* Configuration looks good, so exit if in test mode. */


Tony.
-- 
f.a.n.finch <dot at dotat.at> http://dotat.at/
FISHER GERMAN BIGHT: WESTERLY VEERING NORTHWESTERLY 4 OR 5, OCCASIONALLY 6.
SHOWERS. MODERATE OR GOOD.