Skip to content

Fix #77565: Incorrect locator detection in ZIP-based phars #6507

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Fix #77565: Incorrect locator detection in ZIP-based phars #6507

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
be06536
Fix #77565: Incorrect locator detection in ZIP-based phars
cmb69 Dec 11, 2020
bc03e5b
Re-use zend_memnrstr()
cmb69 Dec 15, 2020
0cbc0a5
Mitigate EOCD misdetections
cmb69 Jan 4, 2021
03742a1
Add assertion and adapt tests
cmb69 Jan 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ext/phar/tests/bug69441.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ $r = new Phar($fname, 0);

==DONE==
--EXPECTF--
UnexpectedValueException: phar error: corrupted central directory entry, no magic signature in zip-based phar "%sbug69441.phar" in %sbug69441.php:%d
UnexpectedValueException: phar error: end of central directory not found in zip-based phar "%sbug69441.phar" in %sbug69441.php:%d
Stack trace:
#0 %s%ebug69441.php(%d): Phar->__construct('%s', 0)
#1 {main}
Expand Down
13 changes: 13 additions & 0 deletions ext/phar/tests/bug77565.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
--TEST--
Bug #77565 (Incorrect locator detection in ZIP-based phars)
--SKIPIF--
<?php
if (!extension_loaded('phar')) die('skip phar extension not available');
?>
--FILE--
<?php
$phar = new PharData(__DIR__ . '/bug77565.zip');
var_dump($phar['1.zip']->getFilename());
?>
--EXPECT--
string(5) "1.zip"
Binary file added ext/phar/tests/bug77565.zip
View file
Open in desktop
Binary file not shown.
2 changes: 1 addition & 1 deletion ext/phar/tests/zip/corrupt_003.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,5 @@ try {
?>
===DONE===
--EXPECTF--
phar error: corrupt zip archive, zip file comment truncated in zip-based phar "%sfilecomment.zip"
phar error: end of central directory not found in zip-based phar "%sfilecomment.zip"
===DONE===
97 changes: 59 additions & 38 deletions ext/phar/zip.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,29 @@ static void phar_zip_u2d_time(time_t time, char *dtime, char *ddate) /* {{{ */
}
/* }}} */

static char *phar_find_eocd(const char *s, size_t n)
{
const char *end = s + n + sizeof("PK\5\6") - 1 - sizeof(phar_zip_dir_end);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is end not just s + n?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because that marker string might (theoretically) be part of the directory record. This code makes sure that we really get the start of the end of central directory record.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this really guarantees it either ... say it's at the start of a 255 byte trailing comment. The -sizeof(phar_zip_dir_end) won't skip over that. Or am I misunderstanding what you mean here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, you're right! It seems to me that the only way to reliably detect the end of central directory header would be to read through all headers and data from the beginning of the file. Anyhow, I'm going to commit a mitigitation for the current approach; maybe this is reasonably sufficient? With that change, two tests fail due to different errors; these would need to be fixed, if we're going that route.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach looks okay to me. Personally I'd start at end = s + n and then check eocd_start + sizeof(phar_zip_dir_end) <= p + n before accessing comment_len ... your current code is safe, but it took me a moment to understand that this is guaranteed due to the used start position.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I see that might be confusing; I added a respective assertion, and also adapted the tests.


/* search backwards for end of central directory signatures */
do {
uint16_t comment_len;
const char *eocd_start = zend_memnrstr(s, "PK\5\6", sizeof("PK\5\6") - 1, end);

if (eocd_start == NULL) {
return NULL;
}
ZEND_ASSERT(eocd_start + sizeof(phar_zip_dir_end) <= s + n);
comment_len = PHAR_GET_16(((phar_zip_dir_end *) eocd_start)->comment_len);
if (eocd_start + sizeof(phar_zip_dir_end) + comment_len == s + n) {
/* we can't be sure, but this looks like the proper EOCD signature */
return (char *) eocd_start;
}
end = eocd_start;
} while (end > s);
return NULL;
}

/**
* Does not check for a previously opened phar in the cache.
*
Expand Down Expand Up @@ -205,57 +228,55 @@ int phar_parse_zipfile(php_stream *fp, char *fname, size_t fname_len, char *alia
return FAILURE;
}

while ((p=(char *) memchr(p + 1, 'P', (size_t) (size - (p + 1 - buf)))) != NULL) {
if ((p - buf) + sizeof(locator) <= (size_t)size && !memcmp(p + 1, "K\5\6", 3)) {
memcpy((void *)&locator, (void *) p, sizeof(locator));
if (PHAR_GET_16(locator.centraldisk) != 0 || PHAR_GET_16(locator.disknumber) != 0) {
/* split archives not handled */
php_stream_close(fp);
if (error) {
spprintf(error, 4096, "phar error: split archives spanning multiple zips cannot be processed in zip-based phar \"%s\"", fname);
}
return FAILURE;
if ((p = phar_find_eocd(buf, size)) != NULL) {
memcpy((void *)&locator, (void *) p, sizeof(locator));
if (PHAR_GET_16(locator.centraldisk) != 0 || PHAR_GET_16(locator.disknumber) != 0) {
/* split archives not handled */
php_stream_close(fp);
if (error) {
spprintf(error, 4096, "phar error: split archives spanning multiple zips cannot be processed in zip-based phar \"%s\"", fname);
}
return FAILURE;
}

if (PHAR_GET_16(locator.counthere) != PHAR_GET_16(locator.count)) {
if (error) {
spprintf(error, 4096, "phar error: corrupt zip archive, conflicting file count in end of central directory record in zip-based phar \"%s\"", fname);
}
php_stream_close(fp);
return FAILURE;
if (PHAR_GET_16(locator.counthere) != PHAR_GET_16(locator.count)) {
if (error) {
spprintf(error, 4096, "phar error: corrupt zip archive, conflicting file count in end of central directory record in zip-based phar \"%s\"", fname);
}
php_stream_close(fp);
return FAILURE;
}

mydata = pecalloc(1, sizeof(phar_archive_data), PHAR_G(persist));
mydata->is_persistent = PHAR_G(persist);
mydata = pecalloc(1, sizeof(phar_archive_data), PHAR_G(persist));
mydata->is_persistent = PHAR_G(persist);

/* read in archive comment, if any */
if (PHAR_GET_16(locator.comment_len)) {
/* read in archive comment, if any */
if (PHAR_GET_16(locator.comment_len)) {

metadata = p + sizeof(locator);
metadata = p + sizeof(locator);

if (PHAR_GET_16(locator.comment_len) != size - (metadata - buf)) {
if (error) {
spprintf(error, 4096, "phar error: corrupt zip archive, zip file comment truncated in zip-based phar \"%s\"", fname);
}
php_stream_close(fp);
pefree(mydata, mydata->is_persistent);
return FAILURE;
if (PHAR_GET_16(locator.comment_len) != size - (metadata - buf)) {
if (error) {
spprintf(error, 4096, "phar error: corrupt zip archive, zip file comment truncated in zip-based phar \"%s\"", fname);
}
php_stream_close(fp);
pefree(mydata, mydata->is_persistent);
return FAILURE;
}

mydata->metadata_len = PHAR_GET_16(locator.comment_len);
mydata->metadata_len = PHAR_GET_16(locator.comment_len);

if (phar_parse_metadata(&metadata, &mydata->metadata, PHAR_GET_16(locator.comment_len)) == FAILURE) {
mydata->metadata_len = 0;
/* if not valid serialized data, it is a regular string */
if (phar_parse_metadata(&metadata, &mydata->metadata, PHAR_GET_16(locator.comment_len)) == FAILURE) {
mydata->metadata_len = 0;
/* if not valid serialized data, it is a regular string */

ZVAL_NEW_STR(&mydata->metadata, zend_string_init(metadata, PHAR_GET_16(locator.comment_len), mydata->is_persistent));
}
} else {
ZVAL_UNDEF(&mydata->metadata);
ZVAL_NEW_STR(&mydata->metadata, zend_string_init(metadata, PHAR_GET_16(locator.comment_len), mydata->is_persistent));
}

goto foundit;
} else {
ZVAL_UNDEF(&mydata->metadata);
}

goto foundit;
}

php_stream_close(fp);
Expand Down