Skip to content

Commit b621b3a

Browse files
ndosschendossche
committed
Fix phpGH-17187: unreachable program point in zend_hash
A bunch of different issues: 1) The referenced value is copied without incrementing the refcount. The reason the refcount isn't incremented is because otherwise the array modifications would violate the RC1 constraints. Solve this by copying the reference itself instead and always read the referenced value. 2) No type checks on the array data, so malicious scripts could cause type confusion bugs. 3) Potential overflow when the arrays resize and we access ctag. Closes phpGH-17205.
1 parent 7be950f commit b621b3a

File tree

4 files changed

+234
-28
lines changed

4 files changed

+234
-28
lines changed

‎NEWS‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,9 @@ PHP NEWS
101101
- Windows:
102102
. Hardened proc_open() against cmd.exe hijacking. (cmb)
103103

104+
- XML:
105+
. Fixed bug GH-1718 (unreachable program point in zend_hash). (nielsdos)
106+
104107
19 Dec 2024, PHP 8.3.15
105108

106109
- Calendar:

‎ext/xml/tests/gh17187_1.phpt‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
--TEST--
2+
GH-17187 (unreachable program point in zend_hash)
3+
--EXTENSIONS--
4+
xml
5+
--CREDITS--
6+
chongwick
7+
--FILE--
8+
<?php
9+
class ImmutableParser {
10+
private $parser;
11+
private $immutableData;
12+
private $arrayCopy;
13+
14+
public function __construct() {
15+
$this->parser = xml_parser_create();
16+
xml_set_element_handler($this->parser, function ($parser, $name, $attrs) {
17+
echo "open\n";
18+
var_dump($name, $attrs);
19+
$this->arrayCopy = [$this]; // Create cycle intentionally
20+
$this->immutableData = $this->arrayCopy;
21+
}, function ($parser, $name) {
22+
echo "close\n";
23+
var_dump($name);
24+
});
25+
}
26+
27+
public function parseXml($xml) {
28+
$this->immutableData = array();
29+
xml_parse_into_struct($this->parser, $xml, $this->immutableData, $this->immutableData);
30+
return $this->immutableData;
31+
}
32+
}
33+
$immutableParser = new ImmutableParser();
34+
$xml = "<container><child/></container>";
35+
$immutableData = $immutableParser->parseXml($xml);
36+
var_dump($immutableData);
37+
?>
38+
--EXPECT--
39+
open
40+
string(9) "CONTAINER"
41+
array(0) {
42+
}
43+
open
44+
string(5) "CHILD"
45+
array(0) {
46+
}
47+
close
48+
string(5) "CHILD"
49+
close
50+
string(9) "CONTAINER"
51+
array(5) {
52+
[0]=>
53+
object(ImmutableParser)#1 (3) {
54+
["parser":"ImmutableParser":private]=>
55+
object(XMLParser)#2 (0) {
56+
}
57+
["immutableData":"ImmutableParser":private]=>
58+
*RECURSION*
59+
["arrayCopy":"ImmutableParser":private]=>
60+
array(1) {
61+
[0]=>
62+
*RECURSION*
63+
}
64+
}
65+
["CHILD"]=>
66+
array(1) {
67+
[0]=>
68+
int(1)
69+
}
70+
[1]=>
71+
array(3) {
72+
["tag"]=>
73+
string(5) "CHILD"
74+
["type"]=>
75+
string(8) "complete"
76+
["level"]=>
77+
int(2)
78+
}
79+
["CONTAINER"]=>
80+
array(1) {
81+
[0]=>
82+
int(2)
83+
}
84+
[2]=>
85+
array(3) {
86+
["tag"]=>
87+
string(9) "CONTAINER"
88+
["type"]=>
89+
string(5) "close"
90+
["level"]=>
91+
int(1)
92+
}
93+
}

‎ext/xml/tests/gh17187_2.phpt‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
--TEST--
2+
GH-17187 (unreachable program point in zend_hash)
3+
--EXTENSIONS--
4+
xml
5+
--CREDITS--
6+
chongwick
7+
--FILE--
8+
<?php
9+
class ImmutableParser {
10+
public $parser;
11+
public $immutableData1;
12+
public $immutableData2;
13+
14+
public function __construct() {
15+
$this->parser = xml_parser_create();
16+
xml_set_element_handler($this->parser, function ($parser, $name, $attrs) {
17+
echo "open\n";
18+
var_dump($name, $attrs);
19+
$this->immutableData1 = 0xdead;
20+
$this->immutableData2 = 0xbeef;
21+
}, function ($parser, $name) {
22+
echo "close\n";
23+
var_dump($name);
24+
});
25+
}
26+
27+
public function parseXml($xml) {
28+
$this->immutableData1 = array();
29+
$this->immutableData2 = array();
30+
xml_parse_into_struct($this->parser, $xml, $this->immutableData1, $this->immutableData2);
31+
}
32+
}
33+
$immutableParser = new ImmutableParser();
34+
$xml = "<container><child/></container>";
35+
$immutableParser->parseXml($xml);
36+
var_dump($immutableParser->immutableData1);
37+
var_dump($immutableParser->immutableData2);
38+
?>
39+
--EXPECT--
40+
open
41+
string(9) "CONTAINER"
42+
array(0) {
43+
}
44+
open
45+
string(5) "CHILD"
46+
array(0) {
47+
}
48+
close
49+
string(5) "CHILD"
50+
close
51+
string(9) "CONTAINER"
52+
int(57005)
53+
int(48879)

‎ext/xml/xml.c‎

Lines changed: 85 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ typedef struct {
7272
/* We return a pointer to these zvals in get_gc(), so it's
7373
* important that a) they are adjacent b) object is the first
7474
* and c) the number of zvals is kept up to date. */
75-
#define XML_PARSER_NUM_ZVALS 12
75+
#define XML_PARSER_NUM_ZVALS 14
7676
zval object;
7777
zval startElementHandler;
7878
zval endElementHandler;
@@ -85,6 +85,8 @@ typedef struct {
8585
zval unknownEncodingHandler;
8686
zval startNamespaceDeclHandler;
8787
zval endNamespaceDeclHandler;
88+
zval data;
89+
zval info;
8890

8991
zend_function *startElementPtr;
9092
zend_function *endElementPtr;
@@ -98,12 +100,10 @@ typedef struct {
98100
zend_function *startNamespaceDeclPtr;
99101
zend_function *endNamespaceDeclPtr;
100102

101-
zval data;
102-
zval info;
103103
int level;
104104
int toffset;
105105
int curtag;
106-
zval *ctag;
106+
zend_long ctag_index;
107107
char **ltags;
108108
int lastwasopen;
109109
int skipwhite;
@@ -326,6 +326,8 @@ static void xml_parser_free_obj(zend_object *object)
326326
{
327327
xml_parser *parser = xml_parser_from_obj(object);
328328

329+
zval_ptr_dtor(&parser->info);
330+
zval_ptr_dtor(&parser->data);
329331
if (parser->parser) {
330332
XML_ParserFree(parser->parser);
331333
}
@@ -551,15 +553,18 @@ static void _xml_add_to_info(xml_parser *parser, const char *name)
551553
{
552554
zval *element;
553555

554-
if (Z_ISUNDEF(parser->info)) {
556+
if (Z_ISUNDEF(parser->info) || UNEXPECTED(Z_TYPE_P(Z_REFVAL(parser->info)) != IS_ARRAY)) {
555557
return;
556558
}
557559

560+
SEPARATE_ARRAY(Z_REFVAL(parser->info));
561+
zend_array *arr = Z_ARRVAL_P(Z_REFVAL(parser->info));
562+
558563
size_t name_len = strlen(name);
559-
if ((element = zend_hash_str_find(Z_ARRVAL(parser->info), name, name_len)) == NULL) {
564+
if ((element = zend_hash_str_find(arr, name, name_len)) == NULL) {
560565
zval values;
561566
array_init(&values);
562-
element = zend_hash_str_update(Z_ARRVAL(parser->info), name, name_len, &values);
567+
element = zend_hash_str_update(arr, name, name_len, &values);
563568
}
564569

565570
add_next_index_long(element, parser->curtag);
@@ -583,6 +588,28 @@ static zend_string *_xml_decode_tag(xml_parser *parser, const XML_Char *tag)
583588
}
584589
/* }}} */
585590

591+
static zval *xml_get_separated_data(xml_parser *parser)
592+
{
593+
if (EXPECTED(Z_TYPE_P(Z_REFVAL(parser->data)) == IS_ARRAY)) {
594+
SEPARATE_ARRAY(Z_REFVAL(parser->data));
595+
return Z_REFVAL(parser->data);
596+
}
597+
return NULL;
598+
}
599+
600+
static zval *xml_get_ctag(xml_parser *parser)
601+
{
602+
zval *data = xml_get_separated_data(parser);
603+
if (EXPECTED(data)) {
604+
zval *zv = zend_hash_index_find_deref(Z_ARRVAL_P(data), parser->ctag_index);
605+
if (EXPECTED(zv && Z_TYPE_P(zv) == IS_ARRAY)) {
606+
SEPARATE_ARRAY(zv);
607+
return zv;
608+
}
609+
}
610+
return NULL;
611+
}
612+
586613
/* {{{ _xml_startElementHandler() */
587614
void _xml_startElementHandler(void *userData, const XML_Char *name, const XML_Char **attributes)
588615
{
@@ -662,7 +689,19 @@ void _xml_startElementHandler(void *userData, const XML_Char *name, const XML_Ch
662689
zval_ptr_dtor(&atr);
663690
}
664691

665-
parser->ctag = zend_hash_next_index_insert(Z_ARRVAL(parser->data), &tag);
692+
zval *data = xml_get_separated_data(parser);
693+
if (EXPECTED(data)) {
694+
/* Note: due to array resizes or user interference,
695+
* we have to store an index instead of a zval into the array's memory. */
696+
zend_array *arr = Z_ARRVAL_P(data);
697+
if (EXPECTED(zend_hash_next_index_insert(arr, &tag))) {
698+
parser->ctag_index = arr->nNextFreeElement - 1;
699+
} else {
700+
zval_ptr_dtor(&tag);
701+
}
702+
} else {
703+
zval_ptr_dtor(&tag);
704+
}
666705
} else if (parser->level == (XML_MAXLEVEL + 1)) {
667706
php_error_docref(NULL, E_WARNING, "Maximum depth exceeded - Results truncated");
668707
}
@@ -697,17 +736,21 @@ void _xml_endElementHandler(void *userData, const XML_Char *name)
697736
zval tag;
698737

699738
if (parser->lastwasopen) {
700-
add_assoc_string(parser->ctag, "type", "complete");
739+
zval *zv = xml_get_ctag(parser);
740+
if (EXPECTED(zv)) {
741+
add_assoc_string(zv, "type", "complete");
742+
}
701743
} else {
702-
array_init(&tag);
703-
704744
_xml_add_to_info(parser, ZSTR_VAL(tag_name) + parser->toffset);
705745

706-
add_assoc_string(&tag, "tag", SKIP_TAGSTART(ZSTR_VAL(tag_name))); /* cast to avoid gcc-warning */
707-
add_assoc_string(&tag, "type", "close");
708-
add_assoc_long(&tag, "level", parser->level);
709-
710-
zend_hash_next_index_insert(Z_ARRVAL(parser->data), &tag);
746+
zval *data = xml_get_separated_data(parser);
747+
if (EXPECTED(data)) {
748+
array_init(&tag);
749+
add_assoc_string(&tag, "tag", SKIP_TAGSTART(ZSTR_VAL(tag_name))); /* cast to avoid gcc-warning */
750+
add_assoc_string(&tag, "type", "close");
751+
add_assoc_long(&tag, "level", parser->level);
752+
zend_hash_next_index_insert(Z_ARRVAL_P(data), &tag);
753+
}
711754
}
712755

713756
parser->lastwasopen = 0;
@@ -765,27 +808,41 @@ void _xml_characterDataHandler(void *userData, const XML_Char *s, int len)
765808
}
766809
}
767810
if (parser->lastwasopen) {
811+
zval *ctag = xml_get_ctag(parser);
812+
if (UNEXPECTED(!ctag)) {
813+
zend_string_release_ex(decoded_value, false);
814+
return;
815+
}
816+
768817
zval *myval;
769818
/* check if the current tag already has a value - if yes append to that! */
770-
if ((myval = zend_hash_find(Z_ARRVAL_P(parser->ctag), ZSTR_KNOWN(ZEND_STR_VALUE)))) {
819+
if ((myval = zend_hash_find(Z_ARRVAL_P(ctag), ZSTR_KNOWN(ZEND_STR_VALUE))) && Z_TYPE_P(myval) == IS_STRING) {
771820
size_t newlen = Z_STRLEN_P(myval) + ZSTR_LEN(decoded_value);
772821
Z_STR_P(myval) = zend_string_extend(Z_STR_P(myval), newlen, 0);
773822
strncpy(Z_STRVAL_P(myval) + Z_STRLEN_P(myval) - ZSTR_LEN(decoded_value),
774823
ZSTR_VAL(decoded_value), ZSTR_LEN(decoded_value) + 1);
775824
zend_string_release_ex(decoded_value, 0);
776825
} else {
777826
if (doprint || (! parser->skipwhite)) {
778-
add_assoc_str(parser->ctag, "value", decoded_value);
827+
add_assoc_str(ctag, "value", decoded_value);
779828
} else {
780829
zend_string_release_ex(decoded_value, 0);
781830
}
782831
}
783832
} else {
784833
zval tag;
785834
zval *curtag, *mytype, *myval;
786-
ZEND_HASH_REVERSE_FOREACH_VAL(Z_ARRVAL(parser->data), curtag) {
787-
if ((mytype = zend_hash_str_find(Z_ARRVAL_P(curtag),"type", sizeof("type") - 1))) {
788-
if (zend_string_equals_literal(Z_STR_P(mytype), "cdata")) {
835+
836+
zval *data = xml_get_separated_data(parser);
837+
if (UNEXPECTED(!data)) {
838+
zend_string_release_ex(decoded_value, false);
839+
return;
840+
}
841+
842+
ZEND_HASH_REVERSE_FOREACH_VAL(Z_ARRVAL_P(data), curtag) {
843+
if (EXPECTED(Z_TYPE_P(curtag) == IS_ARRAY) && (mytype = zend_hash_str_find(Z_ARRVAL_P(curtag),"type", sizeof("type") - 1))) {
844+
if (EXPECTED(Z_TYPE_P(mytype) == IS_STRING) && zend_string_equals_literal(Z_STR_P(mytype), "cdata")) {
845+
SEPARATE_ARRAY(curtag);
789846
if ((myval = zend_hash_find(Z_ARRVAL_P(curtag), ZSTR_KNOWN(ZEND_STR_VALUE)))) {
790847
size_t newlen = Z_STRLEN_P(myval) + ZSTR_LEN(decoded_value);
791848
Z_STR_P(myval) = zend_string_extend(Z_STR_P(myval), newlen, 0);
@@ -805,7 +862,7 @@ void _xml_characterDataHandler(void *userData, const XML_Char *s, int len)
805862
add_assoc_str(&tag, "value", decoded_value);
806863
add_assoc_string(&tag, "type", "cdata");
807864
add_assoc_long(&tag, "level", parser->level);
808-
zend_hash_next_index_insert(Z_ARRVAL(parser->data), &tag);
865+
zend_hash_next_index_insert(Z_ARRVAL_P(data), &tag);
809866
} else if (parser->level == (XML_MAXLEVEL + 1)) {
810867
php_error_docref(NULL, E_WARNING, "Maximum depth exceeded - Results truncated");
811868
} else {
@@ -1266,21 +1323,21 @@ PHP_FUNCTION(xml_parse_into_struct)
12661323
}
12671324

12681325
if (info) {
1269-
info = zend_try_array_init(info);
1270-
if (!info) {
1326+
if (!zend_try_array_init(info)) {
12711327
RETURN_THROWS();
12721328
}
12731329
}
12741330

1275-
xdata = zend_try_array_init(xdata);
1276-
if (!xdata) {
1331+
if (!zend_try_array_init(xdata)) {
12771332
RETURN_THROWS();
12781333
}
12791334

1280-
ZVAL_COPY_VALUE(&parser->data, xdata);
1335+
zval_ptr_dtor(&parser->data);
1336+
ZVAL_COPY(&parser->data, xdata);
12811337

12821338
if (info) {
1283-
ZVAL_COPY_VALUE(&parser->info, info);
1339+
zval_ptr_dtor(&parser->info);
1340+
ZVAL_COPY(&parser->info, info);
12841341
}
12851342

12861343
parser->level = 0;

0 commit comments

Comments
 (0)