Fix out-of-bounds reads parsing ftp:// wrapper server replies

The ftp:// wrapper parses replies in a 512-byte buffer at fixed offsets
and sizes that are not tied to the line actually received:

- get_ftp_result() returns the code of the last line read when the
  stream ends, even if it is not a complete "ddd " status line, while
  the callers parse the reply text from tmp_line + 4.
- php_fopen_do_pasv() copies a fixed ip_size bytes from the dotted
  address inside the buffer, reading past its end when the address is
  near the end of the line, and silently truncating an over-long one.
- The MDTM parsing in php_stream_ftp_url_stat() skips non-digits past
  the terminating NUL, and its bound check allows sscanf() to start one
  past the end of the buffer.

Only return a code from get_ftp_result() for a complete status line,
copy exactly the parsed address and reject one that does not fit, and
stop the MDTM scan at the end of the line.

Closes GH-24340.

Author: alepacheco

NEWS

@@ -343,6 +343,8 @@ PHP                                                                        NEWS
     parsing fails after Transfer-Encoding). (bercedev)
   . Fixed bug GH-24509 (HTTP wrapper leaks Location buffer on empty
     Location header). (bercedev)
+  . Fix out-of-bounds reads parsing ftp:// wrapper server replies.
+    (Alejandro Pacheco)
 
 - Tidy:
   . Fixed tidy_repair_string(), tidy_repair_file(), tidy::repairString() and

ext/ftp/tests/server.inc

@@ -412,6 +412,10 @@ if ($pid) {
                 fclose($fs);
 
 
+        }elseif (preg_match('/^PASV/', $buf, $matches) && isset($pasv_reply_at_buffer_end)) {
+            // The address ends at the 510th byte and the port is cut off.
+            fputs($s, "227 " . str_repeat("x", 496) . "127,0,0,1,4,210)\r\n");
+
         }elseif (preg_match('/^PASV/', $buf, $matches)) {
             $pasv=true;
             $host = "127.0.0.1";
@@ -542,6 +546,13 @@ if ($pid) {
             fclose($fs);
         }elseif (preg_match('/^SIZE \/bug73457/', $buf)) {
             fputs($s, "213 10\r\n");
+        }elseif (preg_match('/^SIZE \/pasvbufferend/', $buf)) {
+            fputs($s, "213 10\r\n");
+        }elseif (preg_match('/^SIZE \/mdtmnodigits/', $buf)) {
+            // A longer line with digits, read into the same buffer before MDTM.
+            fputs($s, "213 10 bytes (19991005213102)\r\n");
+        }elseif (preg_match('/^MDTM \/mdtmnodigits/', $buf)) {
+            fputs($s, "213 unknown\r\n");
         }elseif (preg_match("/^SITE/", $buf)) {
             fputs($s, "500 Syntax error, command unrecognized.\r\n");
         }else {

ext/standard/ftp_fopen_wrapper.c

@@ -74,13 +74,23 @@ typedef struct _php_ftp_dirstream_data {
 } php_ftp_dirstream_data;
 
 /* {{{ get_ftp_result */
+/* Reads reply lines into buffer until a complete status line ("ddd ") and
+ * returns its code, or 0 if the stream ends before one.
+ * On return the buffer always holds a NUL-terminated string, which the
+ * callers rely on when they parse or print the reply text: it is emptied
+ * first in case nothing can be read, and php_stream_gets() terminates every
+ * line it reads. */
 static inline int get_ftp_result(php_stream *stream, char *buffer, size_t buffer_size)
 {
 	buffer[0] = '\0'; /* in case read fails to read anything */
-	while (php_stream_gets(stream, buffer, buffer_size-1) &&
-		   !(isdigit((unsigned char)buffer[0]) && isdigit((unsigned char)buffer[1]) &&
-			 isdigit((unsigned char)buffer[2]) && buffer[3] == ' '));
-	return strtol(buffer, NULL, 10);
+	while (php_stream_gets(stream, buffer, buffer_size-1)) {
+		if (isdigit((unsigned char)buffer[0]) && isdigit((unsigned char)buffer[1]) &&
+			isdigit((unsigned char)buffer[2]) && buffer[3] == ' ') {
+			return strtol(buffer, NULL, 10);
+		}
+	}
+	/* no complete status line, callers parse the reply from buffer + 4 */
+	return 0;
 }
 /* }}} */
 
@@ -352,8 +362,10 @@ static unsigned short php_fopen_do_pasv(php_stream *stream, char *ip, size_t ip_
 			tpath++;
 		}
 		tpath[-1] = '\0';
-		memcpy(ip, hoststart, ip_size);
-		ip[ip_size-1] = '\0';
+		if ((size_t)(tpath - hoststart) > ip_size) {
+			return 0;
+		}
+		memcpy(ip, hoststart, tpath - hoststart);
 		hoststart = ip;
 
 		/* pull out the MSB of the port */
@@ -825,11 +837,11 @@ static int php_stream_ftp_url_stat(php_stream_wrapper *wrapper, const char *url,
 		struct tm tm, tmbuf, *gmt;
 		time_t stamp;
 
-		while ((size_t)(p - tmp_line) < sizeof(tmp_line) && !isdigit((unsigned char)*p)) {
+		while (*p && !isdigit((unsigned char)*p)) {
 			p++;
 		}
 
-		if ((size_t)(p - tmp_line) > sizeof(tmp_line)) {
+		if (!*p) {
 			goto mdtm_error;
 		}
 

ext/standard/tests/streams/ftp_fopen_do_pasv_reply_bounds.phpt

@@ -0,0 +1,20 @@
+--TEST--
+ftp:// wrapper rejects a PASV reply whose address ends at the reply buffer limit
+--SKIPIF--
+<?php
+if (array_search('ftp',stream_get_wrappers()) === FALSE) die("skip ftp wrapper not available.");
+if (!function_exists('pcntl_fork')) die("skip pcntl_fork() not available.");
+?>
+--FILE--
+<?php
+
+$pasv_reply_at_buffer_end = true;
+require __DIR__ . "/../../../ftp/tests/server.inc";
+
+$path = "ftp://localhost:" . $port . "/pasvbufferend";
+
+var_dump(fopen($path, "r"));
+?>
+--EXPECTF--
+Warning: fopen(%s): Failed to open stream: FTP server reports %a in %s on line %d
+bool(false)

ext/standard/tests/streams/ftp_url_stat_mdtm_no_digits.phpt

@@ -0,0 +1,20 @@
+--TEST--
+stat() on ftp:// ignores an MDTM reply without digits
+--SKIPIF--
+<?php
+if (array_search('ftp',stream_get_wrappers()) === FALSE) die("skip ftp wrapper not available.");
+if (!function_exists('pcntl_fork')) die("skip pcntl_fork() not available.");
+?>
+--FILE--
+<?php
+
+require __DIR__ . "/../../../ftp/tests/server.inc";
+
+$path = "ftp://localhost:" . $port . "/mdtmnodigits";
+
+$stat = stat($path);
+var_dump($stat['size'], $stat['mtime']);
+?>
+--EXPECT--
+int(10)
+int(-1)