thr3ads.net - openssh unix dev - [PATCH] improved chroot handling [Jun 2002]

If this information is useful, please help other people find it:
Share via:

Tony Finch

2002-Jun-26 22:18 UTC

[PATCH] improved chroot handling

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.

Luc I. Suryo

2002-Jun-26 23:17 UTC

head link

[PATCH] improved chroot handling

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

Tony Finch

2002-Jun-26 23:23 UTC

head link

[PATCH] improved chroot handling

On Wed, Jun 26, 2002 at 06:17:47PM -0500, Luc I. Suryo
wrote:> 
> 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? 
Why? The point is to make _PATH_PRIVSEP_CHROOT_DIR unnecessary.
> and should not one make sure that there is no overflow in
> emptydir??? malloc/free/strlen and that kinda of stuff 
I suggest you have a look at the manual page for mkdtemp().

Tony.
-- 
f.a.n.finch <dot at dotat.at> http://dotat.at/
NORTH UTSIRE SOUTH UTSIRE: WESTERLY VEERING NORTHWESTERLY 4 OR 5, OCCASIONALLY
6 LATER. SHOWERS. GOOD.

Ben Lindstrom

2002-Jun-26 23:26 UTC

head link

[PATCH] improved chroot handling

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

Tony Finch

2002-Jun-26 23:54 UTC

head link

[PATCH] improved chroot handling

On Wed, Jun 26, 2002 at 06:26:42PM -0500, Ben Lindstrom
wrote:> 
> No chdir("/").. Bad form.
> Trusting where the current path is without an explicist chdir() before it
> is also bad form.
That function is only called from one place shortly after the chdir into
the secure directory. The unusual ordering is to avoid passing around lots
of extraneous information.
> Hard coded directories that one has to sprawl through to find.  Also
> in very bad taste.
I have an updated patch that fixes that, if anyone is interested.

Thanks for your comments.

Tony.
-- 
f.a.n.finch <dot at dotat.at> http://dotat.at/
NORTH UTSIRE SOUTH UTSIRE: WESTERLY VEERING NORTHWESTERLY 4 OR 5, OCCASIONALLY
6 LATER. SHOWERS. GOOD.

Thomas Binder

2002-Jun-27 09:36 UTC

head link

Fixed paths (Was: [PATCH] improved chroot handling)

Hi!

On Wed, Jun 26, 2002 at 06:26:42PM -0500, Ben Lindstrom
wrote:> > +	char emptydir[] = "/var/tmp/sshd.XXXXXXXXXX";
> 
> Hard coded directories that one has to sprawl through to find.
> Also in very bad taste.
That reminds me: The path to the authentication socket is also
hardcoded in session.c and ssh-agent.c


Ciao

Thomas

Apparently Analagous Threads

Search for more possibly parallel threads

openssh unix dev - Jun 2002 - [PATCH] improved chroot handling

[PATCH] improved chroot handling

[PATCH] improved chroot handling

[PATCH] improved chroot handling

[PATCH] improved chroot handling

[PATCH] improved chroot handling

Fixed paths (Was: [PATCH] improved chroot handling)

Apparently Analagous Threads