diff --git a/NEWS b/NEWS index 423fbe94df3..34b6a0de443 100644 --- a/NEWS +++ b/NEWS @@ -34,6 +34,7 @@ PHP NEWS - Phar: . Fixed bug #71465 (PHAR doesn't know about litespeed). (nielsdos) + . Fixed bug GH-13037 (PharData incorrectly extracts zip file). (nielsdos) - Random: . Fixed bug GH-13138 (Randomizer::pickArrayKeys() does not detect broken diff --git a/ext/phar/tests/zip/files/gh13037.zip b/ext/phar/tests/zip/files/gh13037.zip new file mode 100644 index 00000000000..d039461237f Binary files /dev/null and b/ext/phar/tests/zip/files/gh13037.zip differ diff --git a/ext/phar/tests/zip/gh13037.phpt b/ext/phar/tests/zip/gh13037.phpt new file mode 100644 index 00000000000..5e13bd37907 --- /dev/null +++ b/ext/phar/tests/zip/gh13037.phpt @@ -0,0 +1,17 @@ +--TEST-- +GH-13037 (PharData incorrectly extracts zip file) +--EXTENSIONS-- +phar +--FILE-- +extractTo(__DIR__ . '/out_gh13037/'); +echo file_get_contents(__DIR__ . '/out_gh13037/test'); +?> +--CLEAN-- + +--EXPECT-- +hello diff --git a/ext/phar/zip.c b/ext/phar/zip.c index 1804d926b4a..1472906c8b9 100644 --- a/ext/phar/zip.c +++ b/ext/phar/zip.c @@ -386,8 +386,6 @@ foundit: entry.timestamp = phar_zip_d2u_time(zipentry.timestamp, zipentry.datestamp); entry.flags = PHAR_ENT_PERM_DEF_FILE; entry.header_offset = PHAR_GET_32(zipentry.offset); - entry.offset = entry.offset_abs = PHAR_GET_32(zipentry.offset) + sizeof(phar_zip_file_header) + PHAR_GET_16(zipentry.filename_len) + - PHAR_GET_16(zipentry.extra_len); if (PHAR_GET_16(zipentry.flags) & PHAR_ZIP_FLAG_ENCRYPTED) { PHAR_ZIP_FAIL("Cannot process encrypted zip files"); @@ -417,6 +415,42 @@ foundit: entry.is_dir = 0; } + phar_zip_file_header local; /* Warning: only filled in when the entry is not a directory! */ + if (!entry.is_dir) { + /* A file has a central directory entry, and a local file header. Both of these contain the filename + * and the extra field data. However, at least the extra field data does not have to match between the two! + * This happens for example for the "Extended Timestamp extra field" where the local header has 2 extra fields + * in comparison to the central header. So we have to use the local header to find the right offset to the file + * contents, otherwise we're reading some garbage bytes before reading the actual file contents. */ + zend_off_t current_central_dir_pos = php_stream_tell(fp); + + php_stream_seek(fp, entry.header_offset, SEEK_SET); + if (sizeof(local) != php_stream_read(fp, (char *) &local, sizeof(local))) { + pefree(entry.filename, entry.is_persistent); + PHAR_ZIP_FAIL("phar error: internal corruption (cannot read local file header)"); + } + php_stream_seek(fp, current_central_dir_pos, SEEK_SET); + + /* verify local header + * Note: normally I'd check the crc32, and file sizes too here, but that breaks tests zip/bug48791.phpt & zip/odt.phpt, + * suggesting that something may be wrong with those files or the assumption doesn't hold. Anyway, the other checks + * _are_ performed for the alias file as was done in the past too. */ + if (entry.filename_len != PHAR_GET_16(local.filename_len)) { + pefree(entry.filename, entry.is_persistent); + PHAR_ZIP_FAIL("phar error: internal corruption (local file header does not match central directory)"); + } + + entry.offset = entry.offset_abs = entry.header_offset + + sizeof(phar_zip_file_header) + + entry.filename_len + + PHAR_GET_16(local.extra_len); + } else { + entry.offset = entry.offset_abs = entry.header_offset + + sizeof(phar_zip_file_header) + + entry.filename_len + + PHAR_GET_16(zipentry.extra_len); + } + if (entry.filename_len == sizeof(".phar/signature.bin")-1 && !strncmp(entry.filename, ".phar/signature.bin", sizeof(".phar/signature.bin")-1)) { size_t read; php_stream *sigfile; @@ -445,7 +479,7 @@ foundit: if (metadata) { php_stream_write(sigfile, metadata, PHAR_GET_16(locator.comment_len)); } - php_stream_seek(fp, sizeof(phar_zip_file_header) + entry.header_offset + entry.filename_len + PHAR_GET_16(zipentry.extra_len), SEEK_SET); + php_stream_seek(fp, entry.offset, SEEK_SET); sig = (char *) emalloc(entry.uncompressed_filesize); read = php_stream_read(fp, sig, entry.uncompressed_filesize); if (read != entry.uncompressed_filesize || read <= 8) { @@ -563,28 +597,17 @@ foundit: if (!actual_alias && entry.filename_len == sizeof(".phar/alias.txt")-1 && !strncmp(entry.filename, ".phar/alias.txt", sizeof(".phar/alias.txt")-1)) { php_stream_filter *filter; - zend_off_t saveloc; - /* verify local file header */ - phar_zip_file_header local; /* archive alias found */ - saveloc = php_stream_tell(fp); - php_stream_seek(fp, PHAR_GET_32(zipentry.offset), SEEK_SET); - - if (sizeof(local) != php_stream_read(fp, (char *) &local, sizeof(local))) { - pefree(entry.filename, entry.is_persistent); - PHAR_ZIP_FAIL("phar error: internal corruption of zip-based phar (cannot read local file header for alias)"); - } /* verify local header */ - if (entry.filename_len != PHAR_GET_16(local.filename_len) || entry.crc32 != PHAR_GET_32(local.crc32) || entry.uncompressed_filesize != PHAR_GET_32(local.uncompsize) || entry.compressed_filesize != PHAR_GET_32(local.compsize)) { + ZEND_ASSERT(!entry.is_dir); + if (entry.crc32 != PHAR_GET_32(local.crc32) || entry.uncompressed_filesize != PHAR_GET_32(local.uncompsize) || entry.compressed_filesize != PHAR_GET_32(local.compsize)) { pefree(entry.filename, entry.is_persistent); PHAR_ZIP_FAIL("phar error: internal corruption of zip-based phar (local header of alias does not match central directory)"); } - /* construct actual offset to file start - local extra_len can be different from central extra_len */ - entry.offset = entry.offset_abs = - sizeof(local) + entry.header_offset + PHAR_GET_16(local.filename_len) + PHAR_GET_16(local.extra_len); + zend_off_t restore_pos = php_stream_tell(fp); php_stream_seek(fp, entry.offset, SEEK_SET); /* these next lines should be for php < 5.2.6 after 5.3 filters are fixed */ fp->writepos = 0; @@ -680,7 +703,7 @@ foundit: } /* return to central directory parsing */ - php_stream_seek(fp, saveloc, SEEK_SET); + php_stream_seek(fp, restore_pos, SEEK_SET); } phar_set_inode(&entry);