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
diff --git a/ssh_api.c b/ssh_api.c
index 53bbc9b..ab209c4 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 @@
/* 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 @@
* 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 @@
/* 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 @@
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 @@
}
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;
- chop(buf);
- debug("Local version string %.100s", buf);
- if ((*bannerp = strdup(buf)) == NULL)
+ 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;
+ 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 @@
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;