Bug #62577 [Com]: simplexml_load_file does not file if libxml_disable_entity_loader(true)

Edit report at https://bugs.php.net/bug.php?id=62577&edit=1 ID: 62577 Comment by: hanskrentel at yahoo dot de Reported by: ivan dot enderlin at hoa-project dot net Summary: simplexml_load_file does not file if libxml_disable_entity_loader(true) Status: Open Type: Bug Package: SimpleXML related Operating System: All PHP Version: master-Git-2012-07-16 (Git) Block user comment: N Private report: N New Comment: Most likely this is not a bug. Those who disable the entity loader via libxml_disable_entity_loader() are dealing with an underlying problem with an unpatched libxml version. Those who not have forgotten to implement their own entity loader (which is possible) which does not prevent from loading. Same for the default entity loader being enabled. Just my 2 cents. Previous Comments: ------------------------------------------------------------------------ [2018-10-18 21:23:30] gudang at gmail dot com 6 years... ------------------------------------------------------------------------ [2018-10-15 10:50:03] cmb@php.net Hmm, I wonder why we check whether external entity loading is disabled in php_libxml_input_buffer_create_filename()[1] (which is the xmlParserInputBufferCreateFilenameDefault() callback), instead of in _php_libxml_external_entity_loader()[2] (which is the xmlSetExternalEntityLoader() callback). (See the attached move-entity_loader_disabled-check patch.) Wouldn't the latter be sufficient to prevent XXE attacks? Also I wonder whether we need libxml_disable_entity_loader() at all. Only if LIBXML_DTDLOAD|LIBXML_NOENT are given as options, external entities will be resolved. Some of the XML parsers don't accept options, but at least as of libxml 2.9.0 save defaults are used anyway[3]. [1] <https://github.com/php/php-src/blob/php-7.3.0RC3/ext/libxml/libxml.c#L395> [2] <https://github.com/php/php-src/blob/php-7.3.0RC3/ext/libxml/libxml.c#L572> [3] <https://gitlab.gnome.org/GNOME/libxml2/commit/4629ee02ac649c27f9c0cf98ba017c6b5526070f> ------------------------------------------------------------------------ [2018-10-15 10:50:01] cmb@php.net The following patch has been added/updated: Patch Name: move-entity_loader_disabled-check Revision: 1539600601 URL: https://bugs.php.net/patch-display.php?bug=62577&patch=move-entity_loader_disabled-check&revision=1539600601 ------------------------------------------------------------------------ [2018-05-22 11:12:00] phofstetter at sensational dot ch > and if you don't give valid path, you get an error and false. of course. But this bug is about simplexml_load_file failing on *any* valid path if libxml_disable_entity_loader(true) has been called. Here's a test script. IMHO, both assert()s should pass: <?php file_put_contents('/tmp/test.xml', '<doc><foo>bar</foo></doc>'); libxml_disable_entity_loader(false); assert(simplexml_load_file('/tmp/test.xml')->foo == 'bar'); libxml_disable_entity_loader(true); assert(simplexml_load_file('/tmp/test.xml')->foo == 'bar'); unlink('/tmp/test.xml'); ------------------------------------------------------------------------ [2018-05-22 09:34:19] cojubacaso at stelliteop dot info I don't see how this is a bug, the function is called "simplexml_load_file", so the expected behavior is that it will load content of a file, and if you don't give valid path, you get an error and false. It is also documented like that, so please just close this, changing this behavior will probably brake a lot of applications also. ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=62577 -- Edit this bug report at https://bugs.php.net/bug.php?id=62577&edit=1