From: djm Date: Thu, 25 Jul 2024 22:40:08 +0000 (+0000) Subject: Fix proxy multiplexing (-O proxy) bug X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=116a8944a490266d80b3ac9f1b77caa153a87030;p=openbsd Fix proxy multiplexing (-O proxy) bug If a mux started with ControlPersist then later has a forwarding added using mux proxy connection and the forwarding was used, then when the mux proxy session terminates, the mux master process will send a channel close to the server with a bad channel ID and crash the connection. This was caused by my stupidly reusing c->remote_id for mux channel associations when I should have just added another member to struct channel. ok markus@ --- diff --git a/usr.bin/ssh/channels.c b/usr.bin/ssh/channels.c index 281cecf246c..15e702c32fe 100644 --- a/usr.bin/ssh/channels.c +++ b/usr.bin/ssh/channels.c @@ -1,4 +1,4 @@ -/* $OpenBSD: channels.c,v 1.438 2024/05/17 00:30:23 djm Exp $ */ +/* $OpenBSD: channels.c,v 1.439 2024/07/25 22:40:08 djm Exp $ */ /* * Author: Tatu Ylonen * Copyright (c) 1995 Tatu Ylonen , Espoo, Finland @@ -1004,14 +1004,16 @@ channel_format_status(const Channel *c) { char *ret = NULL; - xasprintf(&ret, "t%d [%s] %s%u i%u/%zu o%u/%zu e[%s]/%zu " - "fd %d/%d/%d sock %d cc %d io 0x%02x/0x%02x", + xasprintf(&ret, "t%d [%s] %s%u %s%u i%u/%zu o%u/%zu e[%s]/%zu " + "fd %d/%d/%d sock %d cc %d %s%u io 0x%02x/0x%02x", c->type, c->xctype != NULL ? c->xctype : c->ctype, c->have_remote_id ? "r" : "nr", c->remote_id, + c->mux_ctx != NULL ? "m" : "nm", c->mux_downstream_id, c->istate, sshbuf_len(c->input), c->ostate, sshbuf_len(c->output), channel_format_extended_usage(c), sshbuf_len(c->extended), c->rfd, c->wfd, c->efd, c->sock, c->ctl_chan, + c->have_ctl_child_id ? "c" : "nc", c->ctl_child_id, c->io_want, c->io_ready); return ret; } diff --git a/usr.bin/ssh/channels.h b/usr.bin/ssh/channels.h index de18de246be..8a4615edac6 100644 --- a/usr.bin/ssh/channels.h +++ b/usr.bin/ssh/channels.h @@ -1,4 +1,4 @@ -/* $OpenBSD: channels.h,v 1.156 2024/05/23 23:47:16 jsg Exp $ */ +/* $OpenBSD: channels.h,v 1.157 2024/07/25 22:40:08 djm Exp $ */ /* * Author: Tatu Ylonen @@ -139,6 +139,8 @@ struct Channel { u_int io_ready; /* bitmask of SSH_CHAN_IO_* */ int pfds[4]; /* pollfd entries for rfd/wfd/efd/sock */ int ctl_chan; /* control channel (multiplexed connections) */ + uint32_t ctl_child_id; /* child session for mux controllers */ + int have_ctl_child_id;/* non-zero if ctl_child_id is valid */ int isatty; /* rfd is a tty */ int client_tty; /* (client) TTY has been requested */ int force_drain; /* force close on iEOF */ diff --git a/usr.bin/ssh/mux.c b/usr.bin/ssh/mux.c index c68935668d2..80aa151f382 100644 --- a/usr.bin/ssh/mux.c +++ b/usr.bin/ssh/mux.c @@ -1,4 +1,4 @@ -/* $OpenBSD: mux.c,v 1.101 2023/11/23 03:37:05 dtucker Exp $ */ +/* $OpenBSD: mux.c,v 1.102 2024/07/25 22:40:08 djm Exp $ */ /* * Copyright (c) 2002-2008 Damien Miller * @@ -188,8 +188,8 @@ mux_master_session_cleanup_cb(struct ssh *ssh, int cid, int force, void *unused) fatal_f("channel %d missing control channel %d", c->self, c->ctl_chan); c->ctl_chan = -1; - cc->remote_id = 0; - cc->have_remote_id = 0; + cc->ctl_child_id = 0; + cc->have_ctl_child_id = 0; chan_rcvd_oclose(ssh, cc); } channel_cancel_cleanup(ssh, c->self); @@ -204,12 +204,12 @@ mux_master_control_cleanup_cb(struct ssh *ssh, int cid, int force, void *unused) debug3_f("entering for channel %d", cid); if (c == NULL) fatal_f("channel_by_id(%i) == NULL", cid); - if (c->have_remote_id) { - if ((sc = channel_by_id(ssh, c->remote_id)) == NULL) + if (c->have_ctl_child_id) { + if ((sc = channel_by_id(ssh, c->ctl_child_id)) == NULL) fatal_f("channel %d missing session channel %u", - c->self, c->remote_id); - c->remote_id = 0; - c->have_remote_id = 0; + c->self, c->ctl_child_id); + c->ctl_child_id = 0; + c->have_ctl_child_id = 0; sc->ctl_chan = -1; if (sc->type != SSH_CHANNEL_OPEN && sc->type != SSH_CHANNEL_OPENING) { @@ -405,7 +405,7 @@ mux_master_process_new_session(struct ssh *ssh, u_int rid, new_fd[0], new_fd[1], new_fd[2]); /* XXX support multiple child sessions in future */ - if (c->have_remote_id) { + if (c->have_ctl_child_id) { debug2_f("session already open"); reply_error(reply, MUX_S_FAILURE, rid, "Multiple sessions not supported"); @@ -450,8 +450,8 @@ mux_master_process_new_session(struct ssh *ssh, u_int rid, 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 */ - c->have_remote_id = 1; + c->ctl_child_id = nc->self; /* link control -> session channel */ + c->have_ctl_child_id = 1; if (cctx->want_tty && escape_char != 0xffffffff) { channel_register_filter(ssh, nc->self, @@ -990,7 +990,7 @@ mux_master_process_stdio_fwd(struct ssh *ssh, u_int rid, debug3_f("got fds stdin %d, stdout %d", new_fd[0], new_fd[1]); /* XXX support multiple child sessions in future */ - if (c->have_remote_id) { + if (c->have_ctl_child_id) { debug2_f("session already open"); reply_error(reply, MUX_S_FAILURE, rid, "Multiple sessions not supported"); @@ -1022,8 +1022,8 @@ mux_master_process_stdio_fwd(struct ssh *ssh, u_int rid, free(chost); nc->ctl_chan = c->self; /* link session -> control channel */ - c->remote_id = nc->self; /* link control -> session channel */ - c->have_remote_id = 1; + c->ctl_child_id = nc->self; /* link control -> session channel */ + c->have_ctl_child_id = 1; debug2_f("channel_new: %d control %d", nc->self, nc->ctl_chan); diff --git a/usr.bin/ssh/nchan.c b/usr.bin/ssh/nchan.c index 8757cd9290d..63cecbb7442 100644 --- a/usr.bin/ssh/nchan.c +++ b/usr.bin/ssh/nchan.c @@ -1,4 +1,4 @@ -/* $OpenBSD: nchan.c,v 1.75 2024/02/01 02:37:33 djm Exp $ */ +/* $OpenBSD: nchan.c,v 1.76 2024/07/25 22:40:08 djm Exp $ */ /* * Copyright (c) 1999, 2000, 2001, 2002 Markus Friedl. All rights reserved. * @@ -206,7 +206,7 @@ chan_send_close2(struct ssh *ssh, Channel *c) { int r; - debug2("channel %d: send close", c->self); + debug2("channel %d: send_close2", c->self); if (c->ostate != CHAN_OUTPUT_CLOSED || c->istate != CHAN_INPUT_CLOSED) { error("channel %d: cannot send close for istate/ostate %d/%d", @@ -216,6 +216,8 @@ chan_send_close2(struct ssh *ssh, Channel *c) } else { if (!c->have_remote_id) fatal_f("channel %d: no remote_id", c->self); + debug2("channel %d: send close for remote id %u", c->self, + c->remote_id); if ((r = sshpkt_start(ssh, SSH2_MSG_CHANNEL_CLOSE)) != 0 || (r = sshpkt_put_u32(ssh, c->remote_id)) != 0 || (r = sshpkt_send(ssh)) != 0)