Merge remote-tracking branch 'weierophinney/hotfix/xmlrpc-disable-ext…

…ernal-entities'
zendframework · Jun 19, 2012 · 9881dbb · 9881dbb
commit 9881dbb
Show file tree

Hide file tree

Showing 7 changed files with 58 additions and 1 deletion.
diff --git a/src/Request.php b/src/Request.php
@@ -294,12 +294,16 @@ public function loadXml($request)
             return false;
         }
 
+        // @see ZF-12293 - disable external entities for security purposes
+        $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 Fault(631);
             $this->_fault->setEncoding($this->getEncoding());
+            libxml_disable_entity_loader($loadEntities);
             return false;
         }
 

diff --git a/src/Response.php b/src/Response.php
@@ -167,11 +167,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 Fault(651);

diff --git a/test/RequestTest.php b/test/RequestTest.php
@@ -333,4 +333,19 @@ public function testSetGetEncoding()
         $this->assertEquals('ISO-8859-1', $this->_request->getEncoding());
         $this->assertEquals('ISO-8859-1', Value::getGenerator()->getEncoding());
     }
+
+    /**
+     * @group ZF-12293
+     */
+    public function testDoesNotAllowExternalEntities()
+    {
+        $payload = file_get_contents(dirname(__FILE__) . '/_files/ZF12293-request.xml');
+        $payload = sprintf($payload, 'file://' . realpath(dirname(__FILE__) . '/_files/ZF12293-payload.txt'));
+        $this->_request->loadXml($payload);
+        $method = $this->_request->getMethod();
+        $this->assertTrue(empty($method));
+        if (is_string($method)) {
+            $this->assertNotContains('Local file inclusion', $method);
+        }
+    }
 }
diff --git a/test/ResponseTest.php b/test/ResponseTest.php
@@ -259,4 +259,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/test/_files/ZF12293-payload.txt b/test/_files/ZF12293-payload.txt
@@ -0,0 +1 @@
+Local file inclusion
diff --git a/test/_files/ZF12293-request.xml b/test/_files/ZF12293-request.xml
@@ -0,0 +1,8 @@
+<?xml version="1.0"?>
+<!DOCTYPE foo [
+<!ELEMENT methodName ANY >
+<!ENTITY xxe SYSTEM "%s" >
+]>
+<methodCall>
+    <methodName>&xxe;</methodName>
+</methodCall>
diff --git a/test/_files/ZF12293-response.xml b/test/_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>