Skip to content

Commit 91ac6c9

Browse files
SeigerSeiger
authored
Merge pull request #2449 from elcreator/fix/unserialize-object-injection
fix(core): block object injection via unserialize()
2 parents 4987c88 + 79b2cc2 commit 91ac6c9

3 files changed

Lines changed: 162 additions & 2 deletions

File tree

‎core/src/Core.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -891,7 +891,7 @@ public function getDocumentObjectFromCache($id, $loading = false)
891891
$result = $a[0];
892892
} // return only document content
893893
else {
894-
$docObj = unserialize($a[0]); // rebuild document object
894+
$docObj = unserialize($a[0], ['allowed_classes' => false]); // rebuild document object; arrays only, no POP chains
895895
// check page security
896896
if ($this->isFrontend() && $docObj['privateweb'] && isset($docObj['__MODxDocGroups__'])) {
897897
$pass = false;

‎core/src/Legacy/ManagerApi.php‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -299,7 +299,10 @@ public function getModifiedSystemFilesList($check_files, $checksum)
299299
$_ = [];
300300
$check_files = trim($check_files);
301301
$check_files = explode("\n", $check_files);
302-
$checksum = unserialize($checksum);
302+
$checksum = unserialize($checksum, ['allowed_classes' => false]);
303+
if (!is_array($checksum)) {
304+
$checksum = [];
305+
}
303306
foreach ($check_files as $file) {
304307
$file = trim($file);
305308
$filePath = EVO_BASE_PATH . $file;

‎core/tests/Unit/Security/UnserializeAllowedClassesTest.php‎

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
<?php
2+
3+
/*
4+
|--------------------------------------------------------------------------
5+
| unserialize() object-injection hardening
6+
|--------------------------------------------------------------------------
7+
|
8+
| Two entry points rebuilt PHP objects out of storage an attacker can reach without any code
9+
| privilege:
10+
|
11+
| - Core::getDocumentObjectFromCache() assets/cache/*.pageCache.php
12+
| - ManagerApi::getModifiedSystemFilesList() system_settings.sys_files_checksum
13+
|
14+
| Both only ever store arrays, so a single write into the cache directory - or into the settings
15+
| table through some other flaw - was enough to run a vendor POP chain on the next request. Passing
16+
| allowed_classes => false turns any object in the payload into __PHP_Incomplete_Class, which has no
17+
| magic methods to trigger.
18+
|
19+
| @since 3.5.8
20+
*/
21+
22+
use EvolutionCMS\Core;
23+
use EvolutionCMS\Legacy\ManagerApi;
24+
25+
beforeAll(function () {
26+
if (!defined('IN_INSTALL_MODE')) {
27+
define('IN_INSTALL_MODE', false);
28+
}
29+
if (!defined('EVO_API_MODE')) {
30+
define('EVO_API_MODE', true);
31+
}
32+
if (!defined('IN_MANAGER_MODE')) {
33+
define('IN_MANAGER_MODE', false);
34+
}
35+
$root = str_replace(DIRECTORY_SEPARATOR, '/', dirname(__DIR__, 3)) . '/';
36+
if (!defined('EVO_BASE_PATH')) {
37+
define('EVO_BASE_PATH', $root);
38+
}
39+
if (!defined('EVO_CORE_PATH')) {
40+
define('EVO_CORE_PATH', $root . 'core/');
41+
}
42+
if (!defined('EVO_MANAGER_PATH')) {
43+
define('EVO_MANAGER_PATH', $root . 'manager/');
44+
}
45+
$autoload = EVO_CORE_PATH . 'vendor/autoload.php';
46+
if (file_exists($autoload)) {
47+
require_once $autoload;
48+
}
49+
});
50+
51+
/**
52+
* Stands in for a vendor gadget: any wakeup on this class means the payload was instantiated.
53+
*/
54+
class UnserializeGadget
55+
{
56+
public $payload = 'x';
57+
58+
public function __wakeup()
59+
{
60+
$GLOBALS['evo_unserialize_gadget_fired'] = true;
61+
}
62+
}
63+
64+
/**
65+
* A Core built without the heavy constructor, with the cache path and the event dispatcher
66+
* redirected so the read path can run outside a bootstrapped application.
67+
*/
68+
class UnserializeCacheCore extends Core
69+
{
70+
public $hashFile = '';
71+
72+
public function getConfig($name = '', $default = null)
73+
{
74+
return $default;
75+
}
76+
77+
public function getHashFile($key)
78+
{
79+
return $this->hashFile;
80+
}
81+
82+
public function isFrontend()
83+
{
84+
return false;
85+
}
86+
87+
public function invokeEvent($evtName, $extParams = [])
88+
{
89+
return [];
90+
}
91+
}
92+
93+
function unserializeCacheCore(string $hashFile): UnserializeCacheCore
94+
{
95+
$core = (new ReflectionClass(UnserializeCacheCore::class))->newInstanceWithoutConstructor();
96+
$core->hashFile = $hashFile;
97+
98+
return $core;
99+
}
100+
101+
describe('page cache', function () {
102+
103+
test('an object in the cache file is not instantiated', function () {
104+
$GLOBALS['evo_unserialize_gadget_fired'] = false;
105+
106+
$file = str_replace(DIRECTORY_SEPARATOR, '/', sys_get_temp_dir()) . '/evo_cache_' . bin2hex(random_bytes(6)) . '.php';
107+
$docObj = ['privateweb' => 0, 'gadget' => new UnserializeGadget()];
108+
file_put_contents($file, '<?php die();?>' . serialize($docObj) . '<!--__MODxCacheSpliter__-->CONTENT');
109+
110+
$result = unserializeCacheCore($file)->getDocumentObjectFromCache(1);
111+
@unlink($file);
112+
113+
expect($GLOBALS['evo_unserialize_gadget_fired'])->toBeFalse()
114+
->and($result)->toBe('CONTENT');
115+
});
116+
117+
test('the surrounding document fields still survive the round trip', function () {
118+
$file = str_replace(DIRECTORY_SEPARATOR, '/', sys_get_temp_dir()) . '/evo_cache_' . bin2hex(random_bytes(6)) . '.php';
119+
$docObj = ['privateweb' => 0, 'pagetitle' => 'Home', 'published' => 1];
120+
file_put_contents($file, '<?php die();?>' . serialize($docObj) . '<!--__MODxCacheSpliter__-->CONTENT');
121+
122+
$core = unserializeCacheCore($file);
123+
$core->getDocumentObjectFromCache(1);
124+
@unlink($file);
125+
126+
expect($core->documentObject['pagetitle'])->toBe('Home');
127+
});
128+
});
129+
130+
describe('system files checksum', function () {
131+
132+
test('an object in the stored checksum is not instantiated', function () {
133+
$GLOBALS['evo_unserialize_gadget_fired'] = false;
134+
135+
$api = new ManagerApi();
136+
$api->getModifiedSystemFilesList('composer.json', serialize(['x' => new UnserializeGadget()]));
137+
138+
expect($GLOBALS['evo_unserialize_gadget_fired'])->toBeFalse();
139+
});
140+
141+
test('a checksum that did not hold an array is treated as empty', function () {
142+
// The stored setting used to be handed straight to array_key_exists(), which fatals on
143+
// anything but an array.
144+
$api = new ManagerApi();
145+
146+
expect($api->getModifiedSystemFilesList('composer.json', serialize('corrupted')))
147+
->toBe(['composer.json']);
148+
});
149+
150+
test('a matching checksum still reports no modified files', function () {
151+
$api = new ManagerApi();
152+
$file = EVO_BASE_PATH . 'composer.json';
153+
$checksum = serialize([$file => md5_file($file)]);
154+
155+
expect($api->getModifiedSystemFilesList('composer.json', $checksum))->toBe([]);
156+
});
157+
});

0 commit comments

Comments
 (0)