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: gudang at gmail dot com 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: 6 years... Previous Comments: ------------------------------------------------------------------------ [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. ------------------------------------------------------------------------ [2016-10-17 13:32:58] cmb@php.net Related To: Bug #73328 ------------------------------------------------------------------------ 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