libxml_disable_entity_loader() changes global state so it should be used as local as possible

Fixes #801
Closes #802
Closes #803

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
 

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 @@ class XmlScanner
     {
         $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 @@ class XmlScanner
      */
     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,6 +91,8 @@ class XmlScanner
 
         // Don't rely purely on libxml_disable_entity_loader()
         $pattern = '/\\0?' . implode('\\0?', str_split($this->pattern)) . '\\0?/';
+
+        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');
             }
@@ -112,6 +100,11 @@ class XmlScanner
             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;
     }

tests/PhpSpreadsheetTests/Reader/Security/XmlScannerTest.php

@@ -5,6 +5,7 @@ namespace PhpOffice\PhpSpreadsheetTests\Reader\Security;
 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 @@ class XmlScannerTest extends TestCase
      * @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 @@ class XmlScannerTest extends TestCase
 
         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],
+        ];
+    }
 }