Skip to content

[Config] Disable XML external entity loading for all versions of libxml #38564

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[Config] Disable XML external entity loading for all versions of libxml #38564

wants to merge 1 commit into from

Conversation

fabianbadoi
Copy link

@fabianbadoi fabianbadoi commented Oct 14, 2020 •
edited
Loading

Q A
Branch? 3.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets n/a
License MIT
Doc PR n/a

The libxml_disable_entity_loader() function was deprecated in PHP 8.0 and will trigger a deprecation notice going forward. However, external entity loading is still enabled.

The PR where the function was deprecated, php/php-src#5867, claims that XXE loading is diabled by default since libxml 2.9.0.

However, this was not the case in at least the following systems:

  • the php:7.4-fpm-alpine docker image
  • the php:8.0-rc-cli-alpine
  • the default php-7.4 package from the Ubuntu 20.04 package repositories

All of these installations report LIBXML_VERSION > 20900 and are still vulnerable to XXE attacks unless libxml_disable_entity_loader() is used.

Calling libxml_disable_entity_loader() before Kernel::hande() fails now with a XmlParsingException due to XmlFileLoader not temporarily enabling XXE loading. The external entity in question was src/Symfony/[...]/services-1.0.xsd which was being used to validate src/Symfony/[...]/config/web.xml.

Calling libxml_disable_entity_loader() is recommended by OWASP, and is a very good security practice to globally prevent XXE attacks:
https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#php

You can modify the xxe_schema.xsd file introduced in this commit to point to a https://webhook.site/ URL instead of file:///dev/null. This will demonstrate that these XXE calls will even try to make network requests.

I don't like where I placed the test I added. I chose XmlFileLoaderTest because that XmlFileLoader was causing Symfony to crash in my case. I think it's a good test to have, though, as it will prevent regressions on this. Perhasp you can suggest a better place.

@fabianbadoi fabianbadoi requested a review from dunglas as a code owner October 14, 2020 08:51
@stof
Copy link
Member

stof commented Oct 14, 2020

The PR where the function was deprecated, php/php-src#5867, claims that XXE loading is diabled by default since libxml 2.9.0.

However, this was not the case in at least the following systems:

* the php:7.4-fpm-alpine docker image

* the php:8.0-rc-cli-alpine

* the default php-7.4 package from the Ubuntu 20.04 package repositories

All of these installations report LIBXML_VERSION > 20900 and are still vulnerable to XXE attacks unless libxml_disable_entity_loader() is used.

this should be reported as a bug in the PHP issue tracker IMO, as that might invalidate the decision to deprecate the function.

@fabianbadoi
Copy link
Author

I tried reporting it this morning, the bug tracker was down :(

@fabianbadoi
Copy link
Author

We have a bug report: https://bugs.php.net/bug.php?id=80235

@fabianbadoi
Disable XML external entity loading for all versions of libxml
10a3b6f 
The libxml_disable_entity_loader() function was deprecated in PHP 8.0
and will trigger a deprecation notice going forward. However, external
entity loading is still *enabled*.

The PR where the function was deprecated, php/php-src#5867,
claims that XXE loading is diabled by default since libxml 2.9.0.

However, this was not the case in at least the following systems:
- the php:7.4-fpm-alpine docker image
- the php:8.0-rc-cli-alpine
- the default php-7.4 package from the Ubuntu 20.04 package repositories

All of these installations report LIBXML_VERSION > 20900 and are still
vulnerable to XXE attacks unless libxml_disable_entity_loader() is used.

Calling libxml_disable_entity_loader() before Kernel::hande() fails now
with a XmlParsingException due to XmlFileLoader not temporarily enabling
XXE loading. The external entity in question was src/Symfony/[...]/services-1.0.xsd
which was being used to validate src/Symfony/[...]/config/web.xml.

Calling libxml_disable_entity_loader() is recommended by OWASP, and is
a very good security practice to globally prevent XXE attacks:
https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#php

You can modify the xxe_schema.xsd file introduced in this commit to
point to a https://webhook.site/ URL instead of file:///dev/null. This
will demonstrate that these XXE calls will even try to make network
requests.
@fabianbadoi
Copy link
Author

fabianbadoi commented Oct 14, 2020 •
edited
Loading

I'd like some guidance on this PR:

  1. Travis fails on PHP 5.5 because phpunit is too old there, I can't figure out how to test this myself
  2. Appveyor fails for the same reason
  3. Travis also fails on PHP nigly because of legacy deprecation related to exactly this issue. I'm not sure what to do about this.
  4. fabbot's suggestion changes an error meesage:
+++ src/Symfony/Component/Serializer/Encoder/XmlEncoder.php     2020-10-14 11:56:38.534401092 +0000
@@ -431,7 +431,7 @@
             return $this->appendNode($parentNode, $data, 'data');
         }
 
-        throw new NotEncodableValueException('An unexpected value could not be serialized: '.(!\is_resource($data) ? var_export($data, true) : sprintf('%s resource', get_resource_type($data))));
+        throw new NotEncodableValueException('An unexpected value could not be serialized: '.(!\is_resource($data) ? var_export($data, true) : sprintf('"%s" resource', get_resource_type($data))));
     }
 
     /**

I have nothing against chaning it here, idk if you would accept unrelated changes.

@stof
Copy link
Member

stof commented Oct 14, 2020 

3\. Travis also fails on PHP nigly because of legacy deprecation related to exactly this issue. I'm not sure what to do about this.

well, that's precisely why I asked to report an issue to PHP. Symfony has a policy of not using deprecated APIs. So if using the deprecated API of PHP cannot be avoided, there is an issue with the fact of deprecating.

@stof
Copy link
Member

stof commented Oct 14, 2020

We have a bug report: https://bugs.php.net/bug.php?id=80235

this link is private

@fabianbadoi
Copy link
Author

I also think we should wait to see what the PHP devs do with this, since they might not consider it serious enough to address. Installations are still safe from the traditional XXE attacks that reads local files.

@fabianbadoi
Copy link
Author

We have a bug report: https://bugs.php.net/bug.php?id=80235

this link is private

I think they set all tickets related to security to private by default

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Oct 14, 2020
@nicolas-grekas
Copy link
Member

I rebased the PR for branch 3.4 but there are some failure to fix, could you please have a look?

@nicolas-grekas nicolas-grekas changed the title Disable XML external entity loading for all versions of libxml [Config] Disable XML external entity loading for all versions of libxml Oct 14, 2020
@nicolas-grekas
Copy link
Member

(nvm the rebase for 3.4, that's on another PR)

@jderusse
Copy link
Member

The PHP bug has been closed. Calling libxml_disable_entity_loader is not required and trigger deprecation on PHP 8. Shall we close this PR?

@fabianbadoi
Copy link
Author

The PHP bug has been closed. Calling libxml_disable_entity_loader is not required and trigger deprecation on PHP 8. Shall we close this PR?

Yes, closing.

@fabianbadoi fabianbadoi deleted the xxe-still-alive branch October 16, 2020 06:21
@nicolas-grekas nicolas-grekas mentioned this pull request Mar 23, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

Assignees
No one assigned
Labels
Bug Config DependencyInjection DomCrawler Serializer Status: Needs Review Translation
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
6 participants
@fabianbadoi @stof @nicolas-grekas @jderusse @xabbuh @carsonbot