From 7edc639b9ff1c3576773d79d016abbeed1f93846 Mon Sep 17 00:00:00 2001 From: Stanislav Malyshev Date: Sun, 11 Nov 2018 10:04:01 -0800 Subject: Fix #77020: null pointer dereference in imap_mail If an empty $message is passed to imap_mail(), we must not set message to NULL, since _php_imap_mail() is not supposed to handle NULL pointers (opposed to pointers to NUL). --- NEWS | 1 + ext/imap/php_imap.c | 1 - ext/imap/tests/bug77020.phpt | 15 +++++++++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) create mode 100644 ext/imap/tests/bug77020.phpt diff --git a/NEWS b/NEWS index 5d945dfde1..34ff5848d5 100644 --- a/NEWS +++ b/NEWS @@ -3,6 +3,7 @@ PHP NEWS ?? ??? 2018, PHP 5.6.39 - IMAP: + . Fixed bug #77020 (null pointer dereference in imap_mail). (cmb) . Fixed bug #77153 (imap_open allows to run arbitrary shell commands via mailbox parameter). (Stas) diff --git a/ext/imap/php_imap.c b/ext/imap/php_imap.c index a23e84c085..b30440f000 100644 --- a/ext/imap/php_imap.c +++ b/ext/imap/php_imap.c @@ -4094,7 +4094,6 @@ PHP_FUNCTION(imap_mail) if (!message_len) { /* this is not really an error, so it is allowed. */ php_error_docref(NULL TSRMLS_CC, E_WARNING, "No message string in mail command"); - message = NULL; } if (_php_imap_mail(to, subject, message, headers, cc, bcc, rpath TSRMLS_CC)) { diff --git a/ext/imap/tests/bug77020.phpt b/ext/imap/tests/bug77020.phpt new file mode 100644 index 0000000000..8a65232eec --- /dev/null +++ b/ext/imap/tests/bug77020.phpt @@ -0,0 +1,15 @@ +--TEST-- +Bug #77020 (null pointer dereference in imap_mail) +--SKIPIF-- + +--FILE-- + +===DONE=== +--EXPECTF-- +Warning: imap_mail(): No message string in mail command in %s on line %d +%s +===DONE=== -- cgit v1.2.1 From 54212674b924aab506471060ff64986cda375f71 Mon Sep 17 00:00:00 2001 From: Stanislav Malyshev Date: Mon, 12 Nov 2018 14:02:26 -0800 Subject: Fix bug #77143 - add more checks to buffer reads --- NEWS | 3 ++- ext/phar/phar.c | 30 +++++++++++++++++++++--------- ext/phar/tests/bug73768.phpt | 2 +- ext/phar/tests/bug77143.phar | Bin 0 -> 50 bytes ext/phar/tests/bug77143.phpt | 18 ++++++++++++++++++ 5 files changed, 42 insertions(+), 11 deletions(-) create mode 100644 ext/phar/tests/bug77143.phar create mode 100644 ext/phar/tests/bug77143.phpt diff --git a/NEWS b/NEWS index 34ff5848d5..727e874f97 100644 --- a/NEWS +++ b/NEWS @@ -9,7 +9,8 @@ PHP NEWS - Phar: . Fixed bug #77022 (PharData always creates new files with mode 0666). (Stas) - + . Fixed bug #77143 (Heap Buffer Overflow (READ: 4) in phar_parse_pharfile). + (Stas) 13 Sep 2018, PHP 5.6.38 diff --git a/ext/phar/phar.c b/ext/phar/phar.c index 780be43257..ac48e1ccac 100644 --- a/ext/phar/phar.c +++ b/ext/phar/phar.c @@ -638,6 +638,18 @@ int phar_parse_metadata(char **buffer, zval **metadata, php_uint32 zip_metadata_ } /* }}}*/ +/** + * Size of fixed fields in the manifest. + * See: http://php.net/manual/en/phar.fileformat.phar.php + */ +#define MANIFEST_FIXED_LEN 18 + +#define SAFE_PHAR_GET_32(buffer, endbuffer, var) \ + if (UNEXPECTED(buffer + 4 >= endbuffer)) { \ + MAPPHAR_FAIL("internal corruption of phar \"%s\" (truncated manifest header)"); \ + } \ + PHAR_GET_32(buffer, var); + /** * Does not check for a previously opened phar in the cache. * @@ -721,12 +733,12 @@ static int phar_parse_pharfile(php_stream *fp, char *fname, int fname_len, char savebuf = buffer; endbuffer = buffer + manifest_len; - if (manifest_len < 10 || manifest_len != php_stream_read(fp, buffer, manifest_len)) { + if (manifest_len < MANIFEST_FIXED_LEN || manifest_len != php_stream_read(fp, buffer, manifest_len)) { MAPPHAR_FAIL("internal corruption of phar \"%s\" (truncated manifest header)") } /* extract the number of entries */ - PHAR_GET_32(buffer, manifest_count); + SAFE_PHAR_GET_32(buffer, endbuffer, manifest_count); if (manifest_count == 0) { MAPPHAR_FAIL("in phar \"%s\", manifest claims to have zero entries. Phars must have at least 1 entry"); @@ -746,7 +758,7 @@ static int phar_parse_pharfile(php_stream *fp, char *fname, int fname_len, char return FAILURE; } - PHAR_GET_32(buffer, manifest_flags); + SAFE_PHAR_GET_32(buffer, endbuffer, manifest_flags); manifest_flags &= ~PHAR_HDR_COMPRESSION_MASK; manifest_flags &= ~PHAR_FILE_COMPRESSION_MASK; @@ -966,13 +978,13 @@ static int phar_parse_pharfile(php_stream *fp, char *fname, int fname_len, char } /* extract alias */ - PHAR_GET_32(buffer, tmp_len); + SAFE_PHAR_GET_32(buffer, endbuffer, tmp_len); if (buffer + tmp_len > endbuffer) { MAPPHAR_FAIL("internal corruption of phar \"%s\" (buffer overrun)"); } - if (manifest_len < 10 + tmp_len) { + if (manifest_len < MANIFEST_FIXED_LEN + tmp_len) { MAPPHAR_FAIL("internal corruption of phar \"%s\" (truncated manifest header)") } @@ -1010,7 +1022,7 @@ static int phar_parse_pharfile(php_stream *fp, char *fname, int fname_len, char } /* we have 5 32-bit items plus 1 byte at least */ - if (manifest_count > ((manifest_len - 10 - tmp_len) / (5 * 4 + 1))) { + if (manifest_count > ((manifest_len - MANIFEST_FIXED_LEN - tmp_len) / (5 * 4 + 1))) { /* prevent serious memory issues */ MAPPHAR_FAIL("internal corruption of phar \"%s\" (too many manifest entries for size of manifest)") } @@ -1019,12 +1031,12 @@ static int phar_parse_pharfile(php_stream *fp, char *fname, int fname_len, char mydata->is_persistent = PHAR_G(persist); /* check whether we have meta data, zero check works regardless of byte order */ - PHAR_GET_32(buffer, len); + SAFE_PHAR_GET_32(buffer, endbuffer, len); if (mydata->is_persistent) { mydata->metadata_len = len; - if(!len) { + if (!len) { /* FIXME: not sure why this is needed but removing it breaks tests */ - PHAR_GET_32(buffer, len); + SAFE_PHAR_GET_32(buffer, endbuffer, len); } } if(len > endbuffer - buffer) { diff --git a/ext/phar/tests/bug73768.phpt b/ext/phar/tests/bug73768.phpt index 37a4da0253..3062268b80 100644 --- a/ext/phar/tests/bug73768.phpt +++ b/ext/phar/tests/bug73768.phpt @@ -13,4 +13,4 @@ echo "OK\n"; } ?> --EXPECTF-- -cannot load phar "%sbug73768.phar" with implicit alias "" under different alias "alias.phar" +internal corruption of phar "%sbug73768.phar" (truncated manifest header) diff --git a/ext/phar/tests/bug77143.phar b/ext/phar/tests/bug77143.phar new file mode 100644 index 0000000000..eb797b5195 Binary files /dev/null and b/ext/phar/tests/bug77143.phar differ diff --git a/ext/phar/tests/bug77143.phpt b/ext/phar/tests/bug77143.phpt new file mode 100644 index 0000000000..f9f80fc4f4 --- /dev/null +++ b/ext/phar/tests/bug77143.phpt @@ -0,0 +1,18 @@ +--TEST-- +PHP bug #77143: Heap Buffer Overflow (READ: 4) in phar_parse_pharfile +--INI-- +phar.readonly=0 +--SKIPIF-- + +--FILE-- +getMessage(); +} +?> +--EXPECTF-- +internal corruption of phar "%sbug77143.phar" (truncated manifest header) -- cgit v1.2.1 From 48f0f73f75c0059ba5d9b73cb4e5faeeaea49c47 Mon Sep 17 00:00:00 2001 From: Stanislav Malyshev Date: Mon, 12 Nov 2018 14:02:26 -0800 Subject: Fix bug #77143 - add more checks to buffer reads --- NEWS | 3 ++- ext/phar/phar.c | 30 +++++++++++++++++++++--------- ext/phar/tests/bug73768.phpt | 2 +- ext/phar/tests/bug77143.phar | Bin 0 -> 50 bytes ext/phar/tests/bug77143.phpt | 18 ++++++++++++++++++ 5 files changed, 42 insertions(+), 11 deletions(-) create mode 100644 ext/phar/tests/bug77143.phar create mode 100644 ext/phar/tests/bug77143.phpt diff --git a/NEWS b/NEWS index 34ff5848d5..727e874f97 100644 --- a/NEWS +++ b/NEWS @@ -9,7 +9,8 @@ PHP NEWS - Phar: . Fixed bug #77022 (PharData always creates new files with mode 0666). (Stas) - + . Fixed bug #77143 (Heap Buffer Overflow (READ: 4) in phar_parse_pharfile). + (Stas) 13 Sep 2018, PHP 5.6.38 diff --git a/ext/phar/phar.c b/ext/phar/phar.c index 780be43257..47ff8cd790 100644 --- a/ext/phar/phar.c +++ b/ext/phar/phar.c @@ -638,6 +638,18 @@ int phar_parse_metadata(char **buffer, zval **metadata, php_uint32 zip_metadata_ } /* }}}*/ +/** + * Size of fixed fields in the manifest. + * See: http://php.net/manual/en/phar.fileformat.phar.php + */ +#define MANIFEST_FIXED_LEN 18 + +#define SAFE_PHAR_GET_32(buffer, endbuffer, var) \ + if (UNEXPECTED(buffer + 4 > endbuffer)) { \ + MAPPHAR_FAIL("internal corruption of phar \"%s\" (truncated manifest header)"); \ + } \ + PHAR_GET_32(buffer, var); + /** * Does not check for a previously opened phar in the cache. * @@ -721,12 +733,12 @@ static int phar_parse_pharfile(php_stream *fp, char *fname, int fname_len, char savebuf = buffer; endbuffer = buffer + manifest_len; - if (manifest_len < 10 || manifest_len != php_stream_read(fp, buffer, manifest_len)) { + if (manifest_len < MANIFEST_FIXED_LEN || manifest_len != php_stream_read(fp, buffer, manifest_len)) { MAPPHAR_FAIL("internal corruption of phar \"%s\" (truncated manifest header)") } /* extract the number of entries */ - PHAR_GET_32(buffer, manifest_count); + SAFE_PHAR_GET_32(buffer, endbuffer, manifest_count); if (manifest_count == 0) { MAPPHAR_FAIL("in phar \"%s\", manifest claims to have zero entries. Phars must have at least 1 entry"); @@ -746,7 +758,7 @@ static int phar_parse_pharfile(php_stream *fp, char *fname, int fname_len, char return FAILURE; } - PHAR_GET_32(buffer, manifest_flags); + SAFE_PHAR_GET_32(buffer, endbuffer, manifest_flags); manifest_flags &= ~PHAR_HDR_COMPRESSION_MASK; manifest_flags &= ~PHAR_FILE_COMPRESSION_MASK; @@ -966,13 +978,13 @@ static int phar_parse_pharfile(php_stream *fp, char *fname, int fname_len, char } /* extract alias */ - PHAR_GET_32(buffer, tmp_len); + SAFE_PHAR_GET_32(buffer, endbuffer, tmp_len); if (buffer + tmp_len > endbuffer) { MAPPHAR_FAIL("internal corruption of phar \"%s\" (buffer overrun)"); } - if (manifest_len < 10 + tmp_len) { + if (manifest_len < MANIFEST_FIXED_LEN + tmp_len) { MAPPHAR_FAIL("internal corruption of phar \"%s\" (truncated manifest header)") } @@ -1010,7 +1022,7 @@ static int phar_parse_pharfile(php_stream *fp, char *fname, int fname_len, char } /* we have 5 32-bit items plus 1 byte at least */ - if (manifest_count > ((manifest_len - 10 - tmp_len) / (5 * 4 + 1))) { + if (manifest_count > ((manifest_len - MANIFEST_FIXED_LEN - tmp_len) / (5 * 4 + 1))) { /* prevent serious memory issues */ MAPPHAR_FAIL("internal corruption of phar \"%s\" (too many manifest entries for size of manifest)") } @@ -1019,12 +1031,12 @@ static int phar_parse_pharfile(php_stream *fp, char *fname, int fname_len, char mydata->is_persistent = PHAR_G(persist); /* check whether we have meta data, zero check works regardless of byte order */ - PHAR_GET_32(buffer, len); + SAFE_PHAR_GET_32(buffer, endbuffer, len); if (mydata->is_persistent) { mydata->metadata_len = len; - if(!len) { + if (!len) { /* FIXME: not sure why this is needed but removing it breaks tests */ - PHAR_GET_32(buffer, len); + SAFE_PHAR_GET_32(buffer, endbuffer, len); } } if(len > endbuffer - buffer) { diff --git a/ext/phar/tests/bug73768.phpt b/ext/phar/tests/bug73768.phpt index 37a4da0253..3062268b80 100644 --- a/ext/phar/tests/bug73768.phpt +++ b/ext/phar/tests/bug73768.phpt @@ -13,4 +13,4 @@ echo "OK\n"; } ?> --EXPECTF-- -cannot load phar "%sbug73768.phar" with implicit alias "" under different alias "alias.phar" +internal corruption of phar "%sbug73768.phar" (truncated manifest header) diff --git a/ext/phar/tests/bug77143.phar b/ext/phar/tests/bug77143.phar new file mode 100644 index 0000000000..eb797b5195 Binary files /dev/null and b/ext/phar/tests/bug77143.phar differ diff --git a/ext/phar/tests/bug77143.phpt b/ext/phar/tests/bug77143.phpt new file mode 100644 index 0000000000..f9f80fc4f4 --- /dev/null +++ b/ext/phar/tests/bug77143.phpt @@ -0,0 +1,18 @@ +--TEST-- +PHP bug #77143: Heap Buffer Overflow (READ: 4) in phar_parse_pharfile +--INI-- +phar.readonly=0 +--SKIPIF-- + +--FILE-- +getMessage(); +} +?> +--EXPECTF-- +internal corruption of phar "%sbug77143.phar" (truncated manifest header) -- cgit v1.2.1