From ba80372a58351a45913d66576d9730d3065faa31 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Thu, 28 Dec 2023 22:41:32 +0100 Subject: [PATCH] Fix GH-13037: PharData incorrectly extracts zip file The code currently assumes that the extra field length of the central directory entry and the local entry are the same, but that's not the case. For example, the "Extended Timestamp extra field" differs in size for local vs central directory entries. This causes the file contents offset to be incorrect because it is based on the central directory length instead of the local entry length. Fix it by reading the local entry and getting the size from there as well as checking consistency for the file name length. Closes GH-13045. --- NEWS | 1 + ext/phar/tests/zip/files/gh13037.zip | Bin 0 -> 164 bytes ext/phar/tests/zip/gh13037.phpt | 17 ++++++++ ext/phar/zip.c | 59 +++++++++++++++++++-------- 4 files changed, 59 insertions(+), 18 deletions(-) create mode 100644 ext/phar/tests/zip/files/gh13037.zip create mode 100644 ext/phar/tests/zip/gh13037.phpt 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 0000000000000000000000000000000000000000..d039461237f435f763f8e67a39cbe18be6a22164 GIT binary patch literal 164 zcmWIWW@h1H0D*#)bHWu2tjySeY!GH)kYOlEEiMTS;bdSgxz?Ln1;nKl+zgB?FPMSS zAR;3*CnujPz?+dtjv1FJ5+K7F7=d_6BZvhtlNDknnt1`x)fV2yU!vFwN CjU6fg literal 0 HcmV?d00001 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);