upstream: Refactor creation of KEX proposal.

This adds kex_proposal_populate_entries (and corresponding free) which
populates the KEX proposal array with dynamically allocated strings.
This replaces the previous mix of static and dynamic that has been the
source of previous leaks and bugs.  Remove unused compat functions.
With & ok djm@.

OpenBSD-Commit-ID: f2f99da4aae2233cb18bf9c749320c5e040a9c7b

diff --git a/compat.c b/compat.c
index f967fc829f87e10f3d259e40bd89eeff1a775ee8..b59f0bfc06308e3cd790e393fcf50277599bb822 100644 (file)
--- a/compat.c
+++ b/compat.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: compat.c,v 1.125 2023/02/17 04:22:50 dtucker Exp $ */
+/* $OpenBSD: compat.c,v 1.126 2023/03/06 12:14:48 dtucker Exp $ */
 /*
  * Copyright (c) 1999, 2000, 2001, 2002 Markus Friedl.  All rights reserved.
  *
@@ -36,7 +36,6 @@
 #include "compat.h"
 #include "log.h"
 #include "match.h"
-#include "kex.h"
 
 /* determine bug flags from SSH protocol banner */
 void
@@ -140,21 +139,7 @@ compat_banner(struct ssh *ssh, const char *version)
 
 /* Always returns pointer to allocated memory, caller must free. */
 char *
-compat_cipher_proposal(struct ssh *ssh, char *cipher_prop)
-{
-       return xstrdup(cipher_prop);
-}
-
-/* Always returns pointer to allocated memory, caller must free. */
-char *
-compat_pkalg_proposal(struct ssh *ssh, char *pkalg_prop)
-{
-       return xstrdup(pkalg_prop);
-}
-
-/* Always returns pointer to allocated memory, caller must free. */
-char *
-compat_kex_proposal(struct ssh *ssh, char *p)
+compat_kex_proposal(struct ssh *ssh, const char *p)
 {
        char *cp = NULL, *cp2 = NULL;
 

diff --git a/compat.h b/compat.h
index 1da367e84f791e14967d8e9677d7c937da208476..1a19060fc14c9c77b6086fbb4e4e9f6445695460 100644 (file)
--- a/compat.h
+++ b/compat.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: compat.h,v 1.61 2023/02/17 04:22:50 dtucker Exp $ */
+/* $OpenBSD: compat.h,v 1.62 2023/03/06 12:14:48 dtucker Exp $ */
 
 /*
  * Copyright (c) 1999, 2000, 2001 Markus Friedl.  All rights reserved.
@@ -61,7 +61,5 @@
 struct ssh;
 
 void    compat_banner(struct ssh *, const char *);
-char   *compat_cipher_proposal(struct ssh *, char *);
-char   *compat_pkalg_proposal(struct ssh *, char *);
-char   *compat_kex_proposal(struct ssh *, char *);
+char   *compat_kex_proposal(struct ssh *, const char *);
 #endif

diff --git a/kex.c b/kex.c
index fce848fda5288fe8e95f35c6b94536f875360d1e..2ffc789ce192cbdb9af2bc07f4175786aec37837 100644 (file)
--- a/kex.c
+++ b/kex.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: kex.c,v 1.175 2023/02/28 21:31:50 dtucker Exp $ */
+/* $OpenBSD: kex.c,v 1.176 2023/03/06 12:14:48 dtucker Exp $ */
 /*
  * Copyright (c) 2000, 2001 Markus Friedl.  All rights reserved.
  *
@@ -57,10 +57,12 @@
 #include "misc.h"
 #include "dispatch.h"
 #include "monitor.h"
+#include "myproposal.h"
 
 #include "ssherr.h"
 #include "sshbuf.h"
 #include "digest.h"
+#include "xmalloc.h"
 
 /* prototype */
 static int kex_choose_conf(struct ssh *);
@@ -317,6 +319,61 @@ kex_assemble_names(char **listp, const char *def, const char *all)
        return r;
 }
 
+/*
+ * Fill out a proposal array with dynamically allocated values, which may
+ * be modified as required for compatibility reasons.
+ * Any of the options may be NULL, in which case the default is used.
+ * Array contents must be freed by calling kex_proposal_free_entries.
+ */
+void
+kex_proposal_populate_entries(struct ssh *ssh, char *prop[PROPOSAL_MAX],
+    const char *kexalgos, const char *ciphers, const char *macs,
+    const char *comp, const char *hkalgs)
+{
+       const char *defpropserver[PROPOSAL_MAX] = { KEX_SERVER };
+       const char *defpropclient[PROPOSAL_MAX] = { KEX_CLIENT };
+       const char **defprop = ssh->kex->server ? defpropserver : defpropclient;
+       u_int i;
+
+       if (prop == NULL)
+               fatal_f("proposal missing");
+
+       for (i = 0; i < PROPOSAL_MAX; i++) {
+               switch(i) {
+               case PROPOSAL_KEX_ALGS:
+                       prop[i] = compat_kex_proposal(ssh,
+                           kexalgos ? kexalgos : defprop[i]);
+                       break;
+               case PROPOSAL_ENC_ALGS_CTOS:
+               case PROPOSAL_ENC_ALGS_STOC:
+                       prop[i] = xstrdup(ciphers ? ciphers : defprop[i]);
+                       break;
+               case PROPOSAL_MAC_ALGS_CTOS:
+               case PROPOSAL_MAC_ALGS_STOC:
+                       prop[i]  = xstrdup(macs ? macs : defprop[i]);
+                       break;
+               case PROPOSAL_COMP_ALGS_CTOS:
+               case PROPOSAL_COMP_ALGS_STOC:
+                       prop[i] = xstrdup(comp ? comp : defprop[i]);
+                       break;
+               case PROPOSAL_SERVER_HOST_KEY_ALGS:
+                       prop[i] = xstrdup(hkalgs ? hkalgs : defprop[i]);
+                       break;
+               default:
+                       prop[i] = xstrdup(defprop[i]);
+               }
+       }
+}
+
+void
+kex_proposal_free_entries(char *prop[PROPOSAL_MAX])
+{
+       u_int i;
+
+       for (i = 0; i < PROPOSAL_MAX; i++)
+               free(prop[i]);
+}
+
 /* put algorithm proposal into buffer */
 int
 kex_prop2buf(struct sshbuf *b, char *proposal[PROPOSAL_MAX])

diff --git a/kex.h b/kex.h
index c35329501871a3a446c202691aed72aef76910b6..8b54e3f4b9120ecc52a40a243c88980a847b3553 100644 (file)
--- a/kex.h
+++ b/kex.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: kex.h,v 1.117 2022/01/06 21:55:23 djm Exp $ */
+/* $OpenBSD: kex.h,v 1.118 2023/03/06 12:14:48 dtucker Exp $ */
 
 /*
  * Copyright (c) 2000, 2001 Markus Friedl.  All rights reserved.
@@ -182,6 +182,9 @@ int  kex_names_valid(const char *);
 char   *kex_alg_list(char);
 char   *kex_names_cat(const char *, const char *);
 int     kex_assemble_names(char **, const char *, const char *);
+void    kex_proposal_populate_entries(struct ssh *, char *prop[PROPOSAL_MAX],
+    const char *, const char *, const char *, const char *, const char *);
+void    kex_proposal_free_entries(char *prop[PROPOSAL_MAX]);
 
 int     kex_exchange_identification(struct ssh *, int, const char *);
 

diff --git a/sshconnect2.c b/sshconnect2.c
index 5b232e1b195eaa0815a97a4a89a43304b4b6b0a6..03d00d33b7c01dff672e976ce90427b7758eca56 100644 (file)
--- a/sshconnect2.c
+++ b/sshconnect2.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: sshconnect2.c,v 1.363 2023/03/03 02:34:29 dtucker Exp $ */
+/* $OpenBSD: sshconnect2.c,v 1.364 2023/03/06 12:14:48 dtucker Exp $ */
 /*
  * Copyright (c) 2000 Markus Friedl.  All rights reserved.
  * Copyright (c) 2008 Damien Miller.  All rights reserved.
@@ -56,7 +56,6 @@
 #include "cipher.h"
 #include "sshkey.h"
 #include "kex.h"
-#include "myproposal.h"
 #include "sshconnect.h"
 #include "authfile.h"
 #include "dh.h"
@@ -221,24 +220,17 @@ void
 ssh_kex2(struct ssh *ssh, char *host, struct sockaddr *hostaddr, u_short port,
     const struct ssh_conn_info *cinfo)
 {
-       char *myproposal[PROPOSAL_MAX] = { KEX_CLIENT };
-       char *s, *all_key;
-       char *prop_kex = NULL, *prop_enc = NULL, *prop_hostkey = NULL;
-       int r, use_known_hosts_order = 0;
+       char *myproposal[PROPOSAL_MAX];
+       char *s, *all_key, *hkalgs = NULL;
+       int r;
 
        xxx_host = host;
        xxx_hostaddr = hostaddr;
        xxx_conn_info = cinfo;
 
-       /*
-        * If the user has not specified HostkeyAlgorithms, or has only
-        * appended or removed algorithms from that list then prefer algorithms
-        * that are in the list that are supported by known_hosts keys.
-        */
-       if (options.hostkeyalgorithms == NULL ||
-           options.hostkeyalgorithms[0] == '-' ||
-           options.hostkeyalgorithms[0] == '+')
-               use_known_hosts_order = 1;
+       if (options.rekey_limit || options.rekey_interval)
+               ssh_packet_set_rekey_limits(ssh, options.rekey_limit,
+                   options.rekey_interval);
 
        /* Expand or fill in HostkeyAlgorithms */
        all_key = sshkey_alg_list(0, 0, 1, ',');
@@ -249,29 +241,22 @@ ssh_kex2(struct ssh *ssh, char *host, struct sockaddr *hostaddr, u_short port,
 
        if ((s = kex_names_cat(options.kex_algorithms, "ext-info-c")) == NULL)
                fatal_f("kex_names_cat");
-       myproposal[PROPOSAL_KEX_ALGS] = prop_kex = compat_kex_proposal(ssh, s);
-       myproposal[PROPOSAL_ENC_ALGS_CTOS] =
-           myproposal[PROPOSAL_ENC_ALGS_STOC] = prop_enc =
-           compat_cipher_proposal(ssh, options.ciphers);
-       myproposal[PROPOSAL_COMP_ALGS_CTOS] =
-           myproposal[PROPOSAL_COMP_ALGS_STOC] =
-           (char *)compression_alg_list(options.compression);
-       myproposal[PROPOSAL_MAC_ALGS_CTOS] =
-           myproposal[PROPOSAL_MAC_ALGS_STOC] = options.macs;
-       if (use_known_hosts_order) {
-               /* Query known_hosts and prefer algorithms that appear there */
-               myproposal[PROPOSAL_SERVER_HOST_KEY_ALGS] = prop_hostkey =
-                   compat_pkalg_proposal(ssh,
-                   order_hostkeyalgs(host, hostaddr, port, cinfo));
-       } else {
-               /* Use specified HostkeyAlgorithms exactly */
-               myproposal[PROPOSAL_SERVER_HOST_KEY_ALGS] = prop_hostkey =
-                   compat_pkalg_proposal(ssh, options.hostkeyalgorithms);
-       }
 
-       if (options.rekey_limit || options.rekey_interval)
-               ssh_packet_set_rekey_limits(ssh, options.rekey_limit,
-                   options.rekey_interval);
+       /*
+        * If the user has not specified HostkeyAlgorithms, or has only
+        * appended or removed algorithms from that list then prefer algorithms
+        * that are in the list that are supported by known_hosts keys.
+        */
+       if (options.hostkeyalgorithms == NULL ||
+           options.hostkeyalgorithms[0] == '-' ||
+           options.hostkeyalgorithms[0] == '+')
+               hkalgs = order_hostkeyalgs(host, hostaddr, port, cinfo);
+
+       kex_proposal_populate_entries(ssh, myproposal, s, options.ciphers,
+           options.macs, compression_alg_list(options.compression),
+           hkalgs ? hkalgs : options.hostkeyalgorithms);
+
+       free(hkalgs);
 
        /* start key exchange */
        if ((r = kex_setup(ssh, myproposal)) != 0)
@@ -295,6 +280,7 @@ ssh_kex2(struct ssh *ssh, char *host, struct sockaddr *hostaddr, u_short port,
        ssh_dispatch_run_fatal(ssh, DISPATCH_BLOCK, &ssh->kex->done);
 
        /* remove ext-info from the KEX proposals for rekeying */
+       free(myproposal[PROPOSAL_KEX_ALGS]);
        myproposal[PROPOSAL_KEX_ALGS] =
            compat_kex_proposal(ssh, options.kex_algorithms);
        if ((r = kex_prop2buf(ssh->kex->my, myproposal)) != 0)
@@ -308,10 +294,7 @@ ssh_kex2(struct ssh *ssh, char *host, struct sockaddr *hostaddr, u_short port,
            (r = ssh_packet_write_wait(ssh)) != 0)
                fatal_fr(r, "send packet");
 #endif
-       /* Free only parts of proposal that were dynamically allocated here. */
-       free(prop_kex);
-       free(prop_enc);
-       free(prop_hostkey);
+       kex_proposal_free_entries(myproposal);
 }
 
 /*

diff --git a/sshd.c b/sshd.c
index 748c15ee2712a6e6b4323969f464cfb7fabb7ee1..c45092ea4dfd92071920fe9993da2d731e0e46af 100644 (file)
--- a/sshd.c
+++ b/sshd.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: sshd.c,v 1.598 2023/03/03 03:12:24 dtucker Exp $ */
+/* $OpenBSD: sshd.c,v 1.599 2023/03/06 12:14:48 dtucker Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -104,7 +104,6 @@
 #include "digest.h"
 #include "sshkey.h"
 #include "kex.h"
-#include "myproposal.h"
 #include "authfile.h"
 #include "pathnames.h"
 #include "atomicio.h"
@@ -2389,30 +2388,23 @@ sshd_hostkey_sign(struct ssh *ssh, struct sshkey *privkey,
 static void
 do_ssh2_kex(struct ssh *ssh)
 {
-       char *myproposal[PROPOSAL_MAX] = { KEX_SERVER };
+       char *hkalgs = NULL, *myproposal[PROPOSAL_MAX];
+       const char *compression = NULL;
        struct kex *kex;
-       char *prop_kex = NULL, *prop_enc = NULL, *prop_hostkey = NULL;
        int r;
 
-       myproposal[PROPOSAL_KEX_ALGS] = prop_kex = compat_kex_proposal(ssh,
-           options.kex_algorithms);
-       myproposal[PROPOSAL_ENC_ALGS_CTOS] =
-           myproposal[PROPOSAL_ENC_ALGS_STOC] = prop_enc =
-           compat_cipher_proposal(ssh, options.ciphers);
-       myproposal[PROPOSAL_MAC_ALGS_CTOS] =
-           myproposal[PROPOSAL_MAC_ALGS_STOC] = options.macs;
-
-       if (options.compression == COMP_NONE) {
-               myproposal[PROPOSAL_COMP_ALGS_CTOS] =
-                   myproposal[PROPOSAL_COMP_ALGS_STOC] = "none";
-       }
-
        if (options.rekey_limit || options.rekey_interval)
                ssh_packet_set_rekey_limits(ssh, options.rekey_limit,
                    options.rekey_interval);
 
-       myproposal[PROPOSAL_SERVER_HOST_KEY_ALGS] = prop_hostkey =
-          compat_pkalg_proposal(ssh, list_hostkey_types());
+       if (options.compression == COMP_NONE)
+               compression = "none";
+       hkalgs = list_hostkey_types();
+
+       kex_proposal_populate_entries(ssh, myproposal, options.kex_algorithms,
+           options.ciphers, options.macs, compression, hkalgs);
+
+       free(hkalgs);
 
        /* start key exchange */
        if ((r = kex_setup(ssh, myproposal)) != 0)
@@ -2447,9 +2439,7 @@ do_ssh2_kex(struct ssh *ssh)
            (r = ssh_packet_write_wait(ssh)) != 0)
                fatal_fr(r, "send test");
 #endif
-       free(prop_kex);
-       free(prop_enc);
-       free(prop_hostkey);
+       kex_proposal_free_entries(myproposal);
        debug("KEX done");
 }