Skip to content

Instantly share code, notes, and snippets.

@smalyshev

smalyshev/77143.diff Secret

Created Nov 12, 2018
Embed
What would you like to do?
commit 48e0ba6848fb6b0778e00705d9c03102e3e22a16
Author: Stanislav Malyshev <stas@php.net>
Date: Mon Nov 12 14:02:26 2018 -0800
Fix bug #77143 - add more checks to buffer reads
diff --git a/ext/phar/phar.c b/ext/phar/phar.c
index 11c9d270a6..3ee01f03fc 100644
--- a/ext/phar/phar.c
+++ b/ext/phar/phar.c
@@ -642,6 +642,18 @@ int phar_parse_metadata(char **buffer, zval *metadata, uint32_t zip_metadata_len
}
/* }}}*/
+/**
+ * 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 >= 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.
*
@@ -725,12 +737,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");
@@ -750,7 +762,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;
@@ -970,13 +982,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)")
}
@@ -1014,7 +1026,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)")
}
@@ -1023,12 +1035,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) {
/* 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 > (size_t)(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 0000000000..eb797b5195
Binary files /dev/null and b/ext/phar/tests/bug77143.phar differ
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.