On 7/29/15 6:01 PM, Stanislav Malyshev wrote:
Hi!
Currently, PHP by default is vulnerable to XXE attacks:
https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Processing
To bypass this, you need to turn off external entity loading:
libxml_disable_entity_loader(true);
AFAIR right now, due to how it is implemented, this blocks loading XML
content from files with something like XMLReader::open() - due to the
use of the same code path by both. It may have changes since last time I
looked, but it definitely was a major reason why default stayed that
way. What people did is something like that:
libxml_disable_entity_loader( false );
$reader->open( $filename );
libxml_disable_entity_loader( true );
I imagine we could do better. But we need to be careful - if we just set
it as disabled, we could break a lot of unsuspecting apps that do
nothing more that reading XML files.
It hasn't changed since then. I've gone back and forth on this in the
past. I've looked at both allowing the initial file to be read as well
as potentially adding support for file resources. The latter requires
app changes anyways so no big advantage there, tho would get the job done.
I'm still going back and forth on it as a few things come to mind.
If you are already working with a trusted document then you should
safely be able to disable the entity loader. If you aren't then wouldn't
you want to do some sort of checking (especially if you dont have an XML
gateway fronting the system) for other malicious things before even
opening the document regardless if it has external entities or not. The
intial doc is really an external at that point. In our systems the
entity loader is disabled right off the bat so if any developer needs to
work with xml anywhere else in the system, they have to explicitly
enable it, load and then disable it. Requires a conscious effort on
their part as well as makes it easier to audit their usage of it and the
trustworthiness of their data.
imo it might be better to leave the entity loader function as is and
introduce a new function which can be enabled by default yet allow for
the initial data to be read yet not allow any external loading from
there. This way you are not going to have to relax the security aspect
of the current function (where the current function always overrides the
ability to read the initial data if the entity loader is disabled).
One thing to be careful of tho is the breakage of unsuspecting apps. For
anyone who relies on external DTD, schemas, xslt includes, etc.. even
within their own environment, if they are not currently using the entity
loader function now, they are certain to break.
Whichever direction the consensus is for this I'm more than willing to
help implement.
Anyways just my 2 cents.
Rob
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
