From be06536976986633d95d8cff7fcf168c0e49e119 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Fri, 11 Dec 2020 15:28:17 +0100 Subject: [PATCH 1/4] 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. --- ext/phar/tests/bug77565.phpt | 13 ++++++ ext/phar/tests/bug77565.zip | Bin 0 -> 318 bytes ext/phar/zip.c | 86 +++++++++++++++++++---------------- 3 files changed, 61 insertions(+), 38 deletions(-) create mode 100644 ext/phar/tests/bug77565.phpt create mode 100644 ext/phar/tests/bug77565.zip diff --git a/ext/phar/tests/bug77565.phpt b/ext/phar/tests/bug77565.phpt new file mode 100644 index 0000000000000..e15e7ae6477d5 --- /dev/null +++ b/ext/phar/tests/bug77565.phpt @@ -0,0 +1,13 @@ +--TEST-- +Bug #77565 (Incorrect locator detection in ZIP-based phars) +--SKIPIF-- + +--FILE-- +getFilename()); +?> +--EXPECT-- +string(5) "1.zip" diff --git a/ext/phar/tests/bug77565.zip b/ext/phar/tests/bug77565.zip new file mode 100644 index 0000000000000000000000000000000000000000..134d571d7cf4fe3ddd7ccd35ed08517cda8faded GIT binary patch literal 318 zcmWIWW@h1H00E<%?!ZYe&rO~OWP>m(gA9YAUR7p6Xb2|*vwZsHcn~hF;AUWCdBM!U z044(9+O>h&LB_xU3s8GnW=<+tw-8V#2xI7GWRhcsSdXb+0%jD$l12~{Vj3$0GggyW o*+6O-fzS&`+krR?0p1AnF};UwKGd^73$S_=VSzW$=(}JG0Q{&mt^fc4 literal 0 HcmV?d00001 diff --git a/ext/phar/zip.c b/ext/phar/zip.c index 0bf873aff16a9..9b0de1b87589d 100644 --- a/ext/phar/zip.c +++ b/ext/phar/zip.c @@ -161,6 +161,18 @@ static void phar_zip_u2d_time(time_t time, char *dtime, char *ddate) /* {{{ */ } /* }}} */ +static char *phar_find_eocd(char *s, size_t n) +{ + char *p; + + for (p = s + n - 1; p >= s; p--) { + if (*p == 'P' && ((p - s) + sizeof(phar_zip_dir_end) <= n && !memcmp(p + 1, "K\5\6", 3))) { + return p; + } + } + return NULL; +} + /** * Does not check for a previously opened phar in the cache. * @@ -205,57 +217,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_t) 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); From bc03e5ba3015c1ed897c9d5f9997099d1885c7d2 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Tue, 15 Dec 2020 13:20:02 +0100 Subject: [PATCH 2/4] Re-use zend_memnrstr() --- ext/phar/zip.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/ext/phar/zip.c b/ext/phar/zip.c index 9b0de1b87589d..4c9e5709d723a 100644 --- a/ext/phar/zip.c +++ b/ext/phar/zip.c @@ -161,16 +161,10 @@ static void phar_zip_u2d_time(time_t time, char *dtime, char *ddate) /* {{{ */ } /* }}} */ -static char *phar_find_eocd(char *s, size_t n) +static char *phar_find_eocd(const char *s, size_t n) { - char *p; - - for (p = s + n - 1; p >= s; p--) { - if (*p == 'P' && ((p - s) + sizeof(phar_zip_dir_end) <= n && !memcmp(p + 1, "K\5\6", 3))) { - return p; - } - } - return NULL; + const char *end = s + n + sizeof("PK\5\6") - 1 - sizeof(phar_zip_dir_end); + return (char *) zend_memnrstr(s, "PK\5\6", sizeof("PK\5\6") - 1, end); } /** @@ -217,7 +211,7 @@ int phar_parse_zipfile(php_stream *fp, char *fname, size_t fname_len, char *alia return FAILURE; } - if ((p = phar_find_eocd(buf, (size_t) size)) != NULL) { + 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 */ From 0cbc0a5eae3b8544a7d151e6b790f005c9069103 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 4 Jan 2021 16:35:39 +0100 Subject: [PATCH 3/4] Mitigate EOCD misdetections There is 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. --- ext/phar/zip.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/ext/phar/zip.c b/ext/phar/zip.c index 4c9e5709d723a..c3888efd201b4 100644 --- a/ext/phar/zip.c +++ b/ext/phar/zip.c @@ -164,7 +164,23 @@ 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); - return (char *) zend_memnrstr(s, "PK\5\6", sizeof("PK\5\6") - 1, end); + + /* 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; + } + 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; } /** From 03742a195fb76e32aa66c0571c3524b4bb278288 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Tue, 5 Jan 2021 15:52:38 +0100 Subject: [PATCH 4/4] Add assertion and adapt tests --- ext/phar/tests/bug69441.phpt | 2 +- ext/phar/tests/zip/corrupt_003.phpt | 2 +- ext/phar/zip.c | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/ext/phar/tests/bug69441.phpt b/ext/phar/tests/bug69441.phpt index 91e2dcf146a28..7150af0f341aa 100644 --- a/ext/phar/tests/bug69441.phpt +++ b/ext/phar/tests/bug69441.phpt @@ -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} diff --git a/ext/phar/tests/zip/corrupt_003.phpt b/ext/phar/tests/zip/corrupt_003.phpt index e076a4eb8ffd4..9f5bcceeec552 100644 --- a/ext/phar/tests/zip/corrupt_003.phpt +++ b/ext/phar/tests/zip/corrupt_003.phpt @@ -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=== diff --git a/ext/phar/zip.c b/ext/phar/zip.c index c3888efd201b4..c52e87647d11a 100644 --- a/ext/phar/zip.c +++ b/ext/phar/zip.c @@ -173,6 +173,7 @@ static char *phar_find_eocd(const char *s, size_t n) 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 */