From 48f0f73f75c0059ba5d9b73cb4e5faeeaea49c47 Mon Sep 17 00:00:00 2001
From: Stanislav Malyshev <stas@php.net>
Date: Mon, 12 Nov 2018 14:02:26 -0800
Subject: [PATCH] Fix bug #77143 - add more checks to buffer reads

---
 NEWS                         |   3 ++-
 ext/phar/phar.c              |  30 +++++++++++++++++++++---------
 ext/phar/tests/bug73768.phpt |   2 +-
 ext/phar/tests/bug77143.phar | Bin 0 -> 50 bytes
 ext/phar/tests/bug77143.phpt |  18 ++++++++++++++++++
 5 files changed, 42 insertions(+), 11 deletions(-)
 create mode 100644 ext/phar/tests/bug77143.phar
 create mode 100644 ext/phar/tests/bug77143.phpt

diff --git a/NEWS b/NEWS
index 34ff5848d5..727e874f97 100644
--- a/NEWS
+++ b/NEWS
@@ -9,7 +9,8 @@ PHP                                                                        NEWS
 
 - Phar:
   . Fixed bug #77022 (PharData always creates new files with mode 0666). (Stas)
-
+  . Fixed bug #77143 (Heap Buffer Overflow (READ: 4) in phar_parse_pharfile).
+    (Stas)
 
 13 Sep 2018, PHP 5.6.38
 
diff --git a/ext/phar/phar.c b/ext/phar/phar.c
index 780be43257..47ff8cd790 100644
--- a/ext/phar/phar.c
+++ b/ext/phar/phar.c
@@ -638,6 +638,18 @@ int phar_parse_metadata(char **buffer, zval **metadata, php_uint32 zip_metadata_
 }
 /* }}}*/
 
+/**
+ * Size of fixed fields in the manifest.
+ * See: http://php.net/manual/en/phar.fileformat.phar.php
+ */
+#define MANIFEST_FIXED_LEN	18
+
+#define SAFE_PHAR_GET_32(buffer, endbuffer, var) \
+	if (UNEXPECTED(buffer + 4 > endbuffer)) { \
+		MAPPHAR_FAIL("internal corruption of phar \"%s\" (truncated manifest header)"); \
+	} \
+	PHAR_GET_32(buffer, var);
+
 /**
  * Does not check for a previously opened phar in the cache.
  *
@@ -721,12 +733,12 @@ static int phar_parse_pharfile(php_stream *fp, char *fname, int fname_len, char
 	savebuf = buffer;
 	endbuffer = buffer + manifest_len;
 
-	if (manifest_len < 10 || manifest_len != php_stream_read(fp, buffer, manifest_len)) {
+	if (manifest_len < MANIFEST_FIXED_LEN || manifest_len != php_stream_read(fp, buffer, manifest_len)) {
 		MAPPHAR_FAIL("internal corruption of phar \"%s\" (truncated manifest header)")
 	}
 
 	/* extract the number of entries */
-	PHAR_GET_32(buffer, manifest_count);
+	SAFE_PHAR_GET_32(buffer, endbuffer, manifest_count);
 
 	if (manifest_count == 0) {
 		MAPPHAR_FAIL("in phar \"%s\", manifest claims to have zero entries.  Phars must have at least 1 entry");
@@ -746,7 +758,7 @@ static int phar_parse_pharfile(php_stream *fp, char *fname, int fname_len, char
 		return FAILURE;
 	}
 
-	PHAR_GET_32(buffer, manifest_flags);
+	SAFE_PHAR_GET_32(buffer, endbuffer, manifest_flags);
 
 	manifest_flags &= ~PHAR_HDR_COMPRESSION_MASK;
 	manifest_flags &= ~PHAR_FILE_COMPRESSION_MASK;
@@ -966,13 +978,13 @@ static int phar_parse_pharfile(php_stream *fp, char *fname, int fname_len, char
 	}
 
 	/* extract alias */
-	PHAR_GET_32(buffer, tmp_len);
+	SAFE_PHAR_GET_32(buffer, endbuffer, tmp_len);
 
 	if (buffer + tmp_len > endbuffer) {
 		MAPPHAR_FAIL("internal corruption of phar \"%s\" (buffer overrun)");
 	}
 
-	if (manifest_len < 10 + tmp_len) {
+	if (manifest_len < MANIFEST_FIXED_LEN + tmp_len) {
 		MAPPHAR_FAIL("internal corruption of phar \"%s\" (truncated manifest header)")
 	}
 
@@ -1010,7 +1022,7 @@ static int phar_parse_pharfile(php_stream *fp, char *fname, int fname_len, char
 	}
 
 	/* we have 5 32-bit items plus 1 byte at least */
-	if (manifest_count > ((manifest_len - 10 - tmp_len) / (5 * 4 + 1))) {
+	if (manifest_count > ((manifest_len - MANIFEST_FIXED_LEN - tmp_len) / (5 * 4 + 1))) {
 		/* prevent serious memory issues */
 		MAPPHAR_FAIL("internal corruption of phar \"%s\" (too many manifest entries for size of manifest)")
 	}
@@ -1019,12 +1031,12 @@ static int phar_parse_pharfile(php_stream *fp, char *fname, int fname_len, char
 	mydata->is_persistent = PHAR_G(persist);
 
 	/* check whether we have meta data, zero check works regardless of byte order */
-	PHAR_GET_32(buffer, len);
+	SAFE_PHAR_GET_32(buffer, endbuffer, len);
 	if (mydata->is_persistent) {
 		mydata->metadata_len = len;
-		if(!len) {
+		if (!len) {
 			/* FIXME: not sure why this is needed but removing it breaks tests */
-			PHAR_GET_32(buffer, len);
+			SAFE_PHAR_GET_32(buffer, endbuffer, len);
 		}
 	}
 	if(len > endbuffer - buffer) {
diff --git a/ext/phar/tests/bug73768.phpt b/ext/phar/tests/bug73768.phpt
index 37a4da0253..3062268b80 100644
--- a/ext/phar/tests/bug73768.phpt
+++ b/ext/phar/tests/bug73768.phpt
@@ -13,4 +13,4 @@ echo "OK\n";
 }
 ?>
 --EXPECTF--
-cannot load phar "%sbug73768.phar" with implicit alias "" under different alias "alias.phar"
+internal corruption of phar "%sbug73768.phar" (truncated manifest header)
diff --git a/ext/phar/tests/bug77143.phar b/ext/phar/tests/bug77143.phar
new file mode 100644
index 0000000000000000000000000000000000000000..eb797b5195759461adcd8f03222ecda1dec95646
GIT binary patch
literal 50
zcma!#_i*$HiFfw*4e<1F4bsrG=3-!AxT5L0$ziIV2EQejh^^zzO*?994>JM9-JN`$
E0BY<GRR910

literal 0
HcmV?d00001

diff --git a/ext/phar/tests/bug77143.phpt b/ext/phar/tests/bug77143.phpt
new file mode 100644
index 0000000000..f9f80fc4f4
--- /dev/null
+++ b/ext/phar/tests/bug77143.phpt
@@ -0,0 +1,18 @@
+--TEST--
+PHP bug #77143: Heap Buffer Overflow (READ: 4) in phar_parse_pharfile
+--INI--
+phar.readonly=0
+--SKIPIF--
+<?php if (!extension_loaded("phar")) die("skip"); ?>
+--FILE--
+<?php
+chdir(__DIR__);
+try {
+var_dump(new Phar('bug77143.phar',0,'project.phar'));
+echo "OK\n";
+} catch(UnexpectedValueException $e) {
+	echo $e->getMessage();
+}
+?>
+--EXPECTF--
+internal corruption of phar "%sbug77143.phar" (truncated manifest header)
-- 
2.40.0