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]

diff --git a/config.c b/config.c
index 2960cab37a5bfebc2e13abbc9a594e61f56b7b36..e4cfabf86f9ce47e6ecaccf3864617e4b85e433d 100644 (file)
--- 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 df7fc64079b0b3768695954dd416b367447c9ec7..f355a489cfdd0e499dad73ce58bfbf1963ed4fa5 100644 (file)
--- 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 f97c723b02d71371e891a3812c248b3a2e70ba5c..a6578ae0c4c1e9c5eef1d15d6ecae7708a806268 100644 (file)
--- 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 a82a388bf099d64a64332f2eb50673f5b5b0e2cf..e949df95fa46be954569d0362ebda510f9de5438 100644 (file)
--- 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 4eb84f899706a1ccd8bacf574572565d4243af0d..7e63b4e0b0460443ad835035ec5b8d1277a2c889 100644 (file)
--- 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 14b588600120fc13b68bce7f460e9f3f5350e2d1..dabd8b825baec3d984e65a7810a2a0d5a436de1d 100644 (file)
--- 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"