Bump libxml version, deprecate libxml_disable_entity_loader()

[dtakken]

In libxml < 2.9.0 external entity loading was enabled by default. Many applications, including PHP ones, have been vulnerable to XXE processing attacks because of this.

The libxml_disable_entity_loader() function is used to mitigate the problem. It allows applications to disable loading of external entities, but in a rather crude way: Not only loading of external entities is blocked, loading of XML data and validation schemas from anything but strings is blocked, crippling the functionality of XML parsing in PHP.

The behavior of libxml_disable_entity_loader() is generally perceived as being broken. There are several open bug reports about this:

https://bugs.php.net/bug.php?id=62577
https://bugs.php.net/bug.php?id=73328
https://bugs.php.net/bug.php?id=69910

Since the release of libxml 2.9.0 in 2012 external entity loading is disabled in libxml by default. This means that using libxml_disable_entity_loader() is no longer needed. Applications that wish to load external entities need to explicitly indicate this by using the LIBXML_NOENT constant when parsing XML data. Loading XML from resources like files is unaffected this way.

This PR will do two things:

• Change the required libxml version for building PHP to >= 2.9.0, making XXE processing in PHP secure by default.
• Deprecate libxml_disable_entity_loader(). It is no longer needed and has unwanted side effects.

The PR includes a unit test that verifies that external entity loading is indeed disabled by default.

Question: Will this require an RFC? And if so, would it need to accepted before the feature freeze in order to make it into 8.0?

[Girgias]

Seems good to me as CentOS 7 comes with libxml 2.9.1, I also don't think this needs an RFC.

[beberlei]

@dtakken can LIBXML_NOENT be passed to DOMDocument to allow external entities? This question sort of extends to everything xml, is there a way to use this new flag?

[dtakken]

@dtakken can LIBXML_NOENT be passed to DOMDocument to allow external entities? This question sort of extends to everything xml, is there a way to use this new flag?

The various XML processing mechanisms in PHP have a variety of ways to enable XXE processing. Most have an $options parameter that accepts the LIBXML_NOENT constant, which includes the load() method of DOMDocument. This class also has properties like resolveExternals and substituteEntities that can be set. These unit tests demonstrate use of both the constant and the class properties:

https://github.com/php/php-src/blob/e575ebdb2975b4189f42dce6b36c80300320eb98/ext/libxml/tests/libxml_disable_entity_loader.phpt
https://github.com/php/php-src/blob/f116484cf1cd8a796f05c493ace4deca84bc8fdc/ext/dom/tests/DOMDocument_load_variation4.phpt

This PR does not change the fact that XXE processing was already disabled by default when PHP was built using an up to date libxml. I expect that any code that fails to load external entities after the change would already have failed on most OS versions that are currently in use.

I don't know if all XML processing mechanisms support enabling XXE processing (the ones I know do). However I think any issues in that area should be addressed on their own.

[beberlei]

Agreeing with this change.

[beberlei]

@cmb69 this looks good from my POV, libxml 2.9 is from 2012 so that is a long time ago. i would merge it, can you give a second opinion?

[cmb69]

Thanks, @dtakken, this is a very welcome improvement to me!

[nikic]

This looks reasonable to me, but I'd suggest sending a mail to internals@ about this. The libxml version bump part of this is definitely fine. Two questions for the rest:

• Is there any other legitimate reason to disable entity loading completely? Outside XXE prevention that is?
• What is the recommended way to conditionally call libxml_disable_entity_loader() after this change?

[cmb69]

With libxml >= 2.9.0, loading of external entities is disabled, even if libxml_disable_entity_loader(false) is called. You have to explicitly pass LIBXML_NOENT to the respective XML loading functions to load (and resolve) external entities, anyway.

[cmb69]

Why is this test not for Windows? Could it be modified (or split) to test the functionality on Windows as well?

[dtakken]

I don't know to be honest, I just copied another libxml unit test and used it to create a new one.

[cmb69]

Ah, thanks! I just enabled that other test on Windows (24495ba); the same could be applied to this test case.

[dtakken]

I can rebase and apply the same changes to my test tonight.

[dtakken]

Done!

[nikic]

Welp, I just saw that you did post to internals about this. However, your mail went into spam, like all your mails do. Can you please either relax your mail configuration, or use a different address (e.g. gmail) to post to the list? As is, most likely nobody saw this.

[dtakken]

Welp, I just saw that you did post to internals about this. However, your mail went into spam, like all your mails do. Can you please either relax your mail configuration, or use a different address (e.g. gmail) to post to the list? As is, most likely nobody saw this.

Hi Nikita,

Multiple people have contacted me about this problem lately. I am aware of it and will try to sort this out.

Thanks for letting me know!

[dtakken]

This looks reasonable to me, but I'd suggest sending a mail to internals@ about this. The libxml version bump part of this is definitely fine. Two questions for the rest:

• Is there any other legitimate reason to disable entity loading completely? Outside XXE prevention that is?

Well, the only thing I can think of is an application trying to make sure that no part of an application can load XML data from URLs, making it harder for an attacker to exploit a vulnerability in libxml. However, libxml_disable_entity_loader() is not really suitable for that purpose, because any code can use that same method to switch it on again. So my guess is that the answer is 'no'.

• What is the recommended way to conditionally call libxml_disable_entity_loader() after this change?

I'm not sure what the customary way of dealing with deprecated functions is. Apart from checking the PHP version, are there other means that are commonly provided to make this easier?

[nikic]

I'm not sure what the customary way of dealing with deprecated functions is. Apart from checking the PHP version, are there other means that are commonly provided to make this easier?

I'm mainly asking for a specific piece of recommended code. I'd expect that checking the PHP version is not the right thing in this context, but that you'd rather want to check the libxml version. Is that correct? On PHP 8, the observable libxml versions would coincide with always disabling the call.

[dtakken]

I'm mainly asking for a specific piece of recommended code. I'd expect that checking the PHP version is not the right thing in this context, but that you'd rather want to check the libxml version. Is that correct? On PHP 8, the observable libxml versions would coincide with always disabling the call.

Ah, yes. I would go for this simple check:

if (LIBXML_VERSION < 20900) {
    libxml_disable_entity_loader();
}

[dtakken]

Note that the version I just pushed adds one more commit that deletes a unit test that was only run on libxml < 2.9.0. I hope that's OK.

[ausi]

Is there a replacement for safely using DOMDocument::$substituteEntities now?

AFAIK the following code is/was considered “safe”:

libxml_disable_entity_loader(true);
$dom = new DOMDocument;
$dom->substituteEntities = true;
$dom->loadXML($untrustedData);
echo $dom->saveXML();

Without the call to libxml_disable_entity_loader() external entities get loaded, example: https://3v4l.org/9Z5bd

If there is no replacement I think we should document that setting substituteEntities to true can lead to XXE vulnerabilities.

[cmb69]

AFAIK the following code is/was considered “safe”:

Yes, but that code doesn't make sense. Anyhow, documented as http://svn.php.net/viewvc?view=revision&revision=350442.

[ausi]

Yes, but that code doesn't make sense.

Thanks for checking!
Recap: never enable entity substitution for untrusted XML documents.
Did I understand that correctly?

[cmb69]

Yes, that seems to be the most simple, sane solution for untrusted XML documents.

[Mark-H]

This change does not yet seem documented in the docs for libxml_disable_entity_loader, which probably should be done: https://www.php.net/manual/en/function.libxml-disable-entity-loader.php

It looks like there's an online editor for the docs that anyone (even me!) could use, but I'm not sure about conventions or the process for adding deprecations.

[cmb69]

The documentation is not quite ready for PHP 8 related info; we want to remove PHP 5 specific stuff first. Anyhow, instead of using the online editor, IMO PRs are preferable.

[smalyshev]

@beberlei @cmb69 @nikic please check out https://bugs.php.net/bug.php?id=80235

[andrewnicols]

Just noting that the deprecation of this is not sufficiently documented.

I've raised php/doc-en#1408 regarding this.

Essentially:

• the various LIBXML2 flags are poorly documented to begin with (both upstream libxml2, and in php.net docs) - many descriptions are poor and do not adequately detail the behaviours
• the LIBXML_NOENT constant has an XXE warning, but the LIBXML_DTDVALID constant does not and is also susceptible
• use of the LIBXML_DTDLOAD or LIBXML_DTDATTR constants will cause dtds to be fetched (but are not susceptible to XXE without the NOENT or DTDVALID flags) - where the system is not available (i.e. it is loaded from w3.org) there is a 60 second timeout
• the documentation for libxml_disable_entity_loader only mentions the use of LIBXML_NOENT to resolve internal entity references, when the libXML2 source also processes these on the DTDVALID flag

Many projects have essentially just dropped the use of the libxml_disable_entiy_loader() function if the php version is >= 800, but may now be susceptible to various attacks and/or performance issues if they do not fully understand the impact of these flags. Previously they were protected by the disabling of the entity loader.