diff --git a/NEWS b/NEWS index 0d9e42792078..f4b3332a8413 100644 --- a/NEWS +++ b/NEWS @@ -6,6 +6,10 @@ PHP NEWS . Fixed bug GH-15375 (Nested "yield from" skips items after a valid() or next() call on the inner generator). (iliaal) +- Zip: + . Fixed ZipArchive::extractTo() and ZipArchive::getFrom*() reporting success + on corrupted entries. (David Carlier) + 27 Aug 2026, PHP 8.4.25 - Core: diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index fecb9396ace9..8e3a1f3e131f 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -135,6 +135,30 @@ static char * php_zip_make_relative_path(char *path, size_t path_len) /* {{{ */ # define CWD_STATE_ALLOC(l) emalloc(l) # define CWD_STATE_FREE(s) efree(s) +/* {{{ php_zip_file_error + Entry error code, plus its message when message is not NULL. + zip_error_t and its accessors only exist since libzip 1.0. */ +static int php_zip_file_error(struct zip_file *zf, const char **message) +{ +#if LIBZIP_VERSION_MAJOR < 1 + int zep, syp; + + zip_file_error_get(zf, &zep, &syp); + if (message) { + *message = zip_file_strerror(zf); + } + return zep; +#else + zip_error_t *err = zip_file_get_error(zf); + + if (message) { + *message = zip_error_strerror(err); + } + return zip_error_code_zip(err); +#endif +} +/* }}} */ + /* {{{ php_zip_extract_file */ static int php_zip_extract_file(struct zip * za, char *dest, const char *file, size_t file_len, zip_int64_t idx) { @@ -268,7 +292,21 @@ static int php_zip_extract_file(struct zip * za, char *dest, const char *file, s n = 0; while ((n=zip_fread(zf, b, sizeof(b))) > 0) { - php_stream_write(stream, b, n); + if (php_stream_write(stream, b, n) != n) { + n = -1; + break; + } + } + + if (n < 0) { + const char *message; + + if (php_zip_file_error(zf, &message) != ZIP_ER_OK) { + php_error_docref(NULL, E_WARNING, "Cannot extract \"%s\": \"%s\"", file, message); + } + php_stream_close(stream); + zip_fclose(zf); + goto done; } if (stream->wrapper->wops->stream_metadata) { @@ -279,7 +317,7 @@ static int php_zip_extract_file(struct zip * za, char *dest, const char *file, s } php_stream_close(stream); - n = zip_fclose(zf); + n = zip_fclose(zf) == 0 ? 0 : -1; done: efree(fullpath); @@ -2953,10 +2991,6 @@ static void php_zip_get_from(INTERNAL_FUNCTION_PARAMETERS, int type) /* {{{ */ PHP_ZIP_STAT_INDEX(intern, index, flags, sb); } - if (sb.size < 1) { - RETURN_EMPTY_STRING(); - } - if (len < 1) { len = sb.size; } @@ -2971,8 +3005,40 @@ static void php_zip_get_from(INTERNAL_FUNCTION_PARAMETERS, int type) /* {{{ */ } buffer = zend_string_safe_alloc(1, len, 0, 0); - zip_int64_t n = zip_fread(zf, ZSTR_VAL(buffer), ZSTR_LEN(buffer)); - if (n < 1) { + + /* zip_fread() may return short reads, a truncated entry must not pass for a complete one. */ + zip_int64_t n = 0; + while ((zip_uint64_t)n < ZSTR_LEN(buffer)) { + zip_int64_t rd = zip_fread(zf, ZSTR_VAL(buffer) + n, ZSTR_LEN(buffer) - n); + + if (rd < 0) { + n = -1; + break; + } + if (rd == 0) { + break; + } + n += rd; + } + + if (n >= 0 && (zip_uint64_t)n == sb.size) { + /* The whole entry has been consumed, read past its last byte so that + * libzip reaches the end of the stream and validates the CRC. */ + char tmp; + if (zip_fread(zf, &tmp, 1) < 0) { + n = -1; + } + } + if (n < 0) { + const char *message; + + php_zip_file_error(zf, &message); + php_error_docref(NULL, E_WARNING, "Cannot read entry: %s", message); + zip_fclose(zf); + zend_string_efree(buffer); + RETURN_FALSE; + } + if (n == 0) { zip_fclose(zf); zend_string_efree(buffer); RETURN_EMPTY_STRING(); diff --git a/ext/zip/tests/oo_extract_crc.phpt b/ext/zip/tests/oo_extract_crc.phpt new file mode 100644 index 000000000000..bcb31185c139 --- /dev/null +++ b/ext/zip/tests/oo_extract_crc.phpt @@ -0,0 +1,44 @@ +--TEST-- +ZipArchive::extractTo() and getFrom*() fail on a CRC-corrupted entry +--EXTENSIONS-- +zip +--FILE-- +open($file, ZipArchive::CREATE | ZipArchive::OVERWRITE); +$zip->addFromString('a.txt', $payload); +$zip->setCompressionName('a.txt', ZipArchive::CM_STORE); +$zip->close(); + +$raw = file_get_contents($file); +$raw[strpos($raw, 'AAAA') + 2] = 'Z'; +file_put_contents($file, $raw); + +$zip = new ZipArchive(); +$zip->open($file); +var_dump($zip->extractTo($dirname, 'a.txt')); +var_dump($zip->getFromName('a.txt')); +var_dump($zip->getFromIndex(0)); +$zip->close(); +?> +--CLEAN-- + +--EXPECTF-- +Warning: ZipArchive::extractTo(): Cannot extract "a.txt": "CRC error" in %s on line %d +bool(false) + +Warning: ZipArchive::getFromName(): Cannot read entry: CRC error in %s on line %d +bool(false) + +Warning: ZipArchive::getFromIndex(): Cannot read entry: CRC error in %s on line %d +bool(false) diff --git a/ext/zip/tests/oo_get_from_crc_empty.phpt b/ext/zip/tests/oo_get_from_crc_empty.phpt new file mode 100644 index 000000000000..5e48059701e8 --- /dev/null +++ b/ext/zip/tests/oo_get_from_crc_empty.phpt @@ -0,0 +1,52 @@ +--TEST-- +ZipArchive::getFrom*() rejects a CRC-corrupted empty entry +--EXTENSIONS-- +zip +--SKIPIF-- + +--FILE-- +open($file, ZipArchive::CREATE | ZipArchive::OVERWRITE); +$zip->addFromString('empty.txt', ''); +$zip->setCompressionName('empty.txt', ZipArchive::CM_STORE); +$zip->close(); + +/* Corrupt the CRC in both the local and central directory headers. */ +$raw = file_get_contents($file); +for ($i = 0, $length = strlen($raw); $i + 3 < $length; $i++) { + $signature = substr($raw, $i, 4); + if ($signature === "PK\x03\x04") { + $raw[$i + 14] = "\x01"; + } elseif ($signature === "PK\x01\x02") { + $raw[$i + 16] = "\x01"; + } +} +file_put_contents($file, $raw); + +$zip = new ZipArchive(); +$zip->open($file); +var_dump($zip->getFromName('empty.txt')); +var_dump($zip->getFromIndex(0)); +$zip->close(); +?> +--CLEAN-- + +--EXPECTF-- +Warning: ZipArchive::getFromName(): Cannot read entry: CRC error in %s on line %d +bool(false) + +Warning: ZipArchive::getFromIndex(): Cannot read entry: CRC error in %s on line %d +bool(false) diff --git a/ext/zip/tests/oo_get_from_length.phpt b/ext/zip/tests/oo_get_from_length.phpt new file mode 100644 index 000000000000..9eae83f3fec2 --- /dev/null +++ b/ext/zip/tests/oo_get_from_length.phpt @@ -0,0 +1,46 @@ +--TEST-- +ZipArchive::getFrom*() rejects an entry with an inconsistent uncompressed size +--EXTENSIONS-- +zip +--FILE-- +open($file, ZipArchive::CREATE | ZipArchive::OVERWRITE); +$zip->addFromString('a.txt', str_repeat('A', 10)); +$zip->setCompressionName('a.txt', ZipArchive::CM_STORE); +$zip->close(); + +/* Advertise 20 bytes in the central directory, but keep only 10 bytes. */ +$raw = file_get_contents($file); +for ($i = 0, $length = strlen($raw); $i + 27 < $length; $i++) { + if (substr($raw, $i, 4) === "PK\x01\x02") { + $size = unpack('V', substr($raw, $i + 24, 4))[1]; + $raw = substr_replace($raw, pack('V', $size + 10), $i + 24, 4); + break; + } +} +file_put_contents($file, $raw); + +$zip = new ZipArchive(); +$zip->open($file); +var_dump($zip->getFromName('a.txt')); +var_dump($zip->getFromIndex(0)); +$zip->close(); +?> +--CLEAN-- + +--EXPECTF-- +Warning: ZipArchive::getFromName(): Cannot read entry: Zip archive inconsistent%s +bool(false) + +Warning: ZipArchive::getFromIndex(): Cannot read entry: Zip archive inconsistent%s +bool(false)