-
-
Save smalyshev/f2f742d7a5a9702f9942fa83c7d1271e to your computer and use it in GitHub Desktop.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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