[PATCH] improved chroot handling

[Ben Lindstrom]

On Wed, 26 Jun 2002, Tony Finch wrote:

> 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));

No chdir("/").. Bad form.

Trusting where the current path is without an explicist chdir() before it
is also bad form.


>  	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";

Hard coded directories that one has to sprawl through to find.  Also
in very bad taste.

Not very thrilled about the implementation.

- Ben