ssh-keyscan bug (not really exploitable)

[Joachim Schipper]

ssh-keyscan may, under very specific circumstances, be vulnerable to
something akin to a buffer overflow.  It's probably impossible to
exploit, though, if only because ssh-keyscan is not usually run on very
large untrusted input files.

ssh-keyscan uses an fgets() wrapper that uses an unsigned int to keep
track of the length of a buffer holding the current line. On machines
with sufficient address space to hold UINT_MAX / 2 bytes, e.g. amd64,
one can fill those bytes. The next call will be realloc(buf, 0), which
free()s the buffer and returns a non-NULL zero-size chunk of memory. The
program will then try to write to some point in memory about UINT_MAX /
2 past this newly-returned chunk.

Test case:

$ while true; do echo -n 'AAAAAAAAAAAAAAAA'; done | ssh-keyscan -f -

I stumbled upon this a while ago; I was trying to solve
https://bugzilla.mindrot.org/show_bug.cgi?id=1565 ("ssh-keyscan doesn't
like comment-lines"). The patch below rips out the fgets() wrapper and
correctly handles both comments and ridiculously long lines.

		Joachim

Index: ssh-keyscan.c
===================================================================
RCS file: /usr/cvs/src/src/usr.bin/ssh/ssh-keyscan.c,v
retrieving revision 1.81
diff -u -p -r1.81 ssh-keyscan.c
--- ssh-keyscan.c	9 Jan 2010 23:04:13 -0000	1.81
+++ ssh-keyscan.c	6 Mar 2010 20:33:34 -0000
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssh-keyscan.c,v 1.78 2009/01/22 10:02:34 djm Exp $ */
+/* $OpenBSD: ssh-keyscan.c,v 1.81 2010/01/09 23:04:13 dtucker Exp $ */
 /*
  * Copyright 1995, 1996 by David Mazieres <dm at lcs.mit.edu>.
  *
@@ -99,122 +99,6 @@ typedef struct Connection {
 TAILQ_HEAD(conlist, Connection) tq;	/* Timeout Queue */
 con *fdcon;
 
-/*
- *  This is just a wrapper around fgets() to make it usable.
- */
-
-/* Stress-test.  Increase this later. */
-#define LINEBUF_SIZE 16
-
-typedef struct {
-	char *buf;
-	u_int size;
-	int lineno;
-	const char *filename;
-	FILE *stream;
-	void (*errfun) (const char *,...);
-} Linebuf;
-
-static Linebuf *
-Linebuf_alloc(const char *filename, void (*errfun) (const char *,...))
-{
-	Linebuf *lb;
-
-	if (!(lb = malloc(sizeof(*lb)))) {
-		if (errfun)
-			(*errfun) ("linebuf (%s): malloc failed\n",
-			    filename ? filename : "(stdin)");
-		return (NULL);
-	}
-	if (filename) {
-		lb->filename = filename;
-		if (!(lb->stream = fopen(filename, "r"))) {
-			xfree(lb);
-			if (errfun)
-				(*errfun) ("%s: %s\n", filename, strerror(errno));
-			return (NULL);
-		}
-	} else {
-		lb->filename = "(stdin)";
-		lb->stream = stdin;
-	}
-
-	if (!(lb->buf = malloc((lb->size = LINEBUF_SIZE)))) {
-		if (errfun)
-			(*errfun) ("linebuf (%s): malloc failed\n", lb->filename);
-		xfree(lb);
-		return (NULL);
-	}
-	lb->errfun = errfun;
-	lb->lineno = 0;
-	return (lb);
-}
-
-static void
-Linebuf_free(Linebuf * lb)
-{
-	fclose(lb->stream);
-	xfree(lb->buf);
-	xfree(lb);
-}
-
-#if 0
-static void
-Linebuf_restart(Linebuf * lb)
-{
-	clearerr(lb->stream);
-	rewind(lb->stream);
-	lb->lineno = 0;
-}
-
-static int
-Linebuf_lineno(Linebuf * lb)
-{
-	return (lb->lineno);
-}
-#endif
-
-static char *
-Linebuf_getline(Linebuf * lb)
-{
-	size_t n = 0;
-	void *p;
-
-	lb->lineno++;
-	for (;;) {
-		/* Read a line */
-		if (!fgets(&lb->buf[n], lb->size - n, lb->stream)) {
-			if (ferror(lb->stream) && lb->errfun)
-				(*lb->errfun)("%s: %s\n", lb->filename,
-				    strerror(errno));
-			return (NULL);
-		}
-		n = strlen(lb->buf);
-
-		/* Return it or an error if it fits */
-		if (n > 0 && lb->buf[n - 1] == '\n') {
-			lb->buf[n - 1] = '\0';
-			return (lb->buf);
-		}
-		if (n != lb->size - 1) {
-			if (lb->errfun)
-				(*lb->errfun)("%s: skipping incomplete last line\n",
-				    lb->filename);
-			return (NULL);
-		}
-		/* Double the buffer if we need more space */
-		lb->size *= 2;
-		if ((p = realloc(lb->buf, lb->size)) == NULL) {
-			lb->size /= 2;
-			if (lb->errfun)
-				(*lb->errfun)("linebuf (%s): realloc failed\n",
-				    lb->filename);
-			return (NULL);
-		}
-		lb->buf = p;
-	}
-}
-
 static int
 fdlim_get(int hard)
 {
@@ -709,8 +593,10 @@ int
 main(int argc, char **argv)
 {
 	int debug_flag = 0, log_level = SYSLOG_LEVEL_INFO;
-	int opt, fopt_count = 0;
-	char *tname;
+	int opt, fopt_count = 0, j;
+	char *tname, *line;
+	size_t i, line_len;
+	FILE *fp;
 
 	extern int optind;
 	extern char *optarg;
@@ -808,20 +694,52 @@ main(int argc, char **argv)
 	read_wait_nfdset = howmany(maxfd, NFDBITS);
 	read_wait = xcalloc(read_wait_nfdset, sizeof(fd_mask));
 
-	if (fopt_count) {
-		Linebuf *lb;
-		char *line;
-		int j;
-
-		for (j = 0; j < fopt_count; j++) {
-			lb = Linebuf_alloc(argv[j], error);
-			if (!lb)
+	line = NULL;
+
+	for (j = 0; j < fopt_count; j++) {
+		if (line == NULL)
+			line = xmalloc(line_len = BUFSIZ);
+
+		if ((fp = fopen(argv[j], "r")) == NULL)
+			fatal("%s: %s: %s", __progname, argv[j],
+			    strerror(errno));
+
+		i = 0;
+		while (fgets(&line[i], line_len - i, fp)) {
+			/* Read a line */
+			i += strcspn(&line[i], "\n");
+			if (line[i] == '\0' && i == line_len - 1) {
+				if (line_len > SIZE_MAX / 2)
+					fatal("%s: %s: line %.20s too long",
+					    __progname, argv[j], line);
+				line = xrealloc(line, line_len *= 2, 1);
+				continue;
+			}
+
+			/* Strip off comments and whitespace at end */
+			for (i = strcspn(line, "#\n");
+			     i > 0 && strchr(" \t", line[i - 1]);
+			     i--);
+			line[i] = '\0';
+
+			/* Skip empty lines, comments */
+			if (i == 0)
 				continue;
-			while ((line = Linebuf_getline(lb)) != NULL)
-				do_host(line);
-			Linebuf_free(lb);
+
+			do_host(line);
+
+			i = 0;
 		}
+
+		if (ferror(fp))
+			fatal("%s: %s: %s", __progname, argv[j],
+			    strerror(errno));
+
+		fclose(fp);
 	}
+
+	if (line)
+		xfree(line);
 
 	while (optind < argc)
 		do_host(argv[optind++]);