From b5abc3845581ee922ae9ef50e5caecb21f5a4ec7 Mon Sep 17 00:00:00 2001
From: Steve Boyd <emteknetnz@gmail.com>
Date: Mon, 14 Feb 2022 18:25:51 +1300
Subject: [PATCH] CVE-2021-41559 Disable xml entities

---
 src/Core/Convert.php           |  46 ++++++-------
 tests/php/Core/ConvertTest.php | 115 +++++++++++++++++++++------------
 2 files changed, 92 insertions(+), 69 deletions(-)

diff --git a/src/Core/Convert.php b/src/Core/Convert.php
index 872c3fe5e..6d1af179e 100644
--- a/src/Core/Convert.php
+++ b/src/Core/Convert.php
@@ -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.
@@ -29,7 +28,6 @@ use Exception;
  */
 class Convert
 {
-
     /**
      * Convert a value to be suitable for an XML attribute.
      *
@@ -296,40 +294,34 @@ class Convert
      * @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);
+        // 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');
         }
-        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);
-        }
-        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);
     }
 
     /**
diff --git a/tests/php/Core/ConvertTest.php b/tests/php/Core/ConvertTest.php
index 76aec076f..9b724d3cb 100644
--- a/tests/php/Core/ConvertTest.php
+++ b/tests/php/Core/ConvertTest.php
@@ -403,55 +403,86 @@ PHP
      */
     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);
     }
 
     /**