Skip to content

Commit b5abc38

Browse files
emteknetnzemteknetnz
committed
CVE-2021-41559 Disable xml entities
1 parent 0bc3ed4 commit b5abc38

File tree

2 files changed

+92
-69
lines changed

2 files changed

+92
-69
lines changed

‎src/Core/Convert.php‎

Lines changed: 19 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,11 @@
22

33
namespace SilverStripe\Core;
44

5+
use InvalidArgumentException;
6+
use SimpleXMLElement;
57
use SilverStripe\Dev\Deprecation;
68
use SilverStripe\ORM\DB;
79
use SilverStripe\View\Parsers\URLSegmentFilter;
8-
use InvalidArgumentException;
9-
use SimpleXMLElement;
10-
use Exception;
1110

1211
/**
1312
* Library of conversion functions, implemented as static methods.
@@ -29,7 +28,6 @@
2928
*/
3029
class Convert
3130
{
32-
3331
/**
3432
* Convert a value to be suitable for an XML attribute.
3533
*
@@ -296,40 +294,34 @@ public static function json2array($val)
296294
* @param string $val
297295
* @param boolean $disableDoctypes Disables the use of DOCTYPE, and will trigger an error if encountered.
298296
* false by default.
299-
* @param boolean $disableExternals Disables the loading of external entities. false by default. No-op in PHP 8.
297+
* @param boolean $disableExternals Does nothing because xml entities are removed
298+
* @deprecated 4.11.0:5.0.0
300299
* @return array
301300
* @throws Exception
302301
*/
303302
public static function xml2array($val, $disableDoctypes = false, $disableExternals = false)
304303
{
305-
// PHP 8 deprecates libxml_disable_entity_loader() as it is no longer needed
306-
if (\PHP_VERSION_ID >= 80000) {
307-
$disableExternals = false;
308-
}
304+
Deprecation::notice('4.10', 'Use a dedicated XML library instead');
309305

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

315-
// Disable external entity loading
316-
$oldVal = null;
317-
if ($disableExternals) {
318-
$oldVal = libxml_disable_entity_loader($disableExternals);
319-
}
320-
try {
321-
$xml = new SimpleXMLElement($val);
322-
$result = self::recursiveXMLToArray($xml);
323-
} catch (Exception $ex) {
324-
if ($disableExternals) {
325-
libxml_disable_entity_loader($oldVal);
326-
}
327-
throw $ex;
328-
}
329-
if ($disableExternals) {
330-
libxml_disable_entity_loader($oldVal);
311+
// CVE-2021-41559 Ensure entities are removed due to their inherent security risk via
312+
// XXE attacks and quadratic blowup attacks, and also lack of consistent support
313+
$val = preg_replace('/(?s)<!ENTITY.*?>/', '', $val ?? '');
314+
315+
// If there's still an <!ENTITY> present, then it would be the result of a maliciously
316+
// crafted XML document e.g. <!ENTITY><!<!ENTITY>ENTITY ext SYSTEM "http://evil.com">
317+
if (strpos($val ?? '', '<!ENTITY') !== false) {
318+
throw new InvalidArgumentException('Malicious XML entity detected');
331319
}
332-
return $result;
320+
321+
// This will throw an exception if the XML contains references to any internal entities
322+
// that were defined in an <!ENTITY /> before it was removed
323+
$xml = new SimpleXMLElement($val ?? '');
324+
return self::recursiveXMLToArray($xml);
333325
}
334326

335327
/**

‎tests/php/Core/ConvertTest.php‎

Lines changed: 73 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -403,55 +403,86 @@ public function testRaw2JsonWithContext()
403403
*/
404404
public function testXML2Array()
405405
{
406-
// Ensure an XML file at risk of entity expansion can be avoided safely
406+
407407
$inputXML = <<<XML
408408
<?xml version="1.0"?>
409-
<!DOCTYPE results [<!ENTITY long "SOME_SUPER_LONG_STRING">]>
409+
<!DOCTYPE results [
410+
<!ENTITY long "SOME_SUPER_LONG_STRING">
411+
]>
410412
<results>
411-
<result>Now include &long; lots of times to expand the in-memory size of this XML structure</result>
412-
<result>&long;&long;&long;</result>
413+
<result>My para</result>
414+
<result>Ampersand &amp; is retained and not double encoded</result>
413415
</results>
414416
XML
415-
;
416-
try {
417-
Convert::xml2array($inputXML, true);
418-
} catch (Exception $ex) {
419-
}
420-
$this->assertTrue(
421-
isset($ex)
422-
&& $ex instanceof InvalidArgumentException
423-
&& $ex->getMessage() === 'XML Doctype parsing disabled'
424-
);
425-
426-
// Test without doctype validation
417+
;
427418
$expected = [
428-
'result' => [
429-
'Now include SOME_SUPER_LONG_STRING lots of times to expand the in-memory size of this XML structure',
430-
[
431-
'long' => [
432-
[
433-
'long' => 'SOME_SUPER_LONG_STRING'
434-
],
435-
[
436-
'long' => 'SOME_SUPER_LONG_STRING'
437-
],
438-
[
439-
'long' => 'SOME_SUPER_LONG_STRING'
440-
]
441-
]
442-
]
443-
]
419+
'result' => [
420+
'My para',
421+
'Ampersand & is retained and not double encoded'
422+
]
444423
];
445-
$result = Convert::xml2array($inputXML, false, true);
446-
$this->assertEquals(
447-
$expected,
448-
$result
449-
);
450-
$result = Convert::xml2array($inputXML, false, false);
451-
$this->assertEquals(
452-
$expected,
453-
$result
454-
);
424+
$actual = Convert::xml2array($inputXML, false);
425+
$this->assertEquals($expected, $actual);
426+
$this->expectException(InvalidArgumentException::class);
427+
$this->expectExceptionMessage('XML Doctype parsing disabled');
428+
Convert::xml2array($inputXML, true);
429+
}
430+
431+
/**
432+
* Tests {@link Convert::xml2array()} if an exception the contains a reference to a removed <!ENTITY />
433+
*/
434+
public function testXML2ArrayEntityException()
435+
{
436+
$inputXML = <<<XML
437+
<?xml version="1.0"?>
438+
<!DOCTYPE results [
439+
<!ENTITY long "SOME_SUPER_LONG_STRING">
440+
]>
441+
<results>
442+
<result>Now include &long; lots of times to expand the in-memory size of this XML structure</result>
443+
<result>&long;&long;&long;</result>
444+
</results>
445+
XML;
446+
$this->expectException(Exception::class);
447+
$this->expectExceptionMessage('String could not be parsed as XML');
448+
Convert::xml2array($inputXML);
449+
}
450+
451+
/**
452+
* Tests {@link Convert::xml2array()} if an exception the contains a reference to a multiple removed <!ENTITY />
453+
*/
454+
public function testXML2ArrayMultipleEntitiesException()
455+
{
456+
$inputXML = <<<XML
457+
<?xml version="1.0"?>
458+
<!DOCTYPE results [<!ENTITY long "SOME_SUPER_LONG_STRING"><!ENTITY short "SHORT_STRING">]>
459+
<results>
460+
<result>Now include &long; and &short; lots of times</result>
461+
<result>&long;&long;&long;&short;&short;&short;</result>
462+
</results>
463+
XML;
464+
$this->expectException(Exception::class);
465+
$this->expectExceptionMessage('String could not be parsed as XML');
466+
Convert::xml2array($inputXML);
467+
}
468+
469+
/**
470+
* Tests {@link Convert::xml2array()} if there is a malicious <!ENTITY /> present
471+
*/
472+
public function testXML2ArrayMaliciousEntityException()
473+
{
474+
$inputXML = <<<XML
475+
<?xml version="1.0"?>
476+
<!DOCTYPE results [
477+
<!ENTITY><!<!ENTITY>ENTITY ext SYSTEM "http://evil.com">
478+
]>
479+
<results>
480+
<result>Evil document</result>
481+
</results>
482+
XML;
483+
$this->expectException(InvalidArgumentException::class);
484+
$this->expectExceptionMessage('Malicious XML entity detected');
485+
Convert::xml2array($inputXML);
455486
}
456487

457488
/**

0 commit comments

Comments
 (0)