com php-src: Fix #72714: _xml_startElementHandler() s egmentation fault: NEWS ext/xml/tests/bug7271 4.phpt ext/xml/xml.c

Commit: 9164dc11e2323b8b80c389bb13d70789799b44fc Author: Christoph M. Becker <cmbecker69@gmx.de> Tue, 16 Aug 2016 20:36:33 +0200 Parents: f682193609a1f511b7cb405a02746d44032bf8de Branches: PHP-5.6 PHP-7..0 PHP-7.1 master Link: http://git.php.net/?p=php-src.git;a=commitdiff;h=9164dc11e2323b8b80c389bb13d70789799b44fc Log: Fix #72714: _xml_startElementHandler() segmentation fault The issue is caused by an integer overflow when the long passed as XML_OPTION_SKIP_TAGSTART is assigned to xml_parser::toffset which is declared as int. We can simply work around this issue, by clipping resulting negative values to 0 (and raising a notice in this case), because the reasonable range for this value is certainly catered to by positive ints. However, there still remains the issue that xml_parser::toffset is later added to char *s, which can cause OOB reads, so we make sure that the upper bound never exceeds the strlen(). We eschew optimizing SKIP_TAGSTART wrt. to the potentially duplicate strlen() call, because that code path is unexpected anyway. Bugs: https://bugs.php.net/72714 Changed paths: M NEWS A ext/xml/tests/bug72714.phpt M ext/xml/xml.c Diff: diff --git a/NEWS b/NEWS index d74cc19..013d85f 100644 --- a/NEWS +++ b/NEWS @@ -36,6 +36,7 @@ PHP NEWS - XML: . Fixed bug #72085 (SEGV on unknown address zif_xml_parse). (cmb) + . Fixed bug #72714 (_xml_startElementHandler() segmentation fault). (cmb) - ZIP: . Fixed bug #68302 (impossible to compile php with zip support). (cmb) diff --git a/ext/xml/tests/bug72714.phpt b/ext/xml/tests/bug72714.phpt new file mode 100644 index 0000000..192c8f6 --- /dev/null +++ b/ext/xml/tests/bug72714.phpt @@ -0,0 +1,35 @@ +--TEST-- +Bug #72714 (_xml_startElementHandler() segmentation fault) +--SKIPIF-- +<?php +if (!extension_loaded('xml')) die('skip xml extension not available'); +?> +--FILE-- +<?php +function startElement($parser, $name, $attribs) { + var_dump($name); +} + +function endElement($parser, $name) {} + +function parse($tagstart) { + $xml = '<ns1:total>867</ns1:total>'; + + $xml_parser = xml_parser_create(); + xml_set_element_handler($xml_parser, 'startElement', 'endElement'); + + xml_parser_set_option($xml_parser, XML_OPTION_SKIP_TAGSTART, $tagstart); + xml_parse($xml_parser, $xml); + + xml_parser_free($xml_parser); +} + +parse(3015809298423721); +parse(20); +?> +===DONE=== +--EXPECTF-- +Notice: xml_parser_set_option(): tagstart ignored in %s%ebug72714.php on line %d +string(9) "NS1:TOTAL" +string(0) "" +===DONE=== diff --git a/ext/xml/xml.c b/ext/xml/xml.c index 9eba47b..5d5c2e4 100644 --- a/ext/xml/xml.c +++ b/ext/xml/xml.c @@ -66,6 +66,10 @@ ZEND_GET_MODULE(xml) #endif /* COMPILE_DL_XML */ /* }}} */ + +#define SKIP_TAGSTART(str) ((str) + (parser->toffset > strlen(str) ? strlen(str) : + parser->toffset)) + + /* {{{ function prototypes */ PHP_MINIT_FUNCTION(xml); PHP_MINFO_FUNCTION(xml); @@ -785,7 +789,7 @@ void _xml_startElementHandler(void *userData, const XML_Char *name, const XML_Ch if (parser->startElementHandler) { args[0] = _xml_resource_zval(parser->index); - args[1] = _xml_string_zval(((char *) tag_name) + parser->toffset); + args[1] = _xml_string_zval(SKIP_TAGSTART((char *) tag_name)); MAKE_STD_ZVAL(args[2]); array_init(args[2]); @@ -816,9 +820,9 @@ void _xml_startElementHandler(void *userData, const XML_Char *name, const XML_Ch array_init(tag); array_init(atr); - _xml_add_to_info(parser,((char *) tag_name) + parser->toffset); + _xml_add_to_info(parser,SKIP_TAGSTART((char *) tag_name)); - add_assoc_string(tag,"tag",((char *) tag_name) + parser->toffset,1); /* cast to avoid gcc-warning */ + add_assoc_string(tag,"tag",SKIP_TAGSTART((char *) tag_name),1); add_assoc_string(tag,"type","open",1); add_assoc_long(tag,"level",parser->level); @@ -870,7 +874,7 @@ void _xml_endElementHandler(void *userData, const XML_Char *name) if (parser->endElementHandler) { args[0] = _xml_resource_zval(parser->index); - args[1] = _xml_string_zval(((char *) tag_name) + parser->toffset); + args[1] = _xml_string_zval(SKIP_TAGSTART((char *) tag_name)); if ((retval = xml_call_handler(parser, parser->endElementHandler, parser->endElementPtr, 2, args))) { zval_ptr_dtor(&retval); @@ -887,9 +891,9 @@ void _xml_endElementHandler(void *userData, const XML_Char *name) array_init(tag); - _xml_add_to_info(parser,((char *) tag_name) + parser->toffset); + _xml_add_to_info(parser,SKIP_TAGSTART((char *) tag_name)); - add_assoc_string(tag,"tag",((char *) tag_name) + parser->toffset,1); /* cast to avoid gcc-warning */ + add_assoc_string(tag,"tag",SKIP_TAGSTART((char *) tag_name),1); add_assoc_string(tag,"type","close",1); add_assoc_long(tag,"level",parser->level); @@ -990,9 +994,9 @@ void _xml_characterDataHandler(void *userData, const XML_Char *s, int len) array_init(tag); - _xml_add_to_info(parser,parser->ltags[parser->level-1] + parser->toffset); + _xml_add_to_info(parser,SKIP_TAGSTART(parser->ltags[parser->level-1])); - add_assoc_string(tag,"tag",parser->ltags[parser->level-1] + parser->toffset,1); + add_assoc_string(tag,"tag",SKIP_TAGSTART(parser->ltags[parser->level-1]),1); add_assoc_string(tag,"value",decoded_value,0); add_assoc_string(tag,"type","cdata",1); add_assoc_long(tag,"level",parser->level); @@ -1633,6 +1637,10 @@ PHP_FUNCTION(xml_parser_set_option) case PHP_XML_OPTION_SKIP_TAGSTART: convert_to_long_ex(val); parser->toffset = Z_LVAL_PP(val); + if (parser->toffset < 0) { + php_error_docref(NULL TSRMLS_CC, E_NOTICE, "tagstart ignored"); + parser->toffset = 0; + } break; case PHP_XML_OPTION_SKIP_WHITE: convert_to_long_ex(val);