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 1/2] 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); From 4a487294383009d71d9b9ba56f6b589335e70dca Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Wed, 24 Jan 2024 22:12:49 +0100 Subject: [PATCH 2/2] Fix GH-10344: imagettfbbox(): Could not find/open font UNC path libgd uses an incorrect absolute path check in gdft.c. It checks if either the path starts with a '/' (only valid on Posix btw), or whether it contains something of the form C:\ or C:/. However, this overlooks the possibility of using UNC paths on Windows. As we already do PHP-specific stuff with VCWD_ macros, use IS_ABSOLUTE_PATH to check for an absolute path which will take into account UNC paths as well. Closes GH-13241. --- NEWS | 4 ++++ ext/gd/libgd/gdft.c | 12 +++++++++++- ext/gd/tests/gh10344.phpt | 28 ++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 ext/gd/tests/gh10344.phpt diff --git a/NEWS b/NEWS index 34b6a0de443..174233753b0 100644 --- a/NEWS +++ b/NEWS @@ -16,6 +16,10 @@ PHP NEWS . Fixed bug GH-12996 (Incorrect SCRIPT_NAME with Apache ProxyPassMatch when plus in path). (Jakub Zelenka) +- GD: + . Fixed bug GH-10344 (imagettfbbox(): Could not find/open font UNC path). + (nielsdos) + - MySQLnd: . Fixed bug GH-12107 (When running a stored procedure (that returns a result set) twice, PHP crashes). (nielsdos) diff --git a/ext/gd/libgd/gdft.c b/ext/gd/libgd/gdft.c index 554a656e625..6876ca6f6b4 100644 --- a/ext/gd/libgd/gdft.c +++ b/ext/gd/libgd/gdft.c @@ -401,7 +401,17 @@ static void *fontFetch (char **error, void *key) #ifdef NETWARE if (*name == '/' || (name[0] != 0 && strstr(name, ":/"))) { #else - if (*name == '/' || (name[0] != 0 && name[1] == ':' && (name[2] == '/' || name[2] == '\\'))) { + /* Actual length doesn't matter, just the minimum does up to length 2. */ + unsigned int min_length = 0; + if (name[0] != '\0') { + if (name[1] != '\0') { + min_length = 2; + } else { + min_length = 1; + } + } + ZEND_IGNORE_VALUE(min_length); /* On Posix systems this may be unused */ + if (IS_ABSOLUTE_PATH(name, min_length)) { #endif snprintf(fullname, sizeof(fullname) - 1, "%s", name); if (access(fullname, R_OK) == 0) { diff --git a/ext/gd/tests/gh10344.phpt b/ext/gd/tests/gh10344.phpt new file mode 100644 index 00000000000..829f8f9c35c --- /dev/null +++ b/ext/gd/tests/gh10344.phpt @@ -0,0 +1,28 @@ +--TEST-- +GH-10344 (imagettfbbox(): Could not find/open font UNC path) +--EXTENSIONS-- +gd +--SKIPIF-- + +--FILE-- + +--EXPECT-- +int(8)