Skip to content

Commit d1b1c04

Browse files
cmb69cmb69
committed
Fix #77565: Incorrect locator detection in ZIP-based phars
We must not assume that the first end of central dir signature in a ZIP archive actually designates the end of central directory record, since the data in the archive may contain arbitrary byte patterns. Thus, we better search from the end of the data, what is also slightly more efficient. There is, however, no way to detect the end of central directory signature by searching from the end of the ZIP archive with absolute certainty, since the signature could be part of the trailing comment. To mitigate, we check that the comment length fits to the found position, but that might still not be the correct position in rare cases. Closes GH-6507.
1 parent 5c96373 commit d1b1c04

File tree

6 files changed

+77
-40
lines changed

6 files changed

+77
-40
lines changed

‎NEWS

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,9 @@ PHP NEWS
2020
. Fixed bug #77935 (Crash in mysqlnd_fetch_stmt_row_cursor when calling an SP
2121
with a cursor). (Nikita)
2222

23+
- Phar:
24+
. Fixed bug #77565 (Incorrect locator detection in ZIP-based phars). (cmb)
25+
2326
07 Jan 2021, PHP 7.4.14
2427

2528
- Core:

‎ext/phar/tests/bug69441.phpt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ $r = new Phar($fname, 0);
1414

1515
==DONE==
1616
--EXPECTF--
17-
UnexpectedValueException: phar error: corrupted central directory entry, no magic signature in zip-based phar "%sbug69441.phar" in %sbug69441.php:%d
17+
UnexpectedValueException: phar error: end of central directory not found in zip-based phar "%sbug69441.phar" in %sbug69441.php:%d
1818
Stack trace:
1919
#0 %s%ebug69441.php(%d): Phar->__construct('%s', 0)
2020
#1 {main}

‎ext/phar/tests/bug77565.phpt

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
--TEST--
2+
Bug #77565 (Incorrect locator detection in ZIP-based phars)
3+
--SKIPIF--
4+
<?php
5+
if (!extension_loaded('phar')) die('skip phar extension not available');
6+
?>
7+
--FILE--
8+
<?php
9+
$phar = new PharData(__DIR__ . '/bug77565.zip');
10+
var_dump($phar['1.zip']->getFilename());
11+
?>
12+
--EXPECT--
13+
string(5) "1.zip"

‎ext/phar/tests/bug77565.zip

318 Bytes
Binary file not shown.

‎ext/phar/tests/zip/corrupt_003.phpt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,5 +12,5 @@ try {
1212
?>
1313
===DONE===
1414
--EXPECTF--
15-
phar error: corrupt zip archive, zip file comment truncated in zip-based phar "%sfilecomment.zip"
15+
phar error: end of central directory not found in zip-based phar "%sfilecomment.zip"
1616
===DONE===

‎ext/phar/zip.c

Lines changed: 59 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,29 @@ static void phar_zip_u2d_time(time_t time, char *dtime, char *ddate) /* {{{ */
161161
}
162162
/* }}} */
163163

164+
static char *phar_find_eocd(const char *s, size_t n)
165+
{
166+
const char *end = s + n + sizeof("PK\5\6") - 1 - sizeof(phar_zip_dir_end);
167+
168+
/* search backwards for end of central directory signatures */
169+
do {
170+
uint16_t comment_len;
171+
const char *eocd_start = zend_memnrstr(s, "PK\5\6", sizeof("PK\5\6") - 1, end);
172+
173+
if (eocd_start == NULL) {
174+
return NULL;
175+
}
176+
ZEND_ASSERT(eocd_start + sizeof(phar_zip_dir_end) <= s + n);
177+
comment_len = PHAR_GET_16(((phar_zip_dir_end *) eocd_start)->comment_len);
178+
if (eocd_start + sizeof(phar_zip_dir_end) + comment_len == s + n) {
179+
/* we can't be sure, but this looks like the proper EOCD signature */
180+
return (char *) eocd_start;
181+
}
182+
end = eocd_start;
183+
} while (end > s);
184+
return NULL;
185+
}
186+
164187
/**
165188
* Does not check for a previously opened phar in the cache.
166189
*
@@ -205,57 +228,55 @@ int phar_parse_zipfile(php_stream *fp, char *fname, size_t fname_len, char *alia
205228
return FAILURE;
206229
}
207230

208-
while ((p=(char *) memchr(p + 1, 'P', (size_t) (size - (p + 1 - buf)))) != NULL) {
209-
if ((p - buf) + sizeof(locator) <= (size_t)size && !memcmp(p + 1, "K\5\6", 3)) {
210-
memcpy((void *)&locator, (void *) p, sizeof(locator));
211-
if (PHAR_GET_16(locator.centraldisk) != 0 || PHAR_GET_16(locator.disknumber) != 0) {
212-
/* split archives not handled */
213-
php_stream_close(fp);
214-
if (error) {
215-
spprintf(error, 4096, "phar error: split archives spanning multiple zips cannot be processed in zip-based phar \"%s\"", fname);
216-
}
217-
return FAILURE;
231+
if ((p = phar_find_eocd(buf, size)) != NULL) {
232+
memcpy((void *)&locator, (void *) p, sizeof(locator));
233+
if (PHAR_GET_16(locator.centraldisk) != 0 || PHAR_GET_16(locator.disknumber) != 0) {
234+
/* split archives not handled */
235+
php_stream_close(fp);
236+
if (error) {
237+
spprintf(error, 4096, "phar error: split archives spanning multiple zips cannot be processed in zip-based phar \"%s\"", fname);
218238
}
239+
return FAILURE;
240+
}
219241

220-
if (PHAR_GET_16(locator.counthere) != PHAR_GET_16(locator.count)) {
221-
if (error) {
222-
spprintf(error, 4096, "phar error: corrupt zip archive, conflicting file count in end of central directory record in zip-based phar \"%s\"", fname);
223-
}
224-
php_stream_close(fp);
225-
return FAILURE;
242+
if (PHAR_GET_16(locator.counthere) != PHAR_GET_16(locator.count)) {
243+
if (error) {
244+
spprintf(error, 4096, "phar error: corrupt zip archive, conflicting file count in end of central directory record in zip-based phar \"%s\"", fname);
226245
}
246+
php_stream_close(fp);
247+
return FAILURE;
248+
}
227249

228-
mydata = pecalloc(1, sizeof(phar_archive_data), PHAR_G(persist));
229-
mydata->is_persistent = PHAR_G(persist);
250+
mydata = pecalloc(1, sizeof(phar_archive_data), PHAR_G(persist));
251+
mydata->is_persistent = PHAR_G(persist);
230252

231-
/* read in archive comment, if any */
232-
if (PHAR_GET_16(locator.comment_len)) {
253+
/* read in archive comment, if any */
254+
if (PHAR_GET_16(locator.comment_len)) {
233255

234-
metadata = p + sizeof(locator);
256+
metadata = p + sizeof(locator);
235257

236-
if (PHAR_GET_16(locator.comment_len) != size - (metadata - buf)) {
237-
if (error) {
238-
spprintf(error, 4096, "phar error: corrupt zip archive, zip file comment truncated in zip-based phar \"%s\"", fname);
239-
}
240-
php_stream_close(fp);
241-
pefree(mydata, mydata->is_persistent);
242-
return FAILURE;
258+
if (PHAR_GET_16(locator.comment_len) != size - (metadata - buf)) {
259+
if (error) {
260+
spprintf(error, 4096, "phar error: corrupt zip archive, zip file comment truncated in zip-based phar \"%s\"", fname);
243261
}
262+
php_stream_close(fp);
263+
pefree(mydata, mydata->is_persistent);
264+
return FAILURE;
265+
}
244266

245-
mydata->metadata_len = PHAR_GET_16(locator.comment_len);
267+
mydata->metadata_len = PHAR_GET_16(locator.comment_len);
246268

247-
if (phar_parse_metadata(&metadata, &mydata->metadata, PHAR_GET_16(locator.comment_len)) == FAILURE) {
248-
mydata->metadata_len = 0;
249-
/* if not valid serialized data, it is a regular string */
269+
if (phar_parse_metadata(&metadata, &mydata->metadata, PHAR_GET_16(locator.comment_len)) == FAILURE) {
270+
mydata->metadata_len = 0;
271+
/* if not valid serialized data, it is a regular string */
250272

251-
ZVAL_NEW_STR(&mydata->metadata, zend_string_init(metadata, PHAR_GET_16(locator.comment_len), mydata->is_persistent));
252-
}
253-
} else {
254-
ZVAL_UNDEF(&mydata->metadata);
273+
ZVAL_NEW_STR(&mydata->metadata, zend_string_init(metadata, PHAR_GET_16(locator.comment_len), mydata->is_persistent));
255274
}
256-
257-
goto foundit;
275+
} else {
276+
ZVAL_UNDEF(&mydata->metadata);
258277
}
278+
279+
goto foundit;
259280
}
260281

261282
php_stream_close(fp);

0 commit comments

Comments
 (0)