From: Christoph M. Becker <cmbecker69@gmx.de>
Date: Tue, 16 Aug 2016 18:36:33 +0000 (+0200)
Subject: Fix #72714: _xml_startElementHandler() segmentation fault
X-Git-Tag: php-7.0.11RC1~41^2
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=9164dc11e2323b8b80c389bb13d70789799b44fc;p=php

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
`int`s.

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.
---

diff --git a/NEWS b/NEWS
index d74cc191a2..013d85f84b 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 0000000000..192c8f6949
--- /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 9eba47be26..5d5c2e4c19 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);