XPath callback UAF

[afflerbach]

Try to fix #22077

[devnexen]

you have been faster than me, nice but I m afraid your fix is incomplete I can see leaks locally with your changes. Ah well your commit said it all.

[devnexen]

let me think about it unless you come up with the solution in the meantime. I ve been maintaining these extensions for few weeks now, I can tell by experience most of proper fixes are not "one-liners" ;)

[devnexen]

Can you revert your change and try the following ?

Somewhere in ext/dom/xpath.c

+/* in xpath.c, near top */
+static dom_object *dom_xpath_intern_for_doc(dom_xpath_object *xpath_obj, xmlDocPtr doc)
+{
+       if (xpath_obj->dom.document && xpath_obj->dom.document->ptr == doc) {
+               return &xpath_obj->dom;
+       }
+       HashTable *node_list = xpath_obj->xpath_callbacks.node_list;
+       if (node_list) {
+               zval *entry;
+               ZEND_HASH_PACKED_FOREACH_VAL(node_list, entry) {
+                       dom_object *obj = Z_DOMOBJ_P(entry);
+                       if (obj->document && obj->document->ptr == doc) {
+                               return obj;
+                       }
+               } ZEND_HASH_FOREACH_END();
+       }
+       return &xpath_obj->dom;
+}
+

then

HashTable *dom_xpath_get_gc(zend_object *object, zval **table, int *n)
 {
        dom_xpath_object *intern = php_xpath_obj_from_obj(object);
@@ -352,7 +371,8 @@ static void php_xpath_eval(INTERNAL_FUNCTION_PARAMETERS, int type, bool modern)
 
                                                node = php_dom_create_fake_namespace_decl(nsparent, original, &child, parent_intern);
                                        } else {
-                                               php_dom_create_object(node, &child, &intern->dom);
+                                               dom_object *parent = dom_xpath_intern_for_doc(intern, node->doc);
+                                               php_dom_create_object(node, &child, parent);
                                        }
                                        add_next_index_zval(&retval, &child);

[afflerbach]

This looks good (as far as I can tell ;), no UAF, no leak

[devnexen]

Can you do the following ?

• squash in 1 commit adding the ext/dom: prefix.
• adding me with the Co-Authored-by tag.
  thanks.

[devnexen]

lgtm

[devnexen]

just few nits about the test.

here should be

GH-22077 (UAF in custom XPath function)

[devnexen]

name of the name file should be gh22077.phpt (since we moved on from the old bugs.php.net to github).

[afflerbach]

Done.

[devnexen]

The --DESCRIPTION-- block is heavier than php-src precedent. Most phpt tests drop it entirely (only ~10 in the repo use it, and the typical body is a single short
line, e.g. run this with valgrind in Zend/tests/bug60825.phpt). I'd fold the summary into --TEST-- itself

[afflerbach]

Agreed. I had written this before I created the ticket.

[devnexen]

just few nits to address regarding the unit test then I ll commit shortly :)

[devnexen]

Thanks !

[afflerbach]

Thank you for the great support!