Switch to useing XMLReader to parse XML payloads.

XMLParser would expand entities by default, which could make us
susceptible both to XXE attacks, and the billion laughs attack.

By default XMLReader does _not_ expand entities, so it's a safer choice.

This also changes the XmlRpcMethod::parse() function to throw a runtime
exception if the XML payload could not be parsed, and to return null if
the payload does not contain a valid <methodName> element.

In cases where we're unable to parse the payload as a valid XML-RPC
request, we fall back to saving the full request info as before.

3 files changed, 77 insertions, 27 deletions

diff --git a/src/XmlRpcMethod.php b/src/XmlRpcMethod.php
index 2167e53..001169b 100644
--- a/src/XmlRpcMethod.php
+++ b/src/XmlRpcMethod.php
@@ -7,6 +7,8 @@
 
 namespace VolseNet\Webtrap;
 
+use XMLReader;
+
 /**
  * A representation of an XML-RPC method call.
  *
@@ -24,34 +26,41 @@ class XmlRpcMethod
      *
      * @param string $payload   The raw XML representation of the method call.
      *
-     * @return XmlRpcMethod
+     * @return XmlRpcMethod|null
      */
-    public static function parse(string $payload): self
+    public static function parse(string $payload): self|null
     {
-        $parser = xml_parser_create();
-        xml_parse_into_struct($parser, $payload, $elements, $index);
-        xml_parser_free($parser);
+        $errorflag = libxml_use_internal_errors(true);
 
-        $in_param = false;
-        $method_name = null;
+        $xml = XMLReader::XML($payload);
 
-        foreach ($elements as $e) {
-            switch ($e['tag']) {
-                case 'METHODNAME':
-                    $method_name = $e['value'];
-                    break;
+        $method_name = null;
+        $params = [];
+        $expect = null;
 
-                case 'PARAM':
-                    if ($e['type'] === 'open') {
-                        $in_param = true;
+        while ($xml->read()) {
+            $tag = strtolower($xml->name);
+            switch ($tag) {
+                case 'methodname':
+                case 'value':
+                    if ($xml->nodeType === XMLReader::ELEMENT) {
+                        $expect = $tag;
                     } else {
-                        $in_param = false;
+                        $expect = null;
                     }
                     break;
 
-                case 'VALUE':
-                    if ($in_param) {
-                        $params[] = $e['value'];
+                case '#text':
+                    if ($xml->hasValue) {
+                        switch ($expect) {
+                            case 'methodname':
+                                $method_name = $xml->value;
+                                break;
+
+                            case 'value':
+                                $params[] = $xml->value;
+                                break;
+                        }
                     }
                     break;
 
@@ -60,7 +69,21 @@ class XmlRpcMethod
             }
         }
 
-        return new XmlRpcMethod($method_name, $params);
+        $errors = libxml_get_errors();
+        libxml_use_internal_errors($errorflag);
+
+        if (! empty($errors)) {
+            throw new \RuntimeException(
+                'errors while parsing XML',
+                $errors[0]->code
+            );
+        }
+
+        if (! empty($method_name)) {
+            return new XmlRpcMethod($method_name, $params);
+        }
+
+        return null;
     }
 
     /**
diff --git a/src/process-request.php b/src/process-request.php
index d0af0b5..69e1e58 100644
--- a/src/process-request.php
+++ b/src/process-request.php
@@ -29,13 +29,17 @@ $data = [
 ];
 
 if (preg_match('/xmlrpc\.php/i', $data['REQUEST_URI']) && $data['REQUEST_METHOD'] === 'POST') {
-    $method = XmlRpcMethod::parse($data['BODY']);
-    if ($method->name === 'wp.getUsersBlogs') {
-        save_credentials($data['REQUEST_TIME'], $data['REMOTE_ADDR'], $method->params[0], $method->params[1]);
-        error_log("Trapped XML-RPC request: saved credentials");
-
-        header("HTTP/1.1 404 Not Found");
-        die();
+    try {
+        $method = XmlRpcMethod::parse($data['BODY']);
+        if ($method && $method->name === 'wp.getUsersBlogs') {
+            save_credentials($data['REQUEST_TIME'], $data['REMOTE_ADDR'], $method->params[0], $method->params[1]);
+            error_log("Trapped XML-RPC request: saved credentials");
+
+            header("HTTP/1.1 404 Not Found");
+            die();
+        }
+    } catch (RuntimeException $e) {
+        error_log("Webtrap: {$e->getMessage()} ({$e->getCode()}), saving request instead.");
     }
 }
 
diff --git a/tests/unit/XmlRpcTest.php b/tests/unit/XmlRpcTest.php
index c245917..25dc186 100644
--- a/tests/unit/XmlRpcTest.php
+++ b/tests/unit/XmlRpcTest.php
@@ -23,4 +23,27 @@ class XmlRpcTest extends TestCase
         $this->assertEquals('wp.getUsersBlogs', $method->name);
         $this->assertEquals(['someuser', 'verysecretpassword'], $method->params);
     }
+
+    public function testShouldNotExpandEntities(): void
+    {
+        $payload = <<<'XML'
+            <!DOCTYPE foo [ <!ENTITY xxx "expanded entity"> ]>
+            <methodCall>
+                <methodName>&xxx;</methodName>
+            </methodCall>
+            XML;
+
+        $method = XmlRpcMethod::parse($payload);
+
+        $this->assertNull($method);
+    }
+
+    public function testInvalidXMLShouldThrowRuntimeException(): void
+    {
+        $payload = '<someTag>some content</otherTag>';
+
+        $this->expectException(\RuntimeException::class);
+
+        XmlRpcMethod::parse($payload);
+    }
 }