Fix GH-21496: UAF in dom_objects_free_storage.#21500
Fix GH-21496: UAF in dom_objects_free_storage.#21500devnexen wants to merge 3 commits intophp:PHP-8.4from
Conversation
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
Your analysis of the issue is right. However, I believe your solution is too restrictive.
In practice, I expect that people often pass the root of the document to this function. That works but is apparently not tested.
I believe the solution should be this:
- Grab the equivalent $ownerDocument of the passed node. You can look at the dom $ownerDocument property implementation to see how to do that.
- Clone that $ownerDocument.
- It occurs to me that the clone function can be NULL, so you have to add a NULL check too for Z_OBJ_HANDLER_P(docp, clone_obj)
ext/xsl/xsltprocessor.c
Outdated
| RETURN_THROWS(); | ||
| } | ||
|
|
||
| php_dom_create_object((xmlNodePtr) nodep->doc, &owner_zv, php_dom_obj_from_obj(Z_OBJ_P(docp))); |
There was a problem hiding this comment.
I believe the last argument may be NULL because you don't necessarily have a DOM object here.
There was a problem hiding this comment.
NULL creates a disconnected php_libxml_ref_obj which frees the xmlDoc on zval_ptr_dtor while the node still references it — same UAF when $doc is unset. Passing the
node's dom_object shares the existing doc ref.
There was a problem hiding this comment.
Right, but the current code will fail with simplexml, as xslt is generic over the different xml wrappers:
Please add this as a test:
$sxe = simplexml_load_string('<container/>');
$proc = new XSLTProcessor();
$proc->importStylesheet($sxe);To solve this, I think you need to do this:
| php_dom_create_object((xmlNodePtr) nodep->doc, &owner_zv, php_dom_obj_from_obj(Z_OBJ_P(docp))); | |
| /* See dom_import_simplexml_common */ | |
| dom_object *nodeobj = (dom_object *) ((char *) Z_OBJ_P(docp) - Z_OBJ_HT_P(docp)->offset); | |
| php_dom_create_object((xmlNodePtr) nodep->doc, &owner_zv, nodeobj); |
ext/xsl/xsltprocessor.c
Outdated
| docp = &owner_zv; | ||
| } | ||
|
|
||
| if (Z_OBJ_HANDLER_P(docp, clone_obj) == NULL) { |
There was a problem hiding this comment.
This should likely be checked first, because now you can leak owner_zv if this branch hits.
|
You force pushed to the PR but hitting "compare" shows no changes... ? |
|
silly me ! I did not push the changes ... |
Cloning a non-document DOM node creates a copy within the same xmlDoc. importStylesheet then passes that original document to xsltParseStylesheetDoc, which may strip and free nodes during processing, invalidating PHP objects still referencing them. Resolve the ownerDocument for non-document nodes and clone that instead. close phpGH-21500
| * stylesheet document otherwise the node proxies will be a mess. | ||
| * We will clone the object and detach the libxml internals later. */ | ||
| zend_object *clone = Z_OBJ_HANDLER_P(docp, clone_obj)(Z_OBJ_P(docp)); | ||
| if (!clone) { |
There was a problem hiding this comment.
This branch can still leak owner_zv. This can be solved by moving zval_ptr_dtor(&owner_zv); directly after the clone
ext/xsl/xsltprocessor.c
Outdated
| } | ||
|
|
||
| if (Z_OBJ_HANDLER_P(docp, clone_obj) == NULL) { | ||
| zend_argument_type_error(1, "must be a cloneable document"); |
There was a problem hiding this comment.
The error message is inaccurate, it should probably read:
| zend_argument_type_error(1, "must be a cloneable document"); | |
| zend_argument_type_error(1, "must be a cloneable node"); |
| XML); | ||
| $doc->documentElement->appendChild($comment); | ||
| $proc = new XSLTProcessor(); | ||
| var_dump($proc->importStylesheet($comment)); |
There was a problem hiding this comment.
Ideally you should also test this with $doc unset.
Cloning a non-document DOM node creates a copy within the same xmlDoc. importStylesheet then passes that original document to xsltParseStylesheetDoc, which may strip and free nodes during processing, invalidating PHP objects still referencing them. Resolve the ownerDocument for non-document nodes and clone that instead. close phpGH-21500
NULL creates a disconnected php_libxml_ref_obj which frees the xmlDoc on zval_ptr_dtor while the node still references it.
3 participants
Cloning a non-document DOM node creates a copy within the same xmlDoc. importStylesheet then passes that original document to xsltParseStylesheetDoc, which strips and frees nodes (including comment nodes) during processing, invalidating PHP objects still referencing them.
Validate that the imported node is a document node before proceeding with the clone.