Skip to content

PharData incorrectly extracts zip file #13037

Closed
Closed
PharData incorrectly extracts zip file#13037
Assignees
ndossche
Labels
BugExtension: pharStatus: Verified

Description

@ghost
ghost
opened on Dec 27, 2023

Description

The following code:

<?php
function archive_extract(string $path, string $destination = ".")
{
	$phar = new PharData($path);
	$phar->extractTo($destination);
}

archive_extract("file.zip", "extracted_dir1");

Extracts the zip file with invisible characters such as \x14 and other control characters in a few of HUNDREDS of files.
Files that are corrupted when extracted:

diff -r -q /path/to/dir1 /path/to/dir2 output:

Files test/wordpress/wp-includes/blocks/site-tagline/editor-rtl.min.css and test2/wordpress/wp-includes/blocks/site-tagline/editor-rtl.min.css differ
Files test/wordpress/wp-includes/blocks/site-tagline/editor.min.css and test2/wordpress/wp-includes/blocks/site-tagline/editor.min.css differ
Files test/wordpress/wp-includes/blocks/site-title/style-rtl.css and test2/wordpress/wp-includes/blocks/site-title/style-rtl.css differ
Files test/wordpress/wp-includes/blocks/site-title/style-rtl.min.css and test2/wordpress/wp-includes/blocks/site-title/style-rtl.min.css differ
Files test/wordpress/wp-includes/blocks/site-title/style.css and test2/wordpress/wp-includes/blocks/site-title/style.css differ
Files test/wordpress/wp-includes/blocks/site-title/style.min.css and test2/wordpress/wp-includes/blocks/site-title/style.min.css differ
Files test/wordpress/wp-includes/blocks/spacer/style-rtl.css and test2/wordpress/wp-includes/blocks/spacer/style-rtl.css differ
Files test/wordpress/wp-includes/blocks/spacer/style-rtl.min.css and test2/wordpress/wp-includes/blocks/spacer/style-rtl.min.css differ

This code:

<?php
function unzip(string $path, string $destination = ".")
{
    $zip = new ZipArchive;
    $zip->open($path);
    $zip->extractTo($destination);
    $zip->close();
}
archive_extract("file.zip", "extracted_dir2");

correctly extracts the zip file as expected, in the same way as command line tool unzip does.

Working file from ZipArchive:
ziparchive

Corrupted file from PharData:
phardata

PHP Version

PHP 8.2.7

Operating System

Debian 12.4 raspberrypi

Activity

  1. ghost added
    Bug
    on Dec 27, 2023
  2. ghost added
    Status: Needs Triage
    on Dec 27, 2023
  3. ghost changed the title [-]PharData incorrectly extracts zip file[/-] [+]```PharData``` incorrectly extracts zip file[/+] on Dec 27, 2023
  4. devnexen
    added
    Extension: zip
    Extension: phar
    and removed
    Extension: zip
    on Dec 27, 2023
  5. ndossche
    added
    Status: Verified
    and removed
    Status: Needs Triage
    on Dec 27, 2023

  6. ndossche commented on Dec 27, 2023

    @ndossche
    ndossche
    on Dec 27, 2023
    Member

    The corruption only seems to happen when the compression flags of the file entry are zero, i.e. no compression is used.
    What's also interesting is that it seems to read 4 bytes prior to the file, which explains why we see 4 bytes of junk at the front and 4 bytes missing at the end.
    Not sure yet what's going on exactly, but I'm digging into it.

    EDIT: looks like the zip file entry offset is maybe not set up properly, the entry.offset variable set up at phar_parse_zipfile seems wrong, it points to offset 12398848 for vendor/joypixels/emoji-toolkit/joypixels.json, but that's indeed 4 bytes prior to the file start.

  7. ndossche commented on Dec 28, 2023

    @ndossche
    ndossche
    on Dec 28, 2023
    Member

    The difference of 4 bytes comes from an inconsistency in the extra field length in the sample zip files.
    In the central directory entry for the file, this is 24 bytes, but in the local header this is 28 bytes. A difference of 4.
    libzip seems to use that of the local header (confirmed by compiling and debugging libzip), hence it reads at the correct offset. This has probably always been done incorrectly in phar.

    According to https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT

    Immediately following the local header for a file SHOULD be placed the compressed or stored data for the file.
    Note that it does not mention consistency between the local directory and central directory.

    Furthermore, upon finding this I googled a bit further and found this: https://stackoverflow.com/questions/58702783/finding-start-of-compressed-data-for-items-in-a-zip-with-zip4j

    The reason you have to add 4 to the offset in your example is because the size of the extra data field in central directory of this entry (= file header) is different than the one in local file header, and it is perfectly legal as per zip specification to have different extra data field sizes in central directory and local header. In fact the extra data field we are talking about, Extended Timestamp extra field (signature 0x5455), has an official definition which has varied lengths between the two.

    And indeed that's the case here too! What an amazing file format design!

    Here's a very quick and dirty proof of concept patch:

    diff --git a/ext/phar/zip.c b/ext/phar/zip.c
index 1804d926b4..c07ddf9b10 100644
--- a/ext/phar/zip.c
+++ b/ext/phar/zip.c
@@ -386,8 +386,16 @@ int phar_parse_zipfile(php_stream *fp, char *fname, size_t fname_len, char *alia
 		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);
+
+		zend_off_t loc = php_stream_tell(fp);
+		phar_zip_file_header local_file_header;
+		php_stream_seek(fp, entry.header_offset, SEEK_SET);
+		php_stream_read(fp, (char *) &local_file_header, sizeof(local_file_header));
+		php_stream_seek(fp, loc, SEEK_SET);
 		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);
+			PHAR_GET_16(local_file_header.extra_len);
 
 		if (PHAR_GET_16(zipentry.flags) & PHAR_ZIP_FLAG_ENCRYPTED) {
 			PHAR_ZIP_FAIL("Cannot process encrypted zip files");

    I'll write up a proper patch and testcase tomorrow, it's late.

  8. ndossche
    self-assigned this
    on Dec 28, 2023
  9. ndossche
    added a commit that references this issue on Dec 28, 2023

    Fix phpGH-13037: PharData incorrectly extracts zip file

    01ee194
  10. ndossche
    linked a pull request that will close this issueFix GH-13037: PharData incorrectly extracts zip file #13045on Dec 28, 2023
  11. ndossche
    closed this as completedin ba80372on Jan 25, 2024
  12. ndossche
    added 3 commits that reference this issue on Jan 25, 2024

    Merge branch 'PHP-8.2' into PHP-8.3

    78986a6

    Merge branch 'PHP-8.3'

    b8ff8c0

    Fix GH-13037: PharData incorrectly extracts zip file

    ba80372
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

BugExtension: pharStatus: Verified

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions