From 9f53229c2ac97dbc6f5a03657de08a1150a9ac7e Mon Sep 17 00:00:00 2001 From: "djm@openbsd.org" Date: Tue, 12 Sep 2017 06:35:31 +0000 Subject: [PATCH] upstream commit Make remote channel ID a u_int Previously we tracked the remote channel IDs in an int, but this is strictly incorrect: the wire protocol uses uint32 and there is nothing in-principle stopping a SSH implementation from sending, say, 0xffff0000. In practice everyone numbers their channels sequentially, so this has never been a problem. ok markus@ Upstream-ID: b9f4cd3dc53155b4a5c995c0adba7da760d03e73 --- channels.c | 40 ++++++++++++++++++++++++++++++---------- channels.h | 9 +++++---- clientloop.c | 6 +++++- mux.c | 18 +++++++++++------- nchan.c | 10 +++++++++- serverloop.c | 6 +++++- 6 files changed, 65 insertions(+), 24 deletions(-) diff --git a/channels.c b/channels.c index 935625c74..3ab4823a9 100644 --- a/channels.c +++ b/channels.c @@ -1,4 +1,4 @@ -/* $OpenBSD: channels.c,v 1.366 2017/08/30 03:59:08 djm Exp $ */ +/* $OpenBSD: channels.c,v 1.368 2017/09/12 06:35:31 djm Exp $ */ /* * Author: Tatu Ylonen * Copyright (c) 1995 Tatu Ylonen , Espoo, Finland @@ -252,14 +252,14 @@ channel_by_id(struct ssh *ssh, int id) } Channel * -channel_by_remote_id(struct ssh *ssh, int remote_id) +channel_by_remote_id(struct ssh *ssh, u_int remote_id) { Channel *c; u_int i; for (i = 0; i < ssh->chanctxt->channels_alloc; i++) { c = ssh->chanctxt->channels[i]; - if (c != NULL && c->remote_id == remote_id) + if (c != NULL && c->have_remote_id && c->remote_id == remote_id) return c; } return NULL; @@ -387,7 +387,6 @@ channel_new(struct ssh *ssh, char *ctype, int type, int rfd, int wfd, int efd, c->local_window = window; c->local_window_max = window; c->local_maxpacket = maxpack; - c->remote_id = -1; c->remote_name = xstrdup(remote_name); c->ctl_chan = -1; c->delayed = 1; /* prevent call to channel_post handler */ @@ -703,7 +702,7 @@ channel_find_open(struct ssh *ssh) for (i = 0; i < ssh->chanctxt->channels_alloc; i++) { c = ssh->chanctxt->channels[i]; - if (c == NULL || c->remote_id < 0) + if (c == NULL || !c->have_remote_id) continue; switch (c->type) { case SSH_CHANNEL_CLOSED: @@ -778,9 +777,10 @@ channel_open_message(struct ssh *ssh) case SSH_CHANNEL_MUX_PROXY: case SSH_CHANNEL_MUX_CLIENT: if ((r = sshbuf_putf(buf, " #%d %.300s " - "(t%d r%d i%u/%zu o%u/%zu fd %d/%d cc %d)\r\n", + "(t%d %s%u i%u/%zu o%u/%zu fd %d/%d cc %d)\r\n", c->self, c->remote_name, - c->type, c->remote_id, + c->type, + c->have_remote_id ? "r" : "nr", c->remote_id, c->istate, sshbuf_len(c->input), c->ostate, sshbuf_len(c->output), c->rfd, c->wfd, c->ctl_chan)) != 0) @@ -838,6 +838,9 @@ channel_request_start(struct ssh *ssh, int id, char *service, int wantconfirm) logit("%s: %d: unknown channel id", __func__, id); return; } + if (!c->have_remote_id) + fatal(":%s: channel %d: no remote id", __func__, c->self); + debug2("channel %d: request %s confirm %d", id, service, wantconfirm); if ((r = sshpkt_start(ssh, SSH2_MSG_CHANNEL_REQUEST)) != 0 || (r = sshpkt_put_u32(ssh, c->remote_id)) != 0 || @@ -930,6 +933,9 @@ channel_set_fds(struct ssh *ssh, int id, int rfd, int wfd, int efd, if (c == NULL || c->type != SSH_CHANNEL_LARVAL) fatal("channel_activate for non-larval channel %d.", id); + if (!c->have_remote_id) + fatal(":%s: channel %d: no remote id", __func__, c->self); + channel_register_fds(ssh, c, rfd, wfd, efd, extusage, nonblock, is_tty); c->type = SSH_CHANNEL_OPEN; c->local_window = c->local_window_max = window_max; @@ -1698,6 +1704,8 @@ channel_post_connecting(struct ssh *ssh, Channel *c, if (!FD_ISSET(c->sock, writeset)) return; + if (!c->have_remote_id) + fatal(":%s: channel %d: no remote id", __func__, c->self); if (getsockopt(c->sock, SOL_SOCKET, SO_ERROR, &err, &sz) < 0) { err = errno; @@ -1983,6 +1991,9 @@ channel_check_window(struct ssh *ssh, Channel *c) c->local_maxpacket*3) || c->local_window < c->local_window_max/2) && c->local_consumed > 0) { + if (!c->have_remote_id) + fatal(":%s: channel %d: no remote id", + __func__, c->self); if ((r = sshpkt_start(ssh, SSH2_MSG_CHANNEL_WINDOW_ADJUST)) != 0 || (r = sshpkt_put_u32(ssh, c->remote_id)) != 0 || @@ -2338,6 +2349,9 @@ channel_output_poll_input_open(struct ssh *ssh, Channel *c) return; } + if (!c->have_remote_id) + fatal(":%s: channel %d: no remote id", __func__, c->self); + if (c->datagram) { /* Check datagram will fit; drop if not */ if ((r = sshbuf_peek_string_direct(c->input, NULL, &dlen)) != 0) @@ -2404,6 +2418,8 @@ channel_output_poll_extended_read(struct ssh *ssh, Channel *c) len = c->remote_maxpacket; if (len == 0) return; + if (!c->have_remote_id) + fatal(":%s: channel %d: no remote id", __func__, c->self); if ((r = sshpkt_start(ssh, SSH2_MSG_CHANNEL_EXTENDED_DATA)) != 0 || (r = sshpkt_put_u32(ssh, c->remote_id)) != 0 || (r = sshpkt_put_u32(ssh, SSH2_EXTENDED_DATA_STDERR)) != 0 || @@ -2570,6 +2586,7 @@ channel_proxy_downstream(struct ssh *ssh, Channel *downstream) c->mux_ctx = downstream; /* point to mux client */ c->mux_downstream_id = id; c->remote_id = remote_id; + c->have_remote_id = 1; if ((r = sshbuf_put_u32(modified, remote_id)) != 0 || (r = sshbuf_put_u32(modified, c->self)) != 0 || (r = sshbuf_putb(modified, original)) != 0) { @@ -2713,8 +2730,10 @@ channel_proxy_upstream(Channel *c, int type, u_int32_t seq, struct ssh *ssh) switch (type) { case SSH2_MSG_CHANNEL_OPEN_CONFIRMATION: /* record remote_id for SSH2_MSG_CHANNEL_CLOSE */ - if (cp && len > 4) + if (cp && len > 4) { c->remote_id = PEEK_U32(cp); + c->have_remote_id = 1; + } break; case SSH2_MSG_CHANNEL_CLOSE: if (c->flags & CHAN_CLOSE_SENT) @@ -2923,14 +2942,15 @@ channel_input_open_confirmation(int type, u_int32_t seq, struct ssh *ssh) * Record the remote channel number and mark that the channel * is now open. */ - c->remote_id = channel_parse_id(ssh, __func__, "open confirmation"); - if ((r = sshpkt_get_u32(ssh, &remote_window)) != 0 || + if ((r = sshpkt_get_u32(ssh, &c->remote_id)) != 0 || + (r = sshpkt_get_u32(ssh, &remote_window)) != 0 || (r = sshpkt_get_u32(ssh, &remote_maxpacket)) != 0) { error("%s: window/maxpacket: %s", __func__, ssh_err(r)); packet_disconnect("Invalid open confirmation message"); } ssh_packet_check_eom(ssh); + c->have_remote_id = 1; c->remote_window = remote_window; c->remote_maxpacket = remote_maxpacket; c->type = SSH_CHANNEL_OPEN; diff --git a/channels.h b/channels.h index f04c43afa..d1cf5dc6a 100644 --- a/channels.h +++ b/channels.h @@ -1,4 +1,4 @@ -/* $OpenBSD: channels.h,v 1.128 2017/09/12 06:32:07 djm Exp $ */ +/* $OpenBSD: channels.h,v 1.129 2017/09/12 06:35:32 djm Exp $ */ /* * Author: Tatu Ylonen @@ -97,8 +97,9 @@ typedef int mux_callback_fn(struct ssh *, struct Channel *); struct Channel { int type; /* channel type/state */ int self; /* my own channel identifier */ - int remote_id; /* channel identifier for remote peer */ - /* XXX should be uint32_t */ + uint32_t remote_id; /* channel identifier for remote peer */ + int have_remote_id; /* non-zero if remote_id is valid */ + u_int istate; /* input from channel (state of receive half) */ u_int ostate; /* output to channel (state of transmit half) */ int flags; /* close sent/rcvd */ @@ -222,7 +223,7 @@ void channel_init_channels(struct ssh *ssh); /* channel management */ Channel *channel_by_id(struct ssh *, int); -Channel *channel_by_remote_id(struct ssh *, int); +Channel *channel_by_remote_id(struct ssh *, u_int); Channel *channel_lookup(struct ssh *, int); Channel *channel_new(struct ssh *, char *, int, int, int, int, u_int, u_int, int, char *, int); diff --git a/clientloop.c b/clientloop.c index 1218b3b71..829eae024 100644 --- a/clientloop.c +++ b/clientloop.c @@ -1,4 +1,4 @@ -/* $OpenBSD: clientloop.c,v 1.303 2017/09/12 06:32:07 djm Exp $ */ +/* $OpenBSD: clientloop.c,v 1.304 2017/09/12 06:35:32 djm Exp $ */ /* * Author: Tatu Ylonen * Copyright (c) 1995 Tatu Ylonen , Espoo, Finland @@ -1682,6 +1682,7 @@ client_input_channel_open(int type, u_int32_t seq, struct ssh *ssh) } else if (c != NULL) { debug("confirm %s", ctype); c->remote_id = rchan; + c->have_remote_id = 1; c->remote_window = rwindow; c->remote_maxpacket = rmaxpack; if (c->type != SSH_CHANNEL_CONNECTING) { @@ -1749,6 +1750,9 @@ client_input_channel_req(int type, u_int32_t seq, struct ssh *ssh) packet_check_eom(); } if (reply && c != NULL && !(c->flags & CHAN_CLOSE_SENT)) { + if (!c->have_remote_id) + fatal("%s: channel %d: no remote_id", + __func__, c->self); packet_start(success ? SSH2_MSG_CHANNEL_SUCCESS : SSH2_MSG_CHANNEL_FAILURE); packet_put_int(c->remote_id); diff --git a/mux.c b/mux.c index 9eee287b9..f8510426d 100644 --- a/mux.c +++ b/mux.c @@ -1,4 +1,4 @@ -/* $OpenBSD: mux.c,v 1.66 2017/09/12 06:32:07 djm Exp $ */ +/* $OpenBSD: mux.c,v 1.67 2017/09/12 06:35:32 djm Exp $ */ /* * Copyright (c) 2002-2008 Damien Miller * @@ -215,7 +215,8 @@ mux_master_session_cleanup_cb(struct ssh *ssh, int cid, void *unused) fatal("%s: channel %d missing control channel %d", __func__, c->self, c->ctl_chan); c->ctl_chan = -1; - cc->remote_id = -1; + cc->remote_id = 0; + cc->have_remote_id = 0; chan_rcvd_oclose(ssh, cc); } channel_cancel_cleanup(ssh, c->self); @@ -231,11 +232,12 @@ mux_master_control_cleanup_cb(struct ssh *ssh, int cid, void *unused) debug3("%s: entering for channel %d", __func__, cid); if (c == NULL) fatal("%s: channel_by_id(%i) == NULL", __func__, cid); - if (c->remote_id != -1) { + if (c->have_remote_id) { if ((sc = channel_by_id(ssh, c->remote_id)) == NULL) - fatal("%s: channel %d missing session channel %d", + fatal("%s: channel %d missing session channel %u", __func__, c->self, c->remote_id); - c->remote_id = -1; + c->remote_id = 0; + c->have_remote_id = 0; sc->ctl_chan = -1; if (sc->type != SSH_CHANNEL_OPEN && sc->type != SSH_CHANNEL_OPENING) { @@ -413,7 +415,7 @@ process_mux_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->remote_id != -1) { + if (c->have_remote_id) { debug2("%s: session already open", __func__); /* prepare reply */ buffer_put_int(r, MUX_S_FAILURE); @@ -471,6 +473,7 @@ process_mux_new_session(struct ssh *ssh, u_int rid, nc->ctl_chan = c->self; /* link session -> control channel */ c->remote_id = nc->self; /* link control -> session channel */ + c->have_remote_id = 1; if (cctx->want_tty && escape_char != 0xffffffff) { channel_register_filter(ssh, nc->self, @@ -1007,7 +1010,7 @@ process_mux_stdio_fwd(struct ssh *ssh, u_int rid, new_fd[0], new_fd[1]); /* XXX support multiple child sessions in future */ - if (c->remote_id != -1) { + if (c->have_remote_id) { debug2("%s: session already open", __func__); /* prepare reply */ buffer_put_int(r, MUX_S_FAILURE); @@ -1043,6 +1046,7 @@ process_mux_stdio_fwd(struct ssh *ssh, u_int rid, nc->ctl_chan = c->self; /* link session -> control channel */ c->remote_id = nc->self; /* link control -> session channel */ + c->have_remote_id = 1; debug2("%s: channel_new: %d linked to control channel %d", __func__, nc->self, nc->ctl_chan); diff --git a/nchan.c b/nchan.c index 74c855c90..24929556d 100644 --- a/nchan.c +++ b/nchan.c @@ -1,4 +1,4 @@ -/* $OpenBSD: nchan.c,v 1.66 2017/09/12 06:32:07 djm Exp $ */ +/* $OpenBSD: nchan.c,v 1.67 2017/09/12 06:35:32 djm Exp $ */ /* * Copyright (c) 1999, 2000, 2001, 2002 Markus Friedl. All rights reserved. * @@ -183,6 +183,9 @@ chan_send_eof2(struct ssh *ssh, Channel *c) debug2("channel %d: send eof", c->self); switch (c->istate) { case CHAN_INPUT_WAIT_DRAIN: + if (!c->have_remote_id) + fatal("%s: channel %d: no remote_id", + __func__, c->self); if ((r = sshpkt_start(ssh, SSH2_MSG_CHANNEL_EOF)) != 0 || (r = sshpkt_put_u32(ssh, c->remote_id)) != 0 || (r = sshpkt_send(ssh)) != 0) @@ -209,6 +212,9 @@ chan_send_close2(struct ssh *ssh, Channel *c) } else if (c->flags & CHAN_CLOSE_SENT) { error("channel %d: already sent close", c->self); } else { + if (!c->have_remote_id) + fatal("%s: channel %d: no remote_id", + __func__, c->self); if ((r = sshpkt_start(ssh, SSH2_MSG_CHANNEL_CLOSE)) != 0 || (r = sshpkt_put_u32(ssh, c->remote_id)) != 0 || (r = sshpkt_send(ssh)) != 0) @@ -230,6 +236,8 @@ chan_send_eow2(struct ssh *ssh, Channel *c) } if (!(datafellows & SSH_NEW_OPENSSH)) return; + if (!c->have_remote_id) + fatal("%s: channel %d: no remote_id", __func__, c->self); if ((r = sshpkt_start(ssh, SSH2_MSG_CHANNEL_REQUEST)) != 0 || (r = sshpkt_put_u32(ssh, c->remote_id)) != 0 || (r = sshpkt_put_cstring(ssh, "eow@openssh.com")) != 0 || diff --git a/serverloop.c b/serverloop.c index 567159410..ae75fc2ec 100644 --- a/serverloop.c +++ b/serverloop.c @@ -1,4 +1,4 @@ -/* $OpenBSD: serverloop.c,v 1.197 2017/09/12 06:32:07 djm Exp $ */ +/* $OpenBSD: serverloop.c,v 1.198 2017/09/12 06:35:32 djm Exp $ */ /* * Author: Tatu Ylonen * Copyright (c) 1995 Tatu Ylonen , Espoo, Finland @@ -619,6 +619,7 @@ server_input_channel_open(int type, u_int32_t seq, struct ssh *ssh) if (c != NULL) { debug("server_input_channel_open: confirm %s", ctype); c->remote_id = rchan; + c->have_remote_id = 1; c->remote_window = rwindow; c->remote_maxpacket = rmaxpack; if (c->type != SSH_CHANNEL_CONNECTING) { @@ -844,6 +845,9 @@ server_input_channel_req(int type, u_int32_t seq, struct ssh *ssh) c->type == SSH_CHANNEL_OPEN) && strcmp(c->ctype, "session") == 0) success = session_input_channel_req(ssh, c, rtype); if (reply && !(c->flags & CHAN_CLOSE_SENT)) { + if (!c->have_remote_id) + fatal("%s: channel %d: no remote_id", + __func__, c->self); packet_start(success ? SSH2_MSG_CHANNEL_SUCCESS : SSH2_MSG_CHANNEL_FAILURE); packet_put_int(c->remote_id);