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.
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
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 stuffI 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.
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
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.
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
- [PATCH] prevent users from changing their environment
- [nut-commits] svn commit r2940 - in branches/windows_port/scripts/Windows/Installer
- preauth privsep logging via monitor
- [Bug 2167] New: Connection remains when fork() fails.
- HELP: need better understanding of "--delete" flag