Fixed bug #69324 (Buffer Over-read in unserialize when parsing Phar)

diff --git a/ext/phar/phar.c b/ext/phar/phar.c
index ec823514107160b7e6fabc519594012657a2db91..bf0c985a7c3a21741a48188728909b33eb8af983 100644 (file)
--- a/ext/phar/phar.c
+++ b/ext/phar/phar.c
@@ -603,25 +603,18 @@ int phar_open_parsed_phar(char *fname, int fname_len, char *alias, int alias_len
  * 
  * data is the serialized zval
  */
-int phar_parse_metadata(char **buffer, zval **metadata, int zip_metadata_len TSRMLS_DC) /* {{{ */
+int phar_parse_metadata(char **buffer, zval **metadata, php_uint32 zip_metadata_len TSRMLS_DC) /* {{{ */
 {
        const unsigned char *p;
-       php_uint32 buf_len;
        php_unserialize_data_t var_hash;
 
-       if (!zip_metadata_len) {
-               PHAR_GET_32(*buffer, buf_len);
-       } else {
-               buf_len = zip_metadata_len;
-       }
-
-       if (buf_len) {
+       if (zip_metadata_len) {
                ALLOC_ZVAL(*metadata);
                INIT_ZVAL(**metadata);
                p = (const unsigned char*) *buffer;
                PHP_VAR_UNSERIALIZE_INIT(var_hash);
 
-               if (!php_var_unserialize(metadata, &p, p + buf_len, &var_hash TSRMLS_CC)) {
+               if (!php_var_unserialize(metadata, &p, p + zip_metadata_len, &var_hash TSRMLS_CC)) {
                        PHP_VAR_UNSERIALIZE_DESTROY(var_hash);
                        zval_ptr_dtor(metadata);
                        *metadata = NULL;
@@ -633,19 +626,14 @@ int phar_parse_metadata(char **buffer, zval **metadata, int zip_metadata_len TSR
                if (PHAR_G(persist)) {
                        /* lazy init metadata */
                        zval_ptr_dtor(metadata);
-                       *metadata = (zval *) pemalloc(buf_len, 1);
-                       memcpy(*metadata, *buffer, buf_len);
-                       *buffer += buf_len;
+                       *metadata = (zval *) pemalloc(zip_metadata_len, 1);
+                       memcpy(*metadata, *buffer, zip_metadata_len);
                        return SUCCESS;
                }
        } else {
                *metadata = NULL;
        }
 
-       if (!zip_metadata_len) {
-               *buffer += buf_len;
-       }
-
        return SUCCESS;
 }
 /* }}}*/
@@ -666,6 +654,7 @@ static int phar_parse_pharfile(php_stream *fp, char *fname, int fname_len, char
        phar_entry_info entry;
        php_uint32 manifest_len, manifest_count, manifest_flags, manifest_index, tmp_len, sig_flags;
        php_uint16 manifest_ver;
+       php_uint32 len;
        long offset;
        int sig_len, register_alias = 0, temp_alias = 0;
        char *signature = NULL;
@@ -1031,16 +1020,21 @@ 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);
        if (mydata->is_persistent) {
-               PHAR_GET_32(buffer, mydata->metadata_len);
-               if (phar_parse_metadata(&buffer, &mydata->metadata, mydata->metadata_len TSRMLS_CC) == FAILURE) {
-                       MAPPHAR_FAIL("unable to read phar metadata in .phar file \"%s\"");
-               }
-       } else {
-               if (phar_parse_metadata(&buffer, &mydata->metadata, 0 TSRMLS_CC) == FAILURE) {
-                       MAPPHAR_FAIL("unable to read phar metadata in .phar file \"%s\"");
+               mydata->metadata_len = len;
+               if(!len) {
+                       /* FIXME: not sure why this is needed but removing it breaks tests */
+                       PHAR_GET_32(buffer, len);
                }
        }
+       if(len > endbuffer - buffer) {
+               MAPPHAR_FAIL("internal corruption of phar \"%s\" (trying to read past buffer end)");
+       }
+       if (phar_parse_metadata(&buffer, &mydata->metadata, len TSRMLS_CC) == FAILURE) {
+               MAPPHAR_FAIL("unable to read phar metadata in .phar file \"%s\"");
+       }
+       buffer += len;
 
        /* set up our manifest */
        zend_hash_init(&mydata->manifest, manifest_count,
@@ -1075,7 +1069,7 @@ static int phar_parse_pharfile(php_stream *fp, char *fname, int fname_len, char
                        entry.manifest_pos = manifest_index;
                }
 
-               if (buffer + entry.filename_len + 20 > endbuffer) {
+               if (entry.filename_len + 20 > endbuffer - buffer) {
                        MAPPHAR_FAIL("internal corruption of phar \"%s\" (truncated manifest entry)");
                }
 
@@ -1111,19 +1105,20 @@ static int phar_parse_pharfile(php_stream *fp, char *fname, int fname_len, char
                        entry.flags |= PHAR_ENT_PERM_DEF_DIR;
                }
 
+               PHAR_GET_32(buffer, len);
                if (entry.is_persistent) {
-                       PHAR_GET_32(buffer, entry.metadata_len);
-                       if (!entry.metadata_len) buffer -= 4;
-                       if (phar_parse_metadata(&buffer, &entry.metadata, entry.metadata_len TSRMLS_CC) == FAILURE) {
-                               pefree(entry.filename, entry.is_persistent);
-                               MAPPHAR_FAIL("unable to read file metadata in .phar file \"%s\"");
-                       }
+                       entry.metadata_len = len;
                } else {
-                       if (phar_parse_metadata(&buffer, &entry.metadata, 0 TSRMLS_CC) == FAILURE) {
-                               pefree(entry.filename, entry.is_persistent);
-                               MAPPHAR_FAIL("unable to read file metadata in .phar file \"%s\"");
-                       }
+                       entry.metadata_len = 0;
+               }
+               if (len > endbuffer - buffer) {
+                       MAPPHAR_FAIL("internal corruption of phar \"%s\" (truncated manifest entry)");
+               }
+               if (phar_parse_metadata(&buffer, &entry.metadata, len TSRMLS_CC) == FAILURE) {
+                       pefree(entry.filename, entry.is_persistent);
+                       MAPPHAR_FAIL("unable to read file metadata in .phar file \"%s\"");
                }
+               buffer += len;
 
                entry.offset = entry.offset_abs = offset;
                offset += entry.compressed_filesize;

diff --git a/ext/phar/phar_internal.h b/ext/phar/phar_internal.h
index c9306c15f4d8b014c74108d090e9d8c1746984b5..fcfc86457d623350b93e88ab2438a07093bdde86 100644 (file)
--- a/ext/phar/phar_internal.h
+++ b/ext/phar/phar_internal.h
@@ -654,7 +654,7 @@ int phar_mount_entry(phar_archive_data *phar, char *filename, int filename_len,
 char *phar_find_in_include_path(char *file, int file_len, phar_archive_data **pphar TSRMLS_DC);
 char *phar_fix_filepath(char *path, int *new_len, int use_cwd TSRMLS_DC);
 phar_entry_info * phar_open_jit(phar_archive_data *phar, phar_entry_info *entry, char **error TSRMLS_DC);
-int phar_parse_metadata(char **buffer, zval **metadata, int zip_metadata_len TSRMLS_DC);
+int phar_parse_metadata(char **buffer, zval **metadata, php_uint32 zip_metadata_len TSRMLS_DC);
 void destroy_phar_manifest_entry(void *pDest);
 int phar_seek_efp(phar_entry_info *entry, off_t offset, int whence, off_t position, int follow_links TSRMLS_DC);
 php_stream *phar_get_efp(phar_entry_info *entry, int follow_links TSRMLS_DC);

diff --git a/ext/phar/tests/bug69324.phar b/ext/phar/tests/bug69324.phar
new file mode 100644 (file)
index 0000000..0882d88
Binary files /dev/null and b/ext/phar/tests/bug69324.phar differ

diff --git a/ext/phar/tests/bug69324.phpt b/ext/phar/tests/bug69324.phpt
new file mode 100644 (file)
index 0000000..70e3f97
--- /dev/null
+++ b/ext/phar/tests/bug69324.phpt
@@ -0,0 +1,17 @@
+--TEST--
+Bug #69324: Buffer Over-read in unserialize when parsing Phar
+--SKIPIF--
+<?php
+if (!extension_loaded("phar")) die("skip");
+?>
+--FILE--
+<?php
+try {
+$p = new Phar(dirname(__FILE__).'/bug69324.phar', 0);
+$meta=$p->getMetadata();
+var_dump($meta);
+} catch(Exception $e) {
+       echo $e->getMessage();
+}
+--EXPECTF--
+internal corruption of phar "%s" (truncated manifest entry)
\ No newline at end of file