upstream: move client/server SSH-* banners to buffers under

ssh->kex and factor out the banner exchange. This eliminates some common code
from the client and server.

Also be more strict about handling \r characters - these should only
be accepted immediately before \n (pointed out by Jann Horn).

Inspired by a patch from Markus Schmidt.
(lots of) feedback and ok markus@

OpenBSD-Commit-ID: 1cc7885487a6754f63641d7d3279b0941890275b

1 files changed, 70 insertions, 55 deletions

diff --git a/ssh_api.c b/ssh_api.c
index 53bbc9b49..ab209c4ca 100644
--- a/ssh_api.c
+++ b/ssh_api.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssh_api.c,v 1.8 2017/04/30 23:13:25 djm Exp $ */
+/* $OpenBSD: ssh_api.c,v 1.9 2018/12/27 03:25:25 djm Exp $ */
 /*
  * Copyright (c) 2012 Markus Friedl.  All rights reserved.
  *
@@ -34,8 +34,8 @@
 #include <string.h>
 
 int     _ssh_exchange_banner(struct ssh *);
-int     _ssh_send_banner(struct ssh *, char **);
-int     _ssh_read_banner(struct ssh *, char **);
+int     _ssh_send_banner(struct ssh *, struct sshbuf *);
+int     _ssh_read_banner(struct ssh *, struct sshbuf *);
 int     _ssh_order_hostkeyalgs(struct ssh *);
 int     _ssh_verify_host_key(struct sshkey *, struct ssh *);
 struct sshkey *_ssh_host_public_key(int, int, struct ssh *);
@@ -92,7 +92,7 @@ ssh_init(struct ssh **sshp, int is_server, struct kex_params *kex_params)
 
         /* Initialize key exchange */
         proposal = kex_params ? kex_params->proposal : myproposal;
-        if ((r = kex_new(ssh, proposal, &ssh->kex)) != 0) {
+        if ((r = kex_ready(ssh, proposal)) != 0) {
                 ssh_free(ssh);
                 return r;
         }
@@ -236,8 +236,8 @@ ssh_packet_next(struct ssh *ssh, u_char *typep)
          * enough data.
          */
         *typep = SSH_MSG_NONE;
-        if (ssh->kex->client_version_string == NULL ||
-            ssh->kex->server_version_string == NULL)
+        if (sshbuf_len(ssh->kex->client_version) == 0 ||
+            sshbuf_len(ssh->kex->server_version) == 0)
                 return _ssh_exchange_banner(ssh);
         /*
          * If we enough data and a dispatch function then
@@ -312,39 +312,46 @@ ssh_input_space(struct ssh *ssh, size_t len)
 
 /* Read other side's version identification. */
 int
-_ssh_read_banner(struct ssh *ssh, char **bannerp)
+_ssh_read_banner(struct ssh *ssh, struct sshbuf *banner)
 {
-        struct sshbuf *input;
-        const char *s;
-        char buf[256], remote_version[256];     /* must be same size! */
+        struct sshbuf *input = ssh_packet_get_input(ssh);
         const char *mismatch = "Protocol mismatch.\r\n";
-        int r, remote_major, remote_minor;
-        size_t i, n, j, len;
+        const u_char *s = sshbuf_ptr(input);
+        u_char c;
+        char *cp, *remote_version;
+        int r, remote_major, remote_minor, expect_nl;
+        size_t n, j;
 
-        *bannerp = NULL;
-        input = ssh_packet_get_input(ssh);
-        len = sshbuf_len(input);
-        s = (const char *)sshbuf_ptr(input);
         for (j = n = 0;;) {
-                for (i = 0; i < sizeof(buf) - 1; i++) {
-                        if (j >= len)
-                                return (0);
-                        buf[i] = s[j++];
-                        if (buf[i] == '\r') {
-                                buf[i] = '\n';
-                                buf[i + 1] = 0;
-                                continue;               /**XXX wait for \n */
+                sshbuf_reset(banner);
+                expect_nl = 0;
+                for (;;) {
+                        if (j >= sshbuf_len(input))
+                                return 0; /* insufficient data in input buf */
+                        c = s[j++];
+                        if (c == '\r') {
+                                expect_nl = 1;
+                                continue;
                         }
-                        if (buf[i] == '\n') {
-                                buf[i + 1] = 0;
+                        if (c == '\n')
                                 break;
-                        }
+                        if (expect_nl)
+                                goto bad;
+                        if ((r = sshbuf_put_u8(banner, c)) != 0)
+                                return r;
+                        if (sshbuf_len(banner) > SSH_MAX_BANNER_LEN)
+                                goto bad;
                 }
-                buf[sizeof(buf) - 1] = 0;
-                if (strncmp(buf, "SSH-", 4) == 0)
+                if (sshbuf_len(banner) >= 4 &&
+                    memcmp(sshbuf_ptr(banner), "SSH-", 4) == 0)
                         break;
-                debug("ssh_exchange_identification: %s", buf);
-                if (ssh->kex->server || ++n > 65536) {
+                if ((cp = sshbuf_dup_string(banner)) == NULL)
+                        return SSH_ERR_ALLOC_FAIL;
+                debug("%s: %s", __func__, cp);
+                free(cp);
+                /* Accept lines before banner only on client */
+                if (ssh->kex->server || ++n > SSH_MAX_PRE_BANNER_LINES) {
+  bad:
                         if ((r = sshbuf_put(ssh_packet_get_output(ssh),
                            mismatch, strlen(mismatch))) != 0)
                                 return r;
@@ -354,11 +361,17 @@ _ssh_read_banner(struct ssh *ssh, char **bannerp)
         if ((r = sshbuf_consume(input, j)) != 0)
                 return r;
 
+        if ((cp = sshbuf_dup_string(banner)) == NULL)
+                return SSH_ERR_ALLOC_FAIL;
+        /* XXX remote version must be the same size as banner for sscanf */
+        if ((remote_version = calloc(1, sshbuf_len(banner))) == NULL)
+                return SSH_ERR_ALLOC_FAIL;
+
         /*
          * Check that the versions match.  In future this might accept
          * several versions and set appropriate flags to handle them.
          */
-        if (sscanf(buf, "SSH-%d.%d-%[^\n]\n",
+        if (sscanf(cp, "SSH-%d.%d-%[^\n]\n",
             &remote_major, &remote_minor, remote_version) != 3)
                 return SSH_ERR_INVALID_FORMAT;
         debug("Remote protocol version %d.%d, remote software version %.100s",
@@ -371,27 +384,29 @@ _ssh_read_banner(struct ssh *ssh, char **bannerp)
         }
         if (remote_major != 2)
                 return SSH_ERR_PROTOCOL_MISMATCH;
-        chop(buf);
-        debug("Remote version string %.100s", buf);
-        if ((*bannerp = strdup(buf)) == NULL)
-                return SSH_ERR_ALLOC_FAIL;
+        debug("Remote version string %.100s", cp);
+        free(cp);
         return 0;
 }
 
 /* Send our own protocol version identification. */
 int
-_ssh_send_banner(struct ssh *ssh, char **bannerp)
+_ssh_send_banner(struct ssh *ssh, struct sshbuf *banner)
 {
-        char buf[256];
+        char *cp;
         int r;
 
-        snprintf(buf, sizeof buf, "SSH-2.0-%.100s\r\n", SSH_VERSION);
-        if ((r = sshbuf_put(ssh_packet_get_output(ssh), buf, strlen(buf))) != 0)
+        if ((r = sshbuf_putf(banner, "SSH-2.0-%.100s\r\n", SSH_VERSION)) != 0)
+                return r;
+        if ((r = sshbuf_putb(ssh_packet_get_output(ssh), banner)) != 0)
+                return r;
+        /* Remove trailing \r\n */
+        if ((r = sshbuf_consume_end(banner, 2)) != 0)
                 return r;
-        chop(buf);
-        debug("Local version string %.100s", buf);
-        if ((*bannerp = strdup(buf)) == NULL)
+        if ((cp = sshbuf_dup_string(banner)) == NULL)
                 return SSH_ERR_ALLOC_FAIL;
+        debug("Local version string %.100s", cp);
+        free(cp);
         return 0;
 }
 
@@ -408,25 +423,25 @@ _ssh_exchange_banner(struct ssh *ssh)
 
         r = 0;
         if (kex->server) {
-                if (kex->server_version_string == NULL)
-                        r = _ssh_send_banner(ssh, &kex->server_version_string);
+                if (sshbuf_len(ssh->kex->server_version) == 0)
+                        r = _ssh_send_banner(ssh, ssh->kex->server_version);
                 if (r == 0 &&
-                    kex->server_version_string != NULL &&
-                    kex->client_version_string == NULL)
-                        r = _ssh_read_banner(ssh, &kex->client_version_string);
+                    sshbuf_len(ssh->kex->server_version) != 0 &&
+                    sshbuf_len(ssh->kex->client_version) == 0)
+                        r = _ssh_read_banner(ssh, ssh->kex->client_version);
         } else {
-                if (kex->server_version_string == NULL)
-                        r = _ssh_read_banner(ssh, &kex->server_version_string);
+                if (sshbuf_len(ssh->kex->server_version) == 0)
+                        r = _ssh_read_banner(ssh, ssh->kex->server_version);
                 if (r == 0 &&
-                    kex->server_version_string != NULL &&
-                    kex->client_version_string == NULL)
-                        r = _ssh_send_banner(ssh, &kex->client_version_string);
+                    sshbuf_len(ssh->kex->server_version) != 0 &&
+                    sshbuf_len(ssh->kex->client_version) == 0)
+                        r = _ssh_send_banner(ssh, ssh->kex->client_version);
         }
         if (r != 0)
                 return r;
         /* start initial kex as soon as we have exchanged the banners */
-        if (kex->server_version_string != NULL &&
-            kex->client_version_string != NULL) {
+        if (sshbuf_len(ssh->kex->server_version) != 0 &&
+            sshbuf_len(ssh->kex->client_version) != 0) {
                 if ((r = _ssh_order_hostkeyalgs(ssh)) != 0 ||
                     (r = kex_send_kexinit(ssh)) != 0)
                         return r;