Skip to content

ext/dom: Fix use-after-free when an XPath callback modifies the document #25357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
iliaal wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ext/dom: Fix use-after-free when an XPath callback modifies the document #25357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions NEWS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ PHP NEWS
passing an array of CURLFile or CURLStringFile objects to
CURLOPT_POSTFIELDS. (Sjoerd Langkemper)

- DOM:
. Fixed use-after-free when a php:function() callback of DOMXPath or
Dom\XPath modifies nodes that are still being evaluated; such
modifications now throw a DOMException or Error. (Ilia Alshanetsky)

- FPM:
. Use recommended TYPE, HELP order for OpenMetrics metadata.
(Marcel Hernandez)
Expand Down
11 changes: 11 additions & 0 deletions UPGRADING
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,17 @@ PHP 8.7 UPGRADE NOTES
. Nested arrays in CURLOPT_POSTFIELDS now throw a TypeError, instead of being
silently converted to the string "Array".

- DOM:
. While DOMXPath or Dom\XPath evaluates an expression with PHP functions
registered, the nodes it can reach are read-only: the tree of the
evaluated document, the tree of every node a php:function() callback
returned, and the detached subtree of the context node or of a node
passed to a callback. Modifying such a node throws a DOMException with
code DOM_NO_MODIFICATION_ALLOWED_ERR (a warning when strictErrorChecking
is disabled), and modifying it through SimpleXML throws an Error.
Creating and modifying new nodes and setting document properties such as
formatOutput or documentURI are still allowed.

- PDO:
. PDOStatement::bindColumn() now throws a ValueError when the column name is
not present in the result set. It previously reported the condition
Expand Down
4 changes: 4 additions & 0 deletions ext/dom/attr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ PHP_METHOD(DOMAttr, __construct)
RETURN_THROWS();
}

if (dom_throw_if_node_read_only(Z_DOMOBJ_P(ZEND_THIS)->document, dom_object_get_node(Z_DOMOBJ_P(ZEND_THIS)))) {
return;
}

intern = Z_DOMOBJ_P(ZEND_THIS);

name_valid = xmlValidateName(BAD_CAST name, 0);
Expand Down
4 changes: 4 additions & 0 deletions ext/dom/cdatasection.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ PHP_METHOD(DOMCdataSection, __construct)
RETURN_THROWS();
}

if (dom_throw_if_node_read_only(Z_DOMOBJ_P(ZEND_THIS)->document, dom_object_get_node(Z_DOMOBJ_P(ZEND_THIS)))) {
return;
}

nodep = xmlNewCDataBlock(NULL, BAD_CAST value, value_len);

if (!nodep) {
Expand Down
15 changes: 15 additions & 0 deletions ext/dom/characterdata.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,9 @@ static void dom_character_data_append_data(INTERNAL_FUNCTION_PARAMETERS)
}

DOM_GET_OBJ(nodep, ZEND_THIS, xmlNodePtr, intern);
if (dom_throw_if_node_read_only(intern->document, nodep)) {
return;
}
xmlTextConcat(nodep, BAD_CAST arg, arg_len);
}

Expand Down Expand Up @@ -202,6 +205,10 @@ static void dom_character_data_insert_data(INTERNAL_FUNCTION_PARAMETERS, bool re

DOM_GET_OBJ(node, id, xmlNodePtr, intern);

if (dom_throw_if_node_read_only(intern->document, node)) {
RETURN_FALSE;
}

const xmlChar *cur = php_dom_get_content_or_empty(node);

length = xmlUTF8Strlen(cur);
Expand Down Expand Up @@ -267,6 +274,10 @@ static void dom_character_data_delete_data(INTERNAL_FUNCTION_PARAMETERS, bool re

DOM_GET_OBJ(node, id, xmlNodePtr, intern);

if (dom_throw_if_node_read_only(intern->document, node)) {
RETURN_FALSE;
}

const xmlChar *cur = php_dom_get_content_or_empty(node);

length = xmlUTF8Strlen(cur);
Expand Down Expand Up @@ -337,6 +348,10 @@ static void dom_character_data_replace_data(INTERNAL_FUNCTION_PARAMETERS, bool r

DOM_GET_OBJ(node, id, xmlNodePtr, intern);

if (dom_throw_if_node_read_only(intern->document, node)) {
RETURN_FALSE;
}

const xmlChar *cur = php_dom_get_content_or_empty(node);

length = xmlUTF8Strlen(cur);
Expand Down
4 changes: 4 additions & 0 deletions ext/dom/comment.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ PHP_METHOD(DOMComment, __construct)
RETURN_THROWS();
}

if (dom_throw_if_node_read_only(Z_DOMOBJ_P(ZEND_THIS)->document, dom_object_get_node(Z_DOMOBJ_P(ZEND_THIS)))) {
return;
}

nodep = xmlNewComment(BAD_CAST value);

if (!nodep) {
Expand Down
16 changes: 16 additions & 0 deletions ext/dom/document.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1213,6 +1213,10 @@ static void dom_document_adopt_node(INTERNAL_FUNCTION_PARAMETERS, bool modern)
RETURN_FALSE;
}

if (dom_throw_if_node_read_only(dom_object_nodep->document, nodep)) {
RETURN_FALSE;
}

xmlDocPtr new_document;
dom_object *dom_object_new_document;
zval *new_document_zval = ZEND_THIS;
Expand Down Expand Up @@ -1254,6 +1258,10 @@ PHP_METHOD(DOMDocument, normalizeDocument)

DOM_GET_OBJ(docp, id, xmlDocPtr, intern);

if (dom_throw_if_node_read_only(intern->document, (xmlNodePtr) docp)) {
return;
}

php_dom_normalize_legacy((xmlNodePtr) docp);
}
/* }}} end dom_document_normalize_document */
Expand Down Expand Up @@ -1801,6 +1809,10 @@ PHP_METHOD(DOMDocument, xinclude)

DOM_GET_OBJ(docp, ZEND_THIS, xmlDocPtr, intern);

if (dom_throw_if_node_read_only(intern->document, (xmlNodePtr) docp)) {
RETURN_FALSE;
}

int err = dom_perform_xinclude(docp, intern, flags);

if (err) {
Expand All @@ -1827,6 +1839,10 @@ PHP_METHOD(Dom_XMLDocument, xinclude)

DOM_GET_OBJ(docp, ZEND_THIS, xmlDocPtr, intern);

if (dom_throw_if_node_read_only(intern->document, (xmlNodePtr) docp)) {
RETURN_THROWS();
}

int err = dom_perform_xinclude(docp, intern, flags);

if (err < 0) {
Expand Down
8 changes: 8 additions & 0 deletions ext/dom/documentfragment.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ PHP_METHOD(DOMDocumentFragment, __construct)

ZEND_PARSE_PARAMETERS_NONE();

if (dom_throw_if_node_read_only(Z_DOMOBJ_P(ZEND_THIS)->document, dom_object_get_node(Z_DOMOBJ_P(ZEND_THIS)))) {
return;
}

nodep = xmlNewDocFragment(NULL);

if (!nodep) {
Expand Down Expand Up @@ -74,6 +78,10 @@ PHP_METHOD(DOMDocumentFragment, appendXML) {
RETURN_FALSE;
}

if (dom_throw_if_node_read_only(intern->document, nodep)) {
RETURN_FALSE;
}

if (data) {
PHP_LIBXML_SANITIZE_GLOBALS(parse);
err = xmlParseBalancedChunkMemory(nodep->doc, NULL, NULL, 0, BAD_CAST data, &lst);
Expand Down
6 changes: 6 additions & 0 deletions ext/dom/domimplementation.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,9 @@ PHP_METHOD(DOMImplementation, createDocument)
php_dom_throw_error(WRONG_DOCUMENT_ERR, true);
RETURN_THROWS();
}
if (dom_throw_if_node_read_only(doctobj->document, (xmlNodePtr) doctype)) {
RETURN_THROWS();
}
} else {
doctobj = NULL;
}
Expand Down Expand Up @@ -261,6 +264,9 @@ PHP_METHOD(Dom_Implementation, createDocument)

if (dtd != NULL) {
DOM_GET_OBJ(doctype, dtd, xmlDtdPtr, doctobj);
if (dom_throw_if_node_read_only(doctobj->document, (xmlNodePtr) doctype)) {
RETURN_THROWS();
}
}

xmlDocPtr document = NULL;
Expand Down
64 changes: 64 additions & 0 deletions ext/dom/element.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ PHP_METHOD(DOMElement, __construct)
RETURN_THROWS();
}

if (dom_throw_if_node_read_only(Z_DOMOBJ_P(ZEND_THIS)->document, dom_object_get_node(Z_DOMOBJ_P(ZEND_THIS)))) {
return;
}

name_valid = xmlValidateName(BAD_CAST name, 0);
if (name_valid != 0) {
php_dom_throw_error(INVALID_CHARACTER_ERR, true);
Expand Down Expand Up @@ -429,6 +433,10 @@ PHP_METHOD(DOMElement, setAttribute)

DOM_GET_OBJ(nodep, ZEND_THIS, xmlNodePtr, intern);

if (dom_throw_if_node_read_only(intern->document, nodep)) {
RETURN_FALSE;
}

if (php_dom_follow_spec_intern(intern)) {
xmlChar *name_processed = BAD_CAST name;
if (php_dom_ns_is_html_and_document_is_html(nodep)) {
Expand Down Expand Up @@ -619,6 +627,10 @@ PHP_METHOD(DOMElement, removeAttribute)

DOM_GET_OBJ(nodep, ZEND_THIS, xmlNodePtr, intern);

if (dom_throw_if_node_read_only(intern->document, nodep)) {
RETURN_FALSE;
}

attrp = dom_get_attribute_or_nsdecl(intern, nodep, BAD_CAST name, name_len);
if (attrp == NULL) {
RETURN_FALSE;
Expand All @@ -640,6 +652,10 @@ PHP_METHOD(Dom_Element, removeAttribute)

DOM_GET_OBJ(nodep, ZEND_THIS, xmlNodePtr, intern);

if (dom_throw_if_node_read_only(intern->document, nodep)) {
RETURN_THROWS();
}

attrp = dom_get_attribute_or_nsdecl(intern, nodep, BAD_CAST name, name_len);
if (attrp != NULL) {
dom_remove_attribute(nodep, attrp, intern->document);
Expand Down Expand Up @@ -699,6 +715,10 @@ static void dom_element_set_attribute_node_common(INTERNAL_FUNCTION_PARAMETERS,
DOM_GET_OBJ(nodep, id, xmlNodePtr, intern);
DOM_GET_OBJ(attrp, node, xmlAttrPtr, attrobj);

if (dom_throw_if_node_read_only(intern->document, nodep) || dom_throw_if_node_read_only(attrobj->document, (xmlNodePtr) attrp)) {
RETURN_FALSE;
}

/* ZPP Guarantees that a DOMAttr class is given, as it is converted to a xmlAttr
* to pass to libxml (see http://www.xmlsoft.org/html/libxml-tree.html#xmlAttr)
* if it is not of type XML_ATTRIBUTE_NODE it indicates a bug somewhere */
Expand Down Expand Up @@ -795,6 +815,10 @@ static void dom_element_remove_attribute_node(INTERNAL_FUNCTION_PARAMETERS, zend

ZEND_ASSERT(attrp->type == XML_ATTRIBUTE_NODE);

if (dom_throw_if_node_read_only(intern->document, nodep)) {
RETURN_FALSE;
}

if (attrp->parent != nodep) {
php_dom_throw_error(NOT_FOUND_ERR, dom_get_strict_error(intern->document));
RETURN_FALSE;
Expand Down Expand Up @@ -1095,6 +1119,10 @@ PHP_METHOD(DOMElement, setAttributeNS)

DOM_GET_OBJ(elemp, id, xmlNodePtr, intern);

if (dom_throw_if_node_read_only(intern->document, elemp)) {
return;
}

if (php_dom_follow_spec_intern(intern)) {
dom_set_attribute_ns_modern(intern, elemp, uri, name, value);
} else {
Expand Down Expand Up @@ -1185,6 +1213,10 @@ PHP_METHOD(DOMElement, removeAttributeNS)

DOM_GET_OBJ(nodep, id, xmlNodePtr, intern);

if (dom_throw_if_node_read_only(intern->document, nodep)) {
return;
}

bool follow_spec = php_dom_follow_spec_intern(intern);
if (follow_spec && uri_len == 0) {
uri = NULL;
Expand Down Expand Up @@ -1408,6 +1440,10 @@ PHP_METHOD(DOMElement, setIdAttribute)

DOM_GET_OBJ(nodep, id, xmlNodePtr, intern);

if (dom_throw_if_node_read_only(intern->document, nodep)) {
return;
}

attrp = xmlHasNsProp(nodep, BAD_CAST name, NULL);
if (attrp == NULL || attrp->type == XML_ATTRIBUTE_DECL) {
php_dom_throw_error(NOT_FOUND_ERR, dom_get_strict_error(intern->document));
Expand Down Expand Up @@ -1437,6 +1473,10 @@ PHP_METHOD(DOMElement, setIdAttributeNS)

DOM_GET_OBJ(elemp, id, xmlNodePtr, intern);

if (dom_throw_if_node_read_only(intern->document, elemp)) {
return;
}

attrp = xmlHasNsProp(elemp, BAD_CAST name, BAD_CAST uri);
if (attrp == NULL || attrp->type == XML_ATTRIBUTE_DECL) {
php_dom_throw_error(NOT_FOUND_ERR, dom_get_strict_error(intern->document));
Expand Down Expand Up @@ -1465,6 +1505,10 @@ static void dom_element_set_id_attribute_node(INTERNAL_FUNCTION_PARAMETERS, zend
DOM_GET_OBJ(nodep, id, xmlNodePtr, intern);
DOM_GET_OBJ(attrp, node, xmlAttrPtr, attrobj);

if (dom_throw_if_node_read_only(intern->document, nodep)) {
return;
}

if (attrp->parent != nodep) {
php_dom_throw_error(NOT_FOUND_ERR, dom_get_strict_error(intern->document));
} else {
Expand Down Expand Up @@ -1665,6 +1709,10 @@ static void dom_element_insert_adjacent_element(INTERNAL_FUNCTION_PARAMETERS, ze
DOM_GET_THIS_OBJ(thisp, id, xmlNodePtr, this_intern);
DOM_GET_OBJ(otherp, element_zval, xmlNodePtr, other_intern);

if (dom_throw_if_node_read_only(this_intern->document, thisp) || dom_throw_if_node_read_only(other_intern->document, otherp)) {
RETURN_NULL();
}

xmlNodePtr result = dom_insert_adjacent(where, thisp, this_intern, otherp);
if (result == NULL) {
RETURN_NULL();
Expand Down Expand Up @@ -1741,6 +1789,10 @@ static void dom_element_insert_adjacent_text(INTERNAL_FUNCTION_PARAMETERS, zend_

DOM_GET_THIS_OBJ(thisp, id, xmlNodePtr, this_intern);

if (dom_throw_if_node_read_only(this_intern->document, thisp)) {
return;
}

if (UNEXPECTED(ZEND_SIZE_T_INT_OVFL(ZSTR_LEN(data)))) {
zend_argument_value_error(2, "is too long");
RETURN_THROWS();
Expand Down Expand Up @@ -1809,6 +1861,10 @@ PHP_METHOD(Dom_Element, insertAdjacentHTML)

DOM_GET_THIS_OBJ(thisp, id, xmlNodePtr, this_intern);

if (dom_throw_if_node_read_only(this_intern->document, thisp)) {
RETURN_THROWS();
}

/* 1. We don't do injection sinks. */

/* 2. Let context be NULL */
Expand Down Expand Up @@ -1899,6 +1955,10 @@ PHP_METHOD(DOMElement, toggleAttribute)

DOM_GET_THIS_OBJ(thisp, id, xmlNodePtr, intern);

if (dom_throw_if_node_read_only(intern->document, thisp)) {
RETURN_FALSE;
}

/* Step 1 */
if (xmlValidateName(BAD_CAST qname, 0) != 0) {
php_dom_throw_error(INVALID_CHARACTER_ERR, true);
Expand Down Expand Up @@ -2177,6 +2237,10 @@ PHP_METHOD(Dom_Element, rename)
xmlNodePtr nodep;
DOM_GET_THIS_OBJ(nodep, id, xmlNodePtr, intern);

if (dom_throw_if_node_read_only(intern->document, nodep)) {
RETURN_THROWS();
}

xmlChar *localname = NULL, *prefix = NULL;
int errorcode = dom_validate_and_extract(namespace_uri, qualified_name, &localname, &prefix);
if (UNEXPECTED(errorcode != 0)) {
Expand Down
4 changes: 4 additions & 0 deletions ext/dom/entityreference.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ PHP_METHOD(DOMEntityReference, __construct)
RETURN_THROWS();
}

if (dom_throw_if_node_read_only(Z_DOMOBJ_P(ZEND_THIS)->document, dom_object_get_node(Z_DOMOBJ_P(ZEND_THIS)))) {
return;
}

name_valid = xmlValidateName(BAD_CAST name, 0);
if (name_valid != 0) {
php_dom_throw_error(INVALID_CHARACTER_ERR, true);
Expand Down
6 changes: 6 additions & 0 deletions ext/dom/html_document.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1547,6 +1547,9 @@ zend_result dom_html_document_body_write(dom_object *obj, zval *newval)
dom_object *newval_intern = Z_DOMOBJ_P(newval);
if (newval_intern->ptr != NULL) {
xmlNodePtr newval_node = ((php_libxml_node_ptr *) newval_intern->ptr)->node;
if (dom_throw_if_node_read_only(obj->document, (xmlNodePtr) docp) || dom_throw_if_node_read_only(newval_intern->document, newval_node)) {
return FAILURE;
}
/* Note: because this property has type HTMLElement, we know the namespace is correct. */
if (dom_accept_body_name(newval_node->name)) {
/* 2. If the new value is the same as the body element, return. */
Expand Down Expand Up @@ -1684,6 +1687,9 @@ static void dom_string_replace_all(xmlDocPtr docp, xmlNodePtr element, zval *zv)
zend_result dom_html_document_title_write(dom_object *obj, zval *newval)
{
DOM_PROP_NODE(xmlDocPtr, docp, obj);
if (dom_throw_if_node_read_only(obj->document, (xmlNodePtr) docp)) {
return FAILURE;
}
xmlNodePtr root = xmlDocGetRootElement(docp);

if (root == NULL) {
Expand Down
Loading
Loading