Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:87410
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmx.de designates 212.227.17.20 as permitted sender)
Message-ID: <55BA5647.5080304@gmx.de>
Date: Thu, 30 Jul 2015 18:52:23 +0200
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0
MIME-Version: 1.0
To: Anatol Belski <anatol.php@belski.net>, 
 'Pierre Joye' <pierre.php@gmail.com>,
 'Anthony Ferrara' <ircmaxell@gmail.com>
CC: 'PHP internals' <internals@lists.php.net>
References: <CAAyV7nES6A+_68A2eMdxAGw--_R-A7WhhQJ6VyMzi5p7PneBXw@mail.gmail.com> <CAEZPtU7HAne7mvkxUqo2yBV6_FRAibUnjqRiDVL99QRdna=-OQ@mail.gmail.com> <017601d0ca44$2436c360$6ca44a20$@belski.net>
In-Reply-To: <017601d0ca44$2436c360$6ca44a20$@belski.net>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] Disabling External Entities in libxml By Default
From: cmbecker69@gmx.de (Christoph Becker)

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
>>
>> On Jul 29, 2015 11:38 PM, "Anthony Ferrara" <ircmaxell@gmail.com> wrote:
>>>
>>> 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