archive/php-src
1
0
Fork 0
mirror of https://github.com/php/php-src.git synced 2025-08-16 05:58:45 +02:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

Fix GH-13037: PharData incorrectly extracts zip file

Browse source 
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.
This commit is contained in:
Niels Dossche 2023-12-28 22:41:32 +01:00
parent d417072ebe
commit ba80372a58
4 changed files with 59 additions and 18 deletions
Download patch file Download diff file Expand all files Collapse all files

1
NEWS
View file

@ -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

BIN
ext/phar/tests/zip/files/gh13037.zip Normal file
View file

Binary file not shown.

17
ext/phar/tests/zip/gh13037.phpt Normal file
View file

@ -0,0 +1,17 @@
--TEST--
GH-13037 (PharData incorrectly extracts zip file)
--EXTENSIONS--
phar
--FILE--
<?php
$phar = new PharData(__DIR__ . '/files/gh13037.zip');
$phar->extractTo(__DIR__ . '/out_gh13037/');
echo file_get_contents(__DIR__ . '/out_gh13037/test');
?>
--CLEAN--
<?php
@unlink(__DIR__ . '/out_gh13037/test');
@rmdir(__DIR__ . '/out_gh13037');
?>
--EXPECT--
hello

59
ext/phar/zip.c
View file

@ -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);