From 43d5df635286bf17a0964f9867df400f13400856 Mon Sep 17 00:00:00 2001 From: djm Date: Wed, 19 May 2021 01:24:05 +0000 Subject: [PATCH] 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@ --- usr.bin/ssh/channels.c | 67 ++++++++++++++++++++++++++++------------ usr.bin/ssh/channels.h | 17 ++++++++-- usr.bin/ssh/clientloop.c | 10 +----- usr.bin/ssh/mux.c | 21 +++---------- usr.bin/ssh/nchan.c | 8 ++--- usr.bin/ssh/ssh.c | 17 +++------- 6 files changed, 75 insertions(+), 65 deletions(-) diff --git a/usr.bin/ssh/channels.c b/usr.bin/ssh/channels.c index ebeb82926f3..b54d1b8eb6a 100644 --- 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 * Copyright (c) 1995 Tatu Ylonen , 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 b318974ead0..633adc32283 100644 --- 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 @@ -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 2c17cfcba83..33a43baf8e8 100644 --- 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 * Copyright (c) 1995 Tatu Ylonen , 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 55dad8cd832..d37101ae20f 100644 --- 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 * @@ -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 a775b71a5fe..4c465e491ab 100644 --- 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 45c0b87f5bc..9cbb93d5d94 100644 --- 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 * Copyright (c) 1995 Tatu Ylonen , 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); -- 2.20.1