diff --git a/NEWS b/NEWS index 0aa44c4d5006..3a0ed35f3a38 100644 --- a/NEWS +++ b/NEWS @@ -23,6 +23,10 @@ PHP NEWS . Fixed bug GH-15836 (Use-after-free when a user stream filter accesses $this->stream during the close flush). (iliaal) +- Zip: + . Fixed bug GH-22885 (SEGV when setting the comment of an entry that has been + modified). (Khaled Alam) + 30 Jul 2026, PHP 8.4.24 - Calendar: diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index ccc474bc715c..47bd5d518919 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -64,25 +64,106 @@ static int le_zip_entry; } /* }}} */ +/* {{{ Bookkeeping of the entries libzip has copied the central directory record of. + * Modifying an entry makes libzip copy its central directory record, and the copy + * shares the comment string with the record of the entry. As zip_file_set_comment() + * frees that string unconditionally, setting the comment of a modified entry leaves + * the entry with a dangling comment: a use-after-free, and a double free when the + * archive is closed. The comment of such an entry cannot be replaced from the + * outside either, because libzip only keeps an unchanged comment as the shared + * string, so ext/zip tracks the modified entries and refuses instead. + * Fixed upstream by nih-at/libzip@17f10b9, unreleased as of libzip 1.11.4; this + * can go once that is the minimum supported version. */ +static void php_zip_mark_entry_modified(ze_zip_object *obj, zip_int64_t index) +{ + if (index < 0) { + return; + } + + if (!obj->modified_entries) { + ALLOC_HASHTABLE(obj->modified_entries); + zend_hash_init(obj->modified_entries, 0, NULL, NULL, false); + } + + zend_hash_index_add_empty_element(obj->modified_entries, (zend_ulong) index); +} + +static void php_zip_unmark_entry_modified(ze_zip_object *obj, zip_int64_t index) +{ + if (obj->modified_entries && index >= 0) { + zend_hash_index_del(obj->modified_entries, (zend_ulong) index); + } +} + +static void php_zip_forget_modified_entries(ze_zip_object *obj) +{ + if (obj->modified_entries) { + zend_hash_destroy(obj->modified_entries); + FREE_HASHTABLE(obj->modified_entries); + obj->modified_entries = NULL; + } +} + +/* Whether the comment of the entry is the string shared with the copy of its record. */ +static bool php_zip_entry_shares_comment(ze_zip_object *obj, zip_uint64_t index) +{ + zip_error_t *err; + int zip_err, sys_err; + zip_uint32_t comment_len = 0; + bool shared; + + if (!obj->modified_entries || !zend_hash_index_exists(obj->modified_entries, (zend_ulong) index)) { + return false; + } + + /* Only a comment the entry was read with is shared. */ + err = zip_get_error(obj->za); + zip_err = zip_error_code_zip(err); + sys_err = zip_error_code_system(err); + + shared = zip_file_get_comment(obj->za, index, &comment_len, ZIP_FL_UNCHANGED | ZIP_FL_ENC_RAW) != NULL + && comment_len > 0; + + zip_error_set(err, zip_err, sys_err); + + return shared; +} +/* }}} */ + /* {{{ php_zip_set_file_comment */ -static bool php_zip_set_file_comment(struct zip *za, zip_uint64_t index, const char *comment, size_t comment_len) +static bool php_zip_set_file_comment(ze_zip_object *obj, zip_uint64_t index, const char *comment, size_t comment_len) { zip_uint32_t current_comment_len; - const char *current_comment = zip_file_get_comment(za, index, ¤t_comment_len, ZIP_FL_ENC_RAW); + const char *current_comment = zip_file_get_comment(obj->za, index, ¤t_comment_len, ZIP_FL_ENC_RAW); - /* Avoid a libzip use-after-free when resetting an unchanged inherited comment. */ + /* Setting the comment an entry already has is a no-op. */ if (current_comment && current_comment_len == comment_len && memcmp(current_comment, comment, comment_len) == 0) { return true; } + if (php_zip_entry_shares_comment(obj, index)) { + php_error_docref(NULL, E_WARNING, "Cannot set the comment of an entry that has been modified, " + "set the comment before modifying the entry"); + return false; + } + if (comment_len == 0) { /* Passing NULL removes the existing comment. */ - return zip_file_set_comment(za, index, NULL, 0, 0) == 0; + if (zip_file_set_comment(obj->za, index, NULL, 0, 0) != 0) { + return false; + } + } else { + ZEND_ASSERT(comment_len <= 0xffff); + if (zip_file_set_comment(obj->za, index, comment, (zip_uint16_t) comment_len, 0) != 0) { + return false; + } } - ZEND_ASSERT(comment_len <= 0xffff); - return zip_file_set_comment(za, index, comment, (zip_uint16_t) comment_len, 0) == 0; + /* The comment of the entry is no longer shared with the copy of its record. */ + php_zip_unmark_entry_modified(obj, (zip_int64_t) index); + + return true; } /* }}} */ @@ -338,6 +419,7 @@ static int php_zip_add_file(ze_zip_object *obj, const char *filename, size_t fil zip_source_free(zs); return -1; } + php_zip_mark_entry_modified(obj, replace); zip_error_clear(obj->za); return 1; } @@ -347,6 +429,7 @@ static int php_zip_add_file(ze_zip_object *obj, const char *filename, size_t fil zip_source_free(zs); return -1; } + php_zip_mark_entry_modified(obj, obj->last_id); zip_error_clear(obj->za); return 1; } @@ -1072,6 +1155,9 @@ static void php_zip_object_free_storage(zend_object *object) /* {{{ */ if (!intern) { return; } + + php_zip_forget_modified_entries(intern); + if (intern->za) { if (zip_close(intern->za) != 0) { php_error_docref(NULL, E_WARNING, "Cannot destroy the zip context: %s", zip_strerror(intern->za)); @@ -1509,6 +1595,7 @@ PHP_METHOD(ZipArchive, open) RETURN_FALSE; } ze_obj->za = NULL; + php_zip_forget_modified_entries(ze_obj); } if (ze_obj->filename) { efree(ze_obj->filename); @@ -1588,6 +1675,8 @@ PHP_METHOD(ZipArchive, close) ze_obj = Z_ZIP_P(self); + php_zip_forget_modified_entries(ze_obj); + err = zip_close(intern); if (err) { php_error_docref(NULL, E_WARNING, "%s", zip_strerror(intern)); @@ -1740,6 +1829,7 @@ PHP_METHOD(ZipArchive, addEmptyDir) if ((Z_ZIP_P(self)->last_id = zip_dir_add(intern, (const char *)s, flags)) == -1) { RETVAL_FALSE; } else { + php_zip_mark_entry_modified(Z_ZIP_P(self), Z_ZIP_P(self)->last_id); zip_error_clear(intern); RETVAL_TRUE; } @@ -1999,6 +2089,7 @@ PHP_METHOD(ZipArchive, addFromString) zip_source_free(zs); RETURN_FALSE; } else { + php_zip_mark_entry_modified(ze_obj, ze_obj->last_id); zip_error_clear(intern); RETURN_TRUE; } @@ -2217,7 +2308,7 @@ PHP_METHOD(ZipArchive, setCommentName) if (idx < 0) { RETURN_FALSE; } - RETURN_BOOL(php_zip_set_file_comment(intern, (zip_uint64_t) idx, comment, comment_len)); + RETURN_BOOL(php_zip_set_file_comment(Z_ZIP_P(self), (zip_uint64_t) idx, comment, comment_len)); } /* }}} */ @@ -2247,7 +2338,7 @@ PHP_METHOD(ZipArchive, setCommentIndex) PHP_ZIP_STAT_INDEX(intern, index, 0, sb); idx = (zip_uint64_t) index; - RETURN_BOOL(php_zip_set_file_comment(intern, idx, comment, comment_len)); + RETURN_BOOL(php_zip_set_file_comment(Z_ZIP_P(self), idx, comment, comment_len)); } /* }}} */ @@ -2285,6 +2376,8 @@ PHP_METHOD(ZipArchive, setExternalAttributesName) (zip_uint8_t)(opsys&0xff), (zip_uint32_t)attr) < 0) { RETURN_FALSE; } + + php_zip_mark_entry_modified(Z_ZIP_P(self), idx); RETURN_TRUE; } /* }}} */ @@ -2309,6 +2402,8 @@ PHP_METHOD(ZipArchive, setExternalAttributesIndex) (zip_flags_t)flags, (zip_uint8_t)(opsys&0xff), (zip_uint32_t)attr) < 0) { RETURN_FALSE; } + + php_zip_mark_entry_modified(Z_ZIP_P(self), (zip_int64_t)index); RETURN_TRUE; } /* }}} */ @@ -2418,6 +2513,8 @@ PHP_METHOD(ZipArchive, setEncryptionName) if (zip_file_set_encryption(intern, idx, (zip_uint16_t)method, password)) { RETURN_FALSE; } + + php_zip_mark_entry_modified(Z_ZIP_P(self), idx); RETURN_TRUE; } /* }}} */ @@ -2446,6 +2543,8 @@ PHP_METHOD(ZipArchive, setEncryptionIndex) if (zip_file_set_encryption(intern, index, (zip_uint16_t)method, password)) { RETURN_FALSE; } + + php_zip_mark_entry_modified(Z_ZIP_P(self), (zip_int64_t)index); RETURN_TRUE; } /* }}} */ @@ -2541,6 +2640,8 @@ PHP_METHOD(ZipArchive, setCompressionName) (zip_int32_t)comp_method, (zip_uint32_t)comp_flags) != 0) { RETURN_FALSE; } + + php_zip_mark_entry_modified(Z_ZIP_P(this), idx); RETURN_TRUE; } /* }}} */ @@ -2564,6 +2665,8 @@ PHP_METHOD(ZipArchive, setCompressionIndex) (zip_int32_t)comp_method, (zip_uint32_t)comp_flags) != 0) { RETURN_FALSE; } + + php_zip_mark_entry_modified(Z_ZIP_P(this), (zip_int64_t)index); RETURN_TRUE; } /* }}} */ @@ -2601,6 +2704,8 @@ PHP_METHOD(ZipArchive, setMtimeName) (time_t)mtime, (zip_uint32_t)flags) != 0) { RETURN_FALSE; } + + php_zip_mark_entry_modified(Z_ZIP_P(this), idx); RETURN_TRUE; } /* }}} */ @@ -2624,6 +2729,8 @@ PHP_METHOD(ZipArchive, setMtimeIndex) (time_t)mtime, (zip_uint32_t)flags) != 0) { RETURN_FALSE; } + + php_zip_mark_entry_modified(Z_ZIP_P(this), (zip_int64_t)index); RETURN_TRUE; } /* }}} */ @@ -2709,6 +2816,8 @@ PHP_METHOD(ZipArchive, renameIndex) RETURN_FALSE; } + php_zip_mark_entry_modified(Z_ZIP_P(self), (zip_int64_t)index); + RETURN_TRUE; } /* }}} */ @@ -2739,6 +2848,8 @@ PHP_METHOD(ZipArchive, renameName) RETURN_FALSE; } + php_zip_mark_entry_modified(Z_ZIP_P(self), (zip_int64_t)sb.index); + RETURN_TRUE; } /* }}} */ @@ -2762,9 +2873,10 @@ PHP_METHOD(ZipArchive, unchangeIndex) if (zip_unchange(intern, index) != 0) { RETURN_FALSE; - } else { - RETURN_TRUE; } + + php_zip_unmark_entry_modified(Z_ZIP_P(self), (zip_int64_t)index); + RETURN_TRUE; } /* }}} */ @@ -2791,9 +2903,10 @@ PHP_METHOD(ZipArchive, unchangeName) if (zip_unchange(intern, sb.index) != 0) { RETURN_FALSE; - } else { - RETURN_TRUE; } + + php_zip_unmark_entry_modified(Z_ZIP_P(self), (zip_int64_t)sb.index); + RETURN_TRUE; } /* }}} */ @@ -2811,9 +2924,10 @@ PHP_METHOD(ZipArchive, unchangeAll) if (zip_unchange_all(intern) != 0) { RETURN_FALSE; - } else { - RETURN_TRUE; } + + php_zip_forget_modified_entries(Z_ZIP_P(self)); + RETURN_TRUE; } /* }}} */ @@ -2829,6 +2943,7 @@ PHP_METHOD(ZipArchive, unchangeArchive) ZIP_FROM_OBJECT(intern, self); + /* Only the archive level changes are reverted, the entries stay modified. */ if (zip_unchange_archive(intern) != 0) { RETURN_FALSE; } else { diff --git a/ext/zip/php_zip.h b/ext/zip/php_zip.h index 18f5c4eddd1e..6b5faab19bb8 100644 --- a/ext/zip/php_zip.h +++ b/ext/zip/php_zip.h @@ -74,6 +74,11 @@ typedef struct _ze_zip_object { int filename_len; int buffers_cnt; zip_int64_t last_id; + /* Indices of the entries libzip has copied the central directory record of, + * because they were modified. Such a copy shares its comment with the record + * of the entry, which makes setting the comment unsafe. Lazily allocated, + * see php_zip_set_file_comment(). */ + HashTable *modified_entries; int err_zip; int err_sys; #ifdef HAVE_PROGRESS_CALLBACK diff --git a/ext/zip/tests/gh22885.phpt b/ext/zip/tests/gh22885.phpt new file mode 100644 index 000000000000..b9d8e51b2929 --- /dev/null +++ b/ext/zip/tests/gh22885.phpt @@ -0,0 +1,106 @@ +--TEST-- +GH-22885 (SEGV when setting the comment of a modified entry) +--EXTENSIONS-- +zip +--FILE-- +open($file, ZipArchive::CREATE)) { + die('failed'); +} +foreach ($entries as $entry) { + $zip->addFromString($entry . '.txt', 'data of ' . $entry); + $zip->setCommentName($entry . '.txt', 'comment of ' . $entry); +} +$zip->close(); + +/* The comment of an entry that has been modified cannot be changed: libzip + * shares it with the copy it made of the central directory record of the entry. */ +if (!$zip->open($file)) { + die('failed'); +} + +var_dump($zip->addFromString('replaced.txt', 'new data of replaced')); +var_dump($zip->setCommentName('replaced.txt', 'new comment of replaced')); + +var_dump($zip->renameName('renamed.txt', 'renamed2.txt')); +var_dump($zip->setCommentName('renamed2.txt', 'new comment of renamed')); + +var_dump($zip->setMtimeName('mtime.txt', 1000000)); +var_dump($zip->setCommentName('mtime.txt', 'new comment of mtime')); + +var_dump($zip->setCompressionName('compressed.txt', ZipArchive::CM_STORE)); +var_dump($zip->setCommentName('compressed.txt', 'new comment of compressed')); + +var_dump($zip->setExternalAttributesName('attributes.txt', ZipArchive::OPSYS_UNIX, 0100644 << 16)); +var_dump($zip->setCommentName('attributes.txt', 'new comment of attributes')); + +var_dump($zip->setMtimeName('removed.txt', 1000000)); +var_dump($zip->setCommentName('removed.txt', '')); + +/* Setting or removing the comment first works, and it survives the modification. */ +var_dump($zip->setCommentName('cleared.txt', '')); +var_dump($zip->setMtimeName('cleared.txt', 1000000)); + +var_dump($zip->setCommentName('untouched.txt', 'new comment of untouched')); +var_dump($zip->setMtimeName('untouched.txt', 1000000)); + +$zip->close(); + +if (!$zip->open($file)) { + die('failed'); +} +for ($i = 0; $i < $zip->numFiles; $i++) { + printf("%s: %s\n", $zip->getNameIndex($i), $zip->getCommentIndex($i)); +} +var_dump($zip->getFromName('replaced.txt')); +$zip->close(); +?> +--EXPECTF-- +bool(true) + +Warning: ZipArchive::setCommentName(): Cannot set the comment of an entry that has been modified, set the comment before modifying the entry in %s on line %d +bool(false) +bool(true) + +Warning: ZipArchive::setCommentName(): Cannot set the comment of an entry that has been modified, set the comment before modifying the entry in %s on line %d +bool(false) +bool(true) + +Warning: ZipArchive::setCommentName(): Cannot set the comment of an entry that has been modified, set the comment before modifying the entry in %s on line %d +bool(false) +bool(true) + +Warning: ZipArchive::setCommentName(): Cannot set the comment of an entry that has been modified, set the comment before modifying the entry in %s on line %d +bool(false) +bool(true) + +Warning: ZipArchive::setCommentName(): Cannot set the comment of an entry that has been modified, set the comment before modifying the entry in %s on line %d +bool(false) +bool(true) + +Warning: ZipArchive::setCommentName(): Cannot set the comment of an entry that has been modified, set the comment before modifying the entry in %s on line %d +bool(false) +bool(true) +bool(true) +bool(true) +bool(true) +replaced.txt: comment of replaced +renamed2.txt: comment of renamed +mtime.txt: comment of mtime +compressed.txt: comment of compressed +attributes.txt: comment of attributes +removed.txt: comment of removed +cleared.txt: +untouched.txt: new comment of untouched +string(20) "new data of replaced" +--CLEAN-- + diff --git a/ext/zip/tests/gh22885_unchange.phpt b/ext/zip/tests/gh22885_unchange.phpt new file mode 100644 index 000000000000..240126270ed6 --- /dev/null +++ b/ext/zip/tests/gh22885_unchange.phpt @@ -0,0 +1,66 @@ +--TEST-- +GH-22885 (unchangeArchive() must not drop the modified entry bookkeeping) +--EXTENSIONS-- +zip +--FILE-- +open($file, ZipArchive::CREATE)) { + die('failed'); +} +$zip->addFromString('a.txt', 'data of a'); +$zip->setCommentName('a.txt', 'comment of a'); +$zip->addFromString('b.txt', 'data of b'); +$zip->setCommentName('b.txt', 'comment of b'); +$zip->setArchiveComment('archive comment'); +$zip->close(); + +if (!$zip->open($file)) { + die('failed'); +} + +/* unchangeArchive() only reverts the archive level changes, the entries stay + * modified, so the comment of a modified entry must still be refused. */ +var_dump($zip->setMtimeName('a.txt', 1000000)); +var_dump($zip->setArchiveComment('new archive comment')); +var_dump($zip->unchangeArchive()); +var_dump($zip->getArchiveComment()); +var_dump($zip->setCommentName('a.txt', 'new comment of a')); + +/* unchangeName() does revert the entry, so its comment can be set again. */ +var_dump($zip->setMtimeName('b.txt', 1000000)); +var_dump($zip->unchangeName('b.txt')); +var_dump($zip->setCommentName('b.txt', 'new comment of b')); + +$zip->close(); + +if (!$zip->open($file)) { + die('failed'); +} +var_dump($zip->getArchiveComment()); +var_dump($zip->getCommentName('a.txt')); +var_dump($zip->getCommentName('b.txt')); +$zip->close(); +?> +--EXPECTF-- +bool(true) +bool(true) +bool(true) +string(15) "archive comment" + +Warning: ZipArchive::setCommentName(): Cannot set the comment of an entry that has been modified, set the comment before modifying the entry in %s on line %d +bool(false) +bool(true) +bool(true) +bool(true) +string(15) "archive comment" +string(12) "comment of a" +string(16) "new comment of b" +--CLEAN-- +