[PATCH] improved chroot handling

[Luc I. Suryo]

Tony,

it is maybe me but the code:

	char emptydir[] = "/var/tmp/sshd.XXXXXXXXXX";

is hard coded...and we want to use what is defined by

	_PATH_PRIVSEP_CHROOT_DIR 

yes? 

and should not one make sure that there is no overflow in
emptydir??? malloc/free/strlen and that kinda of stuff 


just me quick 25c :)
 

> 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.
> _______________________________________________
> openssh-unix-dev at mindrot.org mailing list
> http://www.mindrot.org/mailman/listinfo/openssh-unix-dev

--- End of dot at dotat.at's quote ---

-- 
Kind regards,
Luc Suryo