Re: 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/bug69324.phpt

Hey: this test fails in my box: $ cat ext/phar/tests/bug69324.diff 001+ internal corruption of phar "/home/huixinchen/opensource/php-5.6/ext/phar/tests/bug69324.phar" (truncated manifest entry)[Tue Apr 14 19:20:28 2015] Script: '/home/huixinchen/opensource/php-5.6/ext/phar/tests/bug69324.php' 001- internal corruption of phar "%s" (truncated manifest entry) 002+ /home/huixinchen/opensource/php-5.6/ext/phar/phar.c(1082) : Freeing 0x2BA44F209000 (2 bytes), script=/home/huixinchen/opensource/php-5.6/ext/phar/tests/bug69324.php 003+ === Total 1 memory leaks detected === please take a look, thanks On Sun, Apr 12, 2015 at 7:53 AM, Stanislav Malyshev <stas@php.net> wrote: > 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 > > > -- > PHP CVS Mailing List (http://www.php.net/) > To unsubscribe, visit: http://www.php.net/unsub.php -- Xinchen Hui @Laruence http://www.laruence.com/