restore blocking status on stdio fds before close

ssh(1) needs to set file descriptors to non-blocking mode to operate
but it was not restoring the original state on exit. This could cause
problems with fds shared with other programs via the shell, e.g.

> $ cat > test.sh << _EOF
> #!/bin/sh
> {
>         ssh -Fnone -oLogLevel=verbose ::1 hostname
>         cat /usr/share/dict/words
> } | sleep 10
> _EOF
> $ ./test.sh
> Authenticated to ::1 ([::1]:22).
> Transferred: sent 2352, received 2928 bytes, in 0.1 seconds
> Bytes per second: sent 44338.9, received 55197.4
> cat: stdout: Resource temporarily unavailable

This restores the blocking status for fds 0,1,2 (stdio) before ssh(1)
abandons/closes them.

This was reported as bz3280 and GHPR246; ok dtucker@

diff --git a/usr.bin/ssh/channels.c b/usr.bin/ssh/channels.c
index ebeb829..b54d1b8 100644 (file)
--- a/usr.bin/ssh/channels.c
+++ b/usr.bin/ssh/channels.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: channels.c,v 1.406 2021/04/03 06:18:40 djm Exp $ */
+/* $OpenBSD: channels.c,v 1.407 2021/05/19 01:24:05 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -323,7 +323,27 @@ channel_register_fds(struct ssh *ssh, Channel *c, int rfd, int wfd, int efd,
                debug2("channel %d: rfd %d isatty", c->self, c->rfd);
 
        /* enable nonblocking mode */
-       if (nonblock) {
+       c->restore_block = 0;
+       if (nonblock == CHANNEL_NONBLOCK_STDIO) {
+               /*
+                * Special handling for stdio file descriptors: do not set
+                * non-blocking mode if they are TTYs. Otherwise prepare to
+                * restore their blocking state on exit to avoid interfering
+                * with other programs that follow.
+                */
+               if (rfd != -1 && !isatty(rfd) && fcntl(rfd, F_GETFL) == 0) {
+                       c->restore_block |= CHANNEL_RESTORE_RFD;
+                       set_nonblock(rfd);
+               }
+               if (wfd != -1 && !isatty(wfd) && fcntl(wfd, F_GETFL) == 0) {
+                       c->restore_block |= CHANNEL_RESTORE_WFD;
+                       set_nonblock(wfd);
+               }
+               if (efd != -1 && !isatty(efd) && fcntl(efd, F_GETFL) == 0) {
+                       c->restore_block |= CHANNEL_RESTORE_EFD;
+                       set_nonblock(efd);
+               }
+       } else if (nonblock) {
                if (rfd != -1)
                        set_nonblock(rfd);
                if (wfd != -1)
@@ -412,17 +432,23 @@ channel_find_maxfd(struct ssh_channels *sc)
 }
 
 int
-channel_close_fd(struct ssh *ssh, int *fdp)
+channel_close_fd(struct ssh *ssh, Channel *c, int *fdp)
 {
        struct ssh_channels *sc = ssh->chanctxt;
-       int ret = 0, fd = *fdp;
+       int ret, fd = *fdp;
 
-       if (fd != -1) {
-               ret = close(fd);
-               *fdp = -1;
-               if (fd == sc->channel_max_fd)
-                       channel_find_maxfd(sc);
-       }
+       if (fd == -1)
+               return 0;
+
+       if ((*fdp == c->rfd && (c->restore_block & CHANNEL_RESTORE_RFD) != 0) ||
+          (*fdp == c->wfd && (c->restore_block & CHANNEL_RESTORE_WFD) != 0) ||
+          (*fdp == c->efd && (c->restore_block & CHANNEL_RESTORE_EFD) != 0))
+               (void)fcntl(*fdp, F_SETFL, 0);  /* restore blocking */
+
+       ret = close(fd);
+       *fdp = -1;
+       if (fd == sc->channel_max_fd)
+               channel_find_maxfd(sc);
        return ret;
 }
 
@@ -432,13 +458,13 @@ channel_close_fds(struct ssh *ssh, Channel *c)
 {
        int sock = c->sock, rfd = c->rfd, wfd = c->wfd, efd = c->efd;
 
-       channel_close_fd(ssh, &c->sock);
+       channel_close_fd(ssh, c, &c->sock);
        if (rfd != sock)
-               channel_close_fd(ssh, &c->rfd);
+               channel_close_fd(ssh, c, &c->rfd);
        if (wfd != sock && wfd != rfd)
-               channel_close_fd(ssh, &c->wfd);
+               channel_close_fd(ssh, c, &c->wfd);
        if (efd != sock && efd != rfd && efd != wfd)
-               channel_close_fd(ssh, &c->efd);
+               channel_close_fd(ssh, c, &c->efd);
 }
 
 static void
@@ -692,7 +718,7 @@ channel_stop_listening(struct ssh *ssh)
                        case SSH_CHANNEL_X11_LISTENER:
                        case SSH_CHANNEL_UNIX_LISTENER:
                        case SSH_CHANNEL_RUNIX_LISTENER:
-                               channel_close_fd(ssh, &c->sock);
+                               channel_close_fd(ssh, c, &c->sock);
                                channel_free(ssh, c);
                                break;
                        }
@@ -1481,7 +1507,8 @@ channel_decode_socks5(Channel *c, struct sshbuf *input, struct sshbuf *output)
 
 Channel *
 channel_connect_stdio_fwd(struct ssh *ssh,
-    const char *host_to_connect, u_short port_to_connect, int in, int out)
+    const char *host_to_connect, u_short port_to_connect,
+    int in, int out, int nonblock)
 {
        Channel *c;
 
@@ -1489,7 +1516,7 @@ channel_connect_stdio_fwd(struct ssh *ssh,
 
        c = channel_new(ssh, "stdio-forward", SSH_CHANNEL_OPENING, in, out,
            -1, CHAN_TCP_WINDOW_DEFAULT, CHAN_TCP_PACKET_DEFAULT,
-           0, "stdio-forward", /*nonblock*/0);
+           0, "stdio-forward", nonblock);
 
        c->path = xstrdup(host_to_connect);
        c->host_port = port_to_connect;
@@ -1639,7 +1666,7 @@ channel_post_x11_listener(struct ssh *ssh, Channel *c,
        if (c->single_connection) {
                oerrno = errno;
                debug2("single_connection: closing X11 listener.");
-               channel_close_fd(ssh, &c->sock);
+               channel_close_fd(ssh, c, &c->sock);
                chan_mark_dead(ssh, c);
                errno = oerrno;
        }
@@ -2028,7 +2055,7 @@ channel_handle_efd_write(struct ssh *ssh, Channel *c,
                return 1;
        if (len <= 0) {
                debug2("channel %d: closing write-efd %d", c->self, c->efd);
-               channel_close_fd(ssh, &c->efd);
+               channel_close_fd(ssh, c, &c->efd);
        } else {
                if ((r = sshbuf_consume(c->extended, len)) != 0)
                        fatal_fr(r, "channel %i: consume", c->self);
@@ -2054,7 +2081,7 @@ channel_handle_efd_read(struct ssh *ssh, Channel *c,
                return 1;
        if (len <= 0) {
                debug2("channel %d: closing read-efd %d", c->self, c->efd);
-               channel_close_fd(ssh, &c->efd);
+               channel_close_fd(ssh, c, &c->efd);
        } else if (c->extended_usage == CHAN_EXTENDED_IGNORE)
                debug3("channel %d: discard efd", c->self);
        else if ((r = sshbuf_put(c->extended, buf, len)) != 0)

diff --git a/usr.bin/ssh/channels.h b/usr.bin/ssh/channels.h
index b318974..633adc3 100644 (file)
--- a/usr.bin/ssh/channels.h
+++ b/usr.bin/ssh/channels.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: channels.h,v 1.137 2021/04/03 06:18:40 djm Exp $ */
+/* $OpenBSD: channels.h,v 1.138 2021/05/19 01:24:05 djm Exp $ */
 
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
@@ -63,6 +63,16 @@
 
 #define CHANNEL_CANCEL_PORT_STATIC     -1
 
+/* nonblocking flags for channel_new */
+#define CHANNEL_NONBLOCK_LEAVE 0 /* don't modify non-blocking state */
+#define CHANNEL_NONBLOCK_SET   1 /* set non-blocking state */
+#define CHANNEL_NONBLOCK_STDIO 2 /* set non-blocking and restore on close */
+
+/* c->restore_block mask flags */
+#define CHANNEL_RESTORE_RFD    0x01
+#define CHANNEL_RESTORE_WFD    0x02
+#define CHANNEL_RESTORE_EFD    0x04
+
 /* TCP forwarding */
 #define FORWARD_DENY           0
 #define FORWARD_REMOTE         (1)
@@ -136,6 +146,7 @@ struct Channel {
                                 * to a matching pre-select handler.
                                 * this way post-select handlers are not
                                 * accidentally called if a FD gets reused */
+       int     restore_block;  /* fd mask to restore blocking status */
        struct sshbuf *input;   /* data read from socket, to be sent over
                                 * encrypted connection */
        struct sshbuf *output;  /* data received over encrypted connection for
@@ -263,7 +274,7 @@ void         channel_register_filter(struct ssh *, int, channel_infilter_fn *,
 void    channel_register_status_confirm(struct ssh *, int,
            channel_confirm_cb *, channel_confirm_abandon_cb *, void *);
 void    channel_cancel_cleanup(struct ssh *, int);
-int     channel_close_fd(struct ssh *, int *);
+int     channel_close_fd(struct ssh *, Channel *, int *);
 void    channel_send_window_changes(struct ssh *);
 
 /* mux proxy support */
@@ -310,7 +321,7 @@ Channel     *channel_connect_to_port(struct ssh *, const char *, u_short,
            char *, char *, int *, const char **);
 Channel *channel_connect_to_path(struct ssh *, const char *, char *, char *);
 Channel        *channel_connect_stdio_fwd(struct ssh *, const char*,
-           u_short, int, int);
+           u_short, int, int, int);
 Channel        *channel_connect_by_listen_address(struct ssh *, const char *,
            u_short, char *, char *);
 Channel        *channel_connect_by_listen_path(struct ssh *, const char *,

diff --git a/usr.bin/ssh/clientloop.c b/usr.bin/ssh/clientloop.c
index 2c17cfc..33a43ba 100644 (file)
--- a/usr.bin/ssh/clientloop.c
+++ b/usr.bin/ssh/clientloop.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: clientloop.c,v 1.362 2021/05/04 22:53:52 dtucker Exp $ */
+/* $OpenBSD: clientloop.c,v 1.363 2021/05/19 01:24:05 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -1397,14 +1397,6 @@ client_loop(struct ssh *ssh, int have_pty, int escape_char_arg,
        if (have_pty)
                leave_raw_mode(options.request_tty == REQUEST_TTY_FORCE);
 
-       /* restore blocking io */
-       if (!isatty(fileno(stdin)))
-               unset_nonblock(fileno(stdin));
-       if (!isatty(fileno(stdout)))
-               unset_nonblock(fileno(stdout));
-       if (!isatty(fileno(stderr)))
-               unset_nonblock(fileno(stderr));
-
        /*
         * If there was no shell or command requested, there will be no remote
         * exit status to be returned.  In that case, clear error code if the

diff --git a/usr.bin/ssh/mux.c b/usr.bin/ssh/mux.c
index 55dad8c..d37101a 100644 (file)
--- a/usr.bin/ssh/mux.c
+++ b/usr.bin/ssh/mux.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: mux.c,v 1.87 2021/04/03 06:18:40 djm Exp $ */
+/* $OpenBSD: mux.c,v 1.88 2021/05/19 01:24:05 djm Exp $ */
 /*
  * Copyright (c) 2002-2008 Damien Miller <djm@openbsd.org>
  *
@@ -439,14 +439,6 @@ mux_master_process_new_session(struct ssh *ssh, u_int rid,
        if (cctx->want_tty && tcgetattr(new_fd[0], &cctx->tio) == -1)
                error_f("tcgetattr: %s", strerror(errno));
 
-       /* enable nonblocking unless tty */
-       if (!isatty(new_fd[0]))
-               set_nonblock(new_fd[0]);
-       if (!isatty(new_fd[1]))
-               set_nonblock(new_fd[1]);
-       if (!isatty(new_fd[2]))
-               set_nonblock(new_fd[2]);
-
        window = CHAN_SES_WINDOW_DEFAULT;
        packetmax = CHAN_SES_PACKET_DEFAULT;
        if (cctx->want_tty) {
@@ -456,7 +448,7 @@ mux_master_process_new_session(struct ssh *ssh, u_int rid,
 
        nc = channel_new(ssh, "session", SSH_CHANNEL_OPENING,
            new_fd[0], new_fd[1], new_fd[2], window, packetmax,
-           CHAN_EXTENDED_WRITE, "client-session", /*nonblock*/0);
+           CHAN_EXTENDED_WRITE, "client-session", CHANNEL_NONBLOCK_STDIO);
 
        nc->ctl_chan = c->self;         /* link session -> control channel */
        c->remote_id = nc->self;        /* link control -> session channel */
@@ -1012,13 +1004,8 @@ mux_master_process_stdio_fwd(struct ssh *ssh, u_int rid,
                }
        }
 
-       /* enable nonblocking unless tty */
-       if (!isatty(new_fd[0]))
-               set_nonblock(new_fd[0]);
-       if (!isatty(new_fd[1]))
-               set_nonblock(new_fd[1]);
-
-       nc = channel_connect_stdio_fwd(ssh, chost, cport, new_fd[0], new_fd[1]);
+       nc = channel_connect_stdio_fwd(ssh, chost, cport, new_fd[0], new_fd[1],
+           CHANNEL_NONBLOCK_STDIO);
        free(chost);
 
        nc->ctl_chan = c->self;         /* link session -> control channel */

diff --git a/usr.bin/ssh/nchan.c b/usr.bin/ssh/nchan.c
index a775b71..4c465e4 100644 (file)
--- a/usr.bin/ssh/nchan.c
+++ b/usr.bin/ssh/nchan.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: nchan.c,v 1.72 2021/01/27 09:26:54 djm Exp $ */
+/* $OpenBSD: nchan.c,v 1.73 2021/05/19 01:24:05 djm Exp $ */
 /*
  * Copyright (c) 1999, 2000, 2001, 2002 Markus Friedl.  All rights reserved.
  *
@@ -382,7 +382,7 @@ chan_shutdown_write(struct ssh *ssh, Channel *c)
                            c->istate, c->ostate, strerror(errno));
                }
        } else {
-               if (channel_close_fd(ssh, &c->wfd) < 0) {
+               if (channel_close_fd(ssh, c, &c->wfd) < 0) {
                        logit_f("channel %d: close() failed for "
                            "fd %d [i%d o%d]: %.100s", c->self, c->wfd,
                            c->istate, c->ostate, strerror(errno));
@@ -405,7 +405,7 @@ chan_shutdown_read(struct ssh *ssh, Channel *c)
                            c->istate, c->ostate, strerror(errno));
                }
        } else {
-               if (channel_close_fd(ssh, &c->rfd) < 0) {
+               if (channel_close_fd(ssh, c, &c->rfd) < 0) {
                        logit_f("channel %d: close() failed for "
                            "fd %d [i%d o%d]: %.100s", c->self, c->rfd,
                            c->istate, c->ostate, strerror(errno));
@@ -424,7 +424,7 @@ chan_shutdown_extended_read(struct ssh *ssh, Channel *c)
        debug_f("channel %d: (i%d o%d sock %d wfd %d efd %d [%s])",
            c->self, c->istate, c->ostate, c->sock, c->rfd, c->efd,
            channel_format_extended_usage(c));
-       if (channel_close_fd(ssh, &c->efd) < 0) {
+       if (channel_close_fd(ssh, c, &c->efd) < 0) {
                logit_f("channel %d: close() failed for "
                    "extended fd %d [i%d o%d]: %.100s", c->self, c->efd,
                    c->istate, c->ostate, strerror(errno));

diff --git a/usr.bin/ssh/ssh.c b/usr.bin/ssh/ssh.c
index 45c0b87..9cbb93d 100644 (file)
--- a/usr.bin/ssh/ssh.c
+++ b/usr.bin/ssh/ssh.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssh.c,v 1.556 2021/05/17 11:43:16 djm Exp $ */
+/* $OpenBSD: ssh.c,v 1.557 2021/05/19 01:24:05 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -1855,9 +1855,10 @@ ssh_init_stdio_forwarding(struct ssh *ssh)
 
        if ((in = dup(STDIN_FILENO)) == -1 ||
            (out = dup(STDOUT_FILENO)) == -1)
-               fatal("channel_connect_stdio_fwd: dup() in/out failed");
+               fatal_f("dup() in/out failed");
        if ((c = channel_connect_stdio_fwd(ssh, options.stdio_forward_host,
-           options.stdio_forward_port, in, out)) == NULL)
+           options.stdio_forward_port, in, out,
+           CHANNEL_NONBLOCK_STDIO)) == NULL)
                fatal_f("channel_connect_stdio_fwd failed");
        channel_register_cleanup(ssh, c->self, client_cleanup_stdio_fwd, 0);
        channel_register_open_confirm(ssh, c->self, ssh_stdio_confirm, NULL);
@@ -2053,14 +2054,6 @@ ssh_session2_open(struct ssh *ssh)
        if (in == -1 || out == -1 || err == -1)
                fatal("dup() in/out/err failed");
 
-       /* enable nonblocking unless tty */
-       if (!isatty(in))
-               set_nonblock(in);
-       if (!isatty(out))
-               set_nonblock(out);
-       if (!isatty(err))
-               set_nonblock(err);
-
        window = CHAN_SES_WINDOW_DEFAULT;
        packetmax = CHAN_SES_PACKET_DEFAULT;
        if (tty_flag) {
@@ -2070,7 +2063,7 @@ ssh_session2_open(struct ssh *ssh)
        c = channel_new(ssh,
            "session", SSH_CHANNEL_OPENING, in, out, err,
            window, packetmax, CHAN_EXTENDED_WRITE,
-           "client-session", /*nonblock*/0);
+           "client-session", CHANNEL_NONBLOCK_STDIO);
 
        debug3_f("channel_new: %d", c->self);