Security improvement suggested by Nils Engelbertz to prevent DDOS by …

…expansion of internally defined entities (XEE)
zks888 · Jan 23, 2019 · c6ca992 · c6ca992
1 parent d9e316a
commit c6ca992
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 10 deletions.
diff --git a/lib/Saml2/Utils.php b/lib/Saml2/Utils.php
@@ -84,14 +84,20 @@ public static function loadXML($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 {

diff --git a/tests/src/OneLogin/Saml2/UtilsTest.php b/tests/src/OneLogin/Saml2/UtilsTest.php
@@ -70,9 +70,9 @@ public function testXMLAttacks()
                       <!ENTITY xxe SYSTEM "file:///etc/passwd" >]><foo>&xxe;</foo>';
         try {
             $res = OneLogin_Saml2_Utils::loadXML($dom, $attackXXE);
-            $this->fail('Exception was not raised');
+            $this->assertFalse($res);
         } 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"?>
@@ -83,8 +83,12 @@ public function testXMLAttacks()
                           <results>
                             <result>test</result>
                           </results>';
-        $res2 = OneLogin_Saml2_Utils::loadXML($dom, $xmlWithDTD);
-        $this->assertTrue($res2 instanceof DOMDocument);
+        try {
+            $res2 = OneLogin_Saml2_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">]>
@@ -93,9 +97,21 @@ public function testXMLAttacks()
                       </results>';
         try {
             $res3 = OneLogin_Saml2_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 = OneLogin_Saml2_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());
         }
     }