Skip to content

Commit

Permalink
Merge pull request #10376 from creative-commoners/pulls/4.10/cve-2021…
Browse files Browse the repository at this point in the history 
…-41559

CVE-2021-41559 Disable xml entities
  • Loading branch information
@GuySartorelli
GuySartorelli committed Jun 28, 2022
2 parents 0bc3ed4 + b5abc38 commit 410c2a8
Show file tree
Hide file tree
Showing 2 changed files with 92 additions and 69 deletions.
46 changes: 19 additions & 27 deletions src/Core/Convert.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,11 @@

namespace SilverStripe\Core;

use InvalidArgumentException;
use SimpleXMLElement;
use SilverStripe\Dev\Deprecation;
use SilverStripe\ORM\DB;
use SilverStripe\View\Parsers\URLSegmentFilter;
use InvalidArgumentException;
use SimpleXMLElement;
use Exception;

/**
* Library of conversion functions, implemented as static methods.
Expand All @@ -29,7 +28,6 @@
*/
class Convert
{

/**
* Convert a value to be suitable for an XML attribute.
*
Expand Down Expand Up @@ -296,40 +294,34 @@ public static function json2array($val)
* @param string $val
* @param boolean $disableDoctypes Disables the use of DOCTYPE, and will trigger an error if encountered.
* false by default.
* @param boolean $disableExternals Disables the loading of external entities. false by default. No-op in PHP 8.
* @param boolean $disableExternals Does nothing because xml entities are removed
* @deprecated 4.11.0:5.0.0
* @return array
* @throws Exception
*/
public static function xml2array($val, $disableDoctypes = false, $disableExternals = false)
{
// PHP 8 deprecates libxml_disable_entity_loader() as it is no longer needed
if (\PHP_VERSION_ID >= 80000) {
$disableExternals = false;
}
Deprecation::notice('4.10', 'Use a dedicated XML library instead');

// Check doctype
if ($disableDoctypes && preg_match('/\<\!DOCTYPE.+]\>/', $val)) {
if ($disableDoctypes && strpos($val ?? '', '<!DOCTYPE') !== false) {
throw new InvalidArgumentException('XML Doctype parsing disabled');
}

// Disable external entity loading
$oldVal = null;
if ($disableExternals) {
$oldVal = libxml_disable_entity_loader($disableExternals);
}
try {
$xml = new SimpleXMLElement($val);
$result = self::recursiveXMLToArray($xml);
} catch (Exception $ex) {
if ($disableExternals) {
libxml_disable_entity_loader($oldVal);
}
throw $ex;
}
if ($disableExternals) {
libxml_disable_entity_loader($oldVal);
// CVE-2021-41559 Ensure entities are removed due to their inherent security risk via
// XXE attacks and quadratic blowup attacks, and also lack of consistent support
$val = preg_replace('/(?s)<!ENTITY.*?>/', '', $val ?? '');

// If there's still an <!ENTITY> present, then it would be the result of a maliciously
// crafted XML document e.g. <!ENTITY><!<!ENTITY>ENTITY ext SYSTEM "http://evil.com">
if (strpos($val ?? '', '<!ENTITY') !== false) {
throw new InvalidArgumentException('Malicious XML entity detected');
}
return $result;

// This will throw an exception if the XML contains references to any internal entities
// that were defined in an <!ENTITY /> before it was removed
$xml = new SimpleXMLElement($val ?? '');
return self::recursiveXMLToArray($xml);
}

/**
Expand Down
115 changes: 73 additions & 42 deletions tests/php/Core/ConvertTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -403,55 +403,86 @@ public function testRaw2JsonWithContext()
*/
public function testXML2Array()
{
// Ensure an XML file at risk of entity expansion can be avoided safely

$inputXML = <<<XML
<?xml version="1.0"?>
<!DOCTYPE results [<!ENTITY long "SOME_SUPER_LONG_STRING">]>
<!DOCTYPE results [
<!ENTITY long "SOME_SUPER_LONG_STRING">
]>
<results>
<result>Now include &long; lots of times to expand the in-memory size of this XML structure</result>
<result>&long;&long;&long;</result>
<result>My para</result>
<result>Ampersand &amp; is retained and not double encoded</result>
</results>
XML
;
try {
Convert::xml2array($inputXML, true);
} catch (Exception $ex) {
}
$this->assertTrue(
isset($ex)
&& $ex instanceof InvalidArgumentException
&& $ex->getMessage() === 'XML Doctype parsing disabled'
);

// Test without doctype validation
;
$expected = [
'result' => [
'Now include SOME_SUPER_LONG_STRING lots of times to expand the in-memory size of this XML structure',
[
'long' => [
[
'long' => 'SOME_SUPER_LONG_STRING'
],
[
'long' => 'SOME_SUPER_LONG_STRING'
],
[
'long' => 'SOME_SUPER_LONG_STRING'
]
]
]
]
'result' => [
'My para',
'Ampersand & is retained and not double encoded'
]
];
$result = Convert::xml2array($inputXML, false, true);
$this->assertEquals(
$expected,
$result
);
$result = Convert::xml2array($inputXML, false, false);
$this->assertEquals(
$expected,
$result
);
$actual = Convert::xml2array($inputXML, false);
$this->assertEquals($expected, $actual);
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage('XML Doctype parsing disabled');
Convert::xml2array($inputXML, true);
}

/**
* Tests {@link Convert::xml2array()} if an exception the contains a reference to a removed <!ENTITY />
*/
public function testXML2ArrayEntityException()
{
$inputXML = <<<XML
<?xml version="1.0"?>
<!DOCTYPE results [
<!ENTITY long "SOME_SUPER_LONG_STRING">
]>
<results>
<result>Now include &long; lots of times to expand the in-memory size of this XML structure</result>
<result>&long;&long;&long;</result>
</results>
XML;
$this->expectException(Exception::class);
$this->expectExceptionMessage('String could not be parsed as XML');
Convert::xml2array($inputXML);
}

/**
* Tests {@link Convert::xml2array()} if an exception the contains a reference to a multiple removed <!ENTITY />
*/
public function testXML2ArrayMultipleEntitiesException()
{
$inputXML = <<<XML
<?xml version="1.0"?>
<!DOCTYPE results [<!ENTITY long "SOME_SUPER_LONG_STRING"><!ENTITY short "SHORT_STRING">]>
<results>
<result>Now include &long; and &short; lots of times</result>
<result>&long;&long;&long;&short;&short;&short;</result>
</results>
XML;
$this->expectException(Exception::class);
$this->expectExceptionMessage('String could not be parsed as XML');
Convert::xml2array($inputXML);
}

/**
* Tests {@link Convert::xml2array()} if there is a malicious <!ENTITY /> present
*/
public function testXML2ArrayMaliciousEntityException()
{
$inputXML = <<<XML
<?xml version="1.0"?>
<!DOCTYPE results [
<!ENTITY><!<!ENTITY>ENTITY ext SYSTEM "http://evil.com">
]>
<results>
<result>Evil document</result>
</results>
XML;
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage('Malicious XML entity detected');
Convert::xml2array($inputXML);
}

/**
Expand Down

0 comments on commit 410c2a8

Please sign in to comment.