Fix #77565: Incorrect locator detection in ZIP-based phars

We must not assume that the first end of central dir signature in a ZIP
archive actually designates the end of central directory record, since
the data in the archive may contain arbitrary byte patterns.  Thus, we
better search from the end of the data, what is also slightly more
efficient.

There is, however, no way to detect the end of central directory
signature by searching from the end of the ZIP archive with absolute
certainty, since the signature could be part of the trailing comment.
To mitigate, we check that the comment length fits to the found
position, but that might still not be the correct position in rare
cases.

Closes GH-6507.

6 files changed, 77 insertions, 40 deletions

diff --git a/NEWS b/NEWS
index 60741d463a..6a12fe4a88 100644
--- a/NEWS
+++ b/NEWS
@@ -20,6 +20,9 @@ PHP                                                                        NEWS
   . Fixed bug #77935 (Crash in mysqlnd_fetch_stmt_row_cursor when calling an SP
     with a cursor). (Nikita)
 
+- Phar:
+  . Fixed bug #77565 (Incorrect locator detection in ZIP-based phars). (cmb)
+
 07 Jan 2021, PHP 7.4.14
 
 - Core:
diff --git a/ext/phar/tests/bug69441.phpt b/ext/phar/tests/bug69441.phpt
index 91e2dcf146..7150af0f34 100644
--- a/ext/phar/tests/bug69441.phpt
+++ b/ext/phar/tests/bug69441.phpt
@@ -14,7 +14,7 @@ $r = new Phar($fname, 0);
 
 ==DONE==
 --EXPECTF--
-UnexpectedValueException: phar error: corrupted central directory entry, no magic signature in zip-based phar "%sbug69441.phar" in %sbug69441.php:%d
+UnexpectedValueException: phar error: end of central directory not found in zip-based phar "%sbug69441.phar" in %sbug69441.php:%d
 Stack trace:
 #0 %s%ebug69441.php(%d): Phar->__construct('%s', 0)
 #1 {main}
diff --git a/ext/phar/tests/bug77565.phpt b/ext/phar/tests/bug77565.phpt
new file mode 100644
index 0000000000..e15e7ae647
--- /dev/null
+++ b/ext/phar/tests/bug77565.phpt
@@ -0,0 +1,13 @@
+--TEST--
+Bug #77565 (Incorrect locator detection in ZIP-based phars)
+--SKIPIF--
+<?php
+if (!extension_loaded('phar')) die('skip phar extension not available');
+?>
+--FILE--
+<?php
+$phar = new PharData(__DIR__ . '/bug77565.zip');
+var_dump($phar['1.zip']->getFilename());
+?>
+--EXPECT--
+string(5) "1.zip"
diff --git a/ext/phar/tests/bug77565.zip b/ext/phar/tests/bug77565.zip
new file mode 100644
index 0000000000..134d571d7c
--- /dev/null
+++ b/ext/phar/tests/bug77565.zip
diff --git a/ext/phar/tests/zip/corrupt_003.phpt b/ext/phar/tests/zip/corrupt_003.phpt
index e076a4eb8f..9f5bcceeec 100644
--- a/ext/phar/tests/zip/corrupt_003.phpt
+++ b/ext/phar/tests/zip/corrupt_003.phpt
@@ -12,5 +12,5 @@ try {
 ?>
 ===DONE===
 --EXPECTF--
-phar error: corrupt zip archive, zip file comment truncated in zip-based phar "%sfilecomment.zip"
+phar error: end of central directory not found in zip-based phar "%sfilecomment.zip"
 ===DONE===
diff --git a/ext/phar/zip.c b/ext/phar/zip.c
index 0bf873aff1..c52e87647d 100644
--- a/ext/phar/zip.c
+++ b/ext/phar/zip.c
@@ -161,6 +161,29 @@ static void phar_zip_u2d_time(time_t time, char *dtime, char *ddate) /* {{{ */
 }
 /* }}} */
 
+static char *phar_find_eocd(const char *s, size_t n)
+{
+	const char *end = s + n + sizeof("PK\5\6") - 1 - sizeof(phar_zip_dir_end);
+
+	/* search backwards for end of central directory signatures */
+	do {
+		uint16_t comment_len;
+		const char *eocd_start = zend_memnrstr(s, "PK\5\6", sizeof("PK\5\6") - 1, end);
+
+		if (eocd_start == NULL) {
+			return NULL;
+		}
+		ZEND_ASSERT(eocd_start + sizeof(phar_zip_dir_end) <= s + n);
+		comment_len = PHAR_GET_16(((phar_zip_dir_end *) eocd_start)->comment_len);
+		if (eocd_start + sizeof(phar_zip_dir_end) + comment_len == s + n) {
+			/* we can't be sure, but this looks like the proper EOCD signature */
+			return (char *) eocd_start;
+		}
+		end = eocd_start;
+	} while (end > s);
+	return NULL;
+}
+
 /**
  * Does not check for a previously opened phar in the cache.
  *
@@ -205,57 +228,55 @@ int phar_parse_zipfile(php_stream *fp, char *fname, size_t fname_len, char *alia
 		return FAILURE;
 	}
 
-	while ((p=(char *) memchr(p + 1, 'P', (size_t) (size - (p + 1 - buf)))) != NULL) {
-		if ((p - buf) + sizeof(locator) <= (size_t)size && !memcmp(p + 1, "K\5\6", 3)) {
-			memcpy((void *)&locator, (void *) p, sizeof(locator));
-			if (PHAR_GET_16(locator.centraldisk) != 0 || PHAR_GET_16(locator.disknumber) != 0) {
-				/* split archives not handled */
-				php_stream_close(fp);
-				if (error) {
-					spprintf(error, 4096, "phar error: split archives spanning multiple zips cannot be processed in zip-based phar \"%s\"", fname);
-				}
-				return FAILURE;
+	if ((p = phar_find_eocd(buf, size)) != NULL) {
+		memcpy((void *)&locator, (void *) p, sizeof(locator));
+		if (PHAR_GET_16(locator.centraldisk) != 0 || PHAR_GET_16(locator.disknumber) != 0) {
+			/* split archives not handled */
+			php_stream_close(fp);
+			if (error) {
+				spprintf(error, 4096, "phar error: split archives spanning multiple zips cannot be processed in zip-based phar \"%s\"", fname);
 			}
+			return FAILURE;
+		}
 
-			if (PHAR_GET_16(locator.counthere) != PHAR_GET_16(locator.count)) {
-				if (error) {
-					spprintf(error, 4096, "phar error: corrupt zip archive, conflicting file count in end of central directory record in zip-based phar \"%s\"", fname);
-				}
-				php_stream_close(fp);
-				return FAILURE;
+		if (PHAR_GET_16(locator.counthere) != PHAR_GET_16(locator.count)) {
+			if (error) {
+				spprintf(error, 4096, "phar error: corrupt zip archive, conflicting file count in end of central directory record in zip-based phar \"%s\"", fname);
 			}
+			php_stream_close(fp);
+			return FAILURE;
+		}
 
-			mydata = pecalloc(1, sizeof(phar_archive_data), PHAR_G(persist));
-			mydata->is_persistent = PHAR_G(persist);
+		mydata = pecalloc(1, sizeof(phar_archive_data), PHAR_G(persist));
+		mydata->is_persistent = PHAR_G(persist);
 
-			/* read in archive comment, if any */
-			if (PHAR_GET_16(locator.comment_len)) {
+		/* read in archive comment, if any */
+		if (PHAR_GET_16(locator.comment_len)) {
 
-				metadata = p + sizeof(locator);
+			metadata = p + sizeof(locator);
 
-				if (PHAR_GET_16(locator.comment_len) != size - (metadata - buf)) {
-					if (error) {
-						spprintf(error, 4096, "phar error: corrupt zip archive, zip file comment truncated in zip-based phar \"%s\"", fname);
-					}
-					php_stream_close(fp);
-					pefree(mydata, mydata->is_persistent);
-					return FAILURE;
+			if (PHAR_GET_16(locator.comment_len) != size - (metadata - buf)) {
+				if (error) {
+					spprintf(error, 4096, "phar error: corrupt zip archive, zip file comment truncated in zip-based phar \"%s\"", fname);
 				}
+				php_stream_close(fp);
+				pefree(mydata, mydata->is_persistent);
+				return FAILURE;
+			}
 
-				mydata->metadata_len = PHAR_GET_16(locator.comment_len);
+			mydata->metadata_len = PHAR_GET_16(locator.comment_len);
 
-				if (phar_parse_metadata(&metadata, &mydata->metadata, PHAR_GET_16(locator.comment_len)) == FAILURE) {
-					mydata->metadata_len = 0;
-					/* if not valid serialized data, it is a regular string */
+			if (phar_parse_metadata(&metadata, &mydata->metadata, PHAR_GET_16(locator.comment_len)) == FAILURE) {
+				mydata->metadata_len = 0;
+				/* if not valid serialized data, it is a regular string */
 
-					ZVAL_NEW_STR(&mydata->metadata, zend_string_init(metadata, PHAR_GET_16(locator.comment_len), mydata->is_persistent));
-				}
-			} else {
-				ZVAL_UNDEF(&mydata->metadata);
+				ZVAL_NEW_STR(&mydata->metadata, zend_string_init(metadata, PHAR_GET_16(locator.comment_len), mydata->is_persistent));
 			}
-
-			goto foundit;
+		} else {
+			ZVAL_UNDEF(&mydata->metadata);
 		}
+
+		goto foundit;
 	}
 
 	php_stream_close(fp);