Skip to content

Commit 2a2b337

Browse files
devnexendevnexen
committed
ext/zip: fix extractTo()/getFrom*() success on CRC-corrupted entries
zip_fclose() returns positive error codes, so the extraction's n < 0 check treated a failed read as success and wrote corrupt data to disk. getFromName()/getFromIndex() read exactly sb.size bytes, never reaching end of file where libzip validates the CRC, and returned the corrupt data silently. Read errors now fail with a warning. Close GH-23240
1 parent 5a326b9 commit 2a2b337

5 files changed

Lines changed: 220 additions & 8 deletions

File tree

‎NEWS‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@ PHP NEWS
66
. Fixed bug GH-15375 (Nested "yield from" skips items after a valid() or
77
next() call on the inner generator). (iliaal)
88

9+
- Zip:
10+
. Fixed ZipArchive::extractTo() and ZipArchive::getFrom*() reporting success
11+
on corrupted entries. (David Carlier)
12+
913
27 Aug 2026, PHP 8.4.25
1014

1115
- Core:

‎ext/zip/php_zip.c‎

Lines changed: 74 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,30 @@ static char * php_zip_make_relative_path(char *path, size_t path_len) /* {{{ */
135135
# define CWD_STATE_ALLOC(l) emalloc(l)
136136
# define CWD_STATE_FREE(s) efree(s)
137137

138+
/* {{{ php_zip_file_error
139+
Entry error code, plus its message when message is not NULL.
140+
zip_error_t and its accessors only exist since libzip 1.0. */
141+
static int php_zip_file_error(struct zip_file *zf, const char **message)
142+
{
143+
#if LIBZIP_VERSION_MAJOR < 1
144+
int zep, syp;
145+
146+
zip_file_error_get(zf, &zep, &syp);
147+
if (message) {
148+
*message = zip_file_strerror(zf);
149+
}
150+
return zep;
151+
#else
152+
zip_error_t *err = zip_file_get_error(zf);
153+
154+
if (message) {
155+
*message = zip_error_strerror(err);
156+
}
157+
return zip_error_code_zip(err);
158+
#endif
159+
}
160+
/* }}} */
161+
138162
/* {{{ php_zip_extract_file */
139163
static int php_zip_extract_file(struct zip * za, char *dest, const char *file, size_t file_len, zip_int64_t idx)
140164
{
@@ -268,7 +292,21 @@ static int php_zip_extract_file(struct zip * za, char *dest, const char *file, s
268292
n = 0;
269293

270294
while ((n=zip_fread(zf, b, sizeof(b))) > 0) {
271-
php_stream_write(stream, b, n);
295+
if (php_stream_write(stream, b, n) != n) {
296+
n = -1;
297+
break;
298+
}
299+
}
300+
301+
if (n < 0) {
302+
const char *message;
303+
304+
if (php_zip_file_error(zf, &message) != ZIP_ER_OK) {
305+
php_error_docref(NULL, E_WARNING, "Cannot extract \"%s\": \"%s\"", file, message);
306+
}
307+
php_stream_close(stream);
308+
zip_fclose(zf);
309+
goto done;
272310
}
273311

274312
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
279317
}
280318

281319
php_stream_close(stream);
282-
n = zip_fclose(zf);
320+
n = zip_fclose(zf) == 0 ? 0 : -1;
283321

284322
done:
285323
efree(fullpath);
@@ -2953,10 +2991,6 @@ static void php_zip_get_from(INTERNAL_FUNCTION_PARAMETERS, int type) /* {{{ */
29532991
PHP_ZIP_STAT_INDEX(intern, index, flags, sb);
29542992
}
29552993

2956-
if (sb.size < 1) {
2957-
RETURN_EMPTY_STRING();
2958-
}
2959-
29602994
if (len < 1) {
29612995
len = sb.size;
29622996
}
@@ -2971,8 +3005,40 @@ static void php_zip_get_from(INTERNAL_FUNCTION_PARAMETERS, int type) /* {{{ */
29713005
}
29723006

29733007
buffer = zend_string_safe_alloc(1, len, 0, 0);
2974-
zip_int64_t n = zip_fread(zf, ZSTR_VAL(buffer), ZSTR_LEN(buffer));
2975-
if (n < 1) {
3008+
3009+
/* zip_fread() may return short reads, a truncated entry must not pass for a complete one. */
3010+
zip_int64_t n = 0;
3011+
while ((zip_uint64_t)n < ZSTR_LEN(buffer)) {
3012+
zip_int64_t rd = zip_fread(zf, ZSTR_VAL(buffer) + n, ZSTR_LEN(buffer) - n);
3013+
3014+
if (rd < 0) {
3015+
n = -1;
3016+
break;
3017+
}
3018+
if (rd == 0) {
3019+
break;
3020+
}
3021+
n += rd;
3022+
}
3023+
3024+
if (n >= 0 && (zip_uint64_t)n == sb.size) {
3025+
/* The whole entry has been consumed, read past its last byte so that
3026+
* libzip reaches the end of the stream and validates the CRC. */
3027+
char tmp;
3028+
if (zip_fread(zf, &tmp, 1) < 0) {
3029+
n = -1;
3030+
}
3031+
}
3032+
if (n < 0) {
3033+
const char *message;
3034+
3035+
php_zip_file_error(zf, &message);
3036+
php_error_docref(NULL, E_WARNING, "Cannot read entry: %s", message);
3037+
zip_fclose(zf);
3038+
zend_string_efree(buffer);
3039+
RETURN_FALSE;
3040+
}
3041+
if (n == 0) {
29763042
zip_fclose(zf);
29773043
zend_string_efree(buffer);
29783044
RETURN_EMPTY_STRING();

‎ext/zip/tests/oo_extract_crc.phpt‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
--TEST--
2+
ZipArchive::extractTo() and getFrom*() fail on a CRC-corrupted entry
3+
--EXTENSIONS--
4+
zip
5+
--FILE--
6+
<?php
7+
$dirname = __DIR__ . '/oo_extract_crc_dir';
8+
mkdir($dirname);
9+
$file = $dirname . '/corrupt.zip';
10+
$payload = str_repeat('A', 64) . 'PAYLOAD-END';
11+
12+
$zip = new ZipArchive();
13+
$zip->open($file, ZipArchive::CREATE | ZipArchive::OVERWRITE);
14+
$zip->addFromString('a.txt', $payload);
15+
$zip->setCompressionName('a.txt', ZipArchive::CM_STORE);
16+
$zip->close();
17+
18+
$raw = file_get_contents($file);
19+
$raw[strpos($raw, 'AAAA') + 2] = 'Z';
20+
file_put_contents($file, $raw);
21+
22+
$zip = new ZipArchive();
23+
$zip->open($file);
24+
var_dump($zip->extractTo($dirname, 'a.txt'));
25+
var_dump($zip->getFromName('a.txt'));
26+
var_dump($zip->getFromIndex(0));
27+
$zip->close();
28+
?>
29+
--CLEAN--
30+
<?php
31+
$dirname = __DIR__ . '/oo_extract_crc_dir';
32+
@unlink($dirname . '/a.txt');
33+
@unlink($dirname . '/corrupt.zip');
34+
@rmdir($dirname);
35+
?>
36+
--EXPECTF--
37+
Warning: ZipArchive::extractTo(): Cannot extract "a.txt": "CRC error" in %s on line %d
38+
bool(false)
39+
40+
Warning: ZipArchive::getFromName(): Cannot read entry: CRC error in %s on line %d
41+
bool(false)
42+
43+
Warning: ZipArchive::getFromIndex(): Cannot read entry: CRC error in %s on line %d
44+
bool(false)

‎ext/zip/tests/oo_get_from_crc_empty.phpt‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
--TEST--
2+
ZipArchive::getFrom*() rejects a CRC-corrupted empty entry
3+
--EXTENSIONS--
4+
zip
5+
--SKIPIF--
6+
<?php
7+
/* libzip < 1.10.0 shortcuts empty entries and never checks their CRC. */
8+
if (version_compare(ZipArchive::LIBZIP_VERSION, '1.10.0', '<')) die('skip libzip < 1.10.0');
9+
?>
10+
--FILE--
11+
<?php
12+
$dirname = __DIR__ . '/oo_get_from_crc_empty_dir';
13+
mkdir($dirname);
14+
$file = $dirname . '/corrupt.zip';
15+
16+
$zip = new ZipArchive();
17+
$zip->open($file, ZipArchive::CREATE | ZipArchive::OVERWRITE);
18+
$zip->addFromString('empty.txt', '');
19+
$zip->setCompressionName('empty.txt', ZipArchive::CM_STORE);
20+
$zip->close();
21+
22+
/* Corrupt the CRC in both the local and central directory headers. */
23+
$raw = file_get_contents($file);
24+
for ($i = 0, $length = strlen($raw); $i + 3 < $length; $i++) {
25+
$signature = substr($raw, $i, 4);
26+
if ($signature === "PK\x03\x04") {
27+
$raw[$i + 14] = "\x01";
28+
} elseif ($signature === "PK\x01\x02") {
29+
$raw[$i + 16] = "\x01";
30+
}
31+
}
32+
file_put_contents($file, $raw);
33+
34+
$zip = new ZipArchive();
35+
$zip->open($file);
36+
var_dump($zip->getFromName('empty.txt'));
37+
var_dump($zip->getFromIndex(0));
38+
$zip->close();
39+
?>
40+
--CLEAN--
41+
<?php
42+
$dirname = __DIR__ . '/oo_get_from_crc_empty_dir';
43+
@unlink($dirname . '/empty.txt');
44+
@unlink($dirname . '/corrupt.zip');
45+
@rmdir($dirname);
46+
?>
47+
--EXPECTF--
48+
Warning: ZipArchive::getFromName(): Cannot read entry: CRC error in %s on line %d
49+
bool(false)
50+
51+
Warning: ZipArchive::getFromIndex(): Cannot read entry: CRC error in %s on line %d
52+
bool(false)

‎ext/zip/tests/oo_get_from_length.phpt‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
--TEST--
2+
ZipArchive::getFrom*() rejects an entry with an inconsistent uncompressed size
3+
--EXTENSIONS--
4+
zip
5+
--FILE--
6+
<?php
7+
$dirname = __DIR__ . '/oo_get_from_length_dir';
8+
mkdir($dirname);
9+
$file = $dirname . '/inconsistent.zip';
10+
11+
$zip = new ZipArchive();
12+
$zip->open($file, ZipArchive::CREATE | ZipArchive::OVERWRITE);
13+
$zip->addFromString('a.txt', str_repeat('A', 10));
14+
$zip->setCompressionName('a.txt', ZipArchive::CM_STORE);
15+
$zip->close();
16+
17+
/* Advertise 20 bytes in the central directory, but keep only 10 bytes. */
18+
$raw = file_get_contents($file);
19+
for ($i = 0, $length = strlen($raw); $i + 27 < $length; $i++) {
20+
if (substr($raw, $i, 4) === "PK\x01\x02") {
21+
$size = unpack('V', substr($raw, $i + 24, 4))[1];
22+
$raw = substr_replace($raw, pack('V', $size + 10), $i + 24, 4);
23+
break;
24+
}
25+
}
26+
file_put_contents($file, $raw);
27+
28+
$zip = new ZipArchive();
29+
$zip->open($file);
30+
var_dump($zip->getFromName('a.txt'));
31+
var_dump($zip->getFromIndex(0));
32+
$zip->close();
33+
?>
34+
--CLEAN--
35+
<?php
36+
$dirname = __DIR__ . '/oo_get_from_length_dir';
37+
@unlink($dirname . '/a.txt');
38+
@unlink($dirname . '/inconsistent.zip');
39+
@rmdir($dirname);
40+
?>
41+
--EXPECTF--
42+
Warning: ZipArchive::getFromName(): Cannot read entry: Zip archive inconsistent%s
43+
bool(false)
44+
45+
Warning: ZipArchive::getFromIndex(): Cannot read entry: Zip archive inconsistent%s
46+
bool(false)

0 commit comments

Comments
 (0)