From aaaf70a0fc0f071b9e9c6a51606c78b16a464841 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 6 Jul 2014 14:05:39 +0000 Subject: [PATCH] Implement this year's consensus on CHANNEL_FAILURE vs CHANNEL_CLOSE. We now expect that after the server has sent us CHANNEL_CLOSE, we should not expect to see any replies to our outstanding channel requests, and conversely after we have sent CHANNEL_CLOSE we avoid sending any reply to channel requests from the server. This was the consensus among implementors discussing the problem on ietf-ssh in April 2014. To cope with current OpenSSH's (and perhaps other servers we don't know about yet) willingness to send request replies after CHANNEL_CLOSE, I introduce a bug-compatibility flag which is detected for every OpenSSH version up to and including the current 6.6 - but not beyond, since https://bugzilla.mindrot.org/show_bug.cgi?id=1818 promises that 6.7 will also implement the new consensus behaviour. [originally from svn r10200] --- config.c | 3 +++ doc/config.but | 25 +++++++++++++++++++++++ putty.h | 1 + settings.c | 2 ++ ssh.c | 51 ++++++++++++++++++++++++++++++++++++++++++----- windows/winhelp.h | 1 + 6 files changed, 78 insertions(+), 5 deletions(-) diff --git a/config.c b/config.c index 2960cab3..e4cfabf8 100644 --- a/config.c +++ b/config.c @@ -2490,6 +2490,9 @@ void setup_config_box(struct controlbox *b, int midsession, ctrl_droplist(s, "Ignores SSH-2 maximum packet size", 'x', 20, HELPCTX(ssh_bugs_maxpkt2), sshbug_handler, I(CONF_sshbug_maxpkt2)); + ctrl_droplist(s, "Replies to channel requests after channel close", + 'q', 20, HELPCTX(ssh_bugs_chanreq), + sshbug_handler, I(CONF_sshbug_chanreq)); } } } diff --git a/doc/config.but b/doc/config.but index df7fc640..f355a489 100644 --- a/doc/config.but +++ b/doc/config.but @@ -3294,6 +3294,31 @@ believes the server has this bug, it will never send its \cq{winadj@putty.projects.tartarus.org} request, and will make do without its timing data. +\S{config-ssh-bug-chanreq} \q{Replies to channel requests after channel close} + +\cfg{winhelp-topic}{ssh.bugs.chanreq} + +The SSH protocol as published in RFC 4254 has an ambiguity which +arises if one side of a connection tries to close a channel, while the +other side simultaneously sends a request within the channel and asks +for a reply. RFC 4254 leaves it unclear whether the closing side +should reply to the channel request after having announced its +intention to close the channel. + +Discussion on the \cw{ietf-ssh} mailing list in April 2014 formed a +clear consensus that the right answer is no. However, because of the +ambiguity in the specification, some SSH servers have implemented the +other policy; for example, +\W{https://bugzilla.mindrot.org/show_bug.cgi?id=1818}{OpenSSH used to} +until it was fixed. + +Because PuTTY sends channel requests with the \q{want reply} flag +throughout channels' lifetime (see \k{config-ssh-bug-winadj}), it's +possible that when connecting to such a server it might receive a +reply to a request after it thinks the channel has entirely closed, +and terminate with an error along the lines of \q{Received +\cw{SSH2_MSG_CHANNEL_FAILURE} for nonexistent channel 256}. + \H{config-serial} The Serial panel The \i{Serial} panel allows you to configure options that only apply diff --git a/putty.h b/putty.h index f97c723b..a6578ae0 100644 --- a/putty.h +++ b/putty.h @@ -838,6 +838,7 @@ void cleanup_exit(int); X(INT, NONE, sshbug_maxpkt2) \ X(INT, NONE, sshbug_ignore2) \ X(INT, NONE, sshbug_winadj) \ + X(INT, NONE, sshbug_chanreq) \ /* \ * ssh_simple means that we promise never to open any channel \ * other than the main one, which means it can safely use a very \ diff --git a/settings.c b/settings.c index a82a388b..e949df95 100644 --- a/settings.c +++ b/settings.c @@ -626,6 +626,7 @@ void save_open_settings(void *sesskey, Conf *conf) write_setting_i(sesskey, "BugRekey2", 2-conf_get_int(conf, CONF_sshbug_rekey2)); write_setting_i(sesskey, "BugMaxPkt2", 2-conf_get_int(conf, CONF_sshbug_maxpkt2)); write_setting_i(sesskey, "BugWinadj", 2-conf_get_int(conf, CONF_sshbug_winadj)); + write_setting_i(sesskey, "BugChanReq", 2-conf_get_int(conf, CONF_sshbug_chanreq)); write_setting_i(sesskey, "StampUtmp", conf_get_int(conf, CONF_stamp_utmp)); write_setting_i(sesskey, "LoginShell", conf_get_int(conf, CONF_login_shell)); write_setting_i(sesskey, "ScrollbarOnLeft", conf_get_int(conf, CONF_scrollbar_on_left)); @@ -970,6 +971,7 @@ void load_open_settings(void *sesskey, Conf *conf) i = gppi_raw(sesskey, "BugRekey2", 0); conf_set_int(conf, CONF_sshbug_rekey2, 2-i); i = gppi_raw(sesskey, "BugMaxPkt2", 0); conf_set_int(conf, CONF_sshbug_maxpkt2, 2-i); i = gppi_raw(sesskey, "BugWinadj", 0); conf_set_int(conf, CONF_sshbug_winadj, 2-i); + i = gppi_raw(sesskey, "BugChanReq", 0); conf_set_int(conf, CONF_sshbug_chanreq, 2-i); conf_set_int(conf, CONF_ssh_simple, FALSE); gppi(sesskey, "StampUtmp", 1, conf, CONF_stamp_utmp); gppi(sesskey, "LoginShell", 1, conf, CONF_login_shell); diff --git a/ssh.c b/ssh.c index 4eb84f89..7e63b4e0 100644 --- a/ssh.c +++ b/ssh.c @@ -75,6 +75,7 @@ static const char *const ssh2_disconnect_reasons[] = { #define BUG_SSH2_MAXPKT 256 #define BUG_CHOKES_ON_SSH2_IGNORE 512 #define BUG_CHOKES_ON_WINADJ 1024 +#define BUG_SENDS_LATE_REQUEST_REPLY 2048 /* * Codes for terminal modes. @@ -2812,6 +2813,19 @@ static void ssh_detect_bugs(Ssh ssh, char *vstring) ssh->remote_bugs |= BUG_CHOKES_ON_WINADJ; logevent("We believe remote version has winadj bug"); } + + if (conf_get_int(ssh->conf, CONF_sshbug_chanreq) == FORCE_ON || + (conf_get_int(ssh->conf, CONF_sshbug_chanreq) == AUTO && + (wc_match("OpenSSH_[2-5].*", imp) || + wc_match("OpenSSH_6.[0-6]*", imp)))) { + /* + * These versions have the SSH-2 channel request bug. 6.7 and + * above do not: + * https://bugzilla.mindrot.org/show_bug.cgi?id=1818 + */ + ssh->remote_bugs |= BUG_SENDS_LATE_REQUEST_REPLY; + logevent("We believe remote version has SSH-2 channel request bug"); + } } /* @@ -7119,10 +7133,11 @@ static void ssh2_queue_chanreq_handler(struct ssh_channel *c, * request-specific data added and be sent. Note that if a handler is * provided, it's essential that the request actually be sent. * - * The handler will usually be passed the response packet in pktin. - * If pktin is NULL, this means that no reply will ever be forthcoming - * (e.g. because the entire connection is being destroyed) and the - * handler should free any storage it's holding. + * The handler will usually be passed the response packet in pktin. If + * pktin is NULL, this means that no reply will ever be forthcoming + * (e.g. because the entire connection is being destroyed, or because + * the server initiated channel closure before we saw the response) + * and the handler should free any storage it's holding. */ static struct Packet *ssh2_chanreq_init(struct ssh_channel *c, char *type, cchandler_fn_t handler, void *ctx) @@ -7612,6 +7627,21 @@ static void ssh2_msg_channel_close(Ssh ssh, struct Packet *pktin) */ ssh2_channel_got_eof(c); + if (!(ssh->remote_bugs & BUG_SENDS_LATE_REQUEST_REPLY)) { + /* + * It also means we stop expecting to see replies to any + * outstanding channel requests, so clean those up too. + * (ssh_chanreq_init will enforce by assertion that we don't + * subsequently put anything back on this list.) + */ + while (c->v.v2.chanreq_head) { + struct outstanding_channel_request *ocr = c->v.v2.chanreq_head; + ocr->handler(c, NULL, ocr->ctx); + c->v.v2.chanreq_head = ocr->next; + sfree(ocr); + } + } + /* * And we also send an outgoing EOF, if we haven't already, on the * assumption that CLOSE is a pretty forceful announcement that @@ -7787,6 +7817,16 @@ static void ssh2_msg_channel_request(Ssh ssh, struct Packet *pktin) ssh_pkt_getstring(pktin, &type, &typelen); want_reply = ssh2_pkt_getbool(pktin); + if (c->closes & CLOSES_SENT_CLOSE) { + /* + * We don't reply to channel requests after we've sent + * CHANNEL_CLOSE for the channel, because our reply might + * cross in the network with the other side's CHANNEL_CLOSE + * and arrive after they have wound the channel up completely. + */ + want_reply = FALSE; + } + /* * Having got the channel number, we now look at * the request type string to see if it's something @@ -8416,7 +8456,8 @@ static void ssh2_msg_authconn(Ssh ssh, struct Packet *pktin) static void ssh2_response_authconn(struct ssh_channel *c, struct Packet *pktin, void *ctx) { - do_ssh2_authconn(c->ssh, NULL, 0, pktin); + if (pktin) + do_ssh2_authconn(c->ssh, NULL, 0, pktin); } static void do_ssh2_authconn(Ssh ssh, unsigned char *in, int inlen, diff --git a/windows/winhelp.h b/windows/winhelp.h index 14b58860..dabd8b82 100644 --- a/windows/winhelp.h +++ b/windows/winhelp.h @@ -146,6 +146,7 @@ #define WINHELP_CTX_ssh_bugs_rekey2 "ssh.bugs.rekey2:config-ssh-bug-rekey" #define WINHELP_CTX_ssh_bugs_maxpkt2 "ssh.bugs.maxpkt2:config-ssh-bug-maxpkt2" #define WINHELP_CTX_ssh_bugs_winadj "ssh.bugs.winadj:config-ssh-bug-winadj" +#define WINHELP_CTX_ssh_bugs_chanreq "ssh.bugs.winadj:config-ssh-bug-chanreq" #define WINHELP_CTX_serial_line "serial.line:config-serial-line" #define WINHELP_CTX_serial_speed "serial.speed:config-serial-speed" #define WINHELP_CTX_serial_databits "serial.databits:config-serial-databits" -- 2.45.2