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 53bbc9b4..ab209c4c 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;