Security improvement suggested by Nils Engelbertz to prevent DDOS by expansion of internally defined entities (XEE)

Author: pitbulk

src/Saml2/Utils.php

@@ -82,14 +82,20 @@ public static function loadXML(DOMDocument $dom, $xml)
         assert($dom instanceof DOMDocument);
         assert(is_string($xml));
 
-        if (strpos($xml, '<!ENTITY') !== false) {
-            throw new Exception('Detected use of ENTITY in XML, disabled to prevent XXE/XEE attacks');
-        }
-
         $oldEntityLoader = libxml_disable_entity_loader(true);
+
         $res = $dom->loadXML($xml);
+
         libxml_disable_entity_loader($oldEntityLoader);
 
+        foreach ($dom->childNodes as $child) {
+            if ($child->nodeType === XML_DOCUMENT_TYPE_NODE) {
+                throw new Exception(
+                    'Detected use of DOCTYPE/ENTITY in XML, disabled to prevent XXE/XEE attacks'
+                );
+            }
+        }
+
         if (!$res) {
             return false;
         } else {

tests/src/OneLogin/Saml2/UtilsTest.php

@@ -60,7 +60,7 @@ public function testXMLAttacks()
             $res = Utils::loadXML($dom, $attackXXE);
             $this->fail('Exception was not raised');
         } catch (Exception $e) {
-            $this->assertEquals('Detected use of ENTITY in XML, disabled to prevent XXE/XEE attacks', $e->getMessage());
+            $this->assertEquals('Detected use of DOCTYPE/ENTITY in XML, disabled to prevent XXE/XEE attacks', $e->getMessage());
         }
 
         $xmlWithDTD = '<?xml version="1.0"?>
@@ -71,8 +71,12 @@ public function testXMLAttacks()
                           <results>
                             <result>test</result>
                           </results>';
-        $res2 = Utils::loadXML($dom, $xmlWithDTD);
-        $this->assertTrue($res2 instanceof DOMDocument);
+        try {
+            $res2 = Utils::loadXML($dom, $xmlWithDTD);
+            $this->assertFalse($res2);
+        } catch (Exception $e) {
+            $this->assertEquals('Detected use of DOCTYPE/ENTITY in XML, disabled to prevent XXE/XEE attacks', $e->getMessage());
+        }
 
         $attackXEE = '<?xml version="1.0"?>
                       <!DOCTYPE results [<!ENTITY harmless "completely harmless">]>
@@ -81,9 +85,21 @@ public function testXMLAttacks()
                       </results>';
         try {
             $res3 = Utils::loadXML($dom, $attackXEE);
-            $this->fail('Exception was not raised');
+            $this->assertFalse($res3);
+        } catch (Exception $e) {
+            $this->assertEquals('Detected use of DOCTYPE/ENTITY in XML, disabled to prevent XXE/XEE attacks', $e->getMessage());
+        }
+
+        $attackXEEutf16 = mb_convert_encoding('<?xml version="1.0" encoding="UTF-16"?>
+                      <!DOCTYPE results [<!ENTITY harmless "completely harmless">]>
+                      <results>
+                        <result>This result is &harmless;</result>
+                      </results>', 'UTF-16');
+        try {
+            $res4 = Utils::loadXML($dom, $attackXEEutf16);
+            $this->assertFalse($res4);
         } catch (Exception $e) {
-            $this->assertEquals('Detected use of ENTITY in XML, disabled to prevent XXE/XEE attacks', $e->getMessage());
+            $this->assertEquals('Detected use of DOCTYPE/ENTITY in XML, disabled to prevent XXE/XEE attacks', $e->getMessage());
         }
     }