Skip to content

Commit c833e31

Browse files
jvoisinjvoisin
committed
ext/libxml: fix use-after-free when re-registering the entity loader from its own callback
The loader callback ran with a borrowed $this (non-closures are not ref-held by zend_call_known_fcc), so re-registering the loader from within its own callback freed the executing callback and its bound object, causing a use-after-free. Hold a reference to the callback for the duration of the call instead, so the loader may safely swap itself out (for instance to run a nested parse) while it is running.
1 parent 840625f commit c833e31

5 files changed

Lines changed: 180 additions & 3 deletions

‎ext/libxml/libxml.c‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -777,12 +777,26 @@ static xmlParserInputPtr php_libxml_external_entity_loader(const char *URL,
777777

778778
#undef ADD_NULL_OR_STRING_KEY
779779

780-
zend_call_known_fcc(&LIBXML(entity_loader_callback), &retval, 3, params, /* named_params */ NULL);
780+
/* Hold our own reference to the callback for the duration of the call.
781+
* The callback may re-register the loader (e.g. to run a nested parse),
782+
* which frees the global fcc and its bound object; without this copy that
783+
* would be a use-after-free of the executing callback. */
784+
zend_fcall_info_cache fcc = LIBXML(entity_loader_callback);
785+
/* A trampoline (__call/__callStatic) handler is heap-allocated and shared
786+
* with the global fcc, so re-registering would free it too: copy it. */
787+
if (UNEXPECTED(fcc.function_handler->common.fn_flags & ZEND_ACC_CALL_VIA_TRAMPOLINE)) {
788+
fcc.function_handler = emalloc(sizeof(zend_function));
789+
memcpy(fcc.function_handler, LIBXML(entity_loader_callback).function_handler, sizeof(zend_function));
790+
zend_string_addref(fcc.function_handler->common.function_name);
791+
}
792+
zend_fcc_addref(&fcc);
793+
794+
zend_call_known_fcc(&fcc, &retval, 3, params, /* named_params */ NULL);
781795

782796
if (Z_ISUNDEF(retval)) {
783797
php_libxml_ctx_error(context,
784798
"Call to user entity loader callback '%s' has failed",
785-
ZSTR_VAL(LIBXML(entity_loader_callback).function_handler->common.function_name));
799+
ZSTR_VAL(fcc.function_handler->common.function_name));
786800
} else {
787801
if (Z_TYPE(retval) == IS_STRING) {
788802
is_string:
@@ -791,7 +805,7 @@ static xmlParserInputPtr php_libxml_external_entity_loader(const char *URL,
791805
php_stream *stream = (php_stream*)zend_fetch_resource2_ex(&retval, NULL, php_file_le_stream(), php_file_le_pstream());
792806
if (UNEXPECTED(stream == NULL)) {
793807
zval callable;
794-
zend_get_callable_zval_from_fcc(&LIBXML(entity_loader_callback), &callable);
808+
zend_get_callable_zval_from_fcc(&fcc, &callable);
795809
zend_string *callable_name = zend_get_callable_name(&callable);
796810
zend_string *func_name = get_active_function_or_method_name();
797811
zend_type_error(
@@ -853,6 +867,7 @@ static xmlParserInputPtr php_libxml_external_entity_loader(const char *URL,
853867
}
854868
}
855869

870+
zend_fcc_dtor(&fcc);
856871
zval_ptr_dtor(&params[0]);
857872
zval_ptr_dtor(&params[1]);
858873
zval_ptr_dtor(&params[2]);

‎ext/libxml/tests/libxml_set_external_entity_loader_callback_bailout.phpt‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
--TEST--
2+
libxml_set_external_entity_loader(): the in-callback state is reset when the callback bails out
3+
--EXTENSIONS--
4+
dom
5+
--INI--
6+
memory_limit=8M
7+
--FILE--
8+
<?php
9+
register_shutdown_function(function () {
10+
try {
11+
libxml_set_external_entity_loader(null);
12+
echo 'loader reset', PHP_EOL;
13+
} catch (\Error $e) {
14+
echo $e::class, ': ', $e->getMessage(), PHP_EOL;
15+
}
16+
});
17+
18+
$loader = function ($public, $system, $context) {
19+
echo 'in loader', PHP_EOL;
20+
$s = str_repeat('a', 1024 * 1024 * 512);
21+
};
22+
23+
libxml_set_external_entity_loader($loader);
24+
25+
$xml = <<<XML
26+
<!DOCTYPE foo PUBLIC "-//FOO/BAR" "http://example.com/foobar">
27+
<foo>bar</foo>
28+
XML;
29+
30+
$dd = new DOMDocument;
31+
$dd->loadXML($xml);
32+
$dd->validate();
33+
34+
echo 'not reached', PHP_EOL;
35+
?>
36+
--EXPECTF--
37+
in loader
38+
39+
Fatal error: Allowed memory size of %d bytes exhausted (tried to allocate %d bytes) in %s on line %d
40+
loader reset

‎ext/libxml/tests/libxml_set_external_entity_loader_reentrancy.phpt‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
--TEST--
2+
libxml_set_external_entity_loader(): re-registering the loader from within its own callback is safe (no use-after-free)
3+
--EXTENSIONS--
4+
dom
5+
--FILE--
6+
<?php
7+
class Loader {
8+
public function load($public, $system, $context) {
9+
/* Re-enter the setter while this callback (and its bound $this) is still
10+
* on the stack. The bound object is only kept alive by the global loader
11+
* fcc, so freeing it here used to trigger a use-after-free. The loader now
12+
* holds its own reference for the duration of the call, so this is safe. */
13+
libxml_set_external_entity_loader(null);
14+
/* $this must still be valid after the re-registration. */
15+
var_dump($this instanceof Loader);
16+
17+
$f = fopen("php://temp", "r+");
18+
fwrite($f, "<!ELEMENT foo (#PCDATA)>");
19+
rewind($f);
20+
return $f;
21+
}
22+
}
23+
24+
$xml = <<<XML
25+
<!DOCTYPE foo PUBLIC "-//FOO/BAR" "http://example.com/foobar">
26+
<foo>bar</foo>
27+
XML;
28+
29+
/* Pass a fresh object as a non-closure callable: after this call returns, the
30+
* only reference to it is the one held by the global entity loader. */
31+
libxml_set_external_entity_loader([new Loader, 'load']);
32+
33+
$dd = new DOMDocument;
34+
$dd->loadXML($xml);
35+
var_dump($dd->validate());
36+
37+
/* The callback reset the loader to null while it was running. */
38+
var_dump(libxml_get_external_entity_loader());
39+
40+
echo "Done.\n";
41+
?>
42+
--EXPECT--
43+
bool(true)
44+
bool(true)
45+
NULL
46+
Done.

‎ext/libxml/tests/libxml_set_external_entity_loader_swap_in_callback.phpt‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
--TEST--
2+
libxml_set_external_entity_loader(): the loader may swap itself out for a nested parse
3+
--EXTENSIONS--
4+
dom
5+
--FILE--
6+
<?php
7+
$loader = null;
8+
$loader = function ($public, $system, $context) use (&$loader) {
9+
libxml_set_external_entity_loader(null);
10+
$inner = new DOMDocument;
11+
$inner->loadXML('<inner/>');
12+
echo $inner->documentElement->nodeName, PHP_EOL;
13+
libxml_set_external_entity_loader($loader);
14+
15+
$f = fopen('php://temp', 'r+');
16+
fwrite($f, '<!ELEMENT foo (#PCDATA)>');
17+
rewind($f);
18+
return $f;
19+
};
20+
21+
libxml_set_external_entity_loader($loader);
22+
23+
$xml = <<<XML
24+
<!DOCTYPE foo PUBLIC "-//FOO/BAR" "http://example.com/foobar">
25+
<foo>bar</foo>
26+
XML;
27+
28+
$dd = new DOMDocument;
29+
$dd->loadXML($xml);
30+
var_dump($dd->validate());
31+
32+
libxml_set_external_entity_loader(null);
33+
var_dump(libxml_get_external_entity_loader());
34+
35+
echo 'Done.', PHP_EOL;
36+
?>
37+
--EXPECT--
38+
inner
39+
bool(true)
40+
NULL
41+
Done.

‎ext/libxml/tests/libxml_set_external_entity_loader_trampoline_reentrancy.phpt‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
--TEST--
2+
libxml_set_external_entity_loader(): trampoline callback resetting the loader from within itself
3+
--EXTENSIONS--
4+
dom
5+
--FILE--
6+
<?php
7+
class TrampolineTest {
8+
public function __call(string $name, array $arguments) {
9+
libxml_set_external_entity_loader(null);
10+
var_dump($this instanceof TrampolineTest);
11+
return 42;
12+
}
13+
}
14+
15+
libxml_set_external_entity_loader([new TrampolineTest, 'entity_loader']);
16+
17+
$dd = new DOMDocument;
18+
$dd->loadXML(<<<XML
19+
<!DOCTYPE foo PUBLIC "-//FOO/BAR" "http://example.com/foobar">
20+
<foo>bar</foo>
21+
XML);
22+
try {
23+
$dd->validate();
24+
} catch (\Error $e) {
25+
echo $e::class, ': ', $e->getMessage(), PHP_EOL;
26+
}
27+
var_dump(libxml_get_external_entity_loader());
28+
?>
29+
--EXPECTF--
30+
bool(true)
31+
32+
Warning: DOMDocument::validate(): I/O warning : failed to load external entity "42" in %s on line %d
33+
34+
Warning: DOMDocument::validate(): Could not load the external subset "http://example.com/foobar" in %s on line %d
35+
NULL

0 commit comments

Comments
 (0)