Skip to content

Commit c5b2917

Browse files
mcop1claude
andauthored
[Security] Harden Serialize::unserialize() callers and deprecate the permissive default (pimcore#19293)
* [Security] Make Serialize::unserialize() safe by default Serialize::unserialize() defaulted its allowedClasses argument to true, so every single-argument caller permitted arbitrary PHP object deserialization. Flip the default to false (no object instantiation) and pass an explicit argument at every existing caller: - callers that only handle scalars/arrays now pass false; - callers that reconstruct known stored value objects pass a scoped allowlist (e.g. Geopolygon/Geopolyline -> GeoCoordinates); - callers that reconstruct open, user-defined object graphs (element versions, recycle bin, session elements, encrypted fields, and object field data) pass an explicit true, to be scoped further as a follow-up. Also restrict two raw unserialize() calls (a CoreBundle migration and the SeoBundle controller) to allowed_classes => false. Adds tests/Unit/Tool/SerializeTest.php covering the safe default, explicit allowlist, explicit true, and scalar/array round-trips. Co-Authored-By: Claude <noreply@anthropic.com> * Keep Serialize::unserialize() default as-is and deprecate omitting the argument Reverts the default flip (true -> false) to preserve backward compatibility: the wrapper is public, non-@internal API, so changing its default behaviour on a minor line would silently break external callers that rely on it to reconstruct objects. Instead, keep the default `true` and emit a deprecation when the $allowedClasses argument is omitted, so callers migrate to an explicit value. The default will be switched to `false` in Pimcore 2027.1. The per-caller hardening (explicit arguments at every core call site) and the raw-unserialize() fixes are unchanged and remain the actual security improvement. Co-Authored-By: Claude <noreply@anthropic.com> * Add upgrade note for the Serialize::unserialize() deprecation Documents the deprecation of the permissive default under the next 12.3 patch, matching the repository's upgrade-notes convention (deprecation + the 2027.1 removal target). Co-Authored-By: Claude <noreply@anthropic.com> * Emit the unserialize() deprecation once per process + cover geo datatypes in blocks Addresses review feedback: - Serialize::unserialize() emitted the "$allowedClasses omitted" deprecation on every call, which could flood the logs when unserializing in a loop (e.g. a listing of many objects). Guard it with a static flag so it fires at most once per process. The unit test now asserts a second omitting call stays silent. - Add geo child fields (geopoint, geobounds, geopolygon, geopolyline) to the test block and a BlockTest case that round-trips all four inside a block, proving Block::getDataFromResource()'s `Serialize::unserialize($data, false)` does not neutralise geo values (they are stored normalized and rebuilt via each sub-field's denormalize()). Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent d247aa6 commit c5b2917

19 files changed

Lines changed: 244 additions & 14 deletions

File tree

bundles/CoreBundle/src/Migrations/Version20230424084415.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ public function up(Schema $schema): void
3333
$editables = $db->fetchAllAssociative('SELECT * FROM documents_editables WHERE type = ?', ['link']);
3434

3535
foreach ($editables as $editable) {
36-
$unserialized = unserialize($editable['data']);
36+
$unserialized = unserialize($editable['data'], ['allowed_classes' => false]);
3737
if (is_array($unserialized) && array_key_exists('attributes', $unserialized)) {
3838
unset($unserialized['attributes']);
3939

bundles/SeoBundle/src/Controller/MiscController.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ public function httpErrorLogDetailAction(Request $request, ?Profiler $profiler):
8787

8888
foreach ($data as $key => &$value) {
8989
if ($key === 'parametersGet') {
90-
$value = unserialize($value);
90+
$value = unserialize($value, ['allowed_classes' => false]);
9191
}
9292
}
9393

doc/23_Installation_and_Upgrade/09_Upgrade_Notes/README.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,16 @@
11
# Upgrade Notes
22

3+
## Pimcore 12.3.12
4+
5+
### Deprecations
6+
7+
#### [Serialization]
8+
9+
Calling `Pimcore\Tool\Serialize::unserialize()` without the `$allowedClasses` argument is deprecated.
10+
Pass an explicit value: `false` to forbid object deserialization (the safe choice for scalar/array data),
11+
or an array of allowed class names when a trusted stored object graph must be reconstructed.
12+
The default changes from `true` to `false` (object deserialization disabled) in Pimcore 2027.1.
13+
314
## Pimcore 12.3.10
415

516
### Deprecations

lib/DataObject/ClassificationstoreDataMarshaller/QuantityValueRange.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ public function marshal(mixed $value, array $params = []): mixed
4141
public function unmarshal(mixed $value, array $params = []): mixed
4242
{
4343
if (is_array($value) && ($value['value'] !== null || $value['value2'] !== null)) {
44-
$minMaxValue = Serialize::unserialize($value['value'] ?? null);
44+
$minMaxValue = Serialize::unserialize($value['value'] ?? null, false);
4545

4646
return [
4747
'minimum' => $minMaxValue['minimum'] ?? null,

lib/DataObject/ClassificationstoreDataMarshaller/Table.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ public function marshal(mixed $value, array $params = []): mixed
3333
public function unmarshal(mixed $value, array $params = []): mixed
3434
{
3535
if (is_array($value)) {
36-
return Serialize::unserialize($value['value']);
36+
return Serialize::unserialize($value['value'], false);
3737
}
3838

3939
return null;

lib/Tool/Serialize.php

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,17 +21,47 @@ final class Serialize
2121
{
2222
protected static array $loopFilterProcessedObjects = [];
2323

24+
/**
25+
* Ensures the "missing $allowedClasses argument" deprecation is emitted at most once per
26+
* process, so callers that unserialize in a loop (e.g. a listing of many objects) cannot
27+
* flood the logs with the identical notice.
28+
*/
29+
private static bool $unserializeWithoutAllowedClassesDeprecationTriggered = false;
30+
2431
public static function serialize(mixed $data): string
2532
{
2633
return serialize($data);
2734
}
2835

36+
/**
37+
* Pass an array of class names to allow only those classes during deserialization, `false` to
38+
* forbid object deserialization entirely (the safe choice for scalar/array data), or `true` to
39+
* allow any class when reconstructing a trusted, non-attacker-writable stored object graph.
40+
*
41+
* Omitting the argument is deprecated: it currently defaults to the permissive `true` for
42+
* backward compatibility, but the default will change to `false` in Pimcore 2027.1. Every caller
43+
* should pass an explicit value before then.
44+
*
45+
* @param array<int, class-string>|bool $allowedClasses
46+
*/
2947
public static function unserialize(?string $data = null, array|bool $allowedClasses = true): mixed
3048
{
3149
if ($data === null || $data === '') {
3250
return $data;
3351
}
3452

53+
if (func_num_args() < 2 && !self::$unserializeWithoutAllowedClassesDeprecationTriggered) {
54+
self::$unserializeWithoutAllowedClassesDeprecationTriggered = true;
55+
trigger_deprecation(
56+
'pimcore/pimcore',
57+
'12.3',
58+
'Calling %s() without the $allowedClasses argument is deprecated. Pass an explicit '
59+
. 'value: the default will change from true to false (object deserialization disabled) '
60+
. 'in Pimcore 2027.1.',
61+
__METHOD__
62+
);
63+
}
64+
3565
return unserialize($data, [
3666
'allowed_classes' => $allowedClasses,
3767
]);

models/DataObject/ClassDefinition/Data/Block.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,7 @@ public function getDataFromResource(mixed $data, ?DataObject\Concrete $object =
170170
}, $data);
171171
}
172172

173-
$unserializedData = Serialize::unserialize($data);
173+
$unserializedData = Serialize::unserialize($data, false);
174174
$result = [];
175175

176176
foreach ($unserializedData as $blockElements) {

models/DataObject/ClassDefinition/Data/ImageGallery.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ public function getDataFromResource(mixed $data, ?DataObject\Concrete $object =
139139

140140
$images = $data[$this->getName() . '__images'];
141141
$hotspots = $data[$this->getName() . '__hotspots'];
142-
$hotspots = $hotspots ? Serialize::unserialize($hotspots) : [];
142+
$hotspots = $hotspots ? Serialize::unserialize($hotspots, true) : [];
143143

144144
if (!$images) {
145145
return $this->createEmptyImageGallery($params);

models/DataObject/ClassDefinition/Data/Video.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ public function getDataForResource(mixed $data, ?DataObject\Concrete $object = n
135135
public function getDataFromResource(mixed $data, ?Concrete $object = null, array $params = []): ?DataObject\Data\Video
136136
{
137137
if ($data) {
138-
$raw = Serialize::unserialize($data);
138+
$raw = Serialize::unserialize($data, false);
139139

140140
if ($raw['type'] === 'asset') {
141141
if ($asset = Asset::getById($raw['data'])) {

models/DataObject/Data/EncryptedField.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ public function __wakeup(): void
105105

106106
$data = Crypto::decrypt($this->encrypted, $key, true);
107107

108-
$data = Serialize::unserialize($data);
108+
$data = Serialize::unserialize($data, true);
109109

110110
if ($data instanceof OwnerAwareFieldInterface) {
111111
$data->_setOwner($this->_owner);

0 commit comments

Comments
 (0)