[ZF-12293] Ensure XML-RPC response does not load entities

Merges r24975 - Disable loading external entities - Does not affect actual internal functionality, but could be used as a potential local DoS on clients git-svn-id: http://framework.zend.com/svn/framework/standard/branches/release-1.12@24977 44c647ce-9c0f-0410-b52a-842ac1e357ba
zendframework · Jun 19, 2012 · 281a325 · 281a325
1 parent 728636d
commit 281a325
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 5 deletions.
diff --git a/library/Zend/XmlRpc/Request.php b/library/Zend/XmlRpc/Request.php
@@ -307,6 +307,7 @@ public function loadXml($request)
         $loadEntities = libxml_disable_entity_loader(true);
         try {
             $xml = new SimpleXMLElement($request);
+            libxml_disable_entity_loader($loadEntities);
         } catch (Exception $e) {
             // Not valid XML
             $this->_fault = new Zend_XmlRpc_Fault(631);
@@ -320,7 +321,6 @@ public function loadXml($request)
             // Missing method name
             $this->_fault = new Zend_XmlRpc_Fault(632);
             $this->_fault->setEncoding($this->getEncoding());
-            libxml_disable_entity_loader($loadEntities);
             return false;
         }
 
@@ -334,7 +334,6 @@ public function loadXml($request)
                 if (!isset($param->value)) {
                     $this->_fault = new Zend_XmlRpc_Fault(633);
                     $this->_fault->setEncoding($this->getEncoding());
-                    libxml_disable_entity_loader($loadEntities);
                     return false;
                 }
 
@@ -345,7 +344,6 @@ public function loadXml($request)
                 } catch (Exception $e) {
                     $this->_fault = new Zend_XmlRpc_Fault(636);
                     $this->_fault->setEncoding($this->getEncoding());
-                    libxml_disable_entity_loader($loadEntities);
                     return false;
                 }
             }
@@ -354,7 +352,6 @@ public function loadXml($request)
             $this->_params = $argv;
         }
 
-        libxml_disable_entity_loader($loadEntities);
         $this->_xml = $request;
 
         return true;

diff --git a/library/Zend/XmlRpc/Response.php b/library/Zend/XmlRpc/Response.php
@@ -176,11 +176,15 @@ public function loadXml($response)
             return false;
         }
 
+        // @see ZF-12293 - disable external entities for security purposes
+        $loadEntities         = libxml_disable_entity_loader(true);
+        $useInternalXmlErrors = libxml_use_internal_errors(true);
         try {
-            $useInternalXmlErrors = libxml_use_internal_errors(true);
             $xml = new SimpleXMLElement($response);
+            libxml_disable_entity_loader($loadEntities);
             libxml_use_internal_errors($useInternalXmlErrors);
         } catch (Exception $e) {
+            libxml_disable_entity_loader($loadEntities);
             libxml_use_internal_errors($useInternalXmlErrors);
             // Not valid XML
             $this->_fault = new Zend_XmlRpc_Fault(651);
@@ -205,6 +209,7 @@ public function loadXml($response)
 
         try {
             if (!isset($xml->params) || !isset($xml->params->param) || !isset($xml->params->param->value)) {
+                require_once 'Zend/XmlRpc/Value/Exception.php';
                 throw new Zend_XmlRpc_Value_Exception('Missing XML-RPC value in XML');
             }
             $valueXml = $xml->params->param->value->asXML();

diff --git a/tests/Zend/XmlRpc/ResponseTest.php b/tests/Zend/XmlRpc/ResponseTest.php
@@ -252,4 +252,19 @@ public function trackError($error)
     {
         $this->_errorOccured = true;
     }
+
+    /**
+     * @group ZF-12293
+     */
+    public function testDoesNotAllowExternalEntities()
+    {
+        $payload = file_get_contents(dirname(__FILE__) . '/_files/ZF12293-response.xml');
+        $payload = sprintf($payload, 'file://' . realpath(dirname(__FILE__) . '/_files/ZF12293-payload.txt'));
+        $this->_response->loadXml($payload);
+        $value = $this->_response->getReturnValue();
+        $this->assertTrue(empty($value));
+        if (is_string($value)) {
+            $this->assertNotContains('Local file inclusion', $value);
+        }
+    }
 }
diff --git a/tests/Zend/XmlRpc/_files/ZF12293-response.xml b/tests/Zend/XmlRpc/_files/ZF12293-response.xml
@@ -0,0 +1,10 @@
+<?xml version="1.0"?>
+<!DOCTYPE foo [
+<!ELEMENT methodResponse ANY >
+<!ENTITY xxe SYSTEM "%s" >
+]>
+<methodResponse>
+    <params>
+        <param><value><string>&xxe;</string></value></param>
+    </params>
+</methodResponse>