Fixes for potential XXE/XEE vulnerabilities

- Patch provided by Padriac Brady, reviewed by Ralph Schindler and Matthew Weier O'Phinney - Merged from r25031 git-svn-id: http://framework.zend.com/svn/framework/standard/branches/release-1.12@25033 44c647ce-9c0f-0410-b52a-842ac1e357ba
zendframework · Aug 17, 2012 · 1b5e861 · 1b5e861
1 parent 1693f72
commit 1b5e861
Show file tree

Hide file tree

Showing 17 changed files with 847 additions and 8 deletions.
diff --git a/library/Zend/Dom/Query.php b/library/Zend/Dom/Query.php
@@ -245,6 +245,7 @@ public function queryXpath($xpathQuery, $query = null)
 
         $encoding = $this->getEncoding();
         libxml_use_internal_errors(true);
+        libxml_disable_entity_loader(true);
         if (null === $encoding) {
             $domDoc = new DOMDocument('1.0');
         } else {
@@ -254,6 +255,14 @@ public function queryXpath($xpathQuery, $query = null)
         switch ($type) {
             case self::DOC_XML:
                 $success = $domDoc->loadXML($document);
+                foreach ($domDoc->childNodes as $child) {
+                    if ($child->nodeType === XML_DOCUMENT_TYPE_NODE) {
+                        require_once 'Zend/Dom/Exception.php';
+                        throw new Zend_Dom_Exception(
+                            'Invalid XML: Detected use of illegal DOCTYPE'
+                        );
+                    }
+                }
                 break;
             case self::DOC_HTML:
             case self::DOC_XHTML:
@@ -266,6 +275,7 @@ public function queryXpath($xpathQuery, $query = null)
             $this->_documentErrors = $errors;
             libxml_clear_errors();
         }
+        libxml_disable_entity_loader(false);
         libxml_use_internal_errors(false);
 
         if (!$success) {

diff --git a/library/Zend/Feed/Reader.php b/library/Zend/Feed/Reader.php
@@ -333,10 +333,19 @@ public static function importFeed(Zend_Feed_Abstract $feed)
      */
     public static function importString($string)
     {
-
         $libxml_errflag = libxml_use_internal_errors(true);
+        $oldValue = libxml_disable_entity_loader(true);
         $dom = new DOMDocument;
         $status = $dom->loadXML($string);
+        foreach ($dom->childNodes as $child) {
+            if ($child->nodeType === XML_DOCUMENT_TYPE_NODE) {
+                require_once 'Zend/Feed/Exception.php';
+                throw new Zend_Feed_Exception(
+                    'Invalid XML: Detected use of illegal DOCTYPE'
+                );
+            }
+        }
+        libxml_disable_entity_loader($oldValue);
         libxml_use_internal_errors($libxml_errflag);
 
         if (!$status) {
@@ -407,8 +416,10 @@ public static function findFeedLinks($uri)
         }
         $responseHtml = $response->getBody();
         $libxml_errflag = libxml_use_internal_errors(true);
+        $oldValue = libxml_disable_entity_loader(true);
         $dom = new DOMDocument;
         $status = $dom->loadHTML($responseHtml);
+        libxml_disable_entity_loader($oldValue);
         libxml_use_internal_errors($libxml_errflag);
         if (!$status) {
             // Build error message
@@ -442,8 +453,18 @@ public static function detectType($feed, $specOnly = false)
             $dom = $feed;
         } elseif(is_string($feed) && !empty($feed)) {
             @ini_set('track_errors', 1);
+            $oldValue = libxml_disable_entity_loader(true);
             $dom = new DOMDocument;
             $status = @$dom->loadXML($feed);
+            foreach ($dom->childNodes as $child) {
+                if ($child->nodeType === XML_DOCUMENT_TYPE_NODE) {
+                    require_once 'Zend/Feed/Exception.php';
+                    throw new Zend_Feed_Exception(
+                        'Invalid XML: Detected use of illegal DOCTYPE'
+                    );
+                }
+            }
+            libxml_disable_entity_loader($oldValue);
             @ini_restore('track_errors');
             if (!$status) {
                 if (!isset($php_errormsg)) {

diff --git a/library/Zend/Serializer/Adapter/Wddx.php b/library/Zend/Serializer/Adapter/Wddx.php
@@ -100,7 +100,19 @@ public function unserialize($wddx, array $opts = array())
             // check if the returned NULL is valid
             // or based on an invalid wddx string
             try {
-                $simpleXml = new SimpleXMLElement($wddx);
+                $oldLibxmlDisableEntityLoader = libxml_disable_entity_loader(true);
+                $dom = new DOMDocument;
+                $dom->loadXML($wddx);
+                foreach ($dom->childNodes as $child) {
+                    if ($child->nodeType === XML_DOCUMENT_TYPE_NODE) {
+                        require_once 'Zend/Serializer/Exception.php';
+                        throw new Zend_Serializer_Exception(
+                            'Invalid XML: Detected use of illegal DOCTYPE'
+                        );
+                    }
+                }
+                $simpleXml = simplexml_import_dom($dom);
+                libxml_disable_entity_loader($oldLibxmlDisableEntityLoader);
                 if (isset($simpleXml->data[0]->null[0])) {
                     return null; // valid null
                 }

diff --git a/library/Zend/Soap/Client/Local.php b/library/Zend/Soap/Client/Local.php
@@ -84,6 +84,13 @@ public function _doRequest(Zend_Soap_Client_Common $client, $request, $location,
         ob_start();
         $this->_server->handle($request);
         $response = ob_get_clean();
+
+        if ($response === null || $response === '') {
+            $serverResponse = $this->server->getResponse();
+            if ($serverResponse !== null) {
+                $response = $serverResponse;
+            }
+        }
 
         return $response;
     }

diff --git a/library/Zend/Soap/Server.php b/library/Zend/Soap/Server.php
@@ -729,11 +729,21 @@ protected function _setRequest($request)
                 $xml = $request;
             }
 
+            libxml_disable_entity_loader(true);
             $dom = new DOMDocument();
             if(strlen($xml) == 0 || !$dom->loadXML($xml)) {
                 require_once 'Zend/Soap/Server/Exception.php';
                 throw new Zend_Soap_Server_Exception('Invalid XML');
             }
+            foreach ($dom->childNodes as $child) {
+                if ($child->nodeType === XML_DOCUMENT_TYPE_NODE) {
+                    require_once 'Zend/Soap/Server/Exception.php';
+                    throw new Zend_Soap_Server_Exception(
+                        'Invalid XML: Detected use of illegal DOCTYPE'
+                    );
+                }
+            }
+            libxml_disable_entity_loader(false);
         }
         $this->_request = $xml;
         return $this;
@@ -866,16 +876,16 @@ public function handle($request = null)
 
         $soap = $this->_getSoap();
 
+        $fault = false;
         ob_start();
-        if($setRequestException instanceof Exception) {
-            // Send SOAP fault message if we've catched exception
-            $soap->fault("Sender", $setRequestException->getMessage());
+        if ($setRequestException instanceof Exception) {
+            // Create SOAP fault message if we've caught a request exception
+            $fault = $this->fault($setRequestException->getMessage(), 'Sender');
         } else {
             try {
                 $soap->handle($this->_request);
             } catch (Exception $e) {
                 $fault = $this->fault($e);
-                $soap->fault($fault->faultcode, $fault->faultstring);
             }
         }
         $this->_response = ob_get_clean();
@@ -884,6 +894,11 @@ public function handle($request = null)
         restore_error_handler();
         ini_set('display_errors', $displayErrorsOriginalState);
 
+        // Send a fault, if we have one
+        if ($fault) {
+            $this->_response = $fault;
+        }
+
         if (!$this->_returnResponse) {
             echo $this->_response;
             return;

diff --git a/library/Zend/Soap/Wsdl.php b/library/Zend/Soap/Wsdl.php
@@ -96,13 +96,23 @@ public function __construct($name, $uri, $strategy = true)
                     xmlns:xsd='http://www.w3.org/2001/XMLSchema'
                     xmlns:soap-enc='http://schemas.xmlsoap.org/soap/encoding/'
                     xmlns:wsdl='http://schemas.xmlsoap.org/wsdl/'></definitions>";
+        libxml_disable_entity_loader(true);
         $this->_dom = new DOMDocument();
         if (!$this->_dom->loadXML($wsdl)) {
             require_once 'Zend/Server/Exception.php';
             throw new Zend_Server_Exception('Unable to create DomDocument');
         } else {
+            foreach ($this->_dom->childNodes as $child) {
+                if ($child->nodeType === XML_DOCUMENT_TYPE_NODE) {
+                    require_once 'Zend/Server/Exception.php';
+                    throw new Zend_Server_Exception(
+                        'Invalid XML: Detected use of illegal DOCTYPE'
+                    );
+                }
+            }
             $this->_wsdl = $this->_dom->documentElement;
         }
+        libxml_disable_entity_loader(false);
 
         $this->setComplexTypeStrategy($strategy);
     }
@@ -125,8 +135,10 @@ public function setUri($uri)
             // @todo: This is the worst hack ever, but its needed due to design and non BC issues of WSDL generation
             $xml = $this->_dom->saveXML();
             $xml = str_replace($oldUri, $uri, $xml);
+            libxml_disable_entity_loader(true);
             $this->_dom = new DOMDocument();
             $this->_dom->loadXML($xml);
+            libxml_disable_entity_loader(false);
         }
 
         return $this;

diff --git a/library/Zend/XmlRpc/Request.php b/library/Zend/XmlRpc/Request.php
@@ -306,7 +306,17 @@ public function loadXml($request)
         // @see ZF-12293 - disable external entities for security purposes
         $loadEntities = libxml_disable_entity_loader(true);
         try {
-            $xml = new SimpleXMLElement($request);
+            $dom = new DOMDocument;
+            $dom->loadXML($request);
+            foreach ($dom->childNodes as $child) {
+                if ($child->nodeType === XML_DOCUMENT_TYPE_NODE) {
+                    require_once 'Zend/XmlRpc/Exception.php';
+                    throw new Zend_XmlRpc_Exception(
+                        'Invalid XML: Detected use of illegal DOCTYPE'
+                    );
+                }
+            }
+            $xml = simplexml_import_dom($dom);
             libxml_disable_entity_loader($loadEntities);
         } catch (Exception $e) {
             // Not valid XML

diff --git a/library/Zend/XmlRpc/Response.php b/library/Zend/XmlRpc/Response.php
@@ -180,6 +180,18 @@ public function loadXml($response)
         $loadEntities         = libxml_disable_entity_loader(true);
         $useInternalXmlErrors = libxml_use_internal_errors(true);
         try {
+            $dom = new DOMDocument;
+            $dom->loadXML($response);
+            foreach ($dom->childNodes as $child) {
+                if ($child->nodeType === XML_DOCUMENT_TYPE_NODE) {
+                    require_once 'Zend/XmlRpc/Exception.php';
+                    throw new Zend_XmlRpc_Exception(
+                        'Invalid XML: Detected use of illegal DOCTYPE'
+                    );
+                }
+            }
+            // TODO: Locate why this passes tests but a simplexml import doesn't
+            // $xml = simplexml_import_dom($dom);
             $xml = new SimpleXMLElement($response);
             libxml_disable_entity_loader($loadEntities);
             libxml_use_internal_errors($useInternalXmlErrors);

diff --git a/tests/Zend/Dom/QueryTest.php b/tests/Zend/Dom/QueryTest.php
@@ -348,6 +348,20 @@ public function testXhtmlDocumentWithXmlAndDoctypeDeclaration()
         $this->query->setDocument($xhtmlWithXmlDecl, 'utf-8');
         $this->assertEquals(1, $this->query->query('//p')->count());
     }
+
+    public function testLoadingXmlContainingDoctypeShouldFailToPreventXxeAndXeeAttacks()
+    {
+        $xml = <<<XML
+<?xml version="1.0"?>
+<!DOCTYPE results [<!ENTITY harmless "completely harmless">]>
+<results>
+    <result>This result is &harmless;</result>
+</results>
+XML;
+        $this->query->setDocumentXml($xml);
+        $this->setExpectedException("Zend_Dom_Exception");
+        $this->query->queryXpath('/');
+    }
 }
 
 // Call Zend_Dom_QueryTest::main() if this source file is executed directly.

diff --git a/tests/Zend/Feed/Reader/_files/Reader/xxe-atom10.xml b/tests/Zend/Feed/Reader/_files/Reader/xxe-atom10.xml
@@ -0,0 +1,5 @@
+<?xml version="1.0" encoding="utf-8"?>
+<!DOCTYPE results [ <!ENTITY discloseInfo SYSTEM "XXE_URI"> ]>
+<feed xmlns="http://www.w3.org/2005/Atom">
+    <title type="text">info:&discloseInfo;</title>
+</feed>
diff --git a/tests/Zend/Feed/Reader/_files/Reader/xxe-info.txt b/tests/Zend/Feed/Reader/_files/Reader/xxe-info.txt
@@ -0,0 +1 @@
+xxe-information-disclosed
diff --git a/tests/Zend/Feed/ReaderTest.php b/tests/Zend/Feed/ReaderTest.php
@@ -336,6 +336,14 @@ public function testImportingUriWithEmptyResponseBodyTriggersException()
         $result = Zend_Feed_Reader::import('http://www.example.com');
     }
 
+     public function testXxePreventionOnFeedParsing()
+     {
+         $string = file_get_contents($this->_feedSamplePath.'/Reader/xxe-atom10.xml');
+         $string = str_replace('XXE_URI', $this->_feedSamplePath.'/Reader/xxe-info.txt', $string);
+         $this->setExpectedException('Zend_Feed_Exception');
+         $feed = Zend_Feed_Reader::importString($string);
+     }
+
     protected function _getTempDirectory()
     {
         $tmpdir = array();

diff --git a/tests/Zend/Serializer/Adapter/WddxTest.php b/tests/Zend/Serializer/Adapter/WddxTest.php
@@ -221,6 +221,28 @@ public function testSerializeStringUtf8() {
         $this->assertEquals($expected, $data);
     }
 
+    public function testUnserializeInvalidXml()
+    {
+        if (!class_exists('SimpleXMLElement', false)) {
+            $this->markTestSkipped('Skipped by missing ext/simplexml');
+        }
+
+        $value = 'not a serialized string';
+        $this->setExpectedException(
+            'Zend_Serializer_Exception',
+            'DOMDocument::loadXML(): Start tag expected'
+        );
+        $this->_adapter->unserialize($value);
+    }
+
+    public function testShouldThrowExceptionIfXmlToUnserializeFromContainsADoctype()
+    {
+        $value    = '<!DOCTYPE>'
+                  . '<wddxPacket version=\'1.0\'><header/>'
+                  . '<data><string>test</string></data></wddxPacket>';
+        $this->setExpectedException("Zend_Serializer_Exception");
+        $data = $this->_adapter->unserialize($value);
+    }
 }
 
 

diff --git a/tests/Zend/Soap/ServerTest.php b/tests/Zend/Soap/ServerTest.php
@@ -27,6 +27,8 @@
 
 require_once "Zend/Config.php";
 
+require_once dirname(__FILE__) . '/TestAsset/commontypes.php';
+
 /**
  * Zend_Soap_Server
  *
@@ -542,6 +544,9 @@ public function testGetPersistence()
         $this->assertEquals(SOAP_PERSISTENCE_SESSION, $server->getPersistence());
     }
 
+    /**
+     * @runInSeparateProcess
+     */
     public function testGetLastRequest()
     {
         if (headers_sent()) {
@@ -575,6 +580,9 @@ public function testGetLastRequest()
         $this->assertEquals($request, $server->getLastRequest());
     }
 
+    /**
+     * @runInSeparateProcess
+     */
     public function testWsiCompliant()
     {
         $server = new Zend_Soap_Server(null, array('wsi_compliant' => true));
@@ -877,11 +885,13 @@ public function testLoadFunctionsIsNotImplemented()
         }
     }
 
+    /**
+     * @runInSeparateProcess
+     */
     public function testErrorHandlingOfSoapServerChangesToThrowingSoapFaultWhenInHandleMode()
     {
         if (headers_sent()) {
             $this->markTestSkipped('Cannot run ' . __METHOD__ . '() when headers have already been sent; enable output buffering to run this test');
-            return;
         }
 
         $server = new Zend_Soap_Server();
@@ -967,6 +977,35 @@ public function testHandleUsesProperRequestParameter()
         $r = $server->handle(new DomDocument('1.0', 'UTF-8'));
         $this->assertTrue(is_string($server->mockSoapServer->handle[0]));
     }
+
+    /**
+     * @runInSeparateProcess
+     */
+    public function testShouldThrowExceptionIfHandledRequestContainsDoctype()
+    {
+        $server = new Zend_Soap_Server();
+        $server->setOptions(array('location'=>'test://', 'uri'=>'http://framework.zend.com'));
+        $server->setReturnResponse(true);
+
+        $server->setClass('Zend_Soap_TestAsset_ServerTestClass');
+
+        $request =
+            '<?xml version="1.0" encoding="UTF-8"?>' . "\n" . '<!DOCTYPE foo>' . "\n"
+          . '<SOAP-ENV:Envelope xmlns:SOAP-ENV="http://schemas.xmlsoap.org/soap/envelope/" '
+                             . 'xmlns:ns1="http://framework.zend.com" '
+                             . 'xmlns:xsd="http://www.w3.org/2001/XMLSchema" '
+                             . 'xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" '
+                             . 'xmlns:SOAP-ENC="http://schemas.xmlsoap.org/soap/encoding/" '
+                             . 'SOAP-ENV:encodingStyle="http://schemas.xmlsoap.org/soap/encoding/">'
+          .     '<SOAP-ENV:Body>'
+          .         '<ns1:testFunc2>'
+          .             '<param0 xsi:type="xsd:string">World</param0>'
+          .         '</ns1:testFunc2>'
+          .     '</SOAP-ENV:Body>'
+          . '</SOAP-ENV:Envelope>' . "\n";
+        $response = $server->handle($request);
+        $this->assertContains('Invalid XML', $response->getMessage());
+    }
 }