Bug #79451 [Ver]: Using DOMDocument->replaceChild on doctype causes double free (PHP 7.x)

Edit report at https://bugs.php.net/bug.php?id=79451&edit=1 ID: 79451 Updated by: cmb@php.net Reported by: giovanni at giacobbi dot net Summary: Using DOMDocument->replaceChild on doctype causes double free (PHP 7.x) Status: Verified Type: Bug Package: DOM XML related Operating System: Linux PHP Version: 7.4.4 Block user comment: N Private report: N New Comment: The problem here is that the document's intSubset is not properly updated, and so after freeing the old DTD, a dangling pointer remains, which leads to the UAF. According to item 11 of the libxml2 FAQ[1], updating the intSubset is supposed to be done by clients. So basically we need special casing for XML_DTD_NODEs whenever we insert or replace nodes. [1] <http://xmlsoft.org/FAQ.html> Previous Comments: ------------------------------------------------------------------------ [2020-04-05 10:29:32] requinix@php.net PHP 7.4.4: ==278== Invalid free() / delete / delete[] / realloc() ==278== at 0x4C30D3B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==278== by 0x5BEA125: xmlFreeDoc (in /usr/lib/x86_64-linux-gnu/libxml2.so.2.9.4) ==278== by 0x2C9793: php_libxml_decrement_doc_ref (libxml.c:1268) ==278== by 0x3A91F9: dom_objects_free_storage (php_dom.c:1047) ==278== by 0x7E6902: zend_objects_store_del (zend_objects_API.c:193) ==278== by 0x78A74F: rc_dtor_func (zend_variables.c:57) ==278== by 0x78A6D2: i_zval_ptr_dtor (zend_variables.h:44) ==278== by 0x78A8E4: zval_ptr_dtor (zend_variables.c:84) ==278== by 0x7A5CD1: _zend_hash_del_el_ex (zend_hash.c:1305) ==278== by 0x7A5DB1: _zend_hash_del_el (zend_hash.c:1328) ==278== by 0x7A7C31: zend_hash_reverse_apply (zend_hash.c:1899) ==278== by 0x77349F: shutdown_destructors (zend_execute_API.c:245) ==278== Address 0xf2b24b0 is 0 bytes inside a block of size 128 free'd ==278== at 0x4C30D3B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==278== by 0x2C636B: php_libxml_node_free (libxml.c:217) ==278== by 0x2C992A: php_libxml_node_free_resource (libxml.c:1314) ==278== by 0x2C99A0: php_libxml_node_decrement_resource (libxml.c:1332) ==278== by 0x3A91DF: dom_objects_free_storage (php_dom.c:1044) ==278== by 0x7E6902: zend_objects_store_del (zend_objects_API.c:193) ==278== by 0x78A74F: rc_dtor_func (zend_variables.c:57) ==278== by 0x7F1B79: i_zval_ptr_dtor (zend_variables.h:44) ==278== by 0x802DD4: ZEND_DO_FCALL_SPEC_RETVAL_UNUSED_HANDLER (zend_vm_execute.h:1637) ==278== by 0x867D9B: execute_ex (zend_vm_execute.h:53817) ==278== by 0x86BE8F: zend_execute (zend_vm_execute.h:57913) ==278== by 0x78EDB8: zend_execute_scripts (zend.c:1665) ==278== Block was alloc'd at ==278== at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==278== by 0x5BE7F6A: xmlCreateIntSubset (in /usr/lib/x86_64-linux-gnu/libxml2.so.2.9.4) ==278== by 0x5CC930E: xmlSAX2InternalSubset (in /usr/lib/x86_64-linux-gnu/libxml2.so.2.9.4) ==278== by 0x5BB7EE2: ??? (in /usr/lib/x86_64-linux-gnu/libxml2.so.2.9.4) ==278== by 0x5C1838B: ??? (in /usr/lib/x86_64-linux-gnu/libxml2.so.2.9.4) ==278== by 0x3AF1C9: dom_load_html (document.c:2075) ==278== by 0x3AF3A3: zim_domdocument_loadHTML (document.c:2126) ==278== by 0x802CBE: ZEND_DO_FCALL_SPEC_RETVAL_UNUSED_HANDLER (zend_vm_execute.h:1618) ==278== by 0x867D9B: execute_ex (zend_vm_execute.h:53817) ==278== by 0x86BE8F: zend_execute (zend_vm_execute.h:57913) ==278== by 0x78EDB8: zend_execute_scripts (zend.c:1665) ==278== by 0x6F0177: php_execute_script (main.c:2617) > Side note: *please* can you provide us with a clear and official documented way to change a > loaded document doctype? Us? No. This is DOM. We don't control how it works, we just expose the libxml API, and they don't control how it works either, they just implement the DOM spec. As far as I can tell with some quick searching, createDocumentType + replaceChild is the "correct" approach. If I try to saveHTML using your code I get some sort of corrupted doctype, so there may be a bug in libxml. A workaround could be to remove the doctype entirely and build your own as a string to prepend to the output. ------------------------------------------------------------------------ [2020-04-05 09:20:45] giovanni at giacobbi dot net Description: ------------ While attempting to change a loaded DOM document's DOCTYPE I ran into a double free crash, this happens after creating a new DOMDocumentType object using DOMImplementation and attempting to use replaceChild on the DOMDocument. Side note: *please* can you provide us with a clear and official documented way to change a loaded document doctype? something that we can do between loadHTML() and saveHTML() to change the doctype..thx. Test script: --------------- <?php $dom = new \DOMDocument(); $dom->loadHTML("<!DOCTYPE html><p>hello</p>"); $impl = new \DOMImplementation(); $dt = $impl->createDocumentType("html", "", ""); $dom->replaceChild($dt, $dom->doctype); Expected result: ---------------- DOCTYPE changed in memory representation Actual result: -------------- free(): double free detected in tcache 2 Aborted (core dumped) ------------------------------------------------------------------------ -- Edit this bug report at https://bugs.php.net/bug.php?id=79451&edit=1