From 9e099151574885f3c717ac10a633a9218db8e7bb Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Fri, 24 Nov 2023 19:20:43 +0000 Subject: [PATCH] Fix check for "ext-info-s". ssh2_scan_kexinits must check to see whether it's behaving as an SSH client or server, in order to decide whether to look for "ext-info-s" in the server's KEXINIT or "ext-info-c" in the client's, respectively. This check was done by testing the pointer 'server_hostkeys' to see if it was non-NULL. I think I must have imagined that a variable of that name meant "the host keys we have available to offer the client, if we are the server", as the similarly named parameter 'our_hostkeys' in write_kexinit_lists in fact does mean. So I expected it to be non-NULL for the server and NULL for the client, and coded accordingly. But in fact it's used by the client: it collects host key types the client has _seen_ from the server, in order to offer them as cross- certification actions in the specials menu. Moreover, it's _always_ non-NULL, because in the server, it's easier to leave it present but empty than to get rid of it. So this code was always behaving as if it was the server, i.e. it was looking for "ext-info-c" in the client KEXINIT. When it was in fact the client, that test would always succeed, because we _sent_ that KEXINIT ourselves! But nobody ever noticed, because when we're the client, it doesn't matter whether we saw "ext-info-c", because we don't have any reason to send EXT_INFO from client to server. We're only concerned with server-to-client EXT_INFO. So this embarrassing bug had no actual effect. --- ssh/transport2.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ssh/transport2.c b/ssh/transport2.c index d1c255fc..3fe358f8 100644 --- a/ssh/transport2.c +++ b/ssh/transport2.c @@ -956,7 +956,7 @@ struct server_hostkeys { }; static bool ssh2_scan_kexinits( - ptrlen client_kexinit, ptrlen server_kexinit, + ptrlen client_kexinit, ptrlen server_kexinit, bool we_are_server, struct kexinit_algorithm_list kexlists[NKEXLIST], const ssh_kex **kex_alg, const ssh_keyalg **hostkey_alg, transport_direction *cs, transport_direction *sc, @@ -1167,9 +1167,9 @@ static bool ssh2_scan_kexinits( */ { ptrlen extinfo_advert = - (server_hostkeys ? PTRLEN_LITERAL("ext-info-c") : + (we_are_server ? PTRLEN_LITERAL("ext-info-c") : PTRLEN_LITERAL("ext-info-s")); - ptrlen list = (server_hostkeys ? clists[KEXLIST_KEX] : + ptrlen list = (we_are_server ? clists[KEXLIST_KEX] : slists[KEXLIST_KEX]); for (ptrlen word; get_commasep_word(&list, &word) ;) if (ptrlen_eq_ptrlen(word, extinfo_advert)) @@ -1463,7 +1463,7 @@ static void ssh2_transport_process_queue(PacketProtocolLayer *ppl) if (!ssh2_scan_kexinits( ptrlen_from_strbuf(s->client_kexinit), - ptrlen_from_strbuf(s->server_kexinit), + ptrlen_from_strbuf(s->server_kexinit), s->ssc != NULL, s->kexlists, &s->kex_alg, &s->hostkey_alg, s->cstrans, s->sctrans, &s->warn_kex, &s->warn_hk, &s->warn_cscipher, &s->warn_sccipher, s->ppl.ssh, NULL, &s->ignorepkt, &hks,