[openssh-commits] [openssh] 01/04: upstream: make reading server banner non-blocking; prevents one

[git+noreply at mindrot.org]

This is an automated email from the git hooks/post-receive script.

djm pushed a commit to branch master
in repository openssh.

commit 407a2b79d439b1272879a910db6c94d1165075a8
Author: djm at openbsd.org <djm at openbsd.org>
AuthorDate: Mon Aug 10 23:24:03 2026 +0000

    upstream: make reading server banner non-blocking; prevents one
    
    stuck server from blocking a many-host keyscan; from Thomas Yiu
    
    ok dtucker@ markus@
    
    OpenBSD-Commit-ID: 4970af00975119ebf9c1e0e132e3317b72ac0c2e
---
 ssh-keyscan.c | 98 +++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 55 insertions(+), 43 deletions(-)

diff --git a/ssh-keyscan.c b/ssh-keyscan.c
index 8f52f7e6e..f093b0108 100644
--- a/ssh-keyscan.c
+++ b/ssh-keyscan.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssh-keyscan.c,v 1.169 2026/07/27 12:28:52 markus Exp $ */
+/* $OpenBSD: ssh-keyscan.c,v 1.170 2026/08/10 23:24:03 djm Exp $ */
 /*
  * Copyright 1995, 1996 by David Mazieres <dm at lcs.mit.edu>.
  *
@@ -110,6 +110,9 @@ typedef struct Connection {
 	char *c_namelist;	/* Pointer to other possible addresses */
 	char *c_output_name;	/* Hostname of connection for output */
 	struct ssh *c_ssh;	/* SSH-connection */
+	char c_banner[256];	/* Partial server greeting */
+	size_t c_banner_len;	/* Length of partial server greeting */
+	int c_banner_sent;	/* Client greeting has been sent */
 	struct timespec c_ts;	/* Time at which connection gets aborted */
 	TAILQ_ENTRY(Connection) c_link;	/* List of connections in timeout order. */
 } con;
@@ -413,6 +416,8 @@ conalloc(const char *iname, const char *oname, int keytype)
 	fdcon[s].c_namelist = namelist;
 	fdcon[s].c_output_name = xstrdup(oname);
 	fdcon[s].c_keytype = keytype;
+	fdcon[s].c_banner_len = 0;
+	fdcon[s].c_banner_sent = 0;
 	monotime_ts(&fdcon[s].c_ts);
 	fdcon[s].c_ts.tv_sec += timeout;
 	TAILQ_INSERT_TAIL(&tq, &fdcon[s], c_link);
@@ -457,52 +462,48 @@ conrecycle(int s)
 static void
 congreet(int s)
 {
-	int n = 0, remote_major = 0, remote_minor = 0;
-	char buf[256], *cp;
+	int remote_major = 0, remote_minor = 0;
+	ssize_t n;
+	char buf[256], ch;
 	char remote_version[sizeof buf];
-	size_t bufsiz;
 	con *c = &fdcon[s];
 
-	/* send client banner */
-	n = snprintf(buf, sizeof buf, "SSH-%d.%d-OpenSSH-keyscan\r\n",
-	    PROTOCOL_MAJOR_2, PROTOCOL_MINOR_2);
-	if (n < 0 || (size_t)n >= sizeof(buf)) {
-		error("snprintf: buffer too small");
-		confree(s);
-		return;
-	}
-	if (atomicio(vwrite, s, buf, n) != (size_t)n) {
-		error("write (%s): %s", c->c_name, strerror(errno));
-		confree(s);
-		return;
+	if (!c->c_banner_sent) {
+		/* send client banner */
+		n = snprintf(buf, sizeof buf, "SSH-%d.%d-OpenSSH-keyscan\r\n",
+		    PROTOCOL_MAJOR_2, PROTOCOL_MINOR_2);
+		if (n < 0 || (size_t)n >= sizeof(buf)) {
+			error("snprintf: buffer too small");
+			confree(s);
+			return;
+		}
+		if (atomicio(vwrite, s, buf, n) != (size_t)n) {
+			error("write (%s): %s", c->c_name, strerror(errno));
+			confree(s);
+			return;
+		}
+		c->c_banner_sent = 1;
 	}
 
 	/*
 	 * Read the server banner as per RFC4253 section 4.2.  The "SSH-"
 	 * protocol identification string may be preceded by an arbitrarily
-	 * large banner which we must read and ignore.  Loop while reading
-	 * newline-terminated lines until we have one starting with "SSH-".
-	 * The ID string cannot be longer than 255 characters although the
-	 * preceding banner lines may (in which case they'll be discarded
-	 * in multiple iterations of the outer loop).
+	 * large banner which we must read and ignore.  Read a single byte
+	 * at a time so that the event loop retains control of connection
+	 * timeouts.  Partial lines are retained in the connection state
+	 * between calls.
 	 */
-	for (;;) {
-		memset(buf, '\0', sizeof(buf));
-		bufsiz = sizeof(buf);
-		cp = buf;
-		while (bufsiz-- &&
-		    (n = atomicio(read, s, cp, 1)) == 1 && *cp != '\n') {
-			if (*cp == '\r')
-				*cp = '\n';
-			cp++;
-		}
-		if (n != 1 || strncmp(buf, "SSH-", 4) == 0)
-			break;
-	}
-	if (n == 0) {
+	n = read(s, &ch, sizeof(ch));
+	if (n == -1 && (errno == EAGAIN || errno == EWOULDBLOCK ||
+	    errno == EINTR))
+		return;
+	if (n != 1) {
+		if (n == 0)
+			errno = EPIPE;
 		switch (errno) {
 		case EPIPE:
-			error("%s: Connection closed by remote host", c->c_name);
+			error("%s: Connection closed by remote host",
+			    c->c_name);
 			break;
 		case ECONNREFUSED:
 			break;
@@ -513,17 +514,28 @@ congreet(int s)
 		conrecycle(s);
 		return;
 	}
-	if (cp >= buf + sizeof(buf)) {
-		error("%s: greeting exceeds allowable length", c->c_name);
-		confree(s);
+	c->c_banner[c->c_banner_len++] = ch == '\r' ? '\n' : ch;
+	if (ch != '\n') {
+		if (c->c_banner_len < sizeof(c->c_banner) - 1)
+			return; /* incomplete line */
+		if (strncmp(c->c_banner, "SSH-", 4) == 0) {
+			/* banner exceeds RFC 4253 s4.2 length limit */
+			error("%s: greeting exceeds allowable length",
+			    c->c_name);
+			confree(s);
+			return;
+		}
+		/* Discard an oversized pre-identification banner chunk. */
+		c->c_banner_len = 0;
 		return;
 	}
-	if (*cp != '\n' && *cp != '\r') {
-		error("%s: bad greeting", c->c_name);
-		confree(s);
+	if (strncmp(c->c_banner, "SSH-", 4) != 0) {
+		/* Ignore non-banner lines */
+		c->c_banner_len = 0;
 		return;
 	}
-	*cp = '\0';
+	c->c_banner[c->c_banner_len++] = '\0';
+	memcpy(buf, c->c_banner, c->c_banner_len);
 	if ((c->c_ssh = ssh_packet_set_connection(NULL, s, s)) == NULL)
 		fatal("ssh_packet_set_connection failed");
 	ssh_packet_set_timeout(c->c_ssh, timeout, 1);

-- 
To stop receiving notification emails like this one, please contact
djm at mindrot.org.