libxml_disable_entity_loader() changes global state so it should be u…

…sed as local as possible Fixes #801 Closes #802 Closes #803
PHPOffice · Jan 1, 2019 · 8918888 · 8918888
1 parent 6a48b50
commit 8918888
Show file tree

Hide file tree

Showing 3 changed files with 57 additions and 27 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org).
 - Fix LOOKUP function which was breaking on edge cases - [#796](https://github.com/PHPOffice/PhpSpreadsheet/issues/796)
 - Fix VLOOKUP with exact matches - [#809](https://github.com/PHPOffice/PhpSpreadsheet/pull/809)
 - Support COUNTIFS multiple arguments - [#830](https://github.com/PHPOffice/PhpSpreadsheet/pull/830)
+- Change `libxml_disable_entity_loader()` as shortly as possible - [#819](https://github.com/PHPOffice/PhpSpreadsheet/pull/819)
 
 ## [1.5.2] - 2018-11-25
 

diff --git a/src/PhpSpreadsheet/Reader/Security/XmlScanner.php b/src/PhpSpreadsheet/Reader/Security/XmlScanner.php
@@ -13,13 +13,6 @@ class XmlScanner
      */
     private $libxmlDisableEntityLoader = false;
 
-    /**
-     * Store the initial setting of libxmlDisableEntityLoader so that we can resore t later.
-     *
-     * @var bool
-     */
-    private $previousLibxmlDisableEntityLoaderValue;
-
     /**
      * String used to identify risky xml elements.
      *
@@ -33,17 +26,6 @@ private function __construct($pattern = '<!DOCTYPE')
     {
         $this->pattern = $pattern;
         $this->libxmlDisableEntityLoader = $this->identifyLibxmlDisableEntityLoaderAvailability();
-
-        if ($this->libxmlDisableEntityLoader) {
-            $this->previousLibxmlDisableEntityLoaderValue = libxml_disable_entity_loader(true);
-        }
-    }
-
-    public function __destruct()
-    {
-        if ($this->libxmlDisableEntityLoader) {
-            libxml_disable_entity_loader($this->previousLibxmlDisableEntityLoaderValue);
-        }
     }
 
     public static function getInstance(Reader\IReader $reader)
@@ -95,6 +77,10 @@ public function setAdditionalCallback(callable $callback)
      */
     public function scan($xml)
     {
+        if ($this->libxmlDisableEntityLoader) {
+            $previousLibxmlDisableEntityLoaderValue = libxml_disable_entity_loader(true);
+        }
+
         $pattern = '/encoding="(.*?)"/';
         $result = preg_match($pattern, $xml, $matches);
         $charset = $result ? $matches[1] : 'UTF-8';
@@ -105,12 +91,19 @@ public function scan($xml)
 
         // Don't rely purely on libxml_disable_entity_loader()
         $pattern = '/\\0?' . implode('\\0?', str_split($this->pattern)) . '\\0?/';
-        if (preg_match($pattern, $xml)) {
-            throw new Reader\Exception('Detected use of ENTITY in XML, spreadsheet file load() aborted to prevent XXE/XEE attacks');
-        }
 
-        if ($this->callback !== null && is_callable($this->callback)) {
-            $xml = call_user_func($this->callback, $xml);
+        try {
+            if (preg_match($pattern, $xml)) {
+                throw new Reader\Exception('Detected use of ENTITY in XML, spreadsheet file load() aborted to prevent XXE/XEE attacks');
+            }
+
+            if ($this->callback !== null && is_callable($this->callback)) {
+                $xml = call_user_func($this->callback, $xml);
+            }
+        } finally {
+            if ($this->libxmlDisableEntityLoader) {
+                libxml_disable_entity_loader($previousLibxmlDisableEntityLoaderValue);
+            }
         }
 
         return $xml;

diff --git a/tests/PhpSpreadsheetTests/Reader/Security/XmlScannerTest.php b/tests/PhpSpreadsheetTests/Reader/Security/XmlScannerTest.php
@@ -5,6 +5,7 @@
 use PhpOffice\PhpSpreadsheet\Reader\Security\XmlScanner;
 use PhpOffice\PhpSpreadsheet\Reader\Xls;
 use PhpOffice\PhpSpreadsheet\Reader\Xlsx;
+use PhpOffice\PhpSpreadsheet\Reader\Xml;
 use PHPUnit\Framework\TestCase;
 
 class XmlScannerTest extends TestCase
@@ -14,19 +15,26 @@ class XmlScannerTest extends TestCase
      *
      * @param mixed $filename
      * @param mixed $expectedResult
+     * @param $libxmlDisableEntityLoader
      */
-    public function testValidXML($filename, $expectedResult)
+    public function testValidXML($filename, $expectedResult, $libxmlDisableEntityLoader)
     {
+        libxml_disable_entity_loader($libxmlDisableEntityLoader);
+
         $reader = XmlScanner::getInstance(new \PhpOffice\PhpSpreadsheet\Reader\Xml());
         $result = $reader->scanFile($filename);
         self::assertEquals($expectedResult, $result);
+        self::assertEquals($libxmlDisableEntityLoader, libxml_disable_entity_loader());
     }
 
     public function providerValidXML()
     {
         $tests = [];
         foreach (glob(__DIR__ . '/../../../data/Reader/Xml/XEETestValid*.xml') as $file) {
-            $tests[basename($file)] = [realpath($file), file_get_contents($file)];
+            $filename = realpath($file);
+            $expectedResult = file_get_contents($file);
+            $tests[basename($file) . '_libxml_entity_loader_disabled'] = [$filename, $expectedResult, true];
+            $tests[basename($file) . '_libxml_entity_loader_enabled'] = [$filename, $expectedResult, false];
         }
 
         return $tests;
@@ -36,22 +44,28 @@ public function providerValidXML()
      * @dataProvider providerInvalidXML
      *
      * @param mixed $filename
+     * @param $libxmlDisableEntityLoader
      */
-    public function testInvalidXML($filename)
+    public function testInvalidXML($filename, $libxmlDisableEntityLoader)
     {
         $this->expectException(\PhpOffice\PhpSpreadsheet\Reader\Exception::class);
 
+        libxml_disable_entity_loader($libxmlDisableEntityLoader);
+
         $reader = XmlScanner::getInstance(new \PhpOffice\PhpSpreadsheet\Reader\Xml());
         $expectedResult = 'FAILURE: Should throw an Exception rather than return a value';
         $result = $reader->scanFile($filename);
         self::assertEquals($expectedResult, $result);
+        self::assertEquals($libxmlDisableEntityLoader, libxml_disable_entity_loader());
     }
 
     public function providerInvalidXML()
     {
         $tests = [];
         foreach (glob(__DIR__ . '/../../../data/Reader/Xml/XEETestInvalidUTF*.xml') as $file) {
-            $tests[basename($file)] = [realpath($file)];
+            $filename = realpath($file);
+            $tests[basename($file) . '_libxml_entity_loader_disabled'] = [$filename, true];
+            $tests[basename($file) . '_libxml_entity_loader_enabled'] = [$filename, false];
         }
 
         return $tests;
@@ -101,4 +115,26 @@ public function providerValidXMLForCallback()
 
         return $tests;
     }
+
+    /**
+     * @dataProvider providerLibxmlSettings
+     *
+     * @param $libxmDisableLoader
+     */
+    public function testNewInstanceCreationDoesntChangeLibxmlSettings($libxmDisableLoader)
+    {
+        libxml_disable_entity_loader($libxmDisableLoader);
+
+        $reader = new Xml();
+
+        self::assertEquals($libxmDisableLoader, libxml_disable_entity_loader($libxmDisableLoader));
+    }
+
+    public function providerLibxmlSettings()
+    {
+        return [
+            [true],
+            [false],
+        ];
+    }
 }