From 5e2acd9af7033b681023e026d4607e554f7ab984 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Tue, 30 Aug 2022 18:51:33 +0100 Subject: [PATCH] New bug workaround: KEXINIT filtering. We've occasionally had reports of SSH servers disconnecting as soon as they receive PuTTY's KEXINIT. I think all such reports have involved the kind of simple ROM-based SSH server software you find in small embedded devices. I've never been able to prove it, but I've always suspected that one possible cause of this is simply that PuTTY's KEXINIT is _too long_, either in number of algorithms listed or in total length (especially given all the ones that end in @very.long.domain.name suffixes). If I'm right about either of those being the cause, then it's just become even more likely to happen, because of all the extra Diffie-Hellman groups and GSSAPI algorithms we just threw into our already-long list in the previous few commits. A workaround I've had in mind for ages is to wait for the server's KEXINIT, and then filter our own down to just the algorithms the server also mentioned. Then our KEXINIT is no longer than that of the server, and hence, presumably fits in whatever buffer it has. So I've implemented that workaround, in anticipation of it being needed in the near future. (Well ... it's not _quite_ true that our KEXINIT is at most the same length as the server. In fact I had to leave in one KEXINIT item that won't match anything in the server's list, namely "ext-info-c" which gates access to SHA-2 based RSA. So if we turn out to support absolutely everything on all the server's lists, then our KEXINIT would be a few bytes longer than the server's, even with this workaround. But that would only cause trouble if the server's outgoing KEXINIT was skating very close to whatever buffer size it has for the incoming one, and I'm guessing that's not very likely.) ((Another possible cause of this kind of disconnection would be a server that simply objects to seeing any KEXINIT string it doesn't know how to speak. But _surely_ no such server would have survived initial testing against any full-featured client at all!)) --- config.c | 4 ++ doc/config.but | 26 ++++++++++++ putty.h | 1 + settings.c | 2 + ssh.h | 1 + ssh/transport2.c | 102 ++++++++++++++++++++++++++++++++++++++++++++--- ssh/verstring.c | 6 +++ windows/help.h | 1 + 8 files changed, 138 insertions(+), 5 deletions(-) diff --git a/config.c b/config.c index 03043912..12c6767c 100644 --- a/config.c +++ b/config.c @@ -3154,6 +3154,10 @@ void setup_config_box(struct controlbox *b, bool midsession, HELPCTX(ssh_bugs_dropstart), sshbug_handler_manual_only, I(CONF_sshbug_dropstart)); + ctrl_droplist(s, "Chokes on PuTTY's full KEXINIT", 'p', 20, + HELPCTX(ssh_bugs_filter_kexinit), + sshbug_handler_manual_only, + I(CONF_sshbug_filter_kexinit)); ctrl_settitle(b, "Connection/SSH/More bugs", "Further workarounds for SSH server bugs"); diff --git a/doc/config.but b/doc/config.but index 918b63d8..c2f8d0cb 100644 --- a/doc/config.but +++ b/doc/config.but @@ -3573,6 +3573,32 @@ auto-detection relies on the version string in the server's greeting, and PuTTY has to decide whether to expect this bug \e{before} it sees the server's greeting. So this is a manual workaround only. +\S{config-ssh-bug-filter-kexinit} \q{Chokes on PuTTY's full \cw{KEXINIT}} + +At the start of an SSH connection, the client and server exchange long +messages of type \cw{SSH_MSG_KEXINIT}, containing lists of all the +cryptographic algorithms they're prepared to use. This is used to +negotiate a set of algorithms that both ends can speak. + +Occasionally, a badly written server might have a length limit on the +list it's prepared to receive, and refuse to make a connection simply +because PuTTY is giving it too many choices. + +A workaround is to enable this flag, which will make PuTTY wait to +send \cw{KEXINIT} until after it receives the one from the server, and +then filter its own \cw{KEXINIT} to leave out any algorithm the server +doesn't also announce support for. This will generally make PuTTY's +\cw{KEXINIT} at most the size of the server's, and will otherwise make +no difference to the algorithm negotiation. + +This flag is a minor violation of the SSH protocol, because both sides +are supposed to send \cw{KEXINIT} proactively. It still works provided +\e{one} side sends its \cw{KEXINIT} without waiting, but if both +client and server waited for the other one to speak first, the +connection would deadlock. We don't know of any servers that do this, +but if there is one, then this flag will make PuTTY unable to speak to +them at all. + \S{config-ssh-bug-sig} \q{Requires padding on SSH-2 \i{RSA} \i{signatures}} Versions below 3.3 of \i{OpenSSH} require SSH-2 RSA signatures to be diff --git a/putty.h b/putty.h index 9ebf4aed..74fbe6f2 100644 --- a/putty.h +++ b/putty.h @@ -2022,6 +2022,7 @@ NORETURN void cleanup_exit(int); X(INT, NONE, sshbug_winadj) \ X(INT, NONE, sshbug_chanreq) \ X(INT, NONE, sshbug_dropstart) \ + X(INT, NONE, sshbug_filter_kexinit) \ /* \ * 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 d2cd6a7e..c6c81562 100644 --- a/settings.c +++ b/settings.c @@ -778,6 +778,7 @@ void save_open_settings(settings_w *sesskey, Conf *conf) 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, "BugDropStart", 2-conf_get_int(conf, CONF_sshbug_dropstart)); + write_setting_i(sesskey, "BugFilterKexinit", 2-conf_get_int(conf, CONF_sshbug_filter_kexinit)); write_setting_b(sesskey, "StampUtmp", conf_get_bool(conf, CONF_stamp_utmp)); write_setting_b(sesskey, "LoginShell", conf_get_bool(conf, CONF_login_shell)); write_setting_b(sesskey, "ScrollbarOnLeft", conf_get_bool(conf, CONF_scrollbar_on_left)); @@ -1257,6 +1258,7 @@ void load_open_settings(settings_r *sesskey, Conf *conf) 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); i = gppi_raw(sesskey, "BugDropStart", 1); conf_set_int(conf, CONF_sshbug_dropstart, 2-i); + i = gppi_raw(sesskey, "BugFilterKexinit", 1); conf_set_int(conf, CONF_sshbug_filter_kexinit, 2-i); conf_set_bool(conf, CONF_ssh_simple, false); gppb(sesskey, "StampUtmp", true, conf, CONF_stamp_utmp); gppb(sesskey, "LoginShell", true, conf, CONF_login_shell); diff --git a/ssh.h b/ssh.h index dbdd7eb1..79f1f431 100644 --- a/ssh.h +++ b/ssh.h @@ -1880,6 +1880,7 @@ void old_keyfile_warning(void); X(BUG_CHOKES_ON_WINADJ) \ X(BUG_SENDS_LATE_REQUEST_REPLY) \ X(BUG_SSH2_OLDGEX) \ + X(BUG_REQUIRES_FILTERED_KEXINIT) \ /* end of list */ #define TMP_DECLARE_LOG2_ENUM(thing) log2_##thing, enum { SSH_IMPL_BUG_LIST(TMP_DECLARE_LOG2_ENUM) }; diff --git a/ssh/transport2.c b/ssh/transport2.c index 37093978..f7b08f15 100644 --- a/ssh/transport2.c +++ b/ssh/transport2.c @@ -1196,6 +1196,75 @@ static bool ssh2_scan_kexinits( return true; } +static strbuf *write_filtered_kexinit(struct ssh2_transport_state *s) +{ + strbuf *pktout = strbuf_new(); + BinarySource osrc[1], isrc[1]; + BinarySource_BARE_INIT( + osrc, s->outgoing_kexinit->u, s->outgoing_kexinit->len); + BinarySource_BARE_INIT( + isrc, s->incoming_kexinit->u, s->incoming_kexinit->len); + + /* Skip the packet type bytes from both packets */ + get_byte(osrc); + get_byte(isrc); + + /* Copy our cookie into the real output packet; skip their cookie */ + put_datapl(pktout, get_data(osrc, 16)); + get_data(isrc, 16); + + /* + * Now we expect NKEXLIST+2 name-lists. We write into the outgoing + * packet a subset of our intended outgoing one, containing only + * names mentioned in the incoming out. + * + * NKEXLIST+2 because for this purpose we treat the 'languages' + * lists the same as the rest. In the rest of this code base we + * ignore those. + */ + strbuf *out = strbuf_new(); + for (size_t i = 0; i < NKEXLIST+2; i++) { + strbuf_clear(out); + ptrlen olist = get_string(osrc), ilist = get_string(isrc); + for (ptrlen oword; get_commasep_word(&olist, &oword) ;) { + ptrlen ilist_copy = ilist; + bool add = false; + for (ptrlen iword; get_commasep_word(&ilist_copy, &iword) ;) { + if (ptrlen_eq_ptrlen(oword, iword)) { + /* Found this word in the incoming list. */ + add = true; + break; + } + } + + if (i == KEXLIST_KEX && ptrlen_eq_string(oword, "ext-info-c")) { + /* Special case: this will _never_ match anything from the + * server, and we need it to enable SHA-2 based RSA. + * + * If this ever turns out to confuse any server all by + * itself then I suppose we'll need an even more + * draconian bug flag to exclude that too. (Obv, such + * a server wouldn't be able to speak SHA-2 RSA + * anyway.) */ + add = true; + } + + if (add) + add_to_commasep_pl(out, oword); + } + put_stringpl(pktout, ptrlen_from_strbuf(out)); + } + strbuf_free(out); + + /* + * Finally, copy the remaining parts of our intended KEXINIT. + */ + put_bool(pktout, get_bool(osrc)); /* first-kex-packet-follows */ + put_uint32(pktout, get_uint32(osrc)); /* reserved word */ + + return pktout; +} + void ssh2transport_finalise_exhash(struct ssh2_transport_state *s) { put_datapl(s->exhash, ptrlen_from_strbuf(s->kex_shared_secret)); @@ -1304,12 +1373,14 @@ static void ssh2_transport_process_queue(PacketProtocolLayer *ppl) put_uint32(s->outgoing_kexinit, 0); /* reserved */ /* - * Send our KEXINIT. + * Send our KEXINIT (in the normal case). */ - pktout = ssh_bpp_new_pktout(s->ppl.bpp, SSH2_MSG_KEXINIT); - put_data(pktout, s->outgoing_kexinit->u + 1, - s->outgoing_kexinit->len - 1); /* omit initial packet type byte */ - pq_push(s->ppl.out_pq, pktout); + if (!(s->ppl.remote_bugs & BUG_REQUIRES_FILTERED_KEXINIT)) { + pktout = ssh_bpp_new_pktout(s->ppl.bpp, SSH2_MSG_KEXINIT); + put_data(pktout, s->outgoing_kexinit->u + 1, + s->outgoing_kexinit->len - 1); /* omit type byte */ + pq_push(s->ppl.out_pq, pktout); + } /* * Flag that KEX is in progress. @@ -1331,6 +1402,27 @@ static void ssh2_transport_process_queue(PacketProtocolLayer *ppl) put_byte(s->incoming_kexinit, SSH2_MSG_KEXINIT); put_data(s->incoming_kexinit, get_ptr(pktin), get_avail(pktin)); + /* + * If we've delayed sending our KEXINIT so as to filter it down to + * only things the server won't choke on, send ours now. + */ + if (s->ppl.remote_bugs & BUG_REQUIRES_FILTERED_KEXINIT) { + strbuf *sb = write_filtered_kexinit(s); + + /* Send that data as a packet */ + pktout = ssh_bpp_new_pktout(s->ppl.bpp, SSH2_MSG_KEXINIT); + put_datapl(pktout, ptrlen_from_strbuf(sb)); + pq_push(s->ppl.out_pq, pktout); + + /* And also replace our previous outgoing KEXINIT, since the + * host key signature will be validated against this reduced + * one. */ + strbuf_shrink_to(s->outgoing_kexinit, 1); /* keep the type byte */ + put_datapl(s->outgoing_kexinit, ptrlen_from_strbuf(sb)); + + strbuf_free(sb); + } + /* * Work through the two KEXINIT packets in parallel to find the * selected algorithm identifiers. diff --git a/ssh/verstring.c b/ssh/verstring.c index 567a8e7d..aa4c2c20 100644 --- a/ssh/verstring.c +++ b/ssh/verstring.c @@ -606,6 +606,12 @@ static void ssh_detect_bugs(struct ssh_verstring_state *s) bpp_logevent("We believe remote version has SSH-2 " "channel request bug"); } + + if (conf_get_int(s->conf, CONF_sshbug_filter_kexinit) == FORCE_ON) { + s->remote_bugs |= BUG_REQUIRES_FILTERED_KEXINIT; + bpp_logevent("We believe remote version requires us to " + "filter our KEXINIT"); + } } const char *ssh_verstring_get_remote(BinaryPacketProtocol *bpp) diff --git a/windows/help.h b/windows/help.h index cdd55a11..799e6240 100644 --- a/windows/help.h +++ b/windows/help.h @@ -170,6 +170,7 @@ typedef const char *HelpCtx; #define WINHELP_CTX_ssh_bugs_chanreq "config-ssh-bug-chanreq" #define WINHELP_CTX_ssh_bugs_oldgex2 "config-ssh-bug-oldgex2" #define WINHELP_CTX_ssh_bugs_dropstart "config-ssh-bug-dropstart" +#define WINHELP_CTX_ssh_bugs_filter_kexinit "config-ssh-bug-filter-kexinit" #define WINHELP_CTX_serial_line "config-serial-line" #define WINHELP_CTX_serial_speed "config-serial-speed" #define WINHELP_CTX_serial_databits "config-serial-databits"