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 281cecf..15e702c 100644 (file)
--- 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 <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, 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 de18de2..8a4615e 100644 (file)
--- 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 <ylo@cs.hut.fi>
@@ -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 c689356..80aa151 100644 (file)
--- 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 <djm@openbsd.org>
  *
@@ -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 8757cd9..63cecbb 100644 (file)
--- 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)