From 3a008f5334580732a5c99f2108729011c6093828 Mon Sep 17 00:00:00 2001 From: Philipp Kolesnikov Date: Mon, 17 Dec 2018 13:52:04 +1000 Subject: [PATCH 1/2] libxml_disable_entity_loader() changes global state so it should be used as local as possible --- .../Reader/Security/XmlScanner.php | 38 +++++++--------- .../Reader/Security/XmlScannerTest.php | 44 +++++++++++++++++-- 2 files changed, 55 insertions(+), 27 deletions(-) diff --git a/src/PhpSpreadsheet/Reader/Security/XmlScanner.php b/src/PhpSpreadsheet/Reader/Security/XmlScanner.php index 44324c7ce1..363837606e 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 @@ private function __construct($pattern = '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,18 @@ 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'); - } + 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); + 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 a456f561d0..ad8a431934 100644 --- 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] + ]; + } } From b60f02be72e036d0ca8d17c249c3885b9f502673 Mon Sep 17 00:00:00 2001 From: Philipp Kolesnikov Date: Mon, 17 Dec 2018 14:36:19 +1000 Subject: [PATCH 2/2] #819 fix CS errors --- src/PhpSpreadsheet/Reader/Security/XmlScanner.php | 1 + tests/PhpSpreadsheetTests/Reader/Security/XmlScannerTest.php | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/PhpSpreadsheet/Reader/Security/XmlScanner.php b/src/PhpSpreadsheet/Reader/Security/XmlScanner.php index 363837606e..a8025063f9 100644 --- a/src/PhpSpreadsheet/Reader/Security/XmlScanner.php +++ b/src/PhpSpreadsheet/Reader/Security/XmlScanner.php @@ -91,6 +91,7 @@ public function scan($xml) // 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'); diff --git a/tests/PhpSpreadsheetTests/Reader/Security/XmlScannerTest.php b/tests/PhpSpreadsheetTests/Reader/Security/XmlScannerTest.php index ad8a431934..ea9aff0b4d 100644 --- a/tests/PhpSpreadsheetTests/Reader/Security/XmlScannerTest.php +++ b/tests/PhpSpreadsheetTests/Reader/Security/XmlScannerTest.php @@ -134,7 +134,7 @@ public function providerLibxmlSettings() { return [ [true], - [false] + [false], ]; } }