Re: com php-src: Fix #72714: _xml_startElementHandler() segmentation fault: NEWS ext/xml/tests/bug72714.phpt ext/xml/xml.c

On Sat, Aug 20, 2016 at 1:58 AM, Christoph Michael Becker <cmb@php.net> wrote: > 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)) > There's a stray unary + in the else branch. Is this there to suppress some warning or just a typo? > + > + > /* {{{ 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"); > "tagstart ignored, because it is out of range"? Nikita