[openssh-commits] [openssh] 02/02: Move SSHD_ACQUIRES_CTTY workaround into compat.

[git+noreply at mindrot.org]

This is an automated email from the git hooks/post-receive script.

dtucker pushed a commit to branch master
in repository openssh.

commit b30698662b862f5397116d23688aac0764e0886e
Author: Darren Tucker <dtucker at dtucker.net>
Date:   Fri Feb 11 21:00:35 2022 +1100

    Move SSHD_ACQUIRES_CTTY workaround into compat.
    
    On some (most? all?) SysV based systems with STREAMS based ptys,
    sshd could acquire a controlling terminal during pty setup when
    it pushed the "ptem" module, due to what is probably a bug in
    the STREAMS driver that's old enough to vote.  Because it was the
    privileged sshd's controlling terminal, it was not available for
    the user's session, which ended up without one.  This is known to
    affect at least Solaris <=10, derivatives such as OpenIndiana and
    several other SysV systems.  See bz#245 for the backstory.
    
    In the we past worked around that by not calling setsid in the
    privileged sshd child, which meant it was not a session or process
    group leader.  This solved controlling terminal problem because sshd
    was not eligble to acquire one, but had other side effects such as
    not cleaning up helper subprocesses in the SIGALRM handler since it
    was not PG leader.  Recent cleanups in the signal handler uncovered
    this, resulting in the LoginGraceTime timer not cleaning up privsep
    unprivileged processes.
    
    This change moves the workaround into the STREAMS pty allocation code,
    by allocating a sacrificial pty to act as sshd's controlling terminal
    before allocating user ptys, so those are still available for users'
    sessions.
    
    On the down side:
     - this will waste a pty per ssh connection on affected platforms.
    
    On the up side:
     - it makes the process group behaviour consistent between platforms.
    
     - it puts the workaround nearest the code that actually causes the
       problem and competely out of the mainline code.
    
     - the workaround is only activated if you use the STREAMS code.  If,
       say, Solaris 11 has the bug but also a working openpty() it doesn't
       matter that we defined SSHD_ACQUIRES_CTTY.
    
     - the workaround is only activated when the fist pty is allocated,
       ie in the post-auth privsep monitor.  This means there's no risk
       of fd leaks to the unprivileged processes, and there's no effect on
       sessions that do not allocate a pty.
    
    Based on analysis and work by djm@, ok djm@
---
 openbsd-compat/bsd-openpty.c | 67 +++++++++++++++++++++++++++++++-------------
 sshd.c                       |  7 -----
 2 files changed, 47 insertions(+), 27 deletions(-)

diff --git a/openbsd-compat/bsd-openpty.c b/openbsd-compat/bsd-openpty.c
index 1ab41f42..078d666c 100644
--- a/openbsd-compat/bsd-openpty.c
+++ b/openbsd-compat/bsd-openpty.c
@@ -71,28 +71,11 @@
 #define O_NOCTTY 0
 #endif
 
-int
-openpty(int *amaster, int *aslave, char *name, struct termios *termp,
+#if defined(HAVE_DEV_PTMX) && !defined(HAVE__GETPTY)
+static int
+openpty_streams(int *amaster, int *aslave, char *name, struct termios *termp,
    struct winsize *winp)
 {
-#if defined(HAVE__GETPTY)
-	/*
-	 * _getpty(3) exists in SGI Irix 4.x, 5.x & 6.x -- it generates more
-	 * pty's automagically when needed
-	 */
-	char *slave;
-
-	if ((slave = _getpty(amaster, O_RDWR, 0622, 0)) == NULL)
-		return (-1);
-
-	/* Open the slave side. */
-	if ((*aslave = open(slave, O_RDWR | O_NOCTTY)) == -1) {
-		close(*amaster);
-		return (-1);
-	}
-	return (0);
-
-#elif defined(HAVE_DEV_PTMX)
 	/*
 	 * This code is used e.g. on Solaris 2.x.  (Note that Solaris 2.3
 	 * also has bsd-style ptys, but they simply do not work.)
@@ -141,9 +124,53 @@ openpty(int *amaster, int *aslave, char *name, struct termios *termp,
 # ifndef __hpux
 	ioctl(*aslave, I_PUSH, "ttcompat");
 # endif /* __hpux */
+	return (0);
+}
+#endif
+
+int
+openpty(int *amaster, int *aslave, char *name, struct termios *termp,
+   struct winsize *winp)
+{
+#if defined(HAVE__GETPTY)
+	/*
+	 * _getpty(3) exists in SGI Irix 4.x, 5.x & 6.x -- it generates more
+	 * pty's automagically when needed
+	 */
+	char *slave;
+
+	if ((slave = _getpty(amaster, O_RDWR, 0622, 0)) == NULL)
+		return (-1);
 
+	/* Open the slave side. */
+	if ((*aslave = open(slave, O_RDWR | O_NOCTTY)) == -1) {
+		close(*amaster);
+		return (-1);
+	}
 	return (0);
 
+#elif defined(HAVE_DEV_PTMX)
+
+#ifdef SSHD_ACQUIRES_CTTY
+	/*
+	 * On some (most? all?) SysV based systems with STREAMS based terminals,
+	 * sshd will acquire a controlling terminal when it pushes the "ptem"
+	 * module.  On such platforms, first allocate a sacrificial pty so
+	 * that sshd already has a controlling terminal before allocating the
+	 * one that will be passed back to the user process.  This ensures
+	 * the second pty is not already the controlling terminal for a
+	 * different session and is available to become controlling terminal
+	 * for the client's subprocess.  See bugzilla #245 for details.
+	 */
+	static int junk_ptyfd = -1, junk_ttyfd;
+
+	if (junk_ptyfd == -1)
+		(void)openpty_streams(&junk_ptyfd, &junk_ttyfd, NULL, NULL,
+		    NULL);
+#endif
+
+	return openpty_streams(amaster, aslave, name, termp, winp);
+
 #elif defined(HAVE_DEV_PTS_AND_PTC)
 	/* AIX-style pty code. */
 	const char *ttname;
diff --git a/sshd.c b/sshd.c
index 53526b59..ef18ba46 100644
--- a/sshd.c
+++ b/sshd.c
@@ -2080,15 +2080,8 @@ main(int ac, char **av)
 	 * setlogin() affects the entire process group.  We don't
 	 * want the child to be able to affect the parent.
 	 */
-#if !defined(SSHD_ACQUIRES_CTTY)
-	/*
-	 * If setsid is called, on some platforms sshd will later acquire a
-	 * controlling terminal which will result in "could not set
-	 * controlling tty" errors.
-	 */
 	if (!debug_flag && !inetd_flag && setsid() == -1)
 		error("setsid: %.100s", strerror(errno));
-#endif
 
 	if (rexec_flag) {
 		debug("rexec start in %d out %d newsock %d pipe %d sock %d",

-- 
To stop receiving notification emails like this one, please contact
djm at mindrot.org.