Implemented XXE safe xml parser

HarmonyIO · Nov 30, 2018 · 014a258 · 014a258
1 parent 02ec1ba
commit 014a258
Show file tree

Hide file tree

Showing 9 changed files with 111 additions and 14 deletions.
diff --git a/src/Exception/InvalidXml.php b/src/Exception/InvalidXml.php
@@ -0,0 +1,7 @@
+<?php declare(strict_types=1);
+
+namespace HarmonyIO\Validation\Exception;
+
+class InvalidXml extends Exception
+{
+}
diff --git a/src/Rule/File/Image/Type/Svg/ValidAttributes.php b/src/Rule/File/Image/Type/Svg/ValidAttributes.php
@@ -5,9 +5,11 @@
 use Amp\Promise;
 use Amp\Success;
 use HarmonyIO\Validation\Enum\File\Image\Svg\Attribute;
+use HarmonyIO\Validation\Exception\InvalidXml;
 use HarmonyIO\Validation\Rule\File\MimeType;
 use HarmonyIO\Validation\Rule\FileSystem\File;
 use HarmonyIO\Validation\Rule\Rule;
+use HarmonyIO\Validation\Xml\SafeParser;
 use function Amp\call;
 use function Amp\ParallelFunctions\parallel;
 
@@ -41,21 +43,20 @@ public function validate($value): Promise
 
             return parallel(function () use ($value) {
                 // @codeCoverageIgnoreStart
-                $useInternalErrors = libxml_use_internal_errors(true);
-
-                $domDocument = new \DOMDocument();
-                $domDocument->load($value);
+                try {
+                    $xmlParser = new SafeParser(file_get_contents($value));
+                } catch (InvalidXml $e) {
+                    return false;
+                }
 
-                foreach ($domDocument->getElementsByTagName('*') as $node) {
+                foreach ($xmlParser->getElementsByTagName('*') as $node) {
                     foreach ($node->attributes as $attribute) {
                         if (!$this->attribute->exists($attribute->nodeName)) {
                             return false;
                         }
                     }
                 }
 
-                libxml_use_internal_errors($useInternalErrors);
-
                 return true;
                 // @codeCoverageIgnoreEnd
             })();

diff --git a/src/Rule/File/Image/Type/Svg/ValidElements.php b/src/Rule/File/Image/Type/Svg/ValidElements.php
@@ -5,9 +5,11 @@
 use Amp\Promise;
 use Amp\Success;
 use HarmonyIO\Validation\Enum\File\Image\Svg\Element;
+use HarmonyIO\Validation\Exception\InvalidXml;
 use HarmonyIO\Validation\Rule\File\MimeType;
 use HarmonyIO\Validation\Rule\FileSystem\File;
 use HarmonyIO\Validation\Rule\Rule;
+use HarmonyIO\Validation\Xml\SafeParser;
 use function Amp\call;
 use function Amp\ParallelFunctions\parallel;
 
@@ -41,20 +43,19 @@ public function validate($value): Promise
 
             return parallel(function () use ($value) {
                 // @codeCoverageIgnoreStart
-                $useInternalErrors = libxml_use_internal_errors(true);
-
-                $domDocument = new \DOMDocument();
-                $domDocument->load($value);
+                try {
+                    $xmlParser = new SafeParser(file_get_contents($value));
+                } catch (InvalidXml $e) {
+                    return false;
+                }
 
                 /** @var \DOMElement $node */
-                foreach ($domDocument->getElementsByTagName('*') as $node) {
+                foreach ($xmlParser->getElementsByTagName('*') as $node) {
                     if (!$this->element->exists($node->nodeName)) {
                         return false;
                     }
                 }
 
-                libxml_use_internal_errors($useInternalErrors);
-
                 return true;
                 // @codeCoverageIgnoreEnd
             })();

diff --git a/src/Xml/SafeParser.php b/src/Xml/SafeParser.php
@@ -0,0 +1,38 @@
+<?php declare(strict_types=1);
+
+namespace HarmonyIO\Validation\Xml;
+
+use HarmonyIO\Validation\Exception\InvalidXml;
+
+class SafeParser
+{
+    /** @var \DOMDocument */
+    private $domDocument;
+
+    public function __construct(string $xml)
+    {
+        $useInternalErrors   = libxml_use_internal_errors(true);
+        $disableEntityLoader = libxml_disable_entity_loader(true);
+
+        $domDocument = new \DOMDocument();
+        $domDocument->loadXML($xml);
+
+        $libXmlErrors = libxml_get_errors();
+
+        libxml_clear_errors();
+
+        libxml_use_internal_errors($useInternalErrors);
+        libxml_disable_entity_loader($disableEntityLoader);
+
+        if ($libXmlErrors) {
+            throw new InvalidXml($libXmlErrors[0]->message, $libXmlErrors[0]->code);
+        }
+
+        $this->domDocument = $domDocument;
+    }
+
+    public function getElementsByTagName(string $tagName): \DOMNodeList
+    {
+        return $this->domDocument->getElementsByTagName($tagName);
+    }
+}
diff --git a/tests/Data/image/broken.svg b/tests/Data/image/broken.svg
diff --git a/tests/Unit/Rule/File/Image/Type/Svg/ValidAttributesTest.php b/tests/Unit/Rule/File/Image/Type/Svg/ValidAttributesTest.php
@@ -79,6 +79,11 @@ public function testValidateReturnsFalseWhenNotMatchingMimeType(): void
         $this->assertFalse((new ValidAttributes())->validate(TEST_DATA_DIR . '/image/mspaint.gif'));
     }
 
+    public function testValidateReturnsFalseWhenImageContainsBrokenXml(): void
+    {
+        $this->assertFalse((new ValidAttributes())->validate(TEST_DATA_DIR . '/image/broken.svg'));
+    }
+
     public function testValidateReturnsFalseWhenImageContainsInvalidElements(): void
     {
         $this->assertFalse((new ValidAttributes())->validate(TEST_DATA_DIR . '/image/invalid-attributes.svg'));

diff --git a/tests/Unit/Rule/File/Image/Type/Svg/ValidElementsTest.php b/tests/Unit/Rule/File/Image/Type/Svg/ValidElementsTest.php
@@ -79,6 +79,11 @@ public function testValidateReturnsFalseWhenNotMatchingMimeType(): void
         $this->assertFalse((new ValidElements())->validate(TEST_DATA_DIR . '/image/mspaint.gif'));
     }
 
+    public function testValidateReturnsFalseWhenImageContainsBrokenXml(): void
+    {
+        $this->assertFalse((new ValidElements())->validate(TEST_DATA_DIR . '/image/broken.svg'));
+    }
+
     public function testValidateReturnsFalseWhenImageContainsInvalidElements(): void
     {
         $this->assertFalse((new ValidElements())->validate(TEST_DATA_DIR . '/image/invalid-elements.svg'));

diff --git a/tests/Unit/Rule/File/Image/Type/SvgTest.php b/tests/Unit/Rule/File/Image/Type/SvgTest.php
@@ -79,6 +79,11 @@ public function testValidateReturnsFalseWhenNotMatchingMimeType(): void
         $this->assertFalse((new Svg())->validate(TEST_DATA_DIR . '/image/mspaint.gif'));
     }
 
+    public function testValidateReturnsFalseWhenImageContainsBrokenXml(): void
+    {
+        $this->assertFalse((new Svg())->validate(TEST_DATA_DIR . '/image/broken.svg'));
+    }
+
     public function testValidateReturnsFalseWhenImageContainsInvalidElements(): void
     {
         $this->assertFalse((new Svg())->validate(TEST_DATA_DIR . '/image/invalid-elements.svg'));

diff --git a/tests/Unit/Xml/SafeParserTest.php b/tests/Unit/Xml/SafeParserTest.php
@@ -0,0 +1,31 @@
+<?php declare(strict_types=1);
+
+namespace HarmonyIO\ValidationTest\Unit\Xml;
+
+use HarmonyIO\PHPUnitExtension\TestCase;
+use HarmonyIO\Validation\Exception\InvalidXml;
+use HarmonyIO\Validation\Xml\SafeParser;
+
+class SafeParserTest extends TestCase
+{
+    public function testConstructorThrowsOnBrokenXml(): void
+    {
+        $this->expectException(InvalidXml::class);
+
+        new SafeParser(file_get_contents(TEST_DATA_DIR . '/image/broken.svg'));
+    }
+
+    public function testGetElementsByTagNameReturnsNodeList(): void
+    {
+        $xmlParser = new SafeParser(file_get_contents(TEST_DATA_DIR . '/image/example.svg'));
+
+        $this->assertInstanceOf(\DOMNodeList::class, $xmlParser->getElementsByTagName('*'));
+    }
+
+    public function testGetElementsByTagNameReturnsNodes(): void
+    {
+        $xmlParser = new SafeParser(file_get_contents(TEST_DATA_DIR . '/image/example.svg'));
+
+        $this->assertSame(3, $xmlParser->getElementsByTagName('*')->count());
+    }
+}