From 0e5f17167d21ffd89fe564ffd75c5fe698a729a0 Mon Sep 17 00:00:00 2001
From: djm <djm@openbsd.org>
Date: Fri, 19 Mar 2021 02:18:28 +0000
Subject: [PATCH] increase maximum SSH2_FXP_READ to match the maximum packet
 size. Also handle zero-length reads that are borderline nonsensical but not
 explicitly banned by the spec. Based on patch from Mike Frysinger, feedback
 deraadt@ ok dtucker@

---
 usr.bin/ssh/sftp-server.c | 62 ++++++++++++++++++++++++---------------
 1 file changed, 38 insertions(+), 24 deletions(-)

diff --git a/usr.bin/ssh/sftp-server.c b/usr.bin/ssh/sftp-server.c
index da053cd7567..9a3d6733641 100644
--- a/usr.bin/ssh/sftp-server.c
+++ b/usr.bin/ssh/sftp-server.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: sftp-server.c,v 1.123 2021/03/16 06:15:43 djm Exp $ */
+/* $OpenBSD: sftp-server.c,v 1.124 2021/03/19 02:18:28 djm Exp $ */
 /*
  * Copyright (c) 2000-2004 Markus Friedl.  All rights reserved.
  *
@@ -47,7 +47,7 @@
 char *sftp_realpath(const char *, char *); /* sftp-realpath.c */
 
 /* Maximum data read that we are willing to accept */
-#define SFTP_MAX_READ_LENGTH (64 * 1024)
+#define SFTP_MAX_READ_LENGTH (SFTP_MAX_MSG_LENGTH - 1024)
 
 /* Our verbosity */
 static LogLevel log_level = SYSLOG_LEVEL_ERROR;
@@ -740,7 +740,8 @@ process_close(u_int32_t id)
 static void
 process_read(u_int32_t id)
 {
-	u_char buf[SFTP_MAX_READ_LENGTH];
+	static u_char *buf;
+	static size_t buflen;
 	u_int32_t len;
 	int r, handle, fd, ret, status = SSH2_FX_FAILURE;
 	u_int64_t off;
@@ -750,30 +751,43 @@ process_read(u_int32_t id)
 	    (r = sshbuf_get_u32(iqueue, &len)) != 0)
 		fatal_fr(r, "parse");
 
-	debug("request %u: read \"%s\" (handle %d) off %llu len %d",
+	debug("request %u: read \"%s\" (handle %d) off %llu len %u",
 	    id, handle_to_name(handle), handle, (unsigned long long)off, len);
-	if (len > sizeof buf) {
-		len = sizeof buf;
-		debug2("read change len %d", len);
+	if ((fd = handle_to_fd(handle)) == -1)
+		goto out;
+	if (len > SFTP_MAX_READ_LENGTH) {
+		debug2("read change len %u to %u", len, SFTP_MAX_READ_LENGTH);
+		len = SFTP_MAX_READ_LENGTH;
 	}
-	fd = handle_to_fd(handle);
-	if (fd >= 0) {
-		if (lseek(fd, off, SEEK_SET) == -1) {
-			error("process_read: seek failed");
-			status = errno_to_portable(errno);
-		} else {
-			ret = read(fd, buf, len);
-			if (ret == -1) {
-				status = errno_to_portable(errno);
-			} else if (ret == 0) {
-				status = SSH2_FX_EOF;
-			} else {
-				send_data(id, buf, ret);
-				status = SSH2_FX_OK;
-				handle_update_read(handle, ret);
-			}
-		}
+	if (len > buflen) {
+		debug3_f("allocate %zu => %u", buflen, len);
+		if ((buf = realloc(NULL, len)) == NULL)
+			fatal_f("realloc failed");
+		buflen = len;
 	}
+	if (lseek(fd, off, SEEK_SET) == -1) {
+		status = errno_to_portable(errno);
+		error_f("seek \"%.100s\": %s", handle_to_name(handle),
+		    strerror(errno));
+		goto out;
+	}
+	if (len == 0) {
+		/* weird, but not strictly disallowed */
+		ret = 0;
+	} else if ((ret = read(fd, buf, len)) == -1) {
+		status = errno_to_portable(errno);
+		error_f("read \"%.100s\": %s", handle_to_name(handle),
+		    strerror(errno));
+		goto out;
+	} else if (ret == 0) {
+		status = SSH2_FX_EOF;
+		goto out;
+	}
+	send_data(id, buf, ret);
+	handle_update_read(handle, ret);
+	/* success */
+	status = SSH2_FX_OK;
+ out:
 	if (status != SSH2_FX_OK)
 		send_status(id, status);
 }
-- 
2.20.1