Package: openssh-server
Version: 1:8.4p1-5+deb11u3
Severity: normal
Tags: patch
X-Debbugs-Cc: r...@qoxp.net

The sshd "hostkey none" option -- part of the GSSAPI key-exchange
patch maintained by Debian -- has two bugs:

Bug #1) The Debian GSSAPI code drops all kex algorithms from the
outgoing server proposal here, in the case of "hostkey none":

[sshd.c]
  /*
   * If we don't have a host key, then there's no point advertising
   * the other key exchange algorithms
   */

  if (strlen(myproposal[PROPOSAL_SERVER_HOST_KEY_ALGS]) == 0)
          orig = NULL;

This became buggy with the introduction of extension "signalling" via
kex pseudo-algorithms, originally ext-info-*, and later
kex-strict-*. The code now drops any such signals originated by
kex_proposal_populate_entries(). This prevents ext-info negotiation,
and with the addition of the strict-kex feature, it caused a worse
bug: SSH connections using a MAC (non-AEAD bulk cipher) with a client
that supports strict-kex die like so:

  $ ssh ...
  Bad packet length 1328530089.
  ssh_dispatch_run_fatal: Connection to 172.18.0.2 port 22: Connection corrupted

This is because the upstream server code believes it has advertised
strict-kex, so turns on the feature if it sees a matching
advertisement from the client. But it never *actually* advertised it,
since the GSSAPI patch strips the advertisement from the outgoing kex
proposal, so the client never sees it. Now the server has strict-kex
enabled while the client does not. As soon as the server turns on
encryption with the NEWKEYS message, it resets its outgoing packet
sequence number to zero -- but the client doesn't do this, so a MAC
failure ensues. This only applies to connections using a MAC; the
sequence number is unused in the case of the newer AEAD ciphers.

Bug #2) sshd dies with an invalid free():

[strace -fe signal sshd -ddd]
  debug1: expecting SSH2_MSG_NEWKEYS [preauth]
  debug3: receive packet: type 21 [preauth]
  debug1: ssh_packet_read_poll2: resetting read seqnr 3 [preauth]
  free(): invalid pointer
  debug1: SSH2_MSG_NEWKEYS received [preauth]
  [pid    57] rt_sigprocmask(SIG_UNBLOCK, [ABRT], NULL, 8) = 0
  debug2: ssh_set_newkeys: mode 0 [preauth]
  [pid    57] tgkill(57, 57, SIGABRT)     = 234

This happens because in the case of "hostkey none", the GSSAPI patch
resets the outgoing kex hostkey-algorithm list to contain just the
"null" algorithm:

[sshd.c]
  /*
   * If we've got GSSAPI mechanisms, then we've got the 'null' host
   * key alg, but we can't tell people about it unless its the only
   * host key algorithm we support
   */
  if (gss && (strlen(myproposal[PROPOSAL_SERVER_HOST_KEY_ALGS])) == 0)
      myproposal[PROPOSAL_SERVER_HOST_KEY_ALGS] = "null";

This is fine, except that it uses a static string ("null") to do it,
whereas the upstream kex_proposal_free_entries() expects all entries
in a kex proposal to be dynamically allocated, and tries to free
it. This can be fixed with an easy change to:

        myproposal[PROPOSAL_SERVER_HOST_KEY_ALGS] = xstrdup("null");

I'm including a patch fixing both bugs, against 1:9.6p1-5. A patch
against 'master' will need to apply to sshd-session.c instead of
sshd.c with later changes from upstream, but looks otherwise to be
identical.

My patch proposes to fix bug #1 by altering the upstream
kex_proposal_populate_entries(). The current Debian GSSAPI patch
avoids doing that in favor of separate code that modifies the kex
proposal later, which makes sense as a general strategy for
maintaining the patch but seems problematic now. With the addition of
kex signalling, that code has no way to know which kex algorithms are
"real" and can be safely dropped, and which are signals that it must
retain. It would need to hard-code a list of signals to look for and
keep, and that list will go out of date whenever new signals appear
upstream, easily causing another bug of the same kind. It seems better
to fix it in the upstream code, where it will either continue to work
correctly as new signals are added, or if there's a conflicting
upstream change we'll be forced to examine it and make sure it still
works.

This change could in fact be proposed upstream, since by itself it
does not rely on GSSAPI at all; it merely accommodates a case that
only *arises* with the gss-kex patch.

Alternatively, we could retain the same strategy if the upstream
provided a global list of signal algorithms to refer to, but currently
that list is just coded explicitly inside
kex_proposal_populate_entries():

[kex.c]
  /* Append feature signalling to KexAlgorithms. */
  if ((cp = kex_names_cat(kexalgos, ssh->kex->server ?
      "ext-info-s,kex-strict-s-...@openssh.com" :
      "ext-info-c,kex-strict-c-...@openssh.com")) == NULL)
          fatal_f("kex_names_cat");


Here is the patch against 1:9.6p1-5:

================================================================================
diff --git a/kex.c b/kex.c
index 6f06a4f7..db6717e9 100644
--- a/kex.c
+++ b/kex.c
@@ -378,25 +378,14 @@ kex_proposal_populate_entries(struct ssh *ssh, char 
*prop[PROPOSAL_MAX],
        const char *defpropclient[PROPOSAL_MAX] = { KEX_CLIENT };
        const char **defprop = ssh->kex->server ? defpropserver : defpropclient;
        u_int i;
-       char *cp, *hkalgs_prop;
+       char *cp;
 
        if (prop == NULL)
                fatal_f("proposal missing");
 
-       /* our hostkey algorithm proposal */
-       hkalgs_prop = xstrdup(hkalgs ? hkalgs : 
defprop[PROPOSAL_SERVER_HOST_KEY_ALGS]);
-
-       /*
-        * If we don't have a hostkey (sshd_config "HostKey none" =>
-        * hkalgs_prop list is empty), there's no point in including
-        * the default kex algorithms; start with the empty list
-        * instead. GSSAPI code will later add the dynamically
-        * determined gss-* algorithms.
-        */
+       /* Append EXT_INFO signalling to KexAlgorithms */
        if (kexalgos == NULL)
-               kexalgos = strlen(hkalgs_prop) == 0 ? "" : 
defprop[PROPOSAL_KEX_ALGS];
-
-       /* Append feature signalling to KexAlgorithms. */
+               kexalgos = defprop[PROPOSAL_KEX_ALGS];
        if ((cp = kex_names_cat(kexalgos, ssh->kex->server ?
            "ext-info-s,kex-strict-s-...@openssh.com" :
            "ext-info-c,kex-strict-c-...@openssh.com")) == NULL)
@@ -420,7 +409,7 @@ kex_proposal_populate_entries(struct ssh *ssh, char 
*prop[PROPOSAL_MAX],
                        prop[i] = xstrdup(comp ? comp : defprop[i]);
                        break;
                case PROPOSAL_SERVER_HOST_KEY_ALGS:
-                       prop[i] = hkalgs_prop;
+                       prop[i] = xstrdup(hkalgs ? hkalgs : defprop[i]);
                        break;
                default:
                        prop[i] = xstrdup(defprop[i]);
diff --git a/sshd.c b/sshd.c
index 625c1f32..6dfa5fff 100644
--- a/sshd.c
+++ b/sshd.c
@@ -2505,6 +2505,14 @@ do_ssh2_kex(struct ssh *ssh)
        char *newstr = NULL;
        orig = myproposal[PROPOSAL_KEX_ALGS];
 
+       /*
+        * If we don't have a host key, then there's no point advertising
+        * the other key exchange algorithms
+        */
+
+       if (strlen(myproposal[PROPOSAL_SERVER_HOST_KEY_ALGS]) == 0)
+               orig = NULL;
+
        if (options.gss_keyex)
                gss = ssh_gssapi_server_mechanisms();
        else
@@ -2523,7 +2531,7 @@ do_ssh2_kex(struct ssh *ssh)
         * host key algorithm we support
         */
        if (gss && (strlen(myproposal[PROPOSAL_SERVER_HOST_KEY_ALGS])) == 0)
-               myproposal[PROPOSAL_SERVER_HOST_KEY_ALGS] = xstrdup("null");
+               myproposal[PROPOSAL_SERVER_HOST_KEY_ALGS] = "null";
 
        if (newstr)
                myproposal[PROPOSAL_KEX_ALGS] = newstr;

================================================================================

-- System Information:
Debian Release: 11.11
  APT prefers oldstable-updates
  APT policy: (500, 'oldstable-updates'), (500, 'oldstable-security'), (500, 
'oldstable')
Architecture: amd64 (x86_64)

Kernel: Linux 5.10.0-33-cloud-amd64 (SMP w/2 CPU threads)
Kernel taint flags: TAINT_PROPRIETARY_MODULE, TAINT_OOT_MODULE, 
TAINT_UNSIGNED_MODULE
Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8), LANGUAGE not set
Shell: /bin/sh linked to /usr/bin/dash
Init: systemd (via /run/systemd/system)
LSM: AppArmor: enabled

Versions of packages openssh-server depends on:
ii  adduser                3.118+deb11u1
ii  debconf [debconf-2.0]  1.5.77
ii  dpkg                   1.20.13
ii  libaudit1              1:3.0-2
ii  libc6                  2.31-13+deb11u11
ii  libcom-err2            1.46.2-2+deb11u1
ii  libcrypt1              1:4.4.18-4
ii  libgssapi-krb5-2       1.18.3-6+deb11u5
ii  libkrb5-3              1.18.3-6+deb11u5
ii  libpam-modules         1.4.0-9+deb11u1
ii  libpam-runtime         1.4.0-9+deb11u1
ii  libpam0g               1.4.0-9+deb11u1
ii  libselinux1            3.1-3
ii  libssl1.1              1.1.1w-0+deb11u2
ii  libsystemd0            247.3-7+deb11u6
ii  libwrap0               7.6.q-31
ii  lsb-base               11.1.0
ii  openssh-client         1:8.4p1-5+deb11u3
ii  openssh-sftp-server    1:8.4p1-5+deb11u3
ii  procps                 2:3.3.17-5
ii  runit-helper           2.10.3
ii  ucf                    3.0043
ii  zlib1g                 1:1.2.11.dfsg-2+deb11u2

Versions of packages openssh-server recommends:
ii  libpam-systemd [logind]  247.3-7+deb11u6
pn  ncurses-term             <none>
ii  xauth                    1:1.1-1

Versions of packages openssh-server suggests:
pn  molly-guard   <none>
pn  monkeysphere  <none>
pn  ssh-askpass   <none>
pn  ufw           <none>

-- debconf information excluded

Reply via email to