com php-src: Fixed bug #69324 (Buffer Over-read in unserialize when parsing Phar): ext/phar/phar.c ext/phar/phar_internal.h ext/phar/
tests/bug69324.phar ext/phar/tests/bug693 24.phpt

Commit: 9faaee66fa493372c7340b1ab05f8fd115131a42 Author: Stanislav Malyshev <stas@php.net> Sun, 5 Apr 2015 15:07:36 -0700 Parents: 0ea75af9be8a40836951fc89f723dd5390b8b46f Branches: PHP-5.4 Link: http://git.php.net/?p=php-src.git;a=commitdiff;h=9faaee66fa493372c7340b1ab05f8fd115131a42 Log: Fixed bug #69324 (Buffer Over-read in unserialize when parsing Phar) Bugs: https://bugs.php.net/69324 Changed paths: M ext/phar/phar.c M ext/phar/phar_internal.h A ext/phar/tests/bug69324.phar A ext/phar/tests/bug69324.phpt Diff: diff --git a/ext/phar/phar.c b/ext/phar/phar.c index ec82351..bf0c985 100644 --- 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 c9306c1..fcfc864 100644 --- 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 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 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