From 8918888e7c440dea9a0fa021b698ec645294eaf3 Mon Sep 17 00:00:00 2001 From: Philipp Kolesnikov Date: Mon, 17 Dec 2018 13:52:04 +1000 Subject: [PATCH] libxml_disable_entity_loader() changes global state so it should be used as local as possible Fixes #801 Closes #802 Closes #803 --- CHANGELOG.md | 1 + .../Reader/Security/XmlScanner.php | 39 +++++++--------- .../Reader/Security/XmlScannerTest.php | 44 +++++++++++++++++-- 3 files changed, 57 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8349b481..3e82dce4 100644 --- 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 index 44324c7c..a8025063 100644 --- 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 @@ 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,12 +91,19 @@ class XmlScanner // 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 index a456f561..ea9aff0b 100644 --- a/tests/PhpSpreadsheetTests/Reader/Security/XmlScannerTest.php +++ b/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], + ]; + } }