Skip to content

Commit 0e6238c

Browse files
author
Mark Baker
authored
* Detect doubly-encoded xml to hide XXE attacks Correct use of LibXml_Disable_Entity_Loader * New test for double-encoded xml in security scanner
1 parent 1e71154 commit 0e6238c

File tree

6 files changed

+134
-49
lines changed

6 files changed

+134
-49
lines changed

‎CHANGELOG.md‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org).
77

88
## [Unreleased]
99

10+
11+
## [1.8.0] - 2019-07-01
12+
13+
### Security Fix (CVE-2019-12331)
14+
15+
- Detect double-encoded xml in the Security scanner, and reject as suspicious.
16+
- This change also broadens the scope of the `libxml_disable_entity_loader` setting when reading XML-based formats, so that it is enabled while the xml is being parsed and not simply while it is loaded.
17+
On some versions of PHP, this can cause problems because it is not thread-safe, and can affect other PHP scripts running on the same server. This flag is set to true when instantiating a loader, and back to its original setting when the Reader is no longer in scope, or manually unset.
18+
- Provide a check to identify whether libxml_disable_entity_loader is thread-safe or not.
19+
20+
`XmlScanner::threadSafeLibxmlDisableEntityLoaderAvailability()`
21+
- Provide an option to disable the libxml_disable_entity_loader call through settings. This is not recommended as it reduces the security of the XML-based readers, and should only be used if you understand the consequences and have no other choice.
22+
1023
### Added
1124

1225
- Added support for the SWITCH function - [Issue #963](https://github.com/PHPOffice/PhpSpreadsheet/issues/963) and [PR #983](https://github.com/PHPOffice/PhpSpreadsheet/pull/983)

‎src/PhpSpreadsheet/Reader/BaseReader.php‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,21 @@ abstract class BaseReader implements IReader
5858
public function __construct()
5959
{
6060
$this->readFilter = new DefaultReadFilter();
61+
62+
// A fatal error will bypass the destructor, so we register a shutdown here
63+
register_shutdown_function([$this, '__destruct']);
64+
}
65+
66+
private function shutdown()
67+
{
68+
if ($this->securityScanner !== null) {
69+
$this->securityScanner = null;
70+
}
71+
}
72+
73+
public function __destruct()
74+
{
75+
$this->shutdown();
6176
}
6277

6378
public function getReadDataOnly()

‎src/PhpSpreadsheet/Reader/Security/XmlScanner.php‎

Lines changed: 59 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
namespace PhpOffice\PhpSpreadsheet\Reader\Security;
44

55
use PhpOffice\PhpSpreadsheet\Reader;
6+
use PhpOffice\PhpSpreadsheet\Settings;
67

78
class XmlScanner
89
{
@@ -22,10 +23,16 @@ class XmlScanner
2223

2324
private $callback;
2425

25-
private function __construct($pattern = '<!DOCTYPE')
26+
private static $libxmlDisableEntityLoaderValue;
27+
28+
public function __construct($pattern = '<!DOCTYPE')
2629
{
2730
$this->pattern = $pattern;
28-
$this->libxmlDisableEntityLoader = $this->identifyLibxmlDisableEntityLoaderAvailability();
31+
32+
$this->disableEntityLoaderCheck();
33+
34+
// A fatal error will bypass the destructor, so we register a shutdown here
35+
register_shutdown_function([$this, '__destruct']);
2936
}
3037

3138
public static function getInstance(Reader\IReader $reader)
@@ -43,7 +50,7 @@ public static function getInstance(Reader\IReader $reader)
4350
}
4451
}
4552

46-
private function identifyLibxmlDisableEntityLoaderAvailability()
53+
public static function threadSafeLibxmlDisableEntityLoaderAvailability()
4754
{
4855
if (PHP_MAJOR_VERSION == 7) {
4956
switch (PHP_MINOR_VERSION) {
@@ -61,11 +68,53 @@ private function identifyLibxmlDisableEntityLoaderAvailability()
6168
return false;
6269
}
6370

71+
private function disableEntityLoaderCheck()
72+
{
73+
if (Settings::getLibXmlDisableEntityLoader()) {
74+
$libxmlDisableEntityLoaderValue = libxml_disable_entity_loader(true);
75+
76+
if (self::$libxmlDisableEntityLoaderValue === null) {
77+
self::$libxmlDisableEntityLoaderValue = $libxmlDisableEntityLoaderValue;
78+
}
79+
}
80+
}
81+
82+
private function shutdown()
83+
{
84+
if (self::$libxmlDisableEntityLoaderValue !== null) {
85+
libxml_disable_entity_loader(self::$libxmlDisableEntityLoaderValue);
86+
}
87+
}
88+
89+
public function __destruct()
90+
{
91+
$this->shutdown();
92+
}
93+
6494
public function setAdditionalCallback(callable $callback)
6595
{
6696
$this->callback = $callback;
6797
}
6898

99+
private function toUtf8($xml)
100+
{
101+
$pattern = '/encoding="(.*?)"/';
102+
$result = preg_match($pattern, $xml, $matches);
103+
$charset = $result ? $matches[1] : 'UTF-8';
104+
105+
if ($charset !== 'UTF-8') {
106+
$xml = mb_convert_encoding($xml, 'UTF-8', $charset);
107+
108+
$result = preg_match($pattern, $xml, $matches);
109+
$charset = $result ? $matches[1] : 'UTF-8';
110+
if ($charset !== 'UTF-8') {
111+
throw new Reader\Exception('Suspicious Double-encoded XML, spreadsheet file load() aborted to prevent XXE/XEE attacks');
112+
}
113+
}
114+
115+
return $xml;
116+
}
117+
69118
/**
70119
* Scan the XML for use of <!ENTITY to prevent XXE/XEE attacks.
71120
*
@@ -77,33 +126,19 @@ public function setAdditionalCallback(callable $callback)
77126
*/
78127
public function scan($xml)
79128
{
80-
if ($this->libxmlDisableEntityLoader) {
81-
$previousLibxmlDisableEntityLoaderValue = libxml_disable_entity_loader(true);
82-
}
129+
$this->disableEntityLoaderCheck();
83130

84-
$pattern = '/encoding="(.*?)"/';
85-
$result = preg_match($pattern, $xml, $matches);
86-
$charset = $result ? $matches[1] : 'UTF-8';
87-
88-
if ($charset !== 'UTF-8') {
89-
$xml = mb_convert_encoding($xml, 'UTF-8', $charset);
90-
}
131+
$xml = $this->toUtf8($xml);
91132

92133
// Don't rely purely on libxml_disable_entity_loader()
93134
$pattern = '/\\0?' . implode('\\0?', str_split($this->pattern)) . '\\0?/';
94135

95-
try {
96-
if (preg_match($pattern, $xml)) {
97-
throw new Reader\Exception('Detected use of ENTITY in XML, spreadsheet file load() aborted to prevent XXE/XEE attacks');
98-
}
136+
if (preg_match($pattern, $xml)) {
137+
throw new Reader\Exception('Detected use of ENTITY in XML, spreadsheet file load() aborted to prevent XXE/XEE attacks');
138+
}
99139

100-
if ($this->callback !== null && is_callable($this->callback)) {
101-
$xml = call_user_func($this->callback, $xml);
102-
}
103-
} finally {
104-
if (isset($previousLibxmlDisableEntityLoaderValue)) {
105-
libxml_disable_entity_loader($previousLibxmlDisableEntityLoaderValue);
106-
}
140+
if ($this->callback !== null && is_callable($this->callback)) {
141+
$xml = call_user_func($this->callback, $xml);
107142
}
108143

109144
return $xml;

‎src/PhpSpreadsheet/Settings.php‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,20 @@ class Settings
2424
*/
2525
private static $libXmlLoaderOptions = null;
2626

27+
/**
28+
* Allow/disallow libxml_disable_entity_loader() call when not thread safe.
29+
* Default behaviour is to do the check, but if you're running PHP versions
30+
* 7.2 < 7.2.1
31+
* 7.1 < 7.1.13
32+
* 7.0 < 7.0.27
33+
* 5.6 ANY
34+
* then you may need to disable this check to prevent unwanted behaviour in other threads
35+
* SECURITY WARNING: Changing this flag is not recommended.
36+
*
37+
* @var bool
38+
*/
39+
private static $libXmlDisableEntityLoader = true;
40+
2741
/**
2842
* The cache implementation to be used for cell collection.
2943
*
@@ -101,6 +115,34 @@ public static function getLibXmlLoaderOptions()
101115
return self::$libXmlLoaderOptions;
102116
}
103117

118+
/**
119+
* Enable/Disable the entity loader for libxml loader.
120+
* Allow/disallow libxml_disable_entity_loader() call when not thread safe.
121+
* Default behaviour is to do the check, but if you're running PHP versions
122+
* 7.2 < 7.2.1
123+
* 7.1 < 7.1.13
124+
* 7.0 < 7.0.27
125+
* 5.6 ANY
126+
* then you may need to disable this check to prevent unwanted behaviour in other threads
127+
* SECURITY WARNING: Changing this flag to false is not recommended.
128+
*
129+
* @param bool $state
130+
*/
131+
public static function setLibXmlDisableEntityLoader($state)
132+
{
133+
self::$libXmlDisableEntityLoader = (bool) $state;
134+
}
135+
136+
/**
137+
* Return the state of the entity loader (disabled/enabled) for libxml loader.
138+
*
139+
* @return bool $state
140+
*/
141+
public static function getLibXmlDisableEntityLoader()
142+
{
143+
return self::$libXmlDisableEntityLoader;
144+
}
145+
104146
/**
105147
* Sets the implementation of cache that should be used for cell collection.
106148
*

‎tests/PhpSpreadsheetTests/Reader/Security/XmlScannerTest.php‎

Lines changed: 3 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
use PhpOffice\PhpSpreadsheet\Reader\Security\XmlScanner;
66
use PhpOffice\PhpSpreadsheet\Reader\Xls;
77
use PhpOffice\PhpSpreadsheet\Reader\Xlsx;
8-
use PhpOffice\PhpSpreadsheet\Reader\Xml;
98
use PHPUnit\Framework\TestCase;
109

1110
class XmlScannerTest extends TestCase
@@ -19,12 +18,13 @@ class XmlScannerTest extends TestCase
1918
*/
2019
public function testValidXML($filename, $expectedResult, $libxmlDisableEntityLoader)
2120
{
22-
libxml_disable_entity_loader($libxmlDisableEntityLoader);
21+
$oldDisableEntityLoaderState = libxml_disable_entity_loader($libxmlDisableEntityLoader);
2322

2423
$reader = XmlScanner::getInstance(new \PhpOffice\PhpSpreadsheet\Reader\Xml());
2524
$result = $reader->scanFile($filename);
2625
self::assertEquals($expectedResult, $result);
27-
self::assertEquals($libxmlDisableEntityLoader, libxml_disable_entity_loader());
26+
27+
libxml_disable_entity_loader($oldDisableEntityLoaderState);
2828
}
2929

3030
public function providerValidXML()
@@ -115,26 +115,4 @@ public function providerValidXMLForCallback()
115115

116116
return $tests;
117117
}
118-
119-
/**
120-
* @dataProvider providerLibxmlSettings
121-
*
122-
* @param $libxmlDisableLoader
123-
*/
124-
public function testNewInstanceCreationDoesntChangeLibxmlSettings($libxmlDisableLoader)
125-
{
126-
libxml_disable_entity_loader($libxmlDisableLoader);
127-
128-
$reader = new Xml();
129-
self::assertEquals($libxmlDisableLoader, libxml_disable_entity_loader($libxmlDisableLoader));
130-
unset($reader);
131-
}
132-
133-
public function providerLibxmlSettings()
134-
{
135-
return [
136-
[true],
137-
[false],
138-
];
139-
}
140118
}

‎tests/data/Reader/Xml/XEETestInvalidUTF-7_DoubleEncoded.xml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
<?xml version="1.0" encoding="UTF-7"?>
2+
+-ADwAIQ-DOCTYPE xmlrootname +-AFsAPAAh-ENTITY +-ACU aaa SYSTEM +-ACI-http://127.0.0.1:8080/ext.dtd+-ACIAPgAl-aaa+-ADsAJQ-ccc+-ADsAJQ-ddd+-ADsAXQA+-

0 commit comments

Comments
 (0)